Support lowercase 'access-control-request-headers' #2597
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@martijnwalraven, @abernix, can you take a look at this please? In my experience you can't effectively use CORS with |
abernix
added a commit
that referenced
this pull request
May 15, 2019
…mentation. I hope this will be an alternative implementation for the contribution made in #2597 by @dsanders11. While that solution will certainly works, it expands on our existing double-checking of multiple header CaSe-patterns in multiple locations, prior to GraphQL execution beginning. As a proposed alternative, rather than checking for two types of expected header capitalization combinations in every one of our checks (which I should note leaves out MaNy-OtHeR-Header-patterns which are perfectly valid since HTTP headers are, by specification, case-insensitive (see ref below), this uses the `Headers` implementation which we use within `apollo-server-core` to manage and manipulate headers. The `node-fetch` headers implementation takes care to store the headers in a case-insensitive manner internally: https://github.com/bitinn/node-fetch/blob/bf8b4e8db350ec76dbb9236620f774fcc21b8c12/src/headers.js#L267-L270 ...which should hopefully simplify the logic and make things behave more reliably if someone chooses to use a peculiar case for the header. As to Amazon's own difference in headers, it appears to be that it's a difference between their Application-Load Balancer behavior and the way that their AWS API Gateway functionality sends their `events` — the former being the case of the original request and the latter being lower-cased behind-the-scenes. Ref: https://tools.ietf.org/html/rfc7230#section-3.2 (RFC)
abernix
added a commit
that referenced
this pull request
May 15, 2019
I hope this will be an alternative implementation for the contribution made in #2597 by @dsanders11. While that solution will certainly works, it expands on our existing double-checking of multiple header CaSe-patterns in multiple locations, prior to GraphQL execution beginning. As a proposed alternative, rather than checking for two types of expected header capitalization combinations in every one of our checks (which I should note leaves out MaNy-OtHeR-Header-patterns which are perfectly valid since HTTP headers are, by specification, case-insensitive (see ref below), this uses the `Headers` implementation which we use within `apollo-server-core` to manage and manipulate headers. The `node-fetch` headers implementation takes care to store the headers in a case-insensitive manner internally: https://github.com/bitinn/node-fetch/blob/bf8b4e8db350ec76dbb9236620f774fcc21b8c12/src/headers.js#L267-L270 ...which should hopefully simplify the logic and make things behave more reliably if someone chooses to use a peculiar case for the header. As to Amazon's own difference in headers, it appears to be that it's a difference between their Application-Load Balancer behavior and the way that their AWS API Gateway functionality sends their `events` — the former being the case of the original request and the latter being lower-cased behind-the-scenes. Ref: https://tools.ietf.org/html/rfc7230#section-3.2 (RFC)
abernix
added a commit
that referenced
this pull request
May 15, 2019
…ext. Previously, this was leveraging the global CORS header object. I observed this while implementing an alternative for #2597. It appears the previous implementation had a small risk of returning a different `access-control-allow-origin` based on concurrent requests happening at the same time. While it would have _never_ returned an origin which wasn't specifically allowed, it does seem like under the right circumstances — barring the specifics of AWS's execution dynamics — a race condition could have made it so a different request's `origin` was returned within the `access-control-allow-origin` header of the current request, which might have resulted in a failure in more complex multi-`origin` setups.
|
@dsanders11 Would you be able to check out #2674? If you need a published version in order to test it, I'm happy to do that, but I'd need to land that into #2669 first. (Feedback on #2674 very much welcome!) |
|
Closing in favor of #2674. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When deployed with an AWS API Gateway, it seems the
Access-Control-Request-Headersheader is provided as all lowercase, so support this as well.TODO: