Skip to content

Commit a9dd08f

Browse files
josemduartejosemduarte
authored
Merge pull request #1151 from aalhossary/aa/follow-redirects
Follow the redirects HttpURLConnection declines (307/308, cross-protocol)
2 parents 4a5bfc8 + ce0f394 commit a9dd08f

2 files changed

Lines changed: 344 additions & 9 deletions

File tree

‎biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java‎

Lines changed: 118 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.io.InputStream;
3030
import java.io.PrintStream;
3131
import java.net.HttpURLConnection;
32+
import java.net.MalformedURLException;
3233
import java.net.SocketTimeoutException;
3334
import java.net.URL;
3435
import java.net.URLConnection;
@@ -38,7 +39,9 @@
3839
import java.security.DigestInputStream;
3940
import java.security.MessageDigest;
4041
import java.security.NoSuchAlgorithmException;
42+
import java.util.LinkedHashSet;
4143
import java.util.Scanner;
44+
import java.util.Set;
4245
import java.util.regex.Matcher;
4346
import java.util.regex.Pattern;
4447

@@ -54,6 +57,9 @@ public class FileDownloadUtils {
5457
/** Buffer used when streaming a file through a {@link MessageDigest}. */
5558
private static final int DIGEST_BUFFER_SIZE = 64 * 1024;
5659

60+
/** Redirects to follow before giving up, in case a server sends us in a circle. */
61+
private static final int MAX_REDIRECTS = 5;
62+
5763
/** A bare hex digest, optionally followed by whitespace and a file name (the
5864
* layout written by <code>md5sum</code>, <code>sha1sum</code> and friends). */
5965
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 {
139145
try {
140146
while (true) {
141147
try {
142-
URLConnection connection = prepareURLConnection(url.toString(), timeout);
143-
connection.connect();
148+
URLConnection connection = openConnectionFollowingRedirects(url, timeout);
144149
checkHttpStatus(connection);
145150
try (InputStream inputStream = connection.getInputStream()) {
146151
// Files.copy loops until end of stream. FileChannel.transferFrom(), used
@@ -199,8 +204,7 @@ public static void downloadFileWithValidation(URL url, File destination, URL has
199204

200205
File tempFile = createTempFileFor(destination);
201206
try {
202-
URLConnection connection = prepareURLConnection(url.toString(), timeout);
203-
connection.connect();
207+
URLConnection connection = openConnectionFollowingRedirects(url, timeout);
204208
checkHttpStatus(connection);
205209

206210
long declaredSize = connection.getContentLengthLong();
@@ -252,6 +256,111 @@ public static void downloadFileWithValidation(URL url, File destination, URL has
252256
}
253257
}
254258

259+
/**
260+
* Opens a connection, following any redirect that {@link HttpURLConnection}
261+
* declines to follow itself.
262+
* <p>
263+
* The JDK follows 301, 302 and 303 within a protocol, but it never follows 307 or
264+
* 308, and it never follows a redirect that changes http to https. Both gaps have
265+
* broken downloads in practice: CATH began answering http with a 301 to https, and
266+
* ECOD now answers with a 308 to a rewritten path. A browser follows either without
267+
* comment, so a service making that change has no reason to expect it to break us.
268+
* <p>
269+
* A redirect from https to http is deliberately <em>not</em> followed: a redirect
270+
* must never silently downgrade the transport. Such a response is returned as it is,
271+
* for {@link #checkHttpStatus(URLConnection)} to reject.
272+
*
273+
* @param url the URL to open
274+
* @param timeout connect and read timeout, in milliseconds
275+
* @return a connected {@link URLConnection} at the final location
276+
* @throws HttpStatusException if the redirects loop or exceed the limit
277+
* @throws IOException if the connection could not be opened
278+
* @author Amr ALHOSSARY
279+
* @since 7.3.0
280+
*/
281+
public static URLConnection openConnectionFollowingRedirects(URL url, int timeout) throws IOException {
282+
Set<String> visited = new LinkedHashSet<>();
283+
URL current = url;
284+
for (int hop = 0; hop <= MAX_REDIRECTS; hop++) {
285+
if (!visited.add(current.toString())) {
286+
throw new HttpStatusException(HttpURLConnection.HTTP_SEE_OTHER, url.toString(),
287+
"Redirect loop: " + String.join(" -> ", visited));
288+
}
289+
URLConnection connection = prepareURLConnection(current.toString(), timeout);
290+
connection.connect();
291+
if (!(connection instanceof HttpURLConnection)) {
292+
return connection;
293+
}
294+
URL next = redirectTarget((HttpURLConnection) connection, current);
295+
if (next == null) {
296+
// either not a redirect, or one we decline to follow; the caller's
297+
// checkHttpStatus decides what a non-2xx status means
298+
return connection;
299+
}
300+
logger.info("{} redirects to {}; following.", current, next);
301+
((HttpURLConnection) connection).disconnect();
302+
current = next;
303+
}
304+
throw new HttpStatusException(HttpURLConnection.HTTP_SEE_OTHER, url.toString(),
305+
"More than " + MAX_REDIRECTS + " redirects starting at " + url);
306+
}
307+
308+
/**
309+
* Works out where a response redirects to, for the redirects the JDK leaves to us.
310+
*
311+
* @param http a connected connection whose status has not yet been acted on
312+
* @param current the URL that was requested, used to resolve a relative location
313+
* @return the redirect target, or null if this is not a redirect we should follow
314+
* @throws IOException if the status could not be read
315+
* @since 7.3.0
316+
*/
317+
private static URL redirectTarget(HttpURLConnection http, URL current) throws IOException {
318+
return redirectTargetFor(http.getResponseCode(), http.getHeaderField("Location"), current);
319+
}
320+
321+
/**
322+
* Decides where a response redirects to, given only its status and location. Split
323+
* out from {@link #redirectTarget(HttpURLConnection, URL)} so that the rules can be
324+
* tested without standing up a server.
325+
*
326+
* @param code the HTTP status
327+
* @param location the Location header, may be null, relative or absolute
328+
* @param current the URL that was requested, used to resolve a relative location
329+
* @return the redirect target, or null if this is not a redirect we should follow
330+
* @since 7.3.0
331+
*/
332+
static URL redirectTargetFor(int code, String location, URL current) {
333+
// 301, 302 and 303 only reach us when the JDK declined them, which it does when
334+
// the protocol changes. 307 and 308 it never follows at all.
335+
boolean redirect = code == HttpURLConnection.HTTP_MOVED_PERM
336+
|| code == HttpURLConnection.HTTP_MOVED_TEMP
337+
|| code == HttpURLConnection.HTTP_SEE_OTHER
338+
|| code == 307
339+
|| code == 308;
340+
if (!redirect) {
341+
return null;
342+
}
343+
if (location == null || location.trim().isEmpty()) {
344+
logger.warn("{} returned {} with no Location header.", current, code);
345+
return null;
346+
}
347+
URL target;
348+
try {
349+
// resolves a relative Location, which is what ECOD sends
350+
target = new URL(current, location.trim());
351+
} catch (MalformedURLException e) {
352+
logger.warn("{} returned {} to an unusable Location [{}].", current, code, location);
353+
return null;
354+
}
355+
if ("https".equalsIgnoreCase(current.getProtocol())
356+
&& !"https".equalsIgnoreCase(target.getProtocol())) {
357+
logger.warn("Refusing to follow {} from {} to [{}]: a redirect must not downgrade https to {}.",
358+
code, current, target, target.getProtocol());
359+
return null;
360+
}
361+
return target;
362+
}
363+
255364
/**
256365
* Verifies that an HTTP connection returned a 2xx status. Connections using a
257366
* non-HTTP protocol (<code>file:</code>, <code>ftp:</code>, ...) are left alone.
@@ -277,10 +386,10 @@ public static void checkHttpStatus(URLConnection connection) throws IOException
277386
return;
278387
}
279388
if (code == 301 || code == 302 || code == 307 || code == 308) {
280-
// The JDK follows redirects automatically, but never across protocols, so
281-
// an http -> https redirect surfaces here and is worth naming explicitly.
282-
logger.warn("{} returned redirect {} to [{}], which was not followed "
283-
+ "(the JDK does not follow redirects that change protocol).",
389+
// openConnectionFollowingRedirects handles the redirects the JDK will not,
390+
// so one reaching here was declined deliberately: an https to http
391+
// downgrade, a missing or unusable Location, or too many hops.
392+
logger.warn("{} returned redirect {} to [{}], which was not followed.",
284393
connection.getURL(), code, http.getHeaderField("Location"));
285394
}
286395
throw new HttpStatusException(code, connection.getURL().toString(), http.getResponseMessage());
@@ -318,7 +427,7 @@ public static void createValidationFiles(URL url, File localDestination, URL has
318427
public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash,
319428
ETagPolicy eTagPolicy){
320429
try {
321-
URLConnection resourceConnection = url.openConnection();
430+
URLConnection resourceConnection = openConnectionFollowingRedirects(url, 60000);
322431
createValidationFiles(resourceConnection, localDestination, hashURL, hash, eTagPolicy);
323432
} catch (IOException e) {
324433
logger.warn("could not open connection to resource file due to exception: {}", e.getMessage());

0 commit comments

Comments
 (0)