Redirection http redirection across different hosts

pditommaso · pditommaso · commit e3778120a603 · 2023-07-29T12:19:13.000+02:00
Signed-off-by: Paolo Di Tommaso &lt;paolo.ditommaso@gmail.com&gt;
diff --git a/modules/nf-httpfs/src/main/nextflow/file/http/XFileSystemProvider.groovy b/modules/nf-httpfs/src/main/nextflow/file/http/XFileSystemProvider.groovy
@@ -64,6 +64,7 @@ abstract class XFileSystemProvider extends FileSystemProvider {
 
     private Map<URI, FileSystem> fileSystemMap = new LinkedHashMap<>(20)
 
+    private static final int[] REDIRECT_CODES = [301, 302, 307, 308]
 
     protected static String config(String name, def defValue) {
         return SysEnv.containsKey(name) ? SysEnv.get(name) : defValue.toString()
@@ -185,18 +186,25 @@ abstract class XFileSystemProvider extends FileSystemProvider {
     protected URLConnection toConnection0(URL url, int attempt) {
         final conn = url.openConnection()
         conn.setRequestProperty("User-Agent", 'Nextflow/httpfs')
+        if( conn instanceof HttpURLConnection ) {
+            // by default HttpURLConnection does redirect only within the same host
+            // disable the built-in to implement custom redirection logic (see below)
+            conn.setInstanceFollowRedirects(false)
+        }
         if( url.userInfo ) {
             conn.setRequestProperty("Authorization", auth(url.userInfo));
         }
         else {
             XAuthRegistry.instance.authorize(conn)
         }
-        if ( conn instanceof HttpURLConnection && conn.getResponseCode() in [307, 308] && attempt < MAX_REDIRECT_HOPS ) {
+        if ( conn instanceof HttpURLConnection && conn.getResponseCode() in REDIRECT_CODES && attempt < MAX_REDIRECT_HOPS ) {
             final header = InsensitiveMap.of(conn.getHeaderFields())
-            String location = header.get("Location")?.get(0)
-            URL newPath = new URI(location).toURL()
-            log.debug "Remote redirect URL: $newPath"
-            return toConnection0(newPath, attempt+1)
+            final location = header.get("Location")?.get(0)
+            final newUrl = new URI(location).toURL()
+            if( url.protocol=='https' && newUrl.protocol=='http' )
+                throw new IOException("Refuse to follow redirection from HTTPS to HTTP (unsafe) URL - origin: $url - target: $newUrl")
+            log.debug "Remote redirect URL: $newUrl"
+            return toConnection0(newUrl, attempt+1)
         }
         else if( conn instanceof HttpURLConnection && conn.getResponseCode() in config().retryCodes() && attempt < config().maxAttempts() ) {
             final delay = (Math.pow(config().backOffBase(), attempt) as long) * config().backOffDelay()
diff --git a/modules/nf-httpfs/src/test/nextflow/file/http/HttpFilesTests.groovy b/modules/nf-httpfs/src/test/nextflow/file/http/HttpFilesTests.groovy
@@ -112,7 +112,7 @@ class HttpFilesTests extends Specification {
         def lines = Files.readAllLines(path, Charset.forName('UTF-8'))
         then:
         lines.size()>0
-        lines[0] == '<html>'
+        lines[0] == '<!DOCTYPE html>'
 
     }
 
diff --git a/modules/nf-httpfs/src/test/nextflow/file/http/XFileSystemProviderTest.groovy b/modules/nf-httpfs/src/test/nextflow/file/http/XFileSystemProviderTest.groovy
@@ -21,7 +21,6 @@ import java.nio.file.Path
 
 import com.github.tomakehurst.wiremock.junit.WireMockRule
 import com.github.tomjankes.wiremock.WireMockGroovy
-import nextflow.SysEnv
 import org.junit.Rule
 import spock.lang.Specification
 import spock.lang.Unroll
@@ -106,7 +105,7 @@ class XFileSystemProviderTest extends Specification {
         when:
         def attrs = fsp.readHttpAttributes(path)
         then:
-        attrs.lastModifiedTime() == null
+        attrs.lastModifiedTime()
         attrs.size() > 0
     }
 
@@ -157,6 +156,7 @@ class XFileSystemProviderTest extends Specification {
     @Rule
     WireMockRule wireMockRule = new WireMockRule(18080)
 
+    @Unroll
     def 'should follow a redirect when read a http file '() {
         given:
         def wireMock = new WireMockGroovy(18080)
@@ -180,14 +180,14 @@ class XFileSystemProviderTest extends Specification {
             response {
                 status HTTP_CODE
                 headers {
-                    "Location" "http://localhost:18080/redirected.html"
+                    "Location" "http://localhost:18080/target.html"
                 }
             }
         }
         wireMock.stub {
             request {
                 method "GET"
-                url "/redirected.html"
+                url "/target.html"
             }
             response {
                 status 200
@@ -212,22 +212,19 @@ class XFileSystemProviderTest extends Specification {
         Files.size(path) == EXPECTED
 
         where:
-        HTTP_CODE | REDIRECT_TO        | EXPECTED
-        300       | "/redirected.html" | 10
-        300       | "/index2.html"     | 10
-
-        301       | "/redirected.html" | 10
-        301       | "/index2.html"     | 10
+        HTTP_CODE | REDIRECT_TO         | EXPECTED
+        301       | "/target.html"      | 10
+        301       | "/index2.html"      | 10
 
-        302       | "/redirected.html" | 10
-        302       | "/index2.html"     | 10
+        302       | "/target.html"      | 10
+        302       | "/index2.html"      | 10
 
-        307       | "/redirected.html" | 10
-        307       | "/index2.html"     | 10
+        307       | "/target.html"      | 10
+        307       | "/index2.html"      | 10
 
-        308       | "/redirected.html" | 10
-        308       | "/index2.html"     | 10
+        308       | "/target.html"      | 10
+        308       | "/index2.html"      | 10
         //infinite redirect to himself
-        308       | "/index.html"      | -1
+        308       | "/index.html"       | -1
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -112,7 +112,7 @@ class HttpFilesTests extends Specification {`
`112`	`112`	`def lines = Files.readAllLines(path, Charset.forName('UTF-8'))`
`113`	`113`	`then:`
`114`	`114`	`lines.size()>0`
`115`		`- lines[0] == '<html>'`
	`115`	`+ lines[0] == '<!DOCTYPE html>'`
`116`	`116`
`117`	`117`	`}`
`118`	`118`