Bug 41210 – Cross Origin XMLHttpRequest can not expose headers indicated in Access-Control-Expose-Headers HTTP Response Header

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Stuart Ng]

Send XHR Request to get CORS URL.
CORS URL send back a header "Access-Control-Expose-Headers: A", and also a header "A: Hello"

Upon receiving response, getAllResponseHeaders method does not return the HTTP Header "A: Hello"
Also, getResponseHeader("A") returns NULL.

I can reproduce on both Chrome and Safari.

[Alexey Proskuryakov]

We don't support Access-Control-Expose-Headers yet.

[František Hába]

...and are you planning to?

[Dave Irvine]

Any update on if this is going to be added to WebKit any time soon?

[František Hába]

+1

[Greg Agnew]

All that needs to be done is 

http://trac.webkit.org/browser/trunk/Source/WebCore/loader/CrossOriginAccessControl.cpp#L94

Concatenate allowedCrossOriginResponseHeaders with the list that comes from the exposed header.

Very simple.

[Alexey Proskuryakov]

*** Bug 76192 has been marked as a duplicate of this bug. ***

[Joe Thomas]

Created attachment 122361 [details]
Patch

The functions addToAccessControlAllowList & parseAccessControlAllowList are the same in loader/CrossOriginPreflightResultCache.cpp. Should I move these two functions to a common file (loader/CrossOriginAccessControl.cpp)?

Tested in Chrome.

[WebKit Review Bot]

Attachment 122361 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

LayoutTests/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 122361 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:157
> +static void addToAccessControlAllowList(const String& string, unsigned start, unsigned end, HashSet<String, HashType>& set)

We should use a better name for the first argument than "string".  Perhaps "headerName" ?

> Source/WebCore/xml/XMLHttpRequest.cpp:161
> +    StringImpl* stringImpl = string.impl();
> +    if (!stringImpl)
> +        return;

This can only happen when the string is the null string.  We should filter that case out in parseAccessControlAllowList and skip this check here.

> Source/WebCore/xml/XMLHttpRequest.cpp:173
> +    // Skip white space from start.
> +    while (start <= end && isSpaceOrNewline((*stringImpl)[start]))
> +        ++start;
> +
> +    // only white space
> +    if (start > end) 
> +        return;
> +
> +    // Skip white space from end.
> +    while (end && isSpaceOrNewline((*stringImpl)[end]))
> +        --end;

It's probably better to use String::stripWhiteSpace rather than reimplementing it here.

> Source/WebCore/xml/XMLHttpRequest.cpp:178
> +template<class HashType>

Why is function templated?  It's only ever called with one sort of set.

> Source/WebCore/xml/XMLHttpRequest.cpp:183
> +    while ((end = string.find(',', start)) != notFound) {

Consider using String::split rather than this sort of look.  You can write this whole function in like three lines of code if you use some of the advanced features of String (rather than working with c-style strings).

[Adam Barth]

You've got some problems with tabs too.  :)

Mostly it looks like you should familiarize yourself with some of WebKit's fun string manipulation functions.

[Adam Barth]

> The functions addToAccessControlAllowList & parseAccessControlAllowList are the same in loader/CrossOriginPreflightResultCache.cpp. Should I move these two functions to a common file (loader/CrossOriginAccessControl.cpp)?

Yes.  Please refactor things to share code rather than copy/pasting code.  :)

[Alexey Proskuryakov]

Comment on attachment 122361 [details]
Patch

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

Please make sure that header name matching is case insensitive, and modify the regression test to cover that.

> Source/WebCore/xml/XMLHttpRequest.cpp:936
> +    HashSet<String> headers;

This needs a descriptive name, too.

[Joe Thomas]

Created attachment 122473 [details]
Patch2

[Joe Thomas]

(In reply to comment #9)
> (From update of attachment 122361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122361&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:157
> > +static void addToAccessControlAllowList(const String& string, unsigned start, unsigned end, HashSet<String, HashType>& set)
> 
> We should use a better name for the first argument than "string".  Perhaps "headerName" ?

Changed the name to headerValue
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:161
> > +    StringImpl* stringImpl = string.impl();
> > +    if (!stringImpl)
> > +        return;
> 
> This can only happen when the string is the null string.  We should filter that case out in parseAccessControlAllowList and skip this check here.
> 
Removed.

> > Source/WebCore/xml/XMLHttpRequest.cpp:173
> > +    // Skip white space from start.
> > +    while (start <= end && isSpaceOrNewline((*stringImpl)[start]))
> > +        ++start;
> > +
> > +    // only white space
> > +    if (start > end) 
> > +        return;
> > +
> > +    // Skip white space from end.
> > +    while (end && isSpaceOrNewline((*stringImpl)[end]))
> > +        --end;
> 
> It's probably better to use String::stripWhiteSpace rather than reimplementing it here.
> 
Done

> > Source/WebCore/xml/XMLHttpRequest.cpp:178
> > +template<class HashType>
> 
> Why is function templated?  It's only ever called with one sort of set.
> 
Removed. problem with copy-paste :)

> > Source/WebCore/xml/XMLHttpRequest.cpp:183
> > +    while ((end = string.find(',', start)) != notFound) {
> 
> Consider using String::split rather than this sort of look.  You can write this whole function in like three lines of code if you use some of the advanced features of String (rather than working with c-style strings).

Done

[Joe Thomas]

(In reply to comment #12)
> (From update of attachment 122361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122361&action=review
> 
> Please make sure that header name matching is case insensitive, and modify the regression test to cover that.
> 
Added extra test case.

> > Source/WebCore/xml/XMLHttpRequest.cpp:936
> > +    HashSet<String> headers;
> 
> This needs a descriptive name, too.

Done. Changed to accessControlExposeHeadersSet

[Joe Thomas]

(In reply to comment #10)
> You've got some problems with tabs too.  :)

Corrected.
> 
> Mostly it looks like you should familiarize yourself with some of WebKit's fun string manipulation functions.

[Alexey Proskuryakov]

Comment on attachment 122473 [details]
Patch2

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-headers-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-expose-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.

[Joe Thomas]

Created attachment 122549 [details]
Patch3

Moved the parsing logic to CrossOriginAccessControl.

[Joe Thomas]

(In reply to comment #17)
> (From update of attachment 122473 [details])
> 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.
> 
Done

> > 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
> 
Done.

> 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.
> 
Yes, it strips the trailing and leading whitespace and newline. Also I tested with tabs too.

> Please use HTTPHeaderSet for consistency and performance, like isOnAccessControlResponseHeaderWhitelist does.
> 
Done. Using HashSet<String, CaseFoldingHash>.

> > LayoutTests/http/tests/xmlhttprequest/access-control-response-with-expose-headers-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.
>
Tried with make-new-script-test to create new test html. But while running layout tests, it fails with "Reference error: variable not defined" for the functions in js-test-pre.js file. The file-path is mentioned correctly, but not sure why. 
 
> > LayoutTests/http/tests/xmlhttprequest/resources/access-control-response-with-expose-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.

Modified the cases of actual header and field value of Access-Control-Expose-Headers.

[Alexey Proskuryakov]

Comment on attachment 122549 [details]
Patch3

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

THis is pretty close. I have a few minor nitpicks, and I think that you should make another try converting this to a "shouldBe" test.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:173
> +void parseAccessControlExposeHeadersAllowList(const String& headerValue, HashSet<String, CaseFoldingHash>& headersSet)

Is "headers set" good English grammar? FWIW, existing code says "header set".

> Source/WebCore/loader/CrossOriginAccessControl.cpp:177
> +    for (unsigned int headerCount = 0; headerCount < headers.size(); headerCount++) {

WebKit coding style is to use "unsigned", not "unsigned int".

> Source/WebCore/loader/CrossOriginAccessControl.h:50
> +void parseAccessControlExposeHeadersAllowList(const String& headerValue, HashSet<String, CaseFoldingHash>& headersSet);

I'd suggest moving HTTPHeaderSet definition to the header, and using it for consistency.

[Alexey Proskuryakov]

> Tried with make-new-script-test to create new test html. But while running layout tests, it fails with "Reference error: variable not defined" for the functions in js-test-pre.js file. The file-path is mentioned correctly, but not sure why. 

Where exactly does the error occur? It's possible that recent changes to the scripts broke the boilerplate.

[Joe Thomas]

(In reply to comment #21)
> > Tried with make-new-script-test to create new test html. But while running layout tests, it fails with "Reference error: variable not defined" for the functions in js-test-pre.js file. The file-path is mentioned correctly, but not sure why. 
> 
> Where exactly does the error occur? It's possible that recent changes to the scripts broke the boilerplate.

I created the new test html file in LayoutTests/http/tests/xmlhttprequest. But then if I include the js-test-pre.js with ../../../fast/js/resources/js-test-pre.js, I am getting the Reference error for the JS functions while running LayoutTests. But if I load the page directly into Browser, then everything works fine. 

I also tried moving the test file to LayoutTests/fast/dom/xmlhttprequest, then I could avoid the reference error, but then I was not getting the response for  the xmlhttprequest from localhost. 

Thanks for helping me with this.

[Alexey Proskuryakov]

Ah. Good point - the boilerplate doesn't work for http tests. I should fix that.

These resources are mapped into http server js-test-resources directory. See e.g. LayoutTests/http/tests/cache/subresource-multiple-instances.html for an example of how it's used.

To test interactively in a browser, you can start the server using a run-webkit-httpd script.

[Joe Thomas]

(In reply to comment #23)
> Ah. Good point - the boilerplate doesn't work for http tests. I should fix that.
> 
> These resources are mapped into http server js-test-resources directory. See e.g. LayoutTests/http/tests/cache/subresource-multiple-instances.html for an example of how it's used.
> 
> To test interactively in a browser, you can start the server using a run-webkit-httpd script.

The JS error got resolved after I included it like ../../js-test-resources/js-test-pre.js. But facing a strange issue now. 

If I start including js-test-pre.js, then I never get response for my xmlhttprequest while running LayoutTests script (not sure what's the relation). But I can load and run the test page in Chrome browser without an issue after starting the server with run-webkit-httpd script.  

Below is the content of my test page

<html>
<head>
<script src="../../js-test-resources/js-test-pre.js"></script>
</head>
<body>
<script type="text/javascript">
    if (window.layoutTestController)
        layoutTestController.dumpAsText();
description('Test for bug 41210: Cross Origin XMLHttpRequest can not expose headers indicated in Access-Control-Expose-Headers HTTP Response Header.');
var xhr = new XMLHttpRequest();
xhr.onreadystatechange=function() {
    if (xhr.readyState==4) {
        shouldBe("xhr.getResponseHeader(\"X-FOO\")","'BAR'");
        shouldBe("xhr.getResponseHeader(\"x-foo\")","'BAR'");              
        shouldBeNull("xhr.getResponseHeader(\"X-TEST\")");  
    }
}
var url = "http://localhost:8000/xmlhttprequest/resources/access-control-response-with-expose-headers.php";
xhr.open("GET",url);
xhr.send(null);
</script>
</body>
</html>

[Alexey Proskuryakov]

Right, you still need an equivalent of waitUntilDone/notifyDone.

I think that the most fashionable way to do that is to set window.jsTestIsAsync to true early on, and to call finishJSTest() when done.

You don't need layoutTestController.dumpAsText any more - script tests do that automatically.

[Joe Thomas]

Created attachment 122651 [details]
Patch4

[Joe Thomas]

(In reply to comment #20)
> (From update of attachment 122549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122549&action=review
> 
> THis is pretty close. I have a few minor nitpicks, and I think that you should make another try converting this to a "shouldBe" test.
> 
Done

> > Source/WebCore/loader/CrossOriginAccessControl.cpp:173
> > +void parseAccessControlExposeHeadersAllowList(const String& headerValue, HashSet<String, CaseFoldingHash>& headersSet)
> 
> Is "headers set" good English grammar? FWIW, existing code says "header set".
> 
Changed.

> > Source/WebCore/loader/CrossOriginAccessControl.cpp:177
> > +    for (unsigned int headerCount = 0; headerCount < headers.size(); headerCount++) {
> 
> WebKit coding style is to use "unsigned", not "unsigned int".
> 
Done
> > Source/WebCore/loader/CrossOriginAccessControl.h:50
> > +void parseAccessControlExposeHeadersAllowList(const String& headerValue, HashSet<String, CaseFoldingHash>& headersSet);
> 
> I'd suggest moving HTTPHeaderSet definition to the header, and using it for consistency.

Done

[Joe Thomas]

(In reply to comment #25)
> Right, you still need an equivalent of waitUntilDone/notifyDone.
> 
Thanks. Not sure how I missed it :(

> I think that the most fashionable way to do that is to set window.jsTestIsAsync to true early on, and to call finishJSTest() when done.
> 
> You don't need layoutTestController.dumpAsText any more - script tests do that automatically.

Removed.

[Alexey Proskuryakov]

Comment on attachment 122651 [details]
Patch4

r=me

[WebKit Review Bot]

Comment on attachment 122651 [details]
Patch4

Clearing flags on attachment: 122651

Committed r105076: <http://trac.webkit.org/changeset/105076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

<rdar://problem/10001291>

[Alexey Proskuryakov]

(In reply to comment #23)
> Ah. Good point - the boilerplate doesn't work for http tests. I should fix that.

Bug 76603.