Response headers should be filtered when sent from NetworkProcess to WebProcess
Created attachment 337197 [details] Patch
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
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
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
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
Created attachment 337242 [details] Patch
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
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
Created attachment 337251 [details] Patch
Created attachment 337253 [details] Patch
rdar://problem/38734427
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...?
> 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.
Created attachment 337381 [details] Patch
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.
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.
Created attachment 337413 [details] Patch
Comment on attachment 337413 [details] Patch Clearing flags on attachment: 337413 Committed r230365: <https://trac.webkit.org/changeset/230365>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 337426 [details] Patch
Comment on attachment 337426 [details] Patch Clearing flags on attachment: 337426 Committed r230370: <https://trac.webkit.org/changeset/230370>