Bug 191107 – Some minor X-Content-Type-Options parsing issues

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

See https://github.com/web-platform-tests/wpt/pull/13559 for tests and https://github.com/whatwg/fetch/pull/818 for the new parser definition. In particular treating 0x0B and 0x0C as whitespace (bug 191102), but also not always dealing will with picking the first value when multiple are supplied.

[Rob Buis]

Created attachment 353774 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 353774 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +

Can we please also mention https://fetch.spec.whatwg.org/#x-content-type-options-header for easier review?

> Source/WebCore/platform/network/HTTPParsers.cpp:487
> +        options.truncate(commaPos);

I wonder if there is a way to do that without passing by a non-const copy. Can we use String::split to get a StringView or something similar?

[Frédéric Wang (:fredw)]

cc'ing Darin for suggestions on String handling.

[Chris Dumez]

Comment on attachment 353774 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.cpp:482
>  ContentTypeOptionsDisposition parseContentTypeOptionsHeader(const String& header)

This should probably be:
ContentTypeOptionsDisposition parseContentTypeOptionsHeader(StringView header)

> Source/WebCore/platform/network/HTTPParsers.cpp:484
> +    String options = header;

Not needed

>> Source/WebCore/platform/network/HTTPParsers.cpp:487
>> +        options.truncate(commaPos);
> 
> I wonder if there is a way to do that without passing by a non-const copy. Can we use String::split to get a StringView or something similar?

use StringView::substring()

[Darin Adler]

Comment on attachment 353774 [details]
Patch

Big picture: Best idiom for efficiency in a case like is to make the function take a StringView instead of "const String&". Then in the implementation don’t allocate any memory. And if StringView doesn’t have the functions we need, then add them.

StringView does have everything needed here I think, although the names aren’t exactly the same as String:

- StringView::find(UChar)
- StringView::left
- StringView::stripLeadingAndTrailingMatchedCharacters
- equalLettersIgnoringASCIICase(StringView, <literal>)

You can even leave out the "notFound" check because StringView::left does the right thing (nothing, returns the passed in string) when we pass notFound to it. That’s intentional to make cases like this work.

So while it would not be elegant you could write this all on one line:

    equalLettersIgnoringASCIICase(header.left(header.find(',')).stripLeadingAndTrailingMatchedCharacters(predicate), "nosniff")

But local variables might be needed to make it less chaotic/confusing.

[Darin Adler]

I’m saying the same thing Chris is, just with more detailed suggestions.

[Rob Buis]

Created attachment 353898 [details]
Patch

[Darin Adler]

Comment on attachment 353898 [details]
Patch

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

> Source/WebCore/platform/network/HTTPParsers.cpp:477
> +inline bool isHTTPTabOrSpace(UChar character)

I’m not sure this helper function needs the word "HTTP" in it since it’s just tab and space. Also seems like this function could be in the ASCIICType.h header if it’s likely to come up often.

The "whitespace" functions elsewhere sometimes say HTML or HTTP because whitespace has different meanings in different contexts so we are trying to clarify which one. But "tab" and "space" aren’t so situational as far as I know.

I suspect this could also just use the isHTTPSpace function (not sure I have the name right) because the newline-related can’t possibly exist in the header since it’s already been broken into lines, but not really sure.

Anyway, fine as is but would be nice to delete the "HTTP" and even nicer to not have to define a function here because another happens to be available, either in ASCIICType.h or in the header for other uses.

[Rob Buis]

Created attachment 353946 [details]
Patch

[Rob Buis]

(In reply to Darin Adler from comment #8)
> Comment on attachment 353898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353898&action=review
> 
> > Source/WebCore/platform/network/HTTPParsers.cpp:477
> > +inline bool isHTTPTabOrSpace(UChar character)
> 
> I’m not sure this helper function needs the word "HTTP" in it since it’s
> just tab and space. Also seems like this function could be in the
> ASCIICType.h header if it’s likely to come up often.
> 
> The "whitespace" functions elsewhere sometimes say HTML or HTTP because
> whitespace has different meanings in different contexts so we are trying to
> clarify which one. But "tab" and "space" aren’t so situational as far as I
> know.
> 
> I suspect this could also just use the isHTTPSpace function (not sure I have
> the name right) because the newline-related can’t possibly exist in the
> header since it’s already been broken into lines, but not really sure.
> 
> Anyway, fine as is but would be nice to delete the "HTTP" and even nicer to
> not have to define a function here because another happens to be available,
> either in ASCIICType.h or in the header for other uses.

Thanks for all the feedback! I overlooked it, but it turns out stripLeadingAndTrailingHTTPSpaces does exactly what we want and
so we do not even need to add something like isHTTPTabOrSpace.

[WebKit Commit Bot]

Comment on attachment 353946 [details]
Patch

Rejecting attachment 353946 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 353946, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/9875814

[Anne van Kesteren]

(The reason the specification uses "HTTP tab or space" and not "HTTP whitespace" is mostly to be explicit about LF and CR not being able to be present. Using "HTTP whitespace" here created some confusion. I prefixed it with HTTP mostly because I don't think we group those two code points for any other reason, but maybe I'm wrong there at which point it'll be "tab or space" and moved to the Infra Standard I suppose.)

[Rob Buis]

Created attachment 353948 [details]
Patch

[Rob Buis]

(In reply to Anne van Kesteren from comment #12)
> (The reason the specification uses "HTTP tab or space" and not "HTTP
> whitespace" is mostly to be explicit about LF and CR not being able to be
> present. Using "HTTP whitespace" here created some confusion. I prefixed it
> with HTTP mostly because I don't think we group those two code points for
> any other reason, but maybe I'm wrong there at which point it'll be "tab or
> space" and moved to the Infra Standard I suppose.)

Thanks for the clarification. In WebKit case, as Darin pointed out, LF and CR will not be in the header value so we can reuse the existing stripLeadingAndTrailingHTTPSpaces.

[WebKit Commit Bot]

Comment on attachment 353948 [details]
Patch

Clearing flags on attachment: 353948

Committed r237850: <https://trac.webkit.org/changeset/237850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45837237>