Skip to content

Suggest passing false for disallowed domains, not erroring#175

Merged
dougwilson merged 1 commit intoexpressjs:masterfrom
shackpank:function_usage_doc_update
May 11, 2020
Merged

Suggest passing false for disallowed domains, not erroring#175
dougwilson merged 1 commit intoexpressjs:masterfrom
shackpank:function_usage_doc_update

Conversation

@shackpank
Copy link
Copy Markdown
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.

Comment thread README.md Outdated
@dougwilson
Copy link
Copy Markdown
Contributor

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

Comment thread README.md Outdated
Comment thread README.md Outdated
@dougwilson dougwilson force-pushed the function_usage_doc_update branch from 1bbe0b4 to c1867c3 Compare May 11, 2020 03:01
@dougwilson dougwilson self-assigned this May 11, 2020
Copy link
Copy Markdown
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.

3 participants