From ce0f394f927feff0ad6a2077e441568fe2585dbf Mon Sep 17 00:00:00 2001 From: Amr ALHOSSARY Date: Sun, 30 Aug 2026 09:30:20 -0400 Subject: [PATCH] Follow the redirects HttpURLConnection declines The JDK follows 301, 302 and 303 within a protocol, but never follows 307 or 308, and never follows a redirect that changes http to https. Both gaps have broken this build: CATH began answering http with a 301 to https, and ECOD now answers with a 308 to a rewritten path. A browser follows either without comment, so neither service had reason to expect it would break us. openConnectionFollowingRedirects handles what the JDK leaves, resolving relative locations, capping the chain at five hops and reporting a loop rather than chasing it. A redirect from https to http is refused: the transport must never be downgraded silently. Anything refused is returned as it is, so checkHttpStatus still decides what a non-2xx status means. The decision is split into redirectTargetFor(code, location, url) so the rules can be tested without a server; the end-to-end tests use a local HttpServer rather than a real service. Fixes #1149 --- .../nbio/core/util/FileDownloadUtils.java | 127 +++++++++- .../core/util/FileDownloadRedirectTest.java | 226 ++++++++++++++++++ 2 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadRedirectTest.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 5b8c656658..63fee00d57 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 @@ -29,6 +29,7 @@ import java.io.InputStream; import java.io.PrintStream; import java.net.HttpURLConnection; +import java.net.MalformedURLException; import java.net.SocketTimeoutException; import java.net.URL; import java.net.URLConnection; @@ -38,7 +39,9 @@ import java.security.DigestInputStream; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.util.LinkedHashSet; import java.util.Scanner; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -54,6 +57,9 @@ public class FileDownloadUtils { /** Buffer used when streaming a file through a {@link MessageDigest}. */ private static final int DIGEST_BUFFER_SIZE = 64 * 1024; + /** Redirects to follow before giving up, in case a server sends us in a circle. */ + private static final int MAX_REDIRECTS = 5; + /** 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*].*)?$"); @@ -139,8 +145,7 @@ public static void downloadFile(URL url, File destination) throws IOException { try { while (true) { try { - URLConnection connection = prepareURLConnection(url.toString(), timeout); - connection.connect(); + URLConnection connection = openConnectionFollowingRedirects(url, timeout); checkHttpStatus(connection); try (InputStream inputStream = connection.getInputStream()) { // Files.copy loops until end of stream. FileChannel.transferFrom(), used @@ -199,8 +204,7 @@ public static void downloadFileWithValidation(URL url, File destination, URL has File tempFile = createTempFileFor(destination); try { - URLConnection connection = prepareURLConnection(url.toString(), timeout); - connection.connect(); + URLConnection connection = openConnectionFollowingRedirects(url, timeout); checkHttpStatus(connection); long declaredSize = connection.getContentLengthLong(); @@ -252,6 +256,111 @@ public static void downloadFileWithValidation(URL url, File destination, URL has } } + /** + * Opens a connection, following any redirect that {@link HttpURLConnection} + * declines to follow itself. + *

+ * The JDK follows 301, 302 and 303 within a protocol, but it never follows 307 or + * 308, and it never follows a redirect that changes http to https. Both gaps have + * broken downloads in practice: CATH began answering http with a 301 to https, and + * ECOD now answers with a 308 to a rewritten path. A browser follows either without + * comment, so a service making that change has no reason to expect it to break us. + *

+ * A redirect from https to http is deliberately not followed: a redirect + * must never silently downgrade the transport. Such a response is returned as it is, + * for {@link #checkHttpStatus(URLConnection)} to reject. + * + * @param url the URL to open + * @param timeout connect and read timeout, in milliseconds + * @return a connected {@link URLConnection} at the final location + * @throws HttpStatusException if the redirects loop or exceed the limit + * @throws IOException if the connection could not be opened + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static URLConnection openConnectionFollowingRedirects(URL url, int timeout) throws IOException { + Set visited = new LinkedHashSet<>(); + URL current = url; + for (int hop = 0; hop <= MAX_REDIRECTS; hop++) { + if (!visited.add(current.toString())) { + throw new HttpStatusException(HttpURLConnection.HTTP_SEE_OTHER, url.toString(), + "Redirect loop: " + String.join(" -> ", visited)); + } + URLConnection connection = prepareURLConnection(current.toString(), timeout); + connection.connect(); + if (!(connection instanceof HttpURLConnection)) { + return connection; + } + URL next = redirectTarget((HttpURLConnection) connection, current); + if (next == null) { + // either not a redirect, or one we decline to follow; the caller's + // checkHttpStatus decides what a non-2xx status means + return connection; + } + logger.info("{} redirects to {}; following.", current, next); + ((HttpURLConnection) connection).disconnect(); + current = next; + } + throw new HttpStatusException(HttpURLConnection.HTTP_SEE_OTHER, url.toString(), + "More than " + MAX_REDIRECTS + " redirects starting at " + url); + } + + /** + * Works out where a response redirects to, for the redirects the JDK leaves to us. + * + * @param http a connected connection whose status has not yet been acted on + * @param current the URL that was requested, used to resolve a relative location + * @return the redirect target, or null if this is not a redirect we should follow + * @throws IOException if the status could not be read + * @since 7.3.0 + */ + private static URL redirectTarget(HttpURLConnection http, URL current) throws IOException { + return redirectTargetFor(http.getResponseCode(), http.getHeaderField("Location"), current); + } + + /** + * Decides where a response redirects to, given only its status and location. Split + * out from {@link #redirectTarget(HttpURLConnection, URL)} so that the rules can be + * tested without standing up a server. + * + * @param code the HTTP status + * @param location the Location header, may be null, relative or absolute + * @param current the URL that was requested, used to resolve a relative location + * @return the redirect target, or null if this is not a redirect we should follow + * @since 7.3.0 + */ + static URL redirectTargetFor(int code, String location, URL current) { + // 301, 302 and 303 only reach us when the JDK declined them, which it does when + // the protocol changes. 307 and 308 it never follows at all. + boolean redirect = code == HttpURLConnection.HTTP_MOVED_PERM + || code == HttpURLConnection.HTTP_MOVED_TEMP + || code == HttpURLConnection.HTTP_SEE_OTHER + || code == 307 + || code == 308; + if (!redirect) { + return null; + } + if (location == null || location.trim().isEmpty()) { + logger.warn("{} returned {} with no Location header.", current, code); + return null; + } + URL target; + try { + // resolves a relative Location, which is what ECOD sends + target = new URL(current, location.trim()); + } catch (MalformedURLException e) { + logger.warn("{} returned {} to an unusable Location [{}].", current, code, location); + return null; + } + if ("https".equalsIgnoreCase(current.getProtocol()) + && !"https".equalsIgnoreCase(target.getProtocol())) { + logger.warn("Refusing to follow {} from {} to [{}]: a redirect must not downgrade https to {}.", + code, current, target, target.getProtocol()); + return null; + } + return target; + } + /** * Verifies that an HTTP connection returned a 2xx status. Connections using a * non-HTTP protocol (file:, ftp:, ...) are left alone. @@ -277,10 +386,10 @@ public static void checkHttpStatus(URLConnection connection) throws IOException 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).", + // openConnectionFollowingRedirects handles the redirects the JDK will not, + // so one reaching here was declined deliberately: an https to http + // downgrade, a missing or unusable Location, or too many hops. + logger.warn("{} returned redirect {} to [{}], which was not followed.", connection.getURL(), code, http.getHeaderField("Location")); } throw new HttpStatusException(code, connection.getURL().toString(), http.getResponseMessage()); @@ -318,7 +427,7 @@ public static void createValidationFiles(URL url, File localDestination, URL has public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash, ETagPolicy eTagPolicy){ try { - URLConnection resourceConnection = url.openConnection(); + URLConnection resourceConnection = openConnectionFollowingRedirects(url, 60000); createValidationFiles(resourceConnection, localDestination, hashURL, hash, eTagPolicy); } catch (IOException e) { logger.warn("could not open connection to resource file due to exception: {}", e.getMessage()); diff --git a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadRedirectTest.java b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadRedirectTest.java new file mode 100644 index 0000000000..2c46127ffc --- /dev/null +++ b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadRedirectTest.java @@ -0,0 +1,226 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.io.OutputStream; +import java.net.InetSocketAddress; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import com.sun.net.httpserver.HttpServer; + +/** + * Checks that downloads follow the redirects {@link java.net.HttpURLConnection} does + * not follow by itself. + *

+ * The JDK handles 301, 302 and 303 within a protocol, but never 307 or 308, and never + * a redirect that changes http to https. Both gaps have broken this project's builds: + * CATH began answering http with a 301 to https, and ECOD now answers with a 308 to a + * rewritten path. A browser follows either without comment. + *

+ * The rule tests need no network and no server. The end-to-end tests use a local + * {@link HttpServer} rather than a real service, so that they cannot fail because a + * third party is having a bad day. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ +class FileDownloadRedirectTest { + + private static final String PAYLOAD = "the file you were looking for\n"; + + @Nested + class RedirectRules { + + private final URL from = url("http://example.org/ecod/distributions/ecod.latest.domains.txt"); + + @Test + void aRelativeLocationIsResolvedAgainstTheRequest() { + // exactly what ECOD sends: same host, same protocol, relative path + assertEquals(url("http://example.org/ecod-legacy/distributions/ecod.latest.domains.txt"), + FileDownloadUtils.redirectTargetFor(308, + "/ecod-legacy/distributions/ecod.latest.domains.txt", from)); + } + + @Test + void anAbsoluteLocationIsUsedAsGiven() { + assertEquals(url("https://example.org/elsewhere.txt"), + FileDownloadUtils.redirectTargetFor(301, "https://example.org/elsewhere.txt", from)); + } + + @Test + void everyRedirectStatusWeHandleIsRecognised() { + for (int code : new int[] { 301, 302, 303, 307, 308 }) { + assertEquals(url("http://example.org/x"), + FileDownloadUtils.redirectTargetFor(code, "/x", from), + "status " + code + " should be followed"); + } + } + + @Test + void aSuccessIsNotARedirect() { + assertNull(FileDownloadUtils.redirectTargetFor(200, null, from)); + assertNull(FileDownloadUtils.redirectTargetFor(404, "/x", from)); + } + + /** + * A redirect must never quietly move us onto an unencrypted transport. + */ + @Test + void httpsIsNeverDowngradedToHttp() { + URL secure = url("https://example.org/file.txt"); + assertNull(FileDownloadUtils.redirectTargetFor(301, "http://example.org/file.txt", secure)); + assertNull(FileDownloadUtils.redirectTargetFor(308, "http://elsewhere.org/file.txt", secure)); + } + + @Test + void httpToHttpsIsFollowed() { + // the CATH case + assertEquals(url("https://example.org/file.txt"), + FileDownloadUtils.redirectTargetFor(301, "https://example.org/file.txt", + url("http://example.org/file.txt"))); + } + + @Test + void anUnusableLocationIsNotFollowed() { + assertNull(FileDownloadUtils.redirectTargetFor(308, null, from)); + assertNull(FileDownloadUtils.redirectTargetFor(308, " ", from)); + assertNull(FileDownloadUtils.redirectTargetFor(308, "gopher://example.org/x", from)); + } + } + + @Nested + class EndToEnd { + + private HttpServer server; + private String base; + private File dir; + + @BeforeEach + void start() throws IOException { + server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + base = "http://127.0.0.1:" + server.getAddress().getPort(); + dir = Files.createTempDirectory("redirectTest").toFile(); + + serve("/final", 200, null); + // the ECOD shape: 308 with a relative Location + serve("/moved", 308, "/final"); + serve("/temp", 307, "/final"); + // a chain that returns to its start + serve("/loop-a", 308, "/loop-b"); + serve("/loop-b", 308, "/loop-a"); + // longer than the hop limit + for (int i = 0; i < 9; i++) { + serve("/hop" + i, 308, "/hop" + (i + 1)); + } + serve("/hop9", 200, null); + serve("/nowhere", 308, null); + server.start(); + } + + private void serve(String path, int status, String location) { + server.createContext(path, exchange -> { + byte[] body = PAYLOAD.getBytes(StandardCharsets.UTF_8); + if (location != null) { + exchange.getResponseHeaders().add("Location", location); + } + exchange.sendResponseHeaders(status, status == 200 ? body.length : -1); + if (status == 200) { + try (OutputStream out = exchange.getResponseBody()) { + out.write(body); + } + } + exchange.close(); + }); + } + + @AfterEach + void stop() throws IOException { + server.stop(0); + FileDownloadUtils.deleteDirectory(dir.getAbsolutePath()); + } + + @Test + void a308IsFollowed() throws IOException { + File got = new File(dir, "moved.txt"); + FileDownloadUtils.downloadFile(new URL(base + "/moved"), got); + assertEquals(PAYLOAD, new String(Files.readAllBytes(got.toPath()), StandardCharsets.UTF_8)); + } + + @Test + void a307IsFollowed() throws IOException { + File got = new File(dir, "temp.txt"); + FileDownloadUtils.downloadFile(new URL(base + "/temp"), got); + assertEquals(PAYLOAD, new String(Files.readAllBytes(got.toPath()), StandardCharsets.UTF_8)); + } + + @Test + void theRedirectBodyIsNeverWhatWeStore() throws IOException { + File got = new File(dir, "validated.txt"); + FileDownloadUtils.downloadFileWithValidation(new URL(base + "/moved"), got, null, + FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.ETagPolicy.IGNORE); + assertEquals(PAYLOAD, new String(Files.readAllBytes(got.toPath()), StandardCharsets.UTF_8)); + assertTrue(FileDownloadUtils.validateFile(got), "the recorded size must describe the real file"); + } + + @Test + void aLoopIsReportedRatherThanChasedForever() { + File got = new File(dir, "loop.txt"); + HttpStatusException e = assertThrows(HttpStatusException.class, + () -> FileDownloadUtils.downloadFile(new URL(base + "/loop-a"), got)); + assertTrue(e.getMessage().contains("loop"), e.getMessage()); + } + + @Test + void tooManyHopsGivesUp() { + File got = new File(dir, "hops.txt"); + assertThrows(HttpStatusException.class, + () -> FileDownloadUtils.downloadFile(new URL(base + "/hop0"), got)); + } + + @Test + void aRedirectWithNoDestinationIsAnError() { + File got = new File(dir, "nowhere.txt"); + assertThrows(HttpStatusException.class, + () -> FileDownloadUtils.downloadFile(new URL(base + "/nowhere"), got)); + } + } + + private static URL url(String spec) { + try { + return new URL(spec); + } catch (IOException e) { + throw new IllegalArgumentException(spec, e); + } + } +}