Require origin whitelist for CORS middleware #72
Conversation
RavenNumber of Findings: 7
The finding(s) listed above require security review. |
| if r.Method == "OPTIONS" { | ||
| err = &corsError{code: http.StatusOK} | ||
| } | ||
| if !originMatch { |
There was a problem hiding this comment.
Do you care that your first error will be overwritten if both error conditions are true?
|
| error | ||
| Code() int | ||
| Response() []byte | ||
| type MiddlewareError struct { |
There was a problem hiding this comment.
Looks like this no longer implements error
There was a problem hiding this comment.
Doesn't need to.
There was a problem hiding this comment.
Looks like it's killing the unit tests
|
+1 |
|
security +1 |
|
|
||
| var err *rest.MiddlewareError | ||
| if r.Method == "OPTIONS" { | ||
| err = &rest.MiddlewareError{Code: http.StatusOK} |
There was a problem hiding this comment.
Should the order of these statements be switched? Right now it would always allow options requests, which would indicate to browsers they're allowed to send other requests?
There was a problem hiding this comment.
I believe OPTIONS requests are allowed to pass through as they are a pre-flight request, but the response would not have the Access-Control-* headers on the response if the origin doesn't match. As a result, the browser would know the actual request would not be processed.
|
+1 |
1 similar comment
|
+1 |
|
RM +1 verified dependencies manually |
|
QA review can be found here: https://jira.atl.workiva.net/browse/RM-16257 |
Require an origin whitelist for CORS middleware. Reject requests having an origin that does not match the whitelist. This is a breaking API change but is justified due to the security nature.
@matthewsullivan-wf @Workiva/messaging-pp