Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest passing false for disallowed domains, not erroring #175

Merged
merged 1 commit into from
May 11, 2020

Conversation

shackpank
Copy link
Contributor

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.

README.md Outdated Show resolved Hide resolved
@dougwilson
Copy link
Contributor

@AbhishekBiswal since this PR is removing the section you added, can you review this for your use-case?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dougwilson dougwilson self-assigned this May 11, 2020
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dougwilson dougwilson merged commit c1867c3 into expressjs:master May 11, 2020
@expressjs expressjs deleted a comment from lucaspiller Jan 1, 2023
@expressjs expressjs deleted a comment from alexanderfletcher Jan 1, 2023
@expressjs expressjs deleted a comment from jub0bs Jan 1, 2023
@expressjs expressjs locked as resolved and limited conversation to collaborators Jan 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants