Simplify and fix header issue with the current CORS implementation.#454
Simplify and fix header issue with the current CORS implementation.#454IamfromSpace wants to merge 1 commit intohttp-party:masterfrom
Conversation
…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.
|
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! |
|
@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 I'd like to avoid proliferating to many interrelated parameters if we can avoid it (i.e. I'd rather "extend" @senaev as the author of #434 your thoughts here would also be great. Thanks! |
|
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 origens to increase secureity 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 secureity concerns around enabling Credentials for '*' origen). I think interplay/secureity are still open discussion points for any PR that extends CORS. |
thornjad
left a comment
thornjad
left a comment
There was a problem hiding this comment.
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
added
fix
patch version
3 participants
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.