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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 29 06:30:50 PDT 2020

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

Attachment 400569: Patch


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

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

> 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

More information about the webkit-reviews mailing list