Disable security-related headers on redirect.

TESTED=updated unit tests
Bug: #45410
Change-Id: I7c555a4818fd719d42748b6a18780e3d9b3ee147
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/229947
Reviewed-by: Alexander Aprelev <[email protected]>
Commit-Queue: Brian Quinlan <[email protected]>

CHANGELOG.md

@@ -17,6 +17,13 @@
 - `IdbFactory.supportsDatabaseNames` has been deprecated. It will always return
   `false`.
 
+#### `dart:io`
+
+- **Breaking Change** [#45410](https://github.com/dart-lang/sdk/issues/45410):
+  `HttpClient` no longer transmits some headers (i.e. `authorization`,
+  `www-authenticate`, `cookie`, `cookie2`) when processing redirects to
+  a different domain.
+
 ### Tools
 
 #### Dart command line

sdk/lib/_http/http.dart

@@ -1715,8 +1715,39 @@ abstract class HttpClientRequest implements IOSink {
   /// following the redirect.
   ///
   /// All headers added to the request will be added to the redirection
-  /// request(s). However, any body send with the request will not be
-  /// part of the redirection request(s).
+  /// request(s) except when forwarding sensitive headers like
+  /// "Authorization", "WWW-Authenticate", and "Cookie". Those headers
+  /// will be skipped if following a redirect to a domain that is not a
+  /// subdomain match or exact match of the initial domain.
+  /// For example, a redirect from "foo.com" to either "foo.com" or
+  /// "sub.foo.com" will forward the sensitive headers, but a redirect to
+  /// "bar.com" will not.
+  ///
+  /// Any body send with the request will not be part of the redirection
+  /// request(s).
+  ///
+  /// For precise control of redirect handling, set this property to `false`
+  /// and make a separate HTTP request to process the redirect. For example:
+  ///
+  /// ```dart
+  /// final client = HttpClient();
+  /// var uri = Uri.parse("http://localhost/");
+  /// var request = await client.getUrl(uri);
+  /// request.followRedirects = false;
+  /// var response = await request.close();
+  /// while (response.isRedirect) {
+  ///   response.drain();
+  ///   final location = response.headers.value(HttpHeaders.locationHeader);
+  ///   if (location != null) {
+  ///     uri = uri.resolve(location);
+  ///     request = await client.getUrl(uri);
+  ///     // Set the body or headers as desired.
+  ///     request.followRedirects = false;
+  ///     response = await request.close();
+  ///   }
+  /// }
+  /// // Do something with the final response.
+  /// ```
   bool followRedirects = true;
 
   /// Set this property to the maximum number of redirects to follow

sdk/lib/_http/http_impl.dart

@@ -667,7 +667,7 @@ class _HttpClientResponse extends _HttpInboundMessageListInt
       }
     }
     return _httpClient
-        ._openUrlFromRequest(method, url, _httpRequest)
+        ._openUrlFromRequest(method, url, _httpRequest, isRedirect: true)
         .then((request) {
       request._responseRedirects
         ..addAll(redirects)
@@ -751,7 +751,8 @@ class _HttpClientResponse extends _HttpInboundMessageListInt
       return drain().then((_) {
         return _httpClient
             ._openUrlFromRequest(
-                _httpRequest.method, _httpRequest.uri, _httpRequest)
+                _httpRequest.method, _httpRequest.uri, _httpRequest,
+                isRedirect: false)
             .then((request) => request.close());
       });
     }
@@ -2715,8 +2716,31 @@ class _HttpClient implements HttpClient {
     });
   }
 
+  static bool _isSubdomain(Uri subdomain, Uri domain) {
+    return (subdomain.scheme == domain.scheme &&
+        subdomain.port == domain.port &&
+        (subdomain.host == domain.host ||
+            subdomain.host.endsWith("." + domain.host)));
+  }
+
+  static bool _shouldCopyHeaderOnRedirect(
+      String headerKey, Uri originalUrl, Uri redirectUri) {
+    if (_isSubdomain(redirectUri, originalUrl)) {
+      return true;
+    }
+
+    const nonRedirectHeaders = [
+      "authorization",
+      "www-authenticate",
+      "cookie",
+      "cookie2"
+    ];
+    return !nonRedirectHeaders.contains(headerKey.toLowerCase());
+  }
+
   Future<_HttpClientRequest> _openUrlFromRequest(
-      String method, Uri uri, _HttpClientRequest previous) {
+      String method, Uri uri, _HttpClientRequest previous,
+      {required bool isRedirect}) {
     // If the new URI is relative (to either '/' or some sub-path),
     // construct a full URI from the previous one.
     Uri resolved = previous.uri.resolveUri(uri);
@@ -2728,7 +2752,9 @@ class _HttpClient implements HttpClient {
         ..maxRedirects = previous.maxRedirects;
       // Copy headers.
       for (var header in previous.headers._headers.keys) {
-        if (request.headers[header] == null) {
+        if (request.headers[header] == null &&
+            (!isRedirect ||
+                _shouldCopyHeaderOnRedirect(header, resolved, previous.uri))) {
           request.headers.set(header, previous.headers[header]!);
         }
       }

tests/standalone/io/http_redirect_test.dart

@@ -7,7 +7,7 @@ import "package:expect/expect.dart";
 import "dart:async";
 import "dart:io";
 
-Future<HttpServer> setupServer() {
+Future<HttpServer> setupServer({Uri? targetServer}) {
   final completer = new Completer<HttpServer>();
   HttpServer.bind("127.0.0.1", 0).then((server) {
     var handlers = new Map<String, Function>();
@@ -128,16 +128,33 @@ Future<HttpServer> setupServer() {
     // Setup redirect checking headers.
     addRequestHandler("/src", (HttpRequest request, HttpResponse response) {
       Expect.equals("value", request.headers.value("X-Request-Header"));
+      Expect.isNotNull(request.headers.value("Authorization"),
+          "expected 'Authorization' header to be set");
       response.headers.set(
           HttpHeaders.locationHeader, "http://127.0.0.1:${server.port}/target");
       response.statusCode = HttpStatus.movedPermanently;
       response.close();
     });
     addRequestHandler("/target", (HttpRequest request, HttpResponse response) {
       Expect.equals("value", request.headers.value("X-Request-Header"));
+      Expect.isNotNull(request.headers.value("Authorization"),
+          "expected 'Authorization' header to be set");
       response.close();
     });
 
+    if (targetServer != null) {
+      addRequestHandler("/src-crossdomain",
+          (HttpRequest request, HttpResponse response) {
+        Expect.equals("value", request.headers.value("X-Request-Header"));
+        Expect.isNotNull(request.headers.value("Authorization"),
+            "expected 'Authorization' header to be set");
+        response.headers
+            .set(HttpHeaders.locationHeader, targetServer.toString());
+        response.statusCode = HttpStatus.movedPermanently;
+        response.close();
+      });
+    }
+
     // Setup redirect for 301 where POST should not redirect.
     addRequestHandler("/301src", (HttpRequest request, HttpResponse response) {
       Expect.equals("POST", request.method);
@@ -183,6 +200,36 @@ Future<HttpServer> setupServer() {
   return completer.future;
 }
 
+// A second HTTP server used to validate that redirect requests accross domains
+// do *not* include security-related headers.
+Future<HttpServer> setupTargetServer() {
+  final completer = new Completer<HttpServer>();
+  HttpServer.bind("127.0.0.1", 0).then((server) {
+    var handlers = new Map<String, Function>();
+    addRequestHandler(
+        String path, void handler(HttpRequest request, HttpResponse response)) {
+      handlers[path] = handler;
+    }
+
+    server.listen((HttpRequest request) {
+      if (request.uri.path == "/target") {
+        Expect.equals("value", request.headers.value("X-Request-Header"));
+        Expect.isNull(request.headers.value("Authorization"),
+            "expected 'Authorization' header to be removed on redirect");
+        request.response.close();
+      } else {
+        request.listen((_) {}, onDone: () {
+          request.response.statusCode = 404;
+          request.response.close();
+        });
+      }
+    });
+
+    completer.complete(server);
+  });
+  return completer.future;
+}
+
 void checkRedirects(int redirectCount, HttpClientResponse response) {
   if (redirectCount < 2) {
     Expect.isTrue(response.redirects.isEmpty);
@@ -250,6 +297,7 @@ void testManualRedirectWithHeaders() {
         .then((HttpClientRequest request) {
       request.followRedirects = false;
       request.headers.add("X-Request-Header", "value");
+      request.headers.add("Authorization", "Basic ...");
       return request.close();
     }).then(handleResponse);
   });
@@ -282,6 +330,7 @@ void testAutoRedirectWithHeaders() {
         .getUrl(Uri.parse("http://127.0.0.1:${server.port}/src"))
         .then((HttpClientRequest request) {
       request.headers.add("X-Request-Header", "value");
+      request.headers.add("Authorization", "Basic ...");
       return request.close();
     }).then((HttpClientResponse response) {
       response.listen((_) => Expect.fail("Response data not expected"),
@@ -294,6 +343,33 @@ void testAutoRedirectWithHeaders() {
   });
 }
 
+void testCrossDomainAutoRedirectWithHeaders() {
+  setupTargetServer().then((targetServer) {
+    setupServer(
+            targetServer:
+                Uri.parse("http://127.0.0.1:${targetServer.port}/target"))
+        .then((server) {
+      HttpClient client = new HttpClient();
+
+      client
+          .getUrl(Uri.parse("http://127.0.0.1:${server.port}/src-crossdomain"))
+          .then((HttpClientRequest request) {
+        request.headers.add("X-Request-Header", "value");
+        request.headers.add("Authorization", "Basic ...");
+        return request.close();
+      }).then((HttpClientResponse response) {
+        response.listen((_) => Expect.fail("Response data not expected"),
+            onDone: () {
+          Expect.equals(1, response.redirects.length);
+          targetServer.close();
+          server.close();
+          client.close();
+        });
+      });
+    });
+  });
+}
+
 void testAutoRedirect301POST() {
   setupServer().then((server) {
     HttpClient client = new HttpClient();
@@ -441,6 +517,7 @@ main() {
   testManualRedirectWithHeaders();
   testAutoRedirect();
   testAutoRedirectWithHeaders();
+  testCrossDomainAutoRedirectWithHeaders();
   testAutoRedirect301POST();
   testAutoRedirect303POST();
   testAutoRedirectLimit();

tests/standalone_2/io/http_redirect_test.dart

@@ -9,7 +9,7 @@ import "package:expect/expect.dart";
 import "dart:async";
 import "dart:io";
 
-Future<HttpServer> setupServer() {
+Future<HttpServer> setupServer({Uri targetServer}) {
   Completer completer = new Completer<HttpServer>();
   HttpServer.bind("127.0.0.1", 0).then((server) {
     var handlers = new Map<String, Function>();
@@ -130,16 +130,33 @@ Future<HttpServer> setupServer() {
     // Setup redirect checking headers.
     addRequestHandler("/src", (HttpRequest request, HttpResponse response) {
       Expect.equals("value", request.headers.value("X-Request-Header"));
+      Expect.isNotNull(request.headers.value("Authorization"),
+          "expected 'Authorization' header to be set");
       response.headers.set(
           HttpHeaders.locationHeader, "http://127.0.0.1:${server.port}/target");
       response.statusCode = HttpStatus.movedPermanently;
       response.close();
     });
     addRequestHandler("/target", (HttpRequest request, HttpResponse response) {
       Expect.equals("value", request.headers.value("X-Request-Header"));
+      Expect.isNotNull(request.headers.value("Authorization"),
+          "expected 'Authorization' header to be set");
       response.close();
     });
 
+    if (targetServer != null) {
+      addRequestHandler("/src-crossdomain",
+          (HttpRequest request, HttpResponse response) {
+        Expect.equals("value", request.headers.value("X-Request-Header"));
+        Expect.isNotNull(request.headers.value("Authorization"),
+            "expected 'Authorization' header to be set");
+        response.headers
+            .set(HttpHeaders.locationHeader, targetServer.toString());
+        response.statusCode = HttpStatus.movedPermanently;
+        response.close();
+      });
+    }
+
     // Setup redirect for 301 where POST should not redirect.
     addRequestHandler("/301src", (HttpRequest request, HttpResponse response) {
       Expect.equals("POST", request.method);
@@ -185,6 +202,36 @@ Future<HttpServer> setupServer() {
   return completer.future;
 }
 
+// A second HTTP server used to validate that redirect requests accross domains
+// do *not* include security-related headers.
+Future<HttpServer> setupTargetServer() {
+  Completer completer = new Completer<HttpServer>();
+  HttpServer.bind("127.0.0.1", 0).then((server) {
+    var handlers = new Map<String, Function>();
+    addRequestHandler(
+        String path, void handler(HttpRequest request, HttpResponse response)) {
+      handlers[path] = handler;
+    }
+
+    server.listen((HttpRequest request) {
+      if (request.uri.path == "/target") {
+        Expect.equals("value", request.headers.value("X-Request-Header"));
+        Expect.isNull(request.headers.value("Authorization"),
+            "expected 'Authorization' header to be removed on redirect");
+        request.response.close();
+      } else {
+        request.listen((_) {}, onDone: () {
+          request.response.statusCode = 404;
+          request.response.close();
+        });
+      }
+    });
+
+    completer.complete(server);
+  });
+  return completer.future;
+}
+
 void checkRedirects(int redirectCount, HttpClientResponse response) {
   if (redirectCount < 2) {
     Expect.isTrue(response.redirects.isEmpty);
@@ -252,6 +299,7 @@ void testManualRedirectWithHeaders() {
         .then((HttpClientRequest request) {
       request.followRedirects = false;
       request.headers.add("X-Request-Header", "value");
+      request.headers.add("Authorization", "Basic ...");
       return request.close();
     }).then(handleResponse);
   });
@@ -284,6 +332,7 @@ void testAutoRedirectWithHeaders() {
         .getUrl(Uri.parse("http://127.0.0.1:${server.port}/src"))
         .then((HttpClientRequest request) {
       request.headers.add("X-Request-Header", "value");
+      request.headers.add("Authorization", "Basic ...");
       return request.close();
     }).then((HttpClientResponse response) {
       response.listen((_) => Expect.fail("Response data not expected"),
@@ -296,6 +345,33 @@ void testAutoRedirectWithHeaders() {
   });
 }
 
+void testCrossDomainAutoRedirectWithHeaders() {
+  setupTargetServer().then((targetServer) {
+    setupServer(
+            targetServer:
+                Uri.parse("http://127.0.0.1:${targetServer.port}/target"))
+        .then((server) {
+      HttpClient client = new HttpClient();
+
+      client
+          .getUrl(Uri.parse("http://127.0.0.1:${server.port}/src-crossdomain"))
+          .then((HttpClientRequest request) {
+        request.headers.add("X-Request-Header", "value");
+        request.headers.add("Authorization", "Basic ...");
+        return request.close();
+      }).then((HttpClientResponse response) {
+        response.listen((_) => Expect.fail("Response data not expected"),
+            onDone: () {
+          Expect.equals(1, response.redirects.length);
+          targetServer.close();
+          server.close();
+          client.close();
+        });
+      });
+    });
+  });
+}
+
 void testAutoRedirect301POST() {
   setupServer().then((server) {
     HttpClient client = new HttpClient();
@@ -443,6 +519,7 @@ main() {
   testManualRedirectWithHeaders();
   testAutoRedirect();
   testAutoRedirectWithHeaders();
+  testCrossDomainAutoRedirectWithHeaders();
   testAutoRedirect301POST();
   testAutoRedirect303POST();
   testAutoRedirectLimit();