Bug 41210

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

...and are you planning to?

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

+1

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.

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

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.

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

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.

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

Created attachment 122473 [details] Patch2

(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

(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

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

Created attachment 122549 [details] Patch3 Moved the parsing logic to CrossOriginAccessControl.

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

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.

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

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

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.

(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>

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.

Created attachment 122651 [details] Patch4

(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

(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 on attachment 122651 [details] Patch4 r=me

Comment on attachment 122651 [details] Patch4 Clearing flags on attachment: 122651 Committed r105076: <http://trac.webkit.org/changeset/105076>

All reviewed patches have been landed. Closing bug.

<rdar://problem/10001291>

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