[webkit-reviews] review granted: [Bug 188644] Implement further CORS restrictions : [Attachment 364199] 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 188644: Implement further CORS restrictions
https://bugs.webkit.org/show_bug.cgi?id=188644

Attachment 364199: Patch

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




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

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

> Source/WebCore/ChangeLog:26
> +	   (WebCore::isCrossOriginSafeRequestHeader): check that header length
is less than 128

Code checks for <= 128, but this says < 128. Which is it? Would be nice to have
a test that is exactly at the maximum length and another that is exactly one
more than the maximum length.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:59
> +	       combinedValue = makeString(headers.get(name), ", ",
normalizedValue);

Unfortunate that we need to do this concatenation work both here and inside
HTTPHeaderMap::add. Wasteful because we compute the same string twice. Also
breaks encapsulation since the ", " separator is now here as well as in the
HTTPHeaderMap class. Would be nice to find some way to avoid those problems
will still fixing this.

> Source/WebCore/platform/network/HTTPParsers.cpp:882
> +    return value.length() <= 128;

Why not put this check first? Then we wouldn’t need to make those other
changes. Also needs a brief "why" comment, I think. Not obvious that a 129
character string isn’t safe, but easy to explain with a reference to where that
rule is defined, or even just an "its a rule" remark.