[webkit-reviews] review granted: [Bug 184493] Disallow responses when a response contains invalid header values : [Attachment 400569] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Rob Buis <rbuis at igalia.com>'s
request for review:
Bug 184493: Disallow responses when a response contains invalid header values
https://bugs.webkit.org/show_bug.cgi?id=184493

Attachment 400569: Patch

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




--- Comment #21 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 400569
  --> https://bugs.webkit.org/attachment.cgi?id=400569
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
> +    if (!isValidHTTPHeaderValue(stripLeadingAndTrailingHTTPSpaces(value)))

The idea was that the caller of canWriteHeader would do the stripping.
Maybe we should add an ASSERT here and update the two call sites that are not
explicitly stripping spaces.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:401
> +    if (!m_async && response.containsInvalidHTTPHeaders()) {

Let's move this check in the synchronous code path, in
DocumentThreadableLoader::loadRequest.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:403
> +	   didFail(identifier, error);

Could be a one liner.

> Source/WebCore/loader/SubresourceLoader.cpp:440
> +    if (/*m_resource->type() != CachedResource::Type::MainResource &&*/
response.containsInvalidHTTPHeaders()) {

Should remove the comment.
Also, this seems a bit late, we should probably do this check at the beginning
of didReceiveResponse.

> Source/WebCore/loader/SubresourceLoader.cpp:442
> +	   didFail(error);

Could be a one liner with above line