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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 2 09:51:32 PDT 2020


Darin Adler <darin at apple.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 400724: Patch

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




--- Comment #34 from Darin Adler <darin at apple.com> ---
Comment on attachment 400724
  --> https://bugs.webkit.org/attachment.cgi?id=400724
Patch

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

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:46
> +    ASSERT(value.isEmpty() || (!isHTTPSpace(value[0]) &&
!isHTTPSpace(value[value.length() - 1])));

What guarantees what this asserts? Caller responsibility to call
stripLeadingAndTrailingHTTPSpaces? What's the benefit of this assertion? Seems
like the check will fail in this case, so it won’t "slip by"?

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:81
> +    String normalizedValue =
stripLeadingAndTrailingHTTPSpaces(header.value);

Seems unnecessarily inefficient to allocate a new String if there are leading
or trailing spaces. Cleaner if some day we could refactor to do these
operations with StringView instead, which would avoid memory allocation
entirely.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:212
> +	   String normalizedValue =
stripLeadingAndTrailingHTTPSpaces(header.value);

Ditto.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:845
> +	   if
(!isValidHTTPHeaderValue(stripLeadingAndTrailingHTTPSpaces(header.value)))

Ditto.


More information about the webkit-reviews mailing list