Suggest passing false for disallowed domains, not erroring#175
Merged
dougwilson merged 1 commit intoexpressjs:masterfrom May 11, 2020
Merged
Suggest passing false for disallowed domains, not erroring#175dougwilson merged 1 commit intoexpressjs:masterfrom
dougwilson merged 1 commit intoexpressjs:masterfrom
Conversation
dougwilson
suggested changes
Jun 18, 2019
Contributor
|
@AbhishekBiswal since this PR is removing the section you added, can you review this for your use-case? |
dougwilson
reviewed
Jun 19, 2019
dougwilson
reviewed
Jun 19, 2019
LinusU
approved these changes
Apr 24, 2020
1bbe0b4 to
c1867c3
Compare
dougwilson
approved these changes
May 11, 2020
Contributor
dougwilson
left a comment
There was a problem hiding this comment.
thank you. i don't know how that section of the documentation evolved into the current form, but it was never a good example of the origin function + callback. just like you said, it should not be passing back an error to "reject" an origin -- it should be passing back a value that would have normally be passed to the origin option.
This file contains hidden or 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
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.
The example here to provide a function to allow dynamic origins doesn't behave the same way as when you pass strings, arrays of strings, arrays of regexps etc as the allowed origin.
If you pass an origin function that calls back with an error, the error propagates onwards preventing the route handler that would otherwise be called from being called, and you get an unsuccessful HTTP response.
This has other undesirable side effects like preventing non-browser access to a server (#142 updated the docs to suggest a workaround for this), and preventing same-origin access to a server using the CORS middleware (which there is no workaround for, unless you count including the domains intending to do same-origin requests in the allowed cross-origin list)
If you provide a string domain as the allowed origin, though, and you try to do a cross-origin request from a disallowed origin, the request goes ahead as normal. The access-control-allow-origin header is not included in the response as expected (which prevents the response leaking to scripts on the disallowed origin), but there is no impact on same-origin requests or non-browser access when the module is configured this way.
Which behaviour is the expected one? What I'm getting from the spec is that it's probably the second one, that CORS alone should not be relied on to prevent request side-effects, and a CSRF token should be passed in these cases.
This pull request updates the documentation to suggest calling back with
(null, false)instead of an error, to bring the behaviours between the different configurations more in line, and remove the need for cases like non-browser access to need special workarounds.