Apply cross-origin headers to all incoming requests. #375
Conversation
…route feat: allow options request for token route
phyllisstein
changed the title
|
@phyllisstein this looks great - we'll review/merge/release tomorrow morning. The inclusion of tests much appreciated! |
|
@eduardoboucas we'll need to port this to the 2.2.x branch, I believe |
There was a problem hiding this comment.
This looks really good, thank you @phyllisstein! I've added just a tiny detail/question.
(Thanks for adhering to all the coding styles and conventions 🙏 )
dadi/lib/cors/index.js
Outdated
| return next() | ||
| } | ||
|
|
||
| const method = req.method && req.method.toUpperCase && req.method.toUpperCase() |
There was a problem hiding this comment.
Is it necessary to check for the existence of both req.method and req.method.toUpperCase before using it? Would req.method ever be anything other than a string?
There was a problem hiding this comment.
also, we lowercase it everywhere else ;)
There was a problem hiding this comment.
😃Fair enough! Just pushed a less conservative check.
|
Thanks for the feedback! Should I change the base branch to |
|
@phyllisstein yes please, that would be much appreciated! |
|
I've made a mess of this. Closing this PR - @phyllisstein please reopen against thanks! |
|
Done: #376! Sorry for the not-so-hot suggestion about swapping branches, that never seems to work the way I expect. |
Does this resolve an issue?
Fix/Close #363
Description of changes proposed in this pull request
Creates a global cross-origin middleware that applies spec-compliant CORS headers to all incoming requests.
Who should review this pull request?
@jimlambie, @adamkdean
Testing
Integration tests are passing---there's no way I can think of to do any meaningful acceptance testing, but I have confidence that this is a valid tack.