[webkit-reviews] review granted: [Bug 184310] Response headers should be filtered when sent from NetworkProcess to WebProcess : [Attachment 337381] Patch

Fri Apr 6 17:09:56 PDT 2018

Ryosuke Niwa <rniwa at webkit.org> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 184310: Response headers should be filtered when sent from NetworkProcess
to WebProcess
https://bugs.webkit.org/show_bug.cgi?id=184310

Attachment 337381: Patch

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

--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 337381
  --> https://bugs.webkit.org/attachment.cgi?id=337381
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-Cont
rol-Allow-Origin","Access-Control-Expose-Headers","Cache-Control","Content-Leng
th","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.