cors: set access-control-expose-headers on actual CORS request #3852
Conversation
…S request Signed-off-by: Stanley Cheung <stanleycheung@google.com>
|
Maintainers: this is the first time I try to submit a PR. I noticed the 2 CI tests failures. How should I proceed? Any tips on how to reproduce that locally? Could that be flakes? My change seems unrelated. Please advise. Thanks! |
|
@stanley-cheung you can try to do master merge, hopefully, that will heal the |
Signed-off-by: Stanley Cheung <stanleycheung@google.com>
|
@dio Thanks. That works. I merge |
There was a problem hiding this comment.
Thanks @stanley-cheung for doing this. Only one nit and a comment below:
This patch is interesting since currently, we have this header Access-Control-Expose-Headers being sent only for a response of a preflight request. However, based on this flowchart in this article, we should only send it as part of an actual response for a normal request, i.e. as proposed by this PR. I also did a quick look at this express' cors module: https://github.com/expressjs/cors/blob/master/test/test.js#L570-L622 and the relevant RFC, those confirm the flowchart.
@mattklein123 @codesuki thoughts?
| route->mutable_route()->set_cluster("cluster_0"); | ||
| auto* cors = route->mutable_route()->mutable_cors(); | ||
| cors->add_allow_origin("test-origin-1"); | ||
| cors->set_expose_headers("custom-header-1"); |
There was a problem hiding this comment.
Nit, can we have a comma-separated list of headers here (e.g. X-My-Custom-Header, X-Another-Custom-Header)? Just to make a point that we can express setting multiple headers by doing so.
There was a problem hiding this comment.
@dio, thanks for the review. I have updated the test with multiple expose-headers.
Should I also take out the setting of access-control-expose-headers in the OPTIONS request in this same PR?
|
After re-reading the rfc I agree this is correct. https://www.w3.org/TR/cors/#resource-requests Also expose headers should be removed from the preflight request since it's not noted in 6.2. Additionally I found some points where a preflight request should probably terminate early which it's not doing yet. I'll follow up on that later this week. |
Signed-off-by: Stanley Cheung <stanleycheung@google.com>
Signed-off-by: Stanley Cheung <stanleycheung@google.com>
Signed-off-by: Stanley Cheung <stanleycheung@google.com>
|
@codesuki Thanks for the review! I have updated the PR to not set |
Description: Set the
access-control-expose-headersresponse header on the actual CORS request (not preflight). Currently, if I set a custom header from the upstream server, I can't retrieve the value of that header from the browser getResponseHeaders() API because theaccess-control-expose-headersresponse header is only set on the preflight OPTIONS request, not the actual POST request.Risk Level: low
Testing: bazel test /test/...
Docs Changes: None
Release Notes: None
Signed-off-by: Stanley Cheung stanleycheung@google.com