[webkit-reviews] review denied: [Bug 41210] Cross Origin XMLHttpRequest can not expose headers indicated in Access-Control-Expose-Headers HTTP Response Header : [Attachment 122473] Patch2

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Joe Thomas
<joethomas at motorola.com>'s request for review:
Bug 41210: Cross Origin XMLHttpRequest can not expose headers indicated in
Access-Control-Expose-Headers HTTP Response Header
https://bugs.webkit.org/show_bug.cgi?id=41210

Attachment 122473: Patch2
https://bugs.webkit.org/attachment.cgi?id=122473&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122473&action=review


Thanks, this looks much closer already.

> Source/WebCore/xml/XMLHttpRequest.cpp:155
> +static void parseAccessControlAllowList(const String& headerValue,
Vector<String>& headersSet)

It's somewhat weird to see this in XMLHttpRequest.cpp. It might be the only
CORS client that exposes headers, but the behavior itself is not tied to XHR.
You might find it cleaner to have the checks in CrossOriginAccessControl.cpp -
please consider that possibility.

> Source/WebCore/xml/XMLHttpRequest.cpp:159
> +    headerValue.stripWhiteSpace().upper();
> +    headerValue.split(',', false, headersSet);

I suspect that you need to strip whitespace afterwards, not before:

Access-Control-Expose-Headers: X-FOO,	 X-BAR

Also, is whitespace definition in this function the same as in RFC 2616? There
are many whitespace characters besides plain space, and different specs
disagree on which to ignore.

Please use HTTPHeaderSet for consistency and performance, like
isOnAccessControlResponseHeaderWhitelist does.

>
LayoutTests/http/tests/xmlhttprequest/access-control-response-with-expose-heade
rs-expected.txt:7
> +PASS
> +PASS
> +PASS

It would be very helpful to have explanations of what fails and passes. You can
use shouldBe from script test machinery to easily do that, and simplify test
code at the same time.

Try our make-new-script-test script, which will prepare boilerplate for a
script test.

>
LayoutTests/http/tests/xmlhttprequest/resources/access-control-response-with-ex
pose-headers.php:6
> +    header("X-FOO: BAR");
> +    header("X-TEST: TEST");
> +    header("Access-Control-Expose-Headers: X-FOO");

Another case sensitivity test to add is where actual HTTP header has a
different case than the value in Access-Control-Expose-Headers.