Skip to content
New issue

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

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

Already on GitHub? # to your account

CORS middleware fixed and refactored #165

Merged
merged 3 commits into from
Jul 21, 2021
Merged

CORS middleware fixed and refactored #165

merged 3 commits into from
Jul 21, 2021

Conversation

LuisEGR
Copy link
Contributor

@LuisEGR LuisEGR commented Jul 20, 2021

While I was working with servest and Angular I noticed that the current implementation of CORS is not fully working(I am still getting CORS error), this is because the headers are only added to the preflight request (OPTIONS) and there are three headers that need to also be sent in all the other allowed methods as well (access-control-allow-origin, access-control-expose-headers and access-control-allow-credentials), adding this solves the problem, but to avoid code duplication I did a refactor of the entire file so it is more readable.

  • Fixes CORS missing headers (in other methods distinct to OPTIONS)
  • Fixes Typo in header accessl-control-expose-headers -> access-control-expose-headers
  • Added more description in the documentation blocks

Sources:
CORS spec: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
Express CORS: https://github.com/expressjs/cors/blob/c49ca10e92ac07f98a3b06783d3e6ba0ea5b70c7/lib/index.js

Copy link
Owner

@keroxp keroxp left a comment

Choose a reason for hiding this comment

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

Oh, it's really significant improvement! Thanks @LuisEGR !

@keroxp keroxp merged commit 2e04fe1 into keroxp:main Jul 21, 2021
@LuisEGR
Copy link
Contributor Author

LuisEGR commented Jul 21, 2021

It is a pleasure to help!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants