From f061f5718b597ae63654c0c4618f5e7ba03ba650 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:27:28 -0400 Subject: [PATCH 1/4] Fix FileDownloadUtils validation and implement checksum verification The download-validation helpers added in 7.0.0 (#979, #980) had several gaps that only surface once a caller passes a real hash URL or downloads from a server that can 404. All of them are fixed here, and hash verification is implemented rather than stubbed. Correctness fixes: * createValidationFiles(URL, ...) passed a literal Hash.UNKNOWN to its URLConnection overload instead of the caller's argument, so it could never write a hash file and threw IllegalArgumentException for any caller that supplied a hashURL. * Neither downloadFile nor createValidationFiles checked the HTTP status, so a 404 error page was written into the cache as though it were the requested file. Because the .size sidecar was then taken from that same error response, validateFile subsequently declared it valid. A new HttpStatusException lets callers tell "the resource is not there" apart from a transport failure, which matters for anything that tries several mirrors in turn. * downloadFile used FileChannel.transferFrom(rbc, 0, Long.MAX_VALUE), which is not guaranteed to drain a socket-backed channel and could silently truncate a download. Replaced with Files.copy, which loops to end of stream. * downloadFile leaked its temporary file on every failure path. * validateFile threw NullPointerException for a file with no parent directory, and again if listFiles() returned null; an empty .size file raised an unchecked NoSuchElementException that escaped the surrounding catch. * validateFile checked only the first hash sidecar it found, ignoring the rest. New functionality: * validateFile now really verifies MD5, SHA-1 and SHA-256 instead of throwing UnsupportedOperationException. Sidecars are written as bare lowercase hex and parsed tolerantly, so a file downloaded verbatim from a server in coreutils or BSD layout is also understood. A sidecar that cannot be parsed is skipped with a warning rather than failing an otherwise good download. * ETagPolicy lets an ETag that is a bare hex digest be recorded as a checksum without a second request. files.wwpdb.org and files.rcsb.org return the content MD5 as the ETag, so every download from the wwPDB archive now gets a real integrity check for free. The - ETags used by the EBI servers contain a dash and can never be misread as a digest; a test pins that. * downloadFileWithValidation downloads and validates over a single connection. The previous pattern opened one connection to read Content-Length and another to fetch the bytes, so the recorded size described a different response than the one written; if the resource changed in between, the cache entry was left permanently failing validation. Content is digested while streaming to a temp file and only moved into place once length and checksum check out. LocalPDBDirectory uses the new single-connection download, and its two-character directory hash is promoted to the reusable getMiddleHash(String). That hash deliberately counts from the end of the identifier so that both spellings of an entry land in the same bucket: 1cbs and pdb_00001cbs both give "cb", where counting from the start would file the extended form under "db". Defaults change slightly: the existing four-argument createValidationFiles overloads now use ETagPolicy.USE_IF_HEX_DIGEST, so callers start recording checksums where the server offers one. Targeted at 7.3.0. --- .../nbio/core/util/FileDownloadUtils.java | 621 ++++++++++++++++-- .../nbio/core/util/HttpStatusException.java | 84 +++ .../nbio/core/util/FileDownloadUtilsTest.java | 218 +++++- .../nbio/structure/io/LocalPDBDirectory.java | 35 +- 4 files changed, 883 insertions(+), 75 deletions(-) create mode 100644 biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java diff --git a/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java b/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java index 0b132b180e..5b8c656658 100644 --- a/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java +++ b/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java @@ -21,9 +21,9 @@ */ package org.biojava.nbio.core.util; +import java.io.BufferedInputStream; import java.io.File; import java.io.FileNotFoundException; -import java.io.FileOutputStream; import java.io.FilenameFilter; import java.io.IOException; import java.io.InputStream; @@ -32,11 +32,15 @@ import java.net.SocketTimeoutException; import java.net.URL; import java.net.URLConnection; -import java.nio.channels.Channels; -import java.nio.channels.ReadableByteChannel; +import java.nio.charset.StandardCharsets; import java.nio.file.*; import java.nio.file.attribute.BasicFileAttributes; +import java.security.DigestInputStream; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.Scanner; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,10 +51,45 @@ public class FileDownloadUtils { private static final String HASH_EXT = ".hash"; private static final Logger logger = LoggerFactory.getLogger(FileDownloadUtils.class); + /** Buffer used when streaming a file through a {@link MessageDigest}. */ + private static final int DIGEST_BUFFER_SIZE = 64 * 1024; + + /** A bare hex digest, optionally followed by whitespace and a file name (the + * layout written by md5sum, sha1sum and friends). */ + private static final Pattern BARE_HEX_HASH = Pattern.compile("^([0-9a-fA-F]{32,128})(?:[\\s*].*)?$"); + + /** The BSD layout, e.g. MD5 (file.txt) = d41d8cd9.... */ + private static final Pattern BSD_HASH = Pattern.compile("^\\w+\\s*\\(.*\\)\\s*=\\s*([0-9a-fA-F]{32,128})$"); + public enum Hash{ MD5, SHA1, SHA256, UNKNOWN } + /** + * What to do with the ETag response header when no explicit hash + * URL is available. + *

+ * Some archives — notably files.wwpdb.org and + * files.rcsb.org — return the MD5 digest of the content as + * the bare ETag value, which lets us record a real checksum + * without a second request. Others (the EBI servers, for instance) return a + * <modification-time>-<size> form instead; because that + * always contains a -, it can never be mistaken for a hex digest. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public enum ETagPolicy { + /** Never look at the ETag header. */ + IGNORE, + /** Record the ETag as a checksum when it is a bare hex digest + * of a length matching one of the supported algorithms. */ + USE_IF_HEX_DIGEST, + /** As {@link #USE_IF_HEX_DIGEST}, but log a warning when the header is + * missing or is not a usable digest. */ + REQUIRE + } + /** * Gets the file extension of a file, excluding '.'. * If the file name has no extension the file name is returned. @@ -95,44 +134,158 @@ public static void downloadFile(URL url, File destination) throws IOException { int maxTries = 10; int timeout = 60000; //60 sec - File tempFile = Files.createTempFile(getFilePrefix(destination), "." + getFileExtension(destination)).toFile(); + File tempFile = createTempFileFor(destination); - // Took following recipe from stackoverflow: - // http://stackoverflow.com/questions/921262/how-to-download-and-save-a-file-from-internet-using-java - // It seems to be the most efficient way to transfer a file - // See: http://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileChannel.html - ReadableByteChannel rbc = null; - FileOutputStream fos = null; - while (true) { - try { - URLConnection connection = prepareURLConnection(url.toString(), timeout); - connection.connect(); - InputStream inputStream = connection.getInputStream(); - - rbc = Channels.newChannel(inputStream); - fos = new FileOutputStream(tempFile); - fos.getChannel().transferFrom(rbc, 0, Long.MAX_VALUE); - break; - } catch (SocketTimeoutException e) { - if (++count == maxTries) throw e; - } finally { - if (rbc != null) { - rbc.close(); - } - if (fos != null) { - fos.close(); + try { + while (true) { + try { + URLConnection connection = prepareURLConnection(url.toString(), timeout); + connection.connect(); + checkHttpStatus(connection); + try (InputStream inputStream = connection.getInputStream()) { + // Files.copy loops until end of stream. FileChannel.transferFrom(), used + // here previously, is not guaranteed to drain a socket-backed channel in + // a single call and could silently truncate a download. + Files.copy(inputStream, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + break; + } catch (SocketTimeoutException e) { + if (++count == maxTries) throw e; } } + + logger.debug("Copying temp file [{}] to final location [{}]", tempFile, destination); + Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + } finally { + // on every path, including failure: the temp file used to leak whenever + // the download threw. + deleteQuietly(tempFile); } + } + + /** + * Downloads a file and writes its validation metadata in a single pass over a + * single connection. + *

+ * This is preferable to calling {@link #createValidationFiles(URL, File, URL, Hash)} + * followed by {@link #downloadFile(URL, File)}: those open separate connections, + * so the Content-Length recorded in the .size file + * comes from a different response than the bytes actually written. If the + * resource changes between the two requests, the cache entry is left + * permanently failing validation. + *

+ * The content is streamed to a temporary file and only moved into place once + * the declared length and (where available) the checksum have been confirmed, + * so a failed download never leaves a partial file at destination. + * + * @param url the remote file to download + * @param destination the local file to download into. Its parent directory must exist. + * @param hashURL URL of a file containing the expected hash. May be null. + * @param hash the hashing algorithm matching hashURL. Ignored when + * hashURL is null. + * @param eTagPolicy what to do with an ETag response header when + * hashURL is null. May be null, + * which is treated as {@link ETagPolicy#IGNORE}. + * @throws HttpStatusException if the server answered with a non-2xx status + * @throws IOException if the transfer failed, or the transferred content did not + * match the length or checksum the server declared + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void downloadFileWithValidation(URL url, File destination, URL hashURL, Hash hash, + ETagPolicy eTagPolicy) throws IOException { + int timeout = 60000; //60 sec + ETagPolicy policy = eTagPolicy == null ? ETagPolicy.IGNORE : eTagPolicy; + + File tempFile = createTempFileFor(destination); + try { + URLConnection connection = prepareURLConnection(url.toString(), timeout); + connection.connect(); + checkHttpStatus(connection); - logger.debug("Copying temp file [{}] to final location [{}]", tempFile, destination); - Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + long declaredSize = connection.getContentLengthLong(); + String eTag = connection.getHeaderField("ETag"); - // delete the tmp file - tempFile.delete(); + // Only digest when we have something to compare against; hashing every + // download would cost CPU for no benefit. + Hash eTagHash = policy == ETagPolicy.IGNORE ? Hash.UNKNOWN : hashFromETag(eTag); + if (policy == ETagPolicy.REQUIRE && eTagHash == Hash.UNKNOWN) { + logger.warn("ETag [{}] of {} is not a usable hex digest; no checksum will be recorded.", eTag, url); + } + + MessageDigest digest = eTagHash == Hash.UNKNOWN ? null : newDigest(eTagHash); + long written; + try (InputStream raw = connection.getInputStream(); + InputStream in = digest == null ? raw : new DigestInputStream(raw, digest)) { + written = Files.copy(in, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + + if (declaredSize >= 0 && written != declaredSize) { + throw new IOException(String.format( + "Incomplete download of %s: got %d bytes but the server declared %d.", + url, written, declaredSize)); + } + + String actualDigest = digest == null ? null : toHex(digest.digest()); + if (actualDigest != null && !actualDigest.equalsIgnoreCase(normalizeETag(eTag))) { + throw new IOException(String.format( + "Corrupt download of %s: %s of the content is %s but the server's ETag says %s.", + url, eTagHash, actualDigest, normalizeETag(eTag))); + } + + moveIntoPlace(tempFile, destination); + // Sidecars are written only once the content is known good, so a failed + // download can never leave validation metadata describing a file that is + // not there. + writeSizeFile(destination, written); + if (hashURL != null) { + if (hash == null || hash == Hash.UNKNOWN) { + throw new IllegalArgumentException("Hash URL given but algorithm is unknown"); + } + downloadFile(hashURL, hashFileFor(destination, hash)); + } else if (actualDigest != null) { + writeHashFile(destination, eTagHash, actualDigest); + } + } finally { + deleteQuietly(tempFile); + } } - + + /** + * Verifies that an HTTP connection returned a 2xx status. Connections using a + * non-HTTP protocol (file:, ftp:, ...) are left alone. + *

+ * Without this check a 404 error page is written into the cache as though it + * were the requested file — and because the .size sidecar is + * then taken from that same error response, {@link #validateFile(File)} would + * subsequently declare it valid. + * + * @param connection an already connected {@link URLConnection} + * @throws HttpStatusException if the status is outside the 2xx range + * @throws IOException if the status could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void checkHttpStatus(URLConnection connection) throws IOException { + if (!(connection instanceof HttpURLConnection)) { + return; + } + HttpURLConnection http = (HttpURLConnection) connection; + int code = http.getResponseCode(); + if (code >= 200 && code < 300) { + return; + } + if (code == 301 || code == 302 || code == 307 || code == 308) { + // The JDK follows redirects automatically, but never across protocols, so + // an http -> https redirect surfaces here and is worth naming explicitly. + logger.warn("{} returned redirect {} to [{}], which was not followed " + + "(the JDK does not follow redirects that change protocol).", + connection.getURL(), code, http.getHeaderField("Location")); + } + throw new HttpStatusException(code, connection.getURL().toString(), http.getResponseMessage()); + } + /** * Creates validation files beside a file to be downloaded.
* Whenever possible, for a file.ext file, it creates @@ -146,9 +299,27 @@ public static void downloadFile(URL url, File destination) throws IOException { * @param hash The Hashing algorithm. Ignored if hashURL is null. */ public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash){ + createValidationFiles(url, localDestination, hashURL, hash, ETagPolicy.USE_IF_HEX_DIGEST); + } + + /** + * Creates validation files beside a file to be downloaded, with explicit control + * over how the ETag response header is treated. + * + * @param url the remote file URL to download + * @param localDestination the local file to download into + * @param hashURL the URL of the hash file to download. Can be null. + * @param hash The Hashing algorithm. Ignored if hashURL is null. + * @param eTagPolicy how to treat the ETag header when hashURL + * is null. May be null, treated as {@link ETagPolicy#IGNORE}. + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash, + ETagPolicy eTagPolicy){ try { URLConnection resourceConnection = url.openConnection(); - createValidationFiles(resourceConnection, localDestination, hashURL, FileDownloadUtils.Hash.UNKNOWN); + createValidationFiles(resourceConnection, localDestination, hashURL, hash, eTagPolicy); } catch (IOException e) { logger.warn("could not open connection to resource file due to exception: {}", e.getMessage()); } @@ -169,31 +340,246 @@ public static void createValidationFiles(URL url, File localDestination, URL has * @since 7.0.0 */ public static void createValidationFiles(URLConnection resourceUrlConnection, File localDestination, URL hashURL, Hash hash){ + createValidationFiles(resourceUrlConnection, localDestination, hashURL, hash, ETagPolicy.USE_IF_HEX_DIGEST); + } + + /** + * Creates validation files beside a file to be downloaded, with explicit control + * over how the ETag response header is treated. + *

+ * Nothing is written when the connection reports a non-2xx status: previously an + * error page's Content-Length would be recorded as the expected + * size, so the cached error page then passed validation. + * + * @param resourceUrlConnection the remote file URLConnection to download + * @param localDestination the local file to download into + * @param hashURL the URL of the hash file to download. Can be null. + * @param hash The Hashing algorithm. Ignored if hashURL is null. + * @param eTagPolicy how to treat the ETag header when hashURL + * is null. May be null, treated as {@link ETagPolicy#IGNORE}. + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void createValidationFiles(URLConnection resourceUrlConnection, File localDestination, URL hashURL, + Hash hash, ETagPolicy eTagPolicy){ + try { + checkHttpStatus(resourceUrlConnection); + } catch (IOException e) { + logger.warn("Not writing validation metadata for {}: {}", resourceUrlConnection.getURL(), e.getMessage()); + return; + } + long size = resourceUrlConnection.getContentLengthLong(); if(size == -1) { logger.debug("Could not find expected file size for resource {}. Size validation metadata file won't be available for this download.", resourceUrlConnection.getURL()); } else { logger.debug("Content-Length: {}", size); - File sizeFile = new File(localDestination.getParentFile(), localDestination.getName() + SIZE_EXT); - try (PrintStream sizePrintStream = new PrintStream(sizeFile)) { - sizePrintStream.print(size); - } catch (FileNotFoundException e) { - logger.warn("Could not write size validation metadata file due to exception: {}", e.getMessage()); - } + writeSizeFile(localDestination, size); } - - if(hashURL == null) + + if(hashURL == null) { + ETagPolicy policy = eTagPolicy == null ? ETagPolicy.IGNORE : eTagPolicy; + if (policy != ETagPolicy.IGNORE) { + String eTag = resourceUrlConnection.getHeaderField("ETag"); + Hash eTagHash = hashFromETag(eTag); + if (eTagHash == Hash.UNKNOWN) { + if (policy == ETagPolicy.REQUIRE) { + logger.warn("ETag [{}] of {} is not a usable hex digest; no checksum recorded.", + eTag, resourceUrlConnection.getURL()); + } + } else { + writeHashFile(localDestination, eTagHash, normalizeETag(eTag)); + } + } return; + } - if(hash == Hash.UNKNOWN) + if(hash == null || hash == Hash.UNKNOWN) throw new IllegalArgumentException("Hash URL given but algorithm is unknown"); try { - File hashFile = new File(localDestination.getParentFile(), String.format("%s%s_%s", localDestination.getName(), HASH_EXT, hash)); - downloadFile(hashURL, hashFile); + downloadFile(hashURL, hashFileFor(localDestination, hash)); } catch (IOException e) { logger.warn("Could not write validation hash file due to exception: {}", e.getMessage()); } } + + /** + * Determines which hashing algorithm an ETag header value + * represents, based on the length of the hex digest it contains. + *

+ * Only a value consisting solely of hex characters is considered. The + * <time>-<size> form used by nginx and Apache always + * contains a - and therefore never matches. + * + * @param eTagHeaderValue the raw header value, possibly quoted or weak-prefixed. + * May be null. + * @return the matching algorithm, or {@link Hash#UNKNOWN} if the value is not a + * hex digest of a recognised length + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static Hash hashFromETag(String eTagHeaderValue) { + String value = normalizeETag(eTagHeaderValue); + if (value == null || !value.matches("[0-9a-fA-F]+")) { + return Hash.UNKNOWN; + } + switch (value.length()) { + case 32: return Hash.MD5; + case 40: return Hash.SHA1; + case 64: return Hash.SHA256; + default: return Hash.UNKNOWN; + } + } + + /** + * Strips the weak-validator prefix and surrounding quotes from an + * ETag header value. + * + * @param eTagHeaderValue the raw header value. May be null. + * @return the bare value, or null if the input was null + * or blank + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String normalizeETag(String eTagHeaderValue) { + if (eTagHeaderValue == null) { + return null; + } + String value = eTagHeaderValue.trim(); + if (value.startsWith("W/")) { + value = value.substring(2).trim(); + } + if (value.length() >= 2 && value.startsWith("\"") && value.endsWith("\"")) { + value = value.substring(1, value.length() - 1); + } + return value.isEmpty() ? null : value; + } + + /** + * Writes a <name>.hash_<ALGORITHM> sidecar file + * containing the given digest as bare lowercase hex. + * + * @param localDestination the file the digest describes + * @param hash the hashing algorithm + * @param hexDigest the digest, in hex + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void writeHashFile(File localDestination, Hash hash, String hexDigest) { + if (hash == null || hash == Hash.UNKNOWN || hexDigest == null) { + return; + } + File hashFile = hashFileFor(localDestination, hash); + try (PrintStream out = new PrintStream(hashFile, StandardCharsets.UTF_8.name())) { + out.println(hexDigest.toLowerCase()); + } catch (IOException e) { + logger.warn("Could not write validation hash file due to exception: {}", e.getMessage()); + } + } + + /** + * Computes the digest of a file. + * + * @param file the file to digest + * @param hash the algorithm to use + * @return the digest as bare lowercase hex + * @throws IOException if the file could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String computeHash(File file, Hash hash) throws IOException { + try (InputStream in = new BufferedInputStream(Files.newInputStream(file.toPath()), DIGEST_BUFFER_SIZE)) { + return computeHash(in, hash); + } + } + + /** + * Computes the digest of a stream. The stream is read to its end but not closed. + * + * @param in the stream to digest + * @param hash the algorithm to use + * @return the digest as bare lowercase hex + * @throws IOException if the stream could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String computeHash(InputStream in, Hash hash) throws IOException { + MessageDigest digest = newDigest(hash); + byte[] buffer = new byte[DIGEST_BUFFER_SIZE]; + int read; + while ((read = in.read(buffer)) != -1) { + digest.update(buffer, 0, read); + } + return toHex(digest.digest()); + } + + /** + * Checks a file against an expected digest. + * + * @param file the file to check + * @param hash the algorithm to use + * @param expectedHex the expected digest, in hex; compared case-insensitively + * @return true if the digests match + * @throws IOException if the file could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static boolean verifyHash(File file, Hash hash, String expectedHex) throws IOException { + return expectedHex != null && expectedHex.trim().equalsIgnoreCase(computeHash(file, hash)); + } + + /** + * The JDK name of a hashing algorithm, which differs from the enum constant for + * the SHA variants. + * + * @param hash the algorithm + * @return the name to pass to {@link MessageDigest#getInstance(String)} + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String getAlgorithmName(Hash hash) { + switch (hash) { + case MD5: return "MD5"; + case SHA1: return "SHA-1"; + case SHA256: return "SHA-256"; + default: throw new IllegalArgumentException("Hashing algorithm not known: " + hash); + } + } + + /** + * Reads the expected digest out of a .hash_XXXX sidecar file. + *

+ * The file may have been downloaded verbatim from a server, so several common + * layouts are accepted: a bare hex digest, the md5sum style + * <hex>  <filename>, and the BSD style + * MD5 (<filename>) = <hex>. + * + * @param hashFile the sidecar file + * @return the digest in hex, or null if nothing recognisable was found + */ + static String parseHashFile(File hashFile) { + try (Scanner scanner = new Scanner(hashFile, StandardCharsets.UTF_8.name())) { + while (scanner.hasNextLine()) { + String line = scanner.nextLine().trim(); + if (line.isEmpty()) { + continue; + } + Matcher bare = BARE_HEX_HASH.matcher(line); + if (bare.matches()) { + return bare.group(1); + } + Matcher bsd = BSD_HASH.matcher(line); + if (bsd.matches()) { + return bsd.group(1); + } + return null; // first meaningful line was not a digest + } + } catch (IOException e) { + logger.warn("Could not read hash file [{}]: {}", hashFile, e.getMessage()); + } + return null; + } + /** * Validate a local file based on pre-existing metadata files for size and hash.
@@ -210,45 +596,154 @@ public static void createValidationFiles(URLConnection resourceUrlConnection, Fi * @since 7.0.0 */ public static boolean validateFile(File localFile) { - File sizeFile = new File(localFile.getParentFile(), localFile.getName() + SIZE_EXT); + // getParentFile() is null for a bare relative name such as new File("x.cif"), + // which used to make this method throw a NullPointerException. + File parent = localFile.getAbsoluteFile().getParentFile(); + if (parent == null) { + logger.debug("Cannot determine the parent directory of [{}]; nothing to validate against.", localFile); + return true; + } + + File sizeFile = new File(parent, localFile.getName() + SIZE_EXT); if(sizeFile.exists()) { try (Scanner scanner = new Scanner(sizeFile)) { - long expectedSize = scanner.nextLong(); - long actualSize = localFile.length(); - if (expectedSize != actualSize) { - logger.warn("File [{}] size ({}) does not match expected size ({}).", localFile, actualSize, expectedSize); - return false; + if (!scanner.hasNextLong()) { + // An empty or truncated .size file used to raise an unchecked + // NoSuchElementException that escaped the catch below. + logger.warn("Size metadata file [{}] is empty or malformed; skipping size validation.", sizeFile); + } else { + long expectedSize = scanner.nextLong(); + long actualSize = localFile.length(); + if (expectedSize != actualSize) { + logger.warn("File [{}] size ({}) does not match expected size ({}).", localFile, actualSize, expectedSize); + return false; + } } } catch (FileNotFoundException e) { logger.warn("could not validate size of file [{}] because no size metadata file exists.", localFile); } } - File[] hashFiles = localFile.getParentFile().listFiles(new FilenameFilter() { - final String hashPattern = String.format("%s%s_(%s|%s|%s)", localFile.getName(), HASH_EXT, Hash.MD5, Hash.SHA1, Hash.SHA256); + File[] hashFiles = parent.listFiles(new FilenameFilter() { + final String hashPattern = String.format("%s%s_(%s|%s|%s)", Pattern.quote(localFile.getName()), HASH_EXT, Hash.MD5, Hash.SHA1, Hash.SHA256); @Override public boolean accept(File dir, String name) { return name.matches(hashPattern); } }); - if(hashFiles.length > 0) { - File hashFile = hashFiles[0]; + // listFiles() returns null if the parent is not a directory or cannot be read. + if (hashFiles == null || hashFiles.length == 0) { + return true; + } + + // Verify against every sidecar present, not only the first one found. + for (File hashFile : hashFiles) { String name = hashFile.getName(); String algo = name.substring(name.lastIndexOf('_') + 1); - switch (Hash.valueOf(algo)) { - case MD5: - case SHA1: - case SHA256: - throw new UnsupportedOperationException("Not yet implemented"); - case UNKNOWN: - default: // No need. Already checked above + Hash hash; + try { + hash = Hash.valueOf(algo); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Hashing algorithm not known: " + algo, e); + } + if (hash == Hash.UNKNOWN) { throw new IllegalArgumentException("Hashing algorithm not known: " + algo); } + + String expected = parseHashFile(hashFile); + if (expected == null) { + // A sidecar we cannot read should not condemn an otherwise good download. + logger.warn("Could not read a digest from [{}]; skipping {} validation of [{}].", hashFile, hash, localFile); + continue; + } + try { + if (!verifyHash(localFile, hash, expected)) { + logger.warn("File [{}] {} does not match the expected digest {}.", localFile, hash, expected); + return false; + } + } catch (IOException e) { + logger.warn("Could not compute the {} of [{}]: {}", hash, localFile, e.getMessage()); + return false; + } } - + return true; } + /** + * The <name>.hash_<ALGORITHM> sidecar file for a + * downloaded file. + */ + private static File hashFileFor(File localDestination, Hash hash) { + return new File(localDestination.getAbsoluteFile().getParentFile(), + String.format("%s%s_%s", localDestination.getName(), HASH_EXT, hash)); + } + + /** + * Writes the <name>.size sidecar file. + */ + private static void writeSizeFile(File localDestination, long size) { + File sizeFile = new File(localDestination.getAbsoluteFile().getParentFile(), + localDestination.getName() + SIZE_EXT); + try (PrintStream sizePrintStream = new PrintStream(sizeFile, StandardCharsets.UTF_8.name())) { + sizePrintStream.print(size); + } catch (IOException e) { + logger.warn("Could not write size validation metadata file due to exception: {}", e.getMessage()); + } + } + + private static MessageDigest newDigest(Hash hash) { + try { + return MessageDigest.getInstance(getAlgorithmName(hash)); + } catch (NoSuchAlgorithmException e) { + // MD5, SHA-1 and SHA-256 are required of every Java platform. + throw new IllegalStateException("Required hashing algorithm is unavailable: " + hash, e); + } + } + + private static String toHex(byte[] bytes) { + StringBuilder sb = new StringBuilder(bytes.length * 2); + for (byte b : bytes) { + sb.append(Character.forDigit((b >> 4) & 0xF, 16)); + sb.append(Character.forDigit(b & 0xF, 16)); + } + return sb.toString(); + } + + /** + * Creates a temp file whose name is derived from the destination. + * {@link Files#createTempFile} rejects prefixes shorter than 3 characters, so + * short names are padded. + */ + private static File createTempFileFor(File destination) throws IOException { + String prefix = getFilePrefix(destination); + while (prefix.length() < 3) { + prefix = prefix + "_"; + } + return Files.createTempFile(prefix, "." + getFileExtension(destination)).toFile(); + } + + private static void moveIntoPlace(File tempFile, File destination) throws IOException { + try { + Files.move(tempFile.toPath(), destination.toPath(), + StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + // The temp directory is often on a different filesystem than the cache. + Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + } + + private static void deleteQuietly(File file) { + if (file == null) { + return; + } + try { + Files.deleteIfExists(file.toPath()); + } catch (IOException e) { + logger.debug("Could not delete temporary file [{}]: {}", file, e.getMessage()); + } + } + /** * Converts path to Unix convention and adds a terminating slash if it was * omitted. diff --git a/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java b/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java new file mode 100644 index 0000000000..c74c6ffb05 --- /dev/null +++ b/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java @@ -0,0 +1,84 @@ +/** + * BioJava development code + * + * This code may be freely distributed and modified under the terms of the GNU + * Lesser General Public Licence. This should be distributed with the code. If + * you do not have a copy, see: + * + * http://www.gnu.org/copyleft/lesser.html + * + * Copyright for this code is held jointly by the individual authors. These + * should be listed in @author doc comments. + * + * For more information on the BioJava project and its aims, or to join the + * biojava-l mailing list, visit the home page at: + * + * http://www.biojava.org/ + */ +package org.biojava.nbio.core.util; + +import java.io.IOException; + +/** + * Signals that an HTTP request completed but returned a status code outside the + * 2xx range. + *

+ * This exists so that callers can tell apart the two very different reasons a + * download can fail: + *

    + *
  • The resource does not exist ({@link #isNotFound()}, i.e. HTTP 404 or + * 410). For a caller that tries several mirrors or several alternative data + * sources in turn, this simply means "not here, try the next one".
    • + *
  • Anything else — a server error, a redirect that was not + * followed, an authentication failure. These usually mean the whole operation + * should be abandoned rather than silently treated as "no data available".
    • + *
+ * Without a distinct exception type the only way to tell these apart is by + * parsing the message of a plain {@link IOException}, which is brittle. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ +public class HttpStatusException extends IOException { + + private static final long serialVersionUID = 1L; + + private final int statusCode; + private final String url; + + /** + * @param statusCode the HTTP status code returned by the server + * @param url the URL that was requested + * @param responseMessage the HTTP reason phrase, may be null + */ + public HttpStatusException(int statusCode, String url, String responseMessage) { + super(String.format("HTTP %d%s for %s", statusCode, + responseMessage == null || responseMessage.isEmpty() ? "" : " " + responseMessage, url)); + this.statusCode = statusCode; + this.url = url; + } + + /** + * @return the HTTP status code returned by the server + */ + public int getStatusCode() { + return statusCode; + } + + /** + * @return the URL that was requested + */ + public String getUrl() { + return url; + } + + /** + * Whether the status indicates that the resource is simply not there, as + * opposed to a transport or server problem. + * + * @return true for HTTP 404 (Not Found) and 410 (Gone) + */ + public boolean isNotFound() { + return statusCode == 404 || statusCode == 410; + } +} diff --git a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java index 201ad88e48..1177c1d1fc 100644 --- a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java +++ b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java @@ -4,6 +4,7 @@ import static org.biojava.nbio.core.util.FileDownloadUtils.getFilePrefix; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -11,6 +12,7 @@ import java.io.IOException; import java.io.PrintStream; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import org.junit.jupiter.api.Nested; @@ -187,17 +189,223 @@ void testValidationFiles() throws IOException{ assertTrue(sizeFile.exists(), "couldn't create size file"); assertTrue(FileDownloadUtils.validateFile(destFile), "file not detected to be invalid although there is correct size validation file"); + // files.wwpdb.org returns the content MD5 as the ETag, so the default + // ETag policy records a real checksum without a separate hash URL. + assertTrue(hashFile.exists(), "no hash file was derived from the ETag"); + assertTrue(FileDownloadUtils.validateFile(destFile), "correctly downloaded file failed hash validation"); + PrintStream temp2 = new PrintStream(hashFile); - temp2.print("ABCD"); // some wrong hash value + temp2.print("ABCD"); // not a digest of any supported length temp2.close(); - //This is not yet implemented. I am using this test for documentation purpose. - assertThrows(UnsupportedOperationException.class, - () -> FileDownloadUtils.validateFile(destFile), + // An unreadable sidecar must not condemn an otherwise good download. + assertTrue(FileDownloadUtils.validateFile(destFile), + "a malformed hash file should be ignored, not treated as a mismatch"); + + PrintStream temp3 = new PrintStream(hashFile); + temp3.print("00000000000000000000000000000000"); // well-formed but wrong MD5 + temp3.close(); + assertFalse(FileDownloadUtils.validateFile(destFile), "file not detected to be invalid although hash value is wrong."); - + System.out.println("Just ignore the previous warning. It is expected."); + destFile.delete(); sizeFile.delete(); hashFile.delete(); } } + + @Nested + class Hashing { + + private File writeTemp(String name, byte[] content) throws IOException { + File f = new File(System.getProperty("java.io.tmpdir"), name); + Files.write(f.toPath(), content); + f.deleteOnExit(); + return f; + } + + @Test + void digestsOfEmptyFileMatchKnownValues() throws IOException { + File empty = writeTemp("bj-empty.bin", new byte[0]); + assertEquals("d41d8cd98f00b204e9800998ecf8427e", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.MD5)); + assertEquals("da39a3ee5e6b4b0d3255bfef95601890afd80709", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.SHA1)); + assertEquals("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.SHA256)); + } + + @Test + void digestOfKnownContent() throws IOException { + File abc = writeTemp("bj-abc.bin", "abc".getBytes(StandardCharsets.UTF_8)); + assertEquals("900150983cd24fb0d6963f7d28e17f72", + FileDownloadUtils.computeHash(abc, FileDownloadUtils.Hash.MD5)); + assertTrue(FileDownloadUtils.verifyHash(abc, FileDownloadUtils.Hash.MD5, + "900150983CD24FB0D6963F7D28E17F72"), "comparison should be case-insensitive"); + assertFalse(FileDownloadUtils.verifyHash(abc, FileDownloadUtils.Hash.MD5, + "00000000000000000000000000000000")); + } + + @Test + void algorithmNames() { + assertEquals("MD5", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.MD5)); + assertEquals("SHA-1", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.SHA1)); + assertEquals("SHA-256", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.SHA256)); + assertThrows(IllegalArgumentException.class, + () -> FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.UNKNOWN)); + } + } + + @Nested + class HashFileParsing { + + private static final String MD5 = "900150983cd24fb0d6963f7d28e17f72"; + + private String parse(String content) throws IOException { + File f = new File(System.getProperty("java.io.tmpdir"), "bj-hashfile.txt"); + Files.write(f.toPath(), content.getBytes(StandardCharsets.UTF_8)); + f.deleteOnExit(); + return FileDownloadUtils.parseHashFile(f); + } + + @Test + void bareHex() throws IOException { + assertEquals(MD5, parse(MD5)); + assertEquals(MD5, parse(MD5 + "\n")); + } + + @Test + void uppercaseHexIsKeptVerbatim() throws IOException { + assertEquals(MD5.toUpperCase(), parse(MD5.toUpperCase())); + } + + @Test + void coreutilsLayouts() throws IOException { + assertEquals(MD5, parse(MD5 + " somefile.cif.gz\n")); + assertEquals(MD5, parse(MD5 + " *somefile.cif.gz\n")); + } + + @Test + void bsdLayout() throws IOException { + assertEquals(MD5, parse("MD5 (somefile.cif.gz) = " + MD5 + "\n")); + } + + @Test + void blankLeadingLinesAreSkipped() throws IOException { + assertEquals(MD5, parse("\n \n" + MD5 + "\n")); + } + + @Test + void garbageYieldsNull() throws IOException { + assertNull(parse("not a hash at all\n")); + assertNull(parse("ABCD\n")); + assertNull(parse("")); + } + } + + @Nested + class ETagParsing { + + @Test + void wwpdbStyleMd5IsRecognised() { + assertEquals(FileDownloadUtils.Hash.MD5, + FileDownloadUtils.hashFromETag("\"f99fb9d964e1e1c22f2ea559ac5745cf\"")); + assertEquals("f99fb9d964e1e1c22f2ea559ac5745cf", + FileDownloadUtils.normalizeETag("\"f99fb9d964e1e1c22f2ea559ac5745cf\"")); + } + + @Test + void sha1AndSha256LengthsAreRecognised() { + assertEquals(FileDownloadUtils.Hash.SHA1, + FileDownloadUtils.hashFromETag("da39a3ee5e6b4b0d3255bfef95601890afd80709")); + assertEquals(FileDownloadUtils.Hash.SHA256, + FileDownloadUtils.hashFromETag( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")); + } + + @Test + void ebiStyleTimeSizeETagIsNotMistakenForADigest() { + // nginx and Apache emit -; the dash keeps it out of + // the hex-only pattern, which is what stops us recording a bogus checksum. + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("\"67910788-1009e0\"")); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("\"14c95dd-51eedb9922b40\"")); + } + + @Test + void weakAndMissingETags() { + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("W/\"abc\"")); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag(null)); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag(" ")); + assertNull(FileDownloadUtils.normalizeETag(null)); + assertNull(FileDownloadUtils.normalizeETag("\"\"")); + } + } + + @Nested + class ValidateFile { + + @Test + void bareRelativeNameDoesNotThrow() { + // getParentFile() is null here; this used to be a NullPointerException. + assertTrue(FileDownloadUtils.validateFile(new File("no-such-file-in-cwd.cif"))); + } + + @Test + void emptySizeFileIsIgnoredRatherThanThrowing() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "hello".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.size").toPath(), new byte[0]); + assertTrue(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void sizeMismatchIsDetected() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "hello".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.size").toPath(), "99".getBytes(StandardCharsets.UTF_8)); + assertFalse(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void everyHashSidecarIsChecked() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "abc".getBytes(StandardCharsets.UTF_8)); + // correct MD5, wrong SHA1: the second sidecar must still be caught + Files.write(new File(dir, "data.bin.hash_MD5").toPath(), + "900150983cd24fb0d6963f7d28e17f72".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.hash_SHA1").toPath(), + "0000000000000000000000000000000000000000".getBytes(StandardCharsets.UTF_8)); + assertFalse(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void writeHashFileRoundTrip() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "abc".getBytes(StandardCharsets.UTF_8)); + FileDownloadUtils.writeHashFile(data, FileDownloadUtils.Hash.MD5, + FileDownloadUtils.computeHash(data, FileDownloadUtils.Hash.MD5)); + assertTrue(new File(dir, "data.bin.hash_MD5").exists()); + assertTrue(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + } } diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java index 4ec4577f59..d08f6cb05f 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java @@ -550,7 +550,7 @@ private File downloadStructure(PdbId pdbId, String pathOnServer, boolean obsolet ftp = DEFAULT_BCIF_FILE_SERVER + filename; } else { ftp = String.format("%s%s/%s/%s", - serverName, pathOnServer, id.substring(id.length()-3, id.length()-1), getFilename(id)); + serverName, pathOnServer, getMiddleHash(id), getFilename(id)); } URL url = new URL(ftp); @@ -576,21 +576,44 @@ private File downloadStructure(PdbId pdbId, String pathOnServer, boolean obsolet logger.info("Fetching {}", ftp); logger.info("Writing to {}", realFile); - FileDownloadUtils.createValidationFiles(url, realFile, null, FileDownloadUtils.Hash.UNKNOWN); - FileDownloadUtils.downloadFile(url, realFile); + // A single connection, so the recorded size and checksum describe exactly the + // bytes that were written. The wwPDB servers return the content MD5 as the + // ETag, so this also gives the cached file a real integrity check. + FileDownloadUtils.downloadFileWithValidation(url, realFile, null, FileDownloadUtils.Hash.UNKNOWN, + FileDownloadUtils.ETagPolicy.USE_IF_HEX_DIGEST); if(! FileDownloadUtils.validateFile(realFile)) throw new IOException("Downloaded file invalid: "+realFile); return realFile; } + /** + * Returns the two-character directory name under which an entry is filed in the + * PDB's divided layout, e.g. cb for 1cbs. + *

+ * The characters are taken relative to the end of the identifier rather + * than the start, so that both spellings of the same entry land in the same + * bucket: 1cbs and its extended form pdb_00001cbs both + * give cb. Taking them from the start would file the extended form + * under db instead. The extended PDB identifier format is expected + * to keep using this same hashing scheme. + * + * @param pdbId a PDB identifier, in either the short or the extended form + * @return the lowercase two-character directory name + * @since 7.3.0 + */ + public static String getMiddleHash(String pdbId) { + int offset = pdbId.length() - 3; + return pdbId.substring(offset, offset + 2).toLowerCase(); + } + /** * Get the last modified time of the file in given url by retrieveing the "Last-Modified" header. * Note that this only works for http URLs * @param url * @return the last modified date or null if it couldn't be retrieved (in that case a warning will be logged) */ - private Date getLastModifiedTime(URL url) { + protected static Date getLastModifiedTime(URL url) { // see http://stackoverflow.com/questions/2416872/how-do-you-obtain-modified-date-from-a-remote-file-java Date date = null; @@ -629,14 +652,12 @@ private Date getLastModifiedTime(URL url) { protected File getDir(String pdbId, boolean obsolete) { File dir = null; - int offset = pdbId.length() - 3; + String middle = getMiddleHash(pdbId); if (obsolete) { // obsolete is always split - String middle = pdbId.substring(offset, offset + 2).toLowerCase(); dir = new File(obsoleteDirPath, middle); } else { - String middle = pdbId.substring(offset, offset + 2).toLowerCase(); dir = new File(splitDirPath, middle); } From 80e9daab55d7269f380239acc7140bf8b1681703 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:28:41 -0400 Subject: [PATCH 2/4] Test that a 404 is neither cached nor recorded as valid Exercises the new status checking against the real wwPDB archive: downloadFile must throw HttpStatusException rather than writing the error page to the destination, and createValidationFiles must not record a .size for it. Without the second half the cached error page would pass validateFile, since its recorded size would match the error body exactly. --- .../nbio/core/util/FileDownloadUtilsTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java index 1177c1d1fc..4a3cae7fb7 100644 --- a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java +++ b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java @@ -214,6 +214,31 @@ void testValidationFiles() throws IOException{ } } + @Nested + class HttpStatus { + + @Test + void notFoundThrowsAndLeavesNothingBehind() throws IOException { + // A path that is guaranteed absent from the wwPDB archive. + URL missing = new URL("https://files.wwpdb.org/pub/pdb/data/structures/divided/mmCIF/zz/zzzz.cif.gz"); + File dest = new File(System.getProperty("java.io.tmpdir"), "bj-missing.cif.gz"); + File sizeFile = new File(dest.getParentFile(), dest.getName() + ".size"); + dest.delete(); + sizeFile.delete(); + + HttpStatusException e = assertThrows(HttpStatusException.class, + () -> FileDownloadUtils.downloadFile(missing, dest)); + assertEquals(404, e.getStatusCode()); + assertTrue(e.isNotFound()); + assertFalse(dest.exists(), "a 404 body must never be written to the destination"); + + // ... and no validation metadata may be recorded for it either, or the + // cached error page would later pass validation. + FileDownloadUtils.createValidationFiles(missing, dest, null, FileDownloadUtils.Hash.UNKNOWN); + assertFalse(sizeFile.exists(), "no size file should be written for a 404 response"); + } + } + @Nested class Hashing { From 66d27c77a427f82812a2e05f5338a33b0b4d14f6 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:42:58 -0400 Subject: [PATCH 3/4] Make LocalPDBDirectory.getLastModifiedTime public It is a general 'has the server got a newer copy' helper with nothing PDB-specific about it, and other caching code outside this package needs the same Last-Modified comparison. Protected access only reached subclasses and the io package itself. --- .../java/org/biojava/nbio/structure/io/LocalPDBDirectory.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java index d08f6cb05f..2e6b09fe9a 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java @@ -612,8 +612,9 @@ public static String getMiddleHash(String pdbId) { * Note that this only works for http URLs * @param url * @return the last modified date or null if it couldn't be retrieved (in that case a warning will be logged) + * @since 7.3.0 made public so that other caching code can reuse it */ - protected static Date getLastModifiedTime(URL url) { + public static Date getLastModifiedTime(URL url) { // see http://stackoverflow.com/questions/2416872/how-do-you-obtain-modified-date-from-a-remote-file-java Date date = null; From 7244f21ae92f37deb22fbd93fb060d8791d72439 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 19:29:20 -0400 Subject: [PATCH 4/4] Fix CATH downloads: use https, and check the response before caching it download.cathdb.info now answers plain http with a 301 to https. HttpURLConnection follows redirects within a protocol but deliberately will not follow one that switches http to https, so the download never reached the real file. downloadFileFromRemote read the response with a bare openStream() and no status check, which meant the body of the 301 was written into the local cache as though it were classification data. Parsing it produced no domains, and the first sign of trouble was a NullPointerException much later, far from the cause: CathDomainTest.test:40 NullPointer Cannot invoke "CathDomain.toCanonical()" because "domain" is null This has been failing on every pull request since the redirect appeared. Two changes. CATH_DOWNLOAD_URL now uses https, which fixes the immediate breakage. And the hand-rolled copy loop is replaced by FileDownloadUtils.downloadFileWithValidation, so a non-2xx response throws instead of being cached, and a redirect that changes protocol is logged explicitly rather than passing silently. That is what turns this class of failure from a mysterious NPE into an error naming the URL and the status. The shared download also records the byte count it actually wrote, so CATH files now get size validation they never had; download.cathdb.info sends no ETag, so there is no checksum to record, but the size check alone would have caught a truncated file. CathDomainTest passes again, in about 11 seconds. --- .../nbio/structure/cath/CathInstallation.java | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index 3f0f44158b..a72e85ed16 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -31,8 +31,6 @@ import java.io.*; import java.net.URL; -import java.nio.file.Files; -import java.nio.file.StandardCopyOption; import java.text.DateFormat; import java.text.DecimalFormat; import java.text.ParseException; @@ -54,7 +52,7 @@ public class CathInstallation implements CathDatabase{ public static final String nodeListFileName = "cath-names-v%s.txt"; public static final String domallFileName = "cath-domain-boundaries-v%s.txt"; - public static final String CATH_DOWNLOAD_URL = "http://download.cathdb.info/cath/releases/"; + public static final String CATH_DOWNLOAD_URL = "https://download.cathdb.info/cath/releases/"; public static final String CATH_DOWNLOAD_ALL_RELEASES_DIR = "all-releases"; public static final String CATH_DOWNLOAD_CLASSIFICATION_DATA_DIR = "cath-classification-data"; @@ -637,27 +635,26 @@ private void parseCathDomall(BufferedReader bufferedReader) throws IOException{ } protected void downloadFileFromRemote(URL remoteURL, File localFile) throws IOException{ -// System.out.println("downloading " + remoteURL + " to: " + localFile); LOGGER.info("Downloading file {} to local file {}", remoteURL, localFile); long timeS = System.currentTimeMillis(); - File tempFile = Files.createTempFile(FileDownloadUtils.getFilePrefix(localFile),"." + FileDownloadUtils.getFileExtension(localFile)).toFile(); - FileOutputStream out = new FileOutputStream(tempFile); - - InputStream in = remoteURL.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); + File parent = localFile.getAbsoluteFile().getParentFile(); + if (parent != null && !parent.isDirectory() && !parent.mkdirs()) { + throw new IOException("Could not create directory " + parent); } - in.close(); - out.close(); - Files.copy(tempFile.toPath(), localFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + // Previously this read the response with a bare remoteURL.openStream() and + // copied whatever came back. That silently accepted error and redirect + // responses: when download.cathdb.info began redirecting http to https, the + // body of the 301 was written here as though it were classification data, + // and the failure only surfaced much later as an unparseable file. + FileDownloadUtils.downloadFileWithValidation(remoteURL, localFile, null, + FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.ETagPolicy.USE_IF_HEX_DIGEST); - // delete the tmp file - tempFile.delete(); + if (!FileDownloadUtils.validateFile(localFile)) { + throw new IOException("Downloaded file invalid: " + localFile); + } long size = localFile.length();