Bug 41210 - Cross Origin XMLHttpRequest can not expose headers indicated in Access-Control-Expose-Headers HTTP Response Header
: Cross Origin XMLHttpRequest can not expose headers indicated in Access-Contro...
Status: RESOLVED FIXED
: WebKit
XML
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 76198
  Show dependency treegraph
 
Reported: 2010-06-25 05:25 PST by
Modified: 2012-01-18 21:48 PST (History)


Attachments
Patch (8.73 KB, patch)
2012-01-12 19:32 PST, Joe Thomas
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Patch2 (8.16 KB, patch)
2012-01-13 11:26 PST, Joe Thomas
ap: review-
Review Patch | Details | Formatted Diff | Diff
Patch3 (9.52 KB, patch)
2012-01-14 09:28 PST, Joe Thomas
ap: review-
Review Patch | Details | Formatted Diff | Diff
Patch4 (9.39 KB, patch)
2012-01-16 09:04 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-25 05:25:03 PST
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.
------- Comment #1 From 2010-06-25 12:26:38 PST -------
We don't support Access-Control-Expose-Headers yet.
------- Comment #2 From 2011-09-16 13:36:40 PST -------
...and are you planning to?
------- Comment #3 From 2011-11-24 12:36:59 PST -------
Any update on if this is going to be added to WebKit any time soon?
------- Comment #4 From 2011-11-24 12:47:12 PST -------
+1
------- Comment #5 From 2012-01-03 09:45:42 PST -------
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.
------- Comment #6 From 2012-01-12 16:10:29 PST -------
*** Bug 76192 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2012-01-12 19:32:31 PST -------
Created an attachment (id=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.
------- Comment #8 From 2012-01-12 19:35:00 PST -------
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.
------- Comment #9 From 2012-01-12 19:45:38 PST -------
(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" ?

> 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).
------- Comment #10 From 2012-01-12 19:46:20 PST -------
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.
------- Comment #11 From 2012-01-12 19:47:09 PST -------
> 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.  :)
------- Comment #12 From 2012-01-12 22:02:39 PST -------
(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.

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

This needs a descriptive name, too.
------- Comment #13 From 2012-01-13 11:26:47 PST -------
Created an attachment (id=122473) [details]
Patch2
------- Comment #14 From 2012-01-13 11:29:09 PST -------
(In reply to comment #9)
> (From update of attachment 122361 [details] [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
------- Comment #15 From 2012-01-13 11:31:01 PST -------
(In reply to comment #12)
> (From update of attachment 122361 [details] [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
------- Comment #16 From 2012-01-13 11:33:09 PST -------
(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.
------- Comment #17 From 2012-01-13 11:57:33 PST -------
(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.

> 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.
------- Comment #18 From 2012-01-14 09:28:36 PST -------
Created an attachment (id=122549) [details]
Patch3

Moved the parsing logic to CrossOriginAccessControl.
------- Comment #19 From 2012-01-14 09:35:19 PST -------
(In reply to comment #17)
> (From update of attachment 122473 [details] [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.
------- Comment #20 From 2012-01-15 19:48:36 PST -------
(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.

> 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.
------- Comment #21 From 2012-01-15 19:49:27 PST -------
> 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.
------- Comment #22 From 2012-01-15 20:37:32 PST -------
(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.
------- Comment #23 From 2012-01-15 21:00:46 PST -------
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.
------- Comment #24 From 2012-01-15 22:26:58 PST -------
(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>
------- Comment #25 From 2012-01-15 22:50:47 PST -------
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.
------- Comment #26 From 2012-01-16 09:04:34 PST -------
Created an attachment (id=122651) [details]
Patch4
------- Comment #27 From 2012-01-16 09:06:06 PST -------
(In reply to comment #20)
> (From update of attachment 122549 [details] [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
------- Comment #28 From 2012-01-16 09:06:55 PST -------
(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.
------- Comment #29 From 2012-01-16 09:14:25 PST -------
(From update of attachment 122651 [details])
r=me
------- Comment #30 From 2012-01-16 10:42:17 PST -------
(From update of attachment 122651 [details])
Clearing flags on attachment: 122651

Committed r105076: <http://trac.webkit.org/changeset/105076>
------- Comment #31 From 2012-01-16 10:42:23 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #32 From 2012-01-18 16:15:37 PST -------
<rdar://problem/10001291>
------- Comment #33 From 2012-01-18 21:48:17 PST -------
(In reply to comment #23)
> Ah. Good point - the boilerplate doesn't work for http tests. I should fix that.

Bug 76603.