Bug 184310 – Response headers should be filtered when sent from NetworkProcess to WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Response headers should be filtered when sent from NetworkProcess to WebProcess

[youenn fablet]

Created attachment 337197 [details]
Patch

[EWS Watchlist]

Comment on attachment 337197 [details]
Patch

Attachment 337197 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7206831

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-entity-body-empty.htm
imported/w3c/web-platform-tests/XMLHttpRequest/headers-normalize-response.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-authentication-basic.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-entity-body-none.htm
imported/w3c/web-platform-tests/XMLHttpRequest/open-method-case-insensitive.htm
imported/w3c/web-platform-tests/XMLHttpRequest/send-content-type-charset.htm
imported/w3c/web-platform-tests/XMLHttpRequest/open-url-encoding.htm
http/tests/cookies/simple-cookies-expired.html
imported/w3c/web-platform-tests/XMLHttpRequest/send-authentication-basic-setrequestheader-and-arguments.htm
http/tests/xmlhttprequest/methods-lower-case.html
http/tests/cookies/simple-cookies-max-age.html
imported/w3c/web-platform-tests/XMLHttpRequest/send-authentication-basic-setrequestheader.htm
http/tests/xmlhttprequest/web-apps/007.html
http/tests/cookies/multiple-cookies.html
imported/w3c/web-platform-tests/XMLHttpRequest/send-content-type-string.htm
http/tests/xmlhttprequest/web-apps/008.html
http/tests/cookies/single-quoted-value.html
imported/w3c/web-platform-tests/XMLHttpRequest/send-entity-body-get-head.htm
http/tests/cookies/http-get-cookie-set-in-js.html
imported/w3c/web-platform-tests/XMLHttpRequest/open-method-case-sensitive.htm
http/tests/cookies/private-cookie-storage.html
imported/w3c/web-platform-tests/XMLHttpRequest/status-basic.htm

[EWS Watchlist]

Created attachment 337214 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 337197 [details]
Patch

Attachment 337197 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7208005

New failing tests:
http/tests/webarchive/test-preload-resources.html
http/tests/webarchive/cross-origin-stylesheet-crash.html

[EWS Watchlist]

Created attachment 337225 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 337242 [details]
Patch

[EWS Watchlist]

Comment on attachment 337242 [details]
Patch

Attachment 337242 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7210488

New failing tests:
http/tests/webarchive/test-preload-resources.html
http/tests/webarchive/cross-origin-stylesheet-crash.html

[EWS Watchlist]

Created attachment 337248 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 337251 [details]
Patch

[youenn fablet]

Created attachment 337253 [details]
Patch

[youenn fablet]

rdar://problem/38734427

[Ryosuke Niwa]

Comment on attachment 337253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337253&action=review

> Source/WebCore/platform/network/ResourceResponseBase.cpp:345
> +static bool isSafeToKeepDefaultResponseHeader(HTTPHeaderName name)

Where does "default" come from? Do you mean non-CORS? If so, we should explicitly say that.
Since "response header" should really be the subject of the question,
about isResponseHeaderSafeForNonCORSRequest or isSafeNonCORSResponseHeader?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:347
> +    // We keep in this list all known response headers used in WebProcesses.

We should just say "All known response headers used in WebProcesses".

> Source/WebCore/platform/network/ResourceResponseBase.cpp:392
> +static inline bool isCORSSafeHeader(HTTPHeaderName name, const HashSet<String>& headerNames)

This

> Source/WebCore/platform/network/ResourceResponseBase.cpp:397
> +static inline bool isCORSSafeHeader(const String& name, const HashSet<String>& headerNames)

and this function seem to be never used anywhere?

Also, it's unclear what the relationships of headerNames & name are.
Presumably, headerNames is the list of headers allowed by CORS?
If so, the argument variable names should reflect that semantics.

But I really don't think we need these helper functions just to call contains on a HashSet.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:419
> +        }

I think } is supposed to be aligned with case.
Also, why don't we return inside the block?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:428
> +        auto corsSafeHeaderSet = parseAccessControlAllowList(httpHeaderField(HTTPHeaderName::AccessControlExposeHeaders));
> +        if (corsSafeHeaderSet) {

Why not if (auto corsSafeHeaderSet = ~) ?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:430
> +                if (!filteredHeaders.contains(headerName)) {

Instead of checking this function, why don't we just go through the entire headers once and check:
for (auto& header : m_httpHeaderFields.commonHeaders()) {
  if (isSafeToKeepDefaultResponseHeader(header.key) || (corsSafeHeaderSet && corsSafeHeaderSet.contains(header.key)))
?

> LayoutTests/ChangeLog:8
> +        Rebased tests for WK2 as Server response header is now filtered out for cross-origin and not fetch/XHR loads.

Can we add more tests for cookies, session IDs, etc...?

[youenn fablet]

> Where does "default" come from? Do you mean non-CORS? If so, we should
> explicitly say that.
> Since "response header" should really be the subject of the question,
> about isResponseHeaderSafeForNonCORSRequest or isSafeNonCORSResponseHeader?

I'll change the name.

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:347
> > +    // We keep in this list all known response headers used in WebProcesses.
> 
> We should just say "All known response headers used in WebProcesses".

OK

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:419
> > +        }
> 
> I think } is supposed to be aligned with case.
> Also, why don't we return inside the block?

OK

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:428
> > +        auto corsSafeHeaderSet = parseAccessControlAllowList(httpHeaderField(HTTPHeaderName::AccessControlExposeHeaders));
> > +        if (corsSafeHeaderSet) {
> 
> Why not if (auto corsSafeHeaderSet = ~) ?

OK

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:430
> > +                if (!filteredHeaders.contains(headerName)) {
> 
> Instead of checking this function, why don't we just go through the entire
> headers once and check:
> for (auto& header : m_httpHeaderFields.commonHeaders()) {
>   if (isSafeToKeepDefaultResponseHeader(header.key) || (corsSafeHeaderSet &&
> corsSafeHeaderSet.contains(header.key)))
> ?

corsSafeHeaderSet contains Strings which might be indexed HTTP header names.
It is just simpler/safer to use HTTPHeaderMap API.

> > LayoutTests/ChangeLog:8
> > +        Rebased tests for WK2 as Server response header is now filtered out for cross-origin and not fetch/XHR loads.
> 
> Can we add more tests for cookies, session IDs, etc...?

Will improve that.

[youenn fablet]

Created attachment 337381 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 337381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337381&action=review

> Source/WebCore/platform/network/HTTPParsers.h:129
> +    // Skip white space from start.

Not saying we should do it in this patch,
but we should use StringView's stripLeadingAndTrailingMatchedCharacters to do this.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:320
> -static bool isSafeToKeepRedirectionHeader(HTTPHeaderName name)
> +static bool isSafeToKeepRedirectionResponseHeader(HTTPHeaderName name)

This is still wordy. How about just isSafeRedirectionResponseHeader?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:345
> +static bool isCrossOriginSafeToKeepResponseHeader(HTTPHeaderName name)

And isSafeCrossOriginResponseHeader?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:347
> +    // We keep in this list all known response headers used in WebProcesses.

"We keep in this list" is unnecessary. Just say: All known response headers used in WebProcesses

> Source/WebCore/testing/ServiceWorkerInternals.cpp:89
> +    Vector<String> headerNames;
> +    headerNames.reserveInitialCapacity(response.internalResponseHeaders().size());
> +    for (auto keyValue : response.internalResponseHeaders())
> +        headerNames.uncheckedAppend(keyValue.key);
> +    return headerNames;

Can't we just do: return WTF::map(response.internalResponseHeaders(), [](const auto& keyValue) { return keyValue.key; }) ?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:338
> +        // FIXME: Sanitize response.

Can we add a Bugzilla/radar number?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:469
> +    fprintf(stderr, "NetworkResourceLoader::sanitizeResponseIfPossible for %s\n", response.url().string().utf8().data());

Debugging code?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:478
> +        fprintf(stderr, "sanitization is %d\n", (int)type);

Ditto.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:436
> +    // FIXME: Use the proper destination once all fetch options are passed.

Can we add Bugzilla / radar number?

> LayoutTests/http/wpt/service-workers/header-filtering.https.html:10
> +var scope = "resources";

Can we use let/const instead throughout the test?

> LayoutTests/http/wpt/service-workers/header-filtering.https.html:52
> +}, "Setup worker");

The test description should explain what we're asserting.
What are are trying to test here?
Ditto for all other test cases.

> LayoutTests/http/wpt/service-workers/header-filtering.https.html:69
> +    assert_array_equals(await promise, ["Access-Control-Allow-Credentials","Access-Control-Allow-Methods","Access-Control-Allow-Origin","Access-Control-Expose-Headers","Cache-Control","Content-Length","Content-Type","Date","Referrer-Policy","Server","SourceMap","Timing-Allow-Origin","X-SourceMap","x-header1","x-header2"]);

Can we wrap lines around 100 characters so that these are easier to read?

> LayoutTests/http/wpt/service-workers/resources/header-filtering-iframe.html:1
> +<html>

Missing DOCTYPE.

[youenn fablet]

Thanks for the review.

> > Source/WebCore/platform/network/HTTPParsers.h:129
> > +    // Skip white space from start.
> 
> Not saying we should do it in this patch,
> but we should use StringView's stripLeadingAndTrailingMatchedCharacters to
> do this.

Agreed, I was tempted to do it in the same patch but resisted to do so.

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:320
> > -static bool isSafeToKeepRedirectionHeader(HTTPHeaderName name)
> > +static bool isSafeToKeepRedirectionResponseHeader(HTTPHeaderName name)
> 
> This is still wordy. How about just isSafeRedirectionResponseHeader?

OK

> > Source/WebCore/platform/network/ResourceResponseBase.cpp:345
> > +static bool isCrossOriginSafeToKeepResponseHeader(HTTPHeaderName name)
> 
> And isSafeCrossOriginResponseHeader?

OK

> Can't we just do: return WTF::map(response.internalResponseHeaders(),
> [](const auto& keyValue) { return keyValue.key; }) ?

Unfortunately not, we would need to teach the collection inspector to handle the header map iterator correctly.

[youenn fablet]

Created attachment 337413 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337413 [details]
Patch

Clearing flags on attachment: 337413

Committed r230365: <https://trac.webkit.org/changeset/230365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 337426 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337426 [details]
Patch

Clearing flags on attachment: 337426

Committed r230370: <https://trac.webkit.org/changeset/230370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.