Bug 1667471 - Add header filter to GeckoSession.loader. r=snorp,esawin

This allows apps to modify the header filtering behavior, and introduces a safe
default (CORS safelisted).

Deprecated `loadUri` methods still maintain the old behavior so that we don't
inadvertently introduce bugs in apps.

Differential Revision: https://phabricator.services.mozilla.com/D91983

mobile/android/geckoview/api.txt

@@ -777,6 +777,8 @@ package org.mozilla.geckoview {
     field public static final int FINDER_FIND_LINKS_ONLY = 8;
     field public static final int FINDER_FIND_MATCH_CASE = 2;
     field public static final int FINDER_FIND_WHOLE_WORD = 4;
+    field public static final int HEADER_FILTER_CORS_SAFELISTED = 1;
+    field public static final int HEADER_FILTER_UNRESTRICTED_UNSAFE = 2;
     field public static final int LOAD_FLAGS_ALLOW_POPUPS = 8;
     field public static final int LOAD_FLAGS_BYPASS_CACHE = 1;
     field public static final int LOAD_FLAGS_BYPASS_CLASSIFIER = 16;
@@ -859,6 +861,7 @@ package org.mozilla.geckoview {
     method @NonNull public GeckoSession.Loader data(@NonNull byte[], @Nullable String);
     method @NonNull public GeckoSession.Loader data(@NonNull String, @Nullable String);
     method @NonNull public GeckoSession.Loader flags(int);
+    method @NonNull public GeckoSession.Loader headerFilter(int);
     method @NonNull public GeckoSession.Loader referrer(@NonNull GeckoSession);
     method @NonNull public GeckoSession.Loader referrer(@NonNull Uri);
     method @NonNull public GeckoSession.Loader referrer(@NonNull String);

mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt

@@ -1372,7 +1372,8 @@ class NavigationDelegateTest : BaseSessionTest() {
     }
 
     private fun loadUriHeaderTest(headers: Map<String?,String?>,
-                                  additional: Map<String?, String?>) {
+                                  additional: Map<String?, String?>,
+                                  filter: Int = GeckoSession.HEADER_FILTER_CORS_SAFELISTED) {
         // First collect default headers with no override
         sessionRule.session.loadUri("$TEST_ENDPOINT/anything")
         sessionRule.session.waitForPageStop()
@@ -1381,26 +1382,40 @@ class NavigationDelegateTest : BaseSessionTest() {
         val defaultBody = JSONObject(defaultContent)
         val defaultHeaders = defaultBody.getJSONObject("headers").asMap<String>()
 
-        val expected = defaultHeaders.plus(additional)
+        val expected = HashMap(additional)
+        for (key in defaultHeaders.keys) {
+            expected[key] = defaultHeaders[key]
+            if (additional.containsKey(key)) {
+                // TODO: Bug 1671294, headers should be replaced, not appended
+                expected[key] += ", " + additional[key]
+            }
+        }
 
         // Now load the page with the header override
         sessionRule.session.load(Loader()
             .uri("$TEST_ENDPOINT/anything")
-            .additionalHeaders(headers))
+            .additionalHeaders(headers)
+            .headerFilter(filter))
         sessionRule.session.waitForPageStop()
 
         val content = sessionRule.session.evaluateJS("document.body.children[0].innerHTML") as String
         val body = JSONObject(content)
         val actualHeaders = body.getJSONObject("headers").asMap<String>()
 
-        assertThat("Headers should match", actualHeaders, equalTo(expected))
+        assertThat("Headers should match", expected as Map<String?, String?>,
+                equalTo(actualHeaders))
     }
 
     @Test fun loadUriHeader() {
         // Basic test
         loadUriHeaderTest(
                 mapOf("Header1" to "Value", "Header2" to "Value1, Value2"),
-                mapOf("Header1" to "Value", "Header2" to "Value1, Value2")
+                mapOf()
+        )
+        loadUriHeaderTest(
+                mapOf("Header1" to "Value", "Header2" to "Value1, Value2"),
+                mapOf("Header1" to "Value", "Header2" to "Value1, Value2"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         // Empty value headers are ignored
@@ -1426,16 +1441,34 @@ class NavigationDelegateTest : BaseSessionTest() {
                       "what" to "what\r\nhost:amazon.com",
                       "Header3" to "Value1, Value2, Value3"
                 ),
+                mapOf()
+        )
+        loadUriHeaderTest(
                 mapOf("Header1" to "Value",
-                      "Header2" to "Value1, Value2",
-                      "Header3" to "Value1, Value2, Value3")
+                        "Header2" to "Value1, Value2",
+                        "this\r\nis invalid" to "test value",
+                        "test key" to "this\r\n is a no-no",
+                        "what" to "what\r\nhost:amazon.com",
+                        "Header3" to "Value1, Value2, Value3"
+                ),
+                mapOf("Header1" to "Value",
+                        "Header2" to "Value1, Value2",
+                        "Header3" to "Value1, Value2, Value3"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
+        loadUriHeaderTest(
+                mapOf("Header1" to "Value",
+                        "Header2" to "Value1, Value2",
+                        "what" to "what\r\nhost:amazon.com"),
+                mapOf()
+        )
         loadUriHeaderTest(
                 mapOf("Header1" to "Value",
                       "Header2" to "Value1, Value2",
                       "what" to "what\r\nhost:amazon.com"),
-                mapOf("Header1" to "Value", "Header2" to "Value1, Value2")
+                mapOf("Header1" to "Value", "Header2" to "Value1, Value2"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         loadUriHeaderTest(
@@ -1451,22 +1484,42 @@ class NavigationDelegateTest : BaseSessionTest() {
         // Connection and Host cannot be overriden, no matter the case spelling
         loadUriHeaderTest(
                 mapOf("Header1" to "Value1", "ConnEction" to "test", "connection" to "test2"),
-                mapOf("Header1" to "Value1")
+                mapOf()
+        )
+        loadUriHeaderTest(
+                mapOf("Header1" to "Value1", "ConnEction" to "test", "connection" to "test2"),
+                mapOf("Header1" to "Value1"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         loadUriHeaderTest(
                 mapOf("Header1" to "Value1", "connection" to "test2"),
-                mapOf("Header1" to "Value1")
+                mapOf()
+        )
+        loadUriHeaderTest(
+                mapOf("Header1" to "Value1", "connection" to "test2"),
+                mapOf("Header1" to "Value1"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         loadUriHeaderTest(
                 mapOf("Header1   " to "Value1", "host" to "test2"),
-                mapOf("Header1" to "Value1")
+                mapOf()
+        )
+        loadUriHeaderTest(
+                mapOf("Header1   " to "Value1", "host" to "test2"),
+                mapOf("Header1" to "Value1"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         loadUriHeaderTest(
                 mapOf("Header1" to "Value1", "host" to "test2"),
-                mapOf("Header1" to "Value1")
+                mapOf()
+        )
+        loadUriHeaderTest(
+                mapOf("Header1" to "Value1", "host" to "test2"),
+                mapOf("Header1" to "Value1"),
+                GeckoSession.HEADER_FILTER_UNRESTRICTED_UNSAFE
         )
 
         // Adding white space at the end of a forbidden header still prevents override
@@ -1483,6 +1536,31 @@ class NavigationDelegateTest : BaseSessionTest() {
                 mapOf("abc\ra\n" to "amazon.com"),
                 mapOf()
         )
+
+        // CORS Safelist test
+        loadUriHeaderTest(
+                mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5",
+                      "Accept" to "text/html",
+                      "Content-Language" to "de-DE, en-CA",
+                      "Content-Type" to "multipart/form-data; boundary=something"),
+                mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5",
+                      "Accept" to "text/html",
+                      "Content-Language" to "de-DE, en-CA",
+                      "Content-Type" to "multipart/form-data; boundary=something"),
+                GeckoSession.HEADER_FILTER_CORS_SAFELISTED
+        )
+
+        // CORS safelist doesn't allow Content-type image/svg
+        loadUriHeaderTest(
+                mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5",
+                      "Accept" to "text/html",
+                      "Content-Language" to "de-DE, en-CA",
+                      "Content-Type" to "image/svg; boundary=something"),
+                mapOf("Accept-Language" to "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5",
+                      "Accept" to "text/html",
+                      "Content-Language" to "de-DE, en-CA"),
+                GeckoSession.HEADER_FILTER_CORS_SAFELISTED
+        )
     }
 
     @Test(expected = GeckoResult.UncaughtException::class)

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java

@@ -1460,6 +1460,32 @@ public class GeckoSession {
      */
     public static final int LOAD_FLAGS_REPLACE_HISTORY = 1 << 6;
 
+    /**
+     * Filter headers according to the CORS safelisted rules.
+     *
+     * See <a href="https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header">
+     *  CORS-safelisted request header
+     * </a>.
+     */
+    public static final int HEADER_FILTER_CORS_SAFELISTED = 1;
+    /**
+     * Allows most headers.
+     *
+     * Note: the <code>Host</code> and <code>Connection</code>
+     * headers are still ignored.
+     *
+     * This should only be used when input is hard-coded from the app or when
+     * properly sanitized, as some headers could cause unexpected consequences
+     * and security issues.
+     *
+     * Only use this if you know what you're doing.
+     */
+    public static final int HEADER_FILTER_UNRESTRICTED_UNSAFE = 2;
+
+    @Retention(RetentionPolicy.SOURCE)
+    @IntDef(value = {HEADER_FILTER_CORS_SAFELISTED, HEADER_FILTER_UNRESTRICTED_UNSAFE})
+            /* package */ @interface HeaderFilter {}
+
     /**
      * Main entry point for loading URIs into a {@link GeckoSession}.
      *
@@ -1502,6 +1528,7 @@ public class GeckoSession {
         private GeckoBundle mHeaders;
         private @LoadFlags int mLoadFlags = LOAD_FLAGS_NONE;
         private boolean mIsDataUri;
+        private @HeaderFilter int mHeaderFilter = HEADER_FILTER_CORS_SAFELISTED;
 
         private static @NonNull String createDataUri(@NonNull final byte[] bytes,
                                                     @Nullable final String mimeType) {
@@ -1603,8 +1630,12 @@ public class GeckoSession {
         /**
          * Add headers for this load.
          *
-         * Note: the <code>Host</code> and <code>Connection</code>
-         * headers are ignored.
+         * Note: only CORS safelisted headers are allowed by default. To modify this
+         * behavior use {@link #headerFilter}.
+         *
+         * See <a href="https://developer.mozilla.org/en-US/docs/Glossary/CORS-safelisted_request_header">
+         *  CORS-safelisted request header
+         * </a>.
          *
          * @param headers a <code>Map</code> containing headers that will
          *                be added to this load.
@@ -1624,6 +1655,20 @@ public class GeckoSession {
             return this;
         }
 
+        /**
+         * Modify the header filter behavior. By default only CORS safelisted headers are allowed.
+         *
+         * @param filter one of the
+         *               {@link GeckoSession#HEADER_FILTER_CORS_SAFELISTED HEADER_FILTER_*}
+         *               constants.
+         * @return this {@link Loader} instance.
+         */
+        @NonNull
+        public Loader headerFilter(final @HeaderFilter int filter) {
+            mHeaderFilter = filter;
+            return this;
+        }
+
         /**
          * Set the load flags for this load.
          * @param flags the load flags to use, an OR-ed value of
@@ -1679,6 +1724,7 @@ public class GeckoSession {
             final GeckoBundle msg = new GeckoBundle();
             msg.putString("uri", request.mUri);
             msg.putInt("flags", loadFlags);
+            msg.putInt("headerFilter", request.mHeaderFilter);
 
             if (request.mReferrerUri != null) {
                 msg.putString("referrerUri", request.mReferrerUri);
@@ -1721,6 +1767,7 @@ public class GeckoSession {
     public void loadUri(final @NonNull String uri, final @Nullable Map<String, String> additionalHeaders) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .additionalHeaders(additionalHeaders));
     }
 
@@ -1736,6 +1783,7 @@ public class GeckoSession {
     public void loadUri(final @NonNull String uri, final @LoadFlags int flags) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .flags(flags));
     }
 
@@ -1753,6 +1801,7 @@ public class GeckoSession {
                         final @LoadFlags int flags) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .flags(flags));
     }
@@ -1772,6 +1821,7 @@ public class GeckoSession {
                         final @LoadFlags int flags, final @Nullable Map<String, String> additionalHeaders) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .flags(flags)
                 .additionalHeaders(additionalHeaders));
@@ -1793,6 +1843,7 @@ public class GeckoSession {
                         final @LoadFlags int flags) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .flags(flags));
     }
@@ -1814,6 +1865,7 @@ public class GeckoSession {
                         final @LoadFlags int flags, final @Nullable Map<String, String> additionalHeaders) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .additionalHeaders(additionalHeaders)
                 .flags(flags));
@@ -1853,6 +1905,7 @@ public class GeckoSession {
     @Deprecated
     public void loadUri(final @NonNull Uri uri) {
         load(new Loader()
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .uri(uri));
     }
 
@@ -1867,6 +1920,7 @@ public class GeckoSession {
     public void loadUri(final @NonNull Uri uri, final @Nullable Map<String, String> additionalHeaders) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .additionalHeaders(additionalHeaders));
     }
 
@@ -1881,6 +1935,7 @@ public class GeckoSession {
     public void loadUri(final @NonNull Uri uri, final @LoadFlags int flags) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .flags(flags));
     }
 
@@ -1897,6 +1952,7 @@ public class GeckoSession {
                         final @LoadFlags int flags) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .flags(flags));
     }
@@ -1915,6 +1971,7 @@ public class GeckoSession {
                         final @LoadFlags int flags, final @Nullable Map<String, String> additionalHeaders) {
         load(new Loader()
                 .uri(uri)
+                .headerFilter(HEADER_FILTER_UNRESTRICTED_UNSAFE)
                 .referrer(referrer)
                 .flags(flags)
                 .additionalHeaders(additionalHeaders));

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md

@@ -34,6 +34,9 @@ exclude: true
 - ⚠️ Deprecated [`GeckoSession#loadUri`][83.6] variants in favor of
   [`GeckoSession#load`][83.7]. See docs for [`Loader`][83.8].
   ([bug 1667471]({{bugzilla}}1667471))
+- Added [`Loader#headerFilter`][83.9] to override the default header filtering
+  behavior.
+  ([bug 1667471]({{bugzilla}}1667471))
 
 [83.1]: {{javadoc_uri}}/WebExtension.MetaData.html#temporary
 [83.2]: {{javadoc_uri}}/MediaSession.Delegate.html#onMetadata-org.mozilla.geckoview.GeckoSession-org.mozilla.geckoview.MediaSession-org.mozilla.geckoview.MediaSession.Metadata-
@@ -43,6 +46,7 @@ exclude: true
 [83.6]: {{javadoc_uri}}/GeckoSession.html#loadUri-java.lang.String-org.mozilla.geckoview.GeckoSession-int-java.util.Map-
 [83.7]: {{javadoc_uri}}/GeckoSession.html#load-org.mozilla.geckoview.GeckoSession.Loader-
 [83.8]: {{javadoc_uri}}/GeckoSession.Loader.html
+[83.9]: {{javadoc_uri}}/GeckoSession.Loader.html#headerFilter-int-
 
 ## v82
 - ⚠️  [`WebNotification.source`][79.2] is now `@Nullable` to account for
@@ -832,4 +836,4 @@ to allow adding gecko profiler markers.
 [65.24]: {{javadoc_uri}}/CrashReporter.html#sendCrashReport-android.content.Context-android.os.Bundle-java.lang.String-
 [65.25]: {{javadoc_uri}}/GeckoResult.html
 
-[api-version]: 0abbabc4fa9445edc2f37ba174b189be25c2b2fa
+[api-version]: cd2ba68c049db26ce20ba332ef2ba46cbde4910a

mobile/android/modules/geckoview/GeckoViewNavigation.jsm

@@ -37,24 +37,9 @@ const BAD_HEADERS = ["connection", "host"];
 // in the header name or value
 const FORBIDDEN_HEADER_CHARACTERS = ["\n", "\r"];
 
-function validateHeader(key, value) {
-  if (!key) {
-    // Key cannot be empty
-    return false;
-  }
-
-  for (const c of FORBIDDEN_HEADER_CHARACTERS) {
-    if (key.includes(c) || value?.includes(c)) {
-      return false;
-    }
-  }
-
-  if (BAD_HEADERS.includes(key.toLowerCase().trim())) {
-    return false;
-  }
-
-  return true;
-}
+// Keep in sync with GeckoSession.java
+const HEADER_FILTER_CORS_SAFELISTED = 1;
+const HEADER_FILTER_UNRESTRICTED_UNSAFE = 2;
 
 // Create default ReferrerInfo instance for the given referrer URI string.
 const createReferrerInfo = aReferrer => {
@@ -143,6 +128,32 @@ class GeckoViewNavigation extends GeckoViewModule {
     this._initialAboutBlank = true;
   }
 
+  validateHeader(key, value, filter) {
+    if (!key) {
+      // Key cannot be empty
+      return false;
+    }
+
+    for (const c of FORBIDDEN_HEADER_CHARACTERS) {
+      if (key.includes(c) || value?.includes(c)) {
+        return false;
+      }
+    }
+
+    if (BAD_HEADERS.includes(key.toLowerCase().trim())) {
+      return false;
+    }
+
+    if (
+      filter == HEADER_FILTER_CORS_SAFELISTED &&
+      !this.window.windowUtils.isCORSSafelistedRequestHeader(key, value)
+    ) {
+      return false;
+    }
+
+    return true;
+  }
+
   // Bundle event handler.
   async onEvent(aEvent, aData, aCallback) {
     debug`onEvent: event=${aEvent}, data=${aData}`;
@@ -158,7 +169,14 @@ class GeckoViewNavigation extends GeckoViewModule {
         this.browser.gotoIndex(aData.index);
         break;
       case "GeckoView:LoadUri":
-        const { uri, referrerUri, referrerSessionId, flags, headers } = aData;
+        const {
+          uri,
+          referrerUri,
+          referrerSessionId,
+          flags,
+          headers,
+          headerFilter,
+        } = aData;
 
         let navFlags = convertFlags(flags);
         // For performance reasons we don't call the LoadUriDelegate.loadUri
@@ -215,7 +233,7 @@ class GeckoViewNavigation extends GeckoViewModule {
         if (headers) {
           additionalHeaders = "";
           for (const [key, value] of Object.entries(headers)) {
-            if (!validateHeader(key, value)) {
+            if (!this.validateHeader(key, value, headerFilter)) {
               Cu.reportError(`Ignoring invalid header '${key}'='${value}'.`);
               continue;
             }