Skip to content

Simplify and fix header issue with the current CORS implementation. #454

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IamfromSpace
Copy link
Contributor

This defers more to the corser middleware that's already in use to reduce code and chances for error in the re-implementation. This also fixes an issue where disallowed headers would be allowed.

…ore to the corser middleware that's already in use to reduce code and chances for error. This also fixes an issue where disallowed headers would be allowed.
@BigBlueHat BigBlueHat added the minor version non-breaking, non-trivial change label Aug 6, 2018
@BigBlueHat
Copy link
Member

Code looks good. I'll try to make time to run tests locally soon.

If anyone else can spare some time to 👍 this (or otherwise), I'd appreciate it!

@BigBlueHat
Copy link
Member

@IamfromSpace I'm also curious to get your thoughts on #434 and how it would relate to this commit--as it passes more config options to corser and explicitly sets Access-Control-Allow-Credentials.

I'd like to avoid proliferating to many interrelated parameters if we can avoid it (i.e. I'd rather "extend" --cors than add --credentials...which magically enables --cors in #434).

@senaev as the author of #434 your thoughts here would also be great. Thanks!

@IamfromSpace
Copy link
Contributor Author

This would certainly make #434 simpler to implement, as it's easier to interface with the corser middleware. Admittedly a motive of mine here was to get it down a path where we could explicitly specify origins to increase security around serving sensitive files locally. Happy to leave that discussion to another PR though, as this change seemed worthwhile on its own.

The interplay between related params does become tricky (and personally, I have security concerns around enabling Credentials for '*' origin). I think interplay/security are still open discussion points for any PR that extends CORS.

@thornjad thornjad self-requested a review April 16, 2019 13:19
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

I like this, except that the tests format has changed since this PR was opened and has merge conflicts. If those are fixed, I'll approve

@thornjad thornjad added this to the v14.0 milestone Jul 16, 2021
@thornjad thornjad removed this from the v14.0 milestone Oct 11, 2021
@thornjad thornjad added fix patch version Small, non-breaking, bug fix or trivial change and removed minor version non-breaking, non-trivial change labels Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix patch version Small, non-breaking, bug fix or trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants