[webkit-reviews] review granted: [Bug 184493] Disallow responses when a response header value contains 0x00 : [Attachment 386319] Patch

[bugzilla-daemon at webkit.org]

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 header value contains 0x00
https://bugs.webkit.org/show_bug.cgi?id=184493

Attachment 386319: Patch

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




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

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

r=me as is, some small style-related comments

> Source/WebCore/ChangeLog:3
> +	   Disallow responses when a response header value contains 0x00

Title doesn’t seem to match the patch.

Added stripping of leading and trailing HTTP spaces is a change that seems to
*allow* more responses and thus *loosens* our checking. This title makes it
sound like this patch only *tightens* checks.

As far as that tightening is concerned, nothing in the patch is specific to NUL
character checking, so a more accurate description of the change is that we
disallow responses when the header value is invalid, which includes the newline
checks as well.

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

This is OK, but to me seems unnecessarily inefficient. We could easily make
another version of the isValidHTTPHeaderValue function that could tolerate the
leading and trailing spaces. Not sure if this is ever performance-critical
enough. We also could change isValidHTTPHeaderValue to take a StringView since
all it does is examine the characters in the string. Then we could use the
StringView version of stripLeadingAndTrailingHTTPSpaces to avoid allocating and
destroying a new WTF::String.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:344
> +	       responseCallback(Exception { TypeError, { } });

Calling this a "type" error seems a little peculiar. But not sure I have a
better idea. Is this standardized?

> Source/WebCore/xml/XMLHttpRequest.cpp:980
> +	   String normalizedValue =
stripLeadingAndTrailingHTTPSpaces(header.value);
> +	   if (!isValidHTTPHeaderValue(normalizedValue))

Why use a local variable here? I don’t think the word "normalized" is valuable
documentation, and the combined line wouldn’t be super-long. And further,
seeing the isValidHTTPHeaderValue wrapped around a call to
stripLeadingAndTrailingHTTPSpaces might lead us to notice this and do a
refactoring in the future to improve performance if it’s a recurring pattern
(occurs twice in this patch).