- Notifications
You must be signed in to change notification settings - Fork928
fix: strip CORS headers from applications#8057
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e84d992 to9aa2e85Compare| // Somehow there are two "Origin"s in Vary even though there should only be | ||
| // one (from the CORS middleware), even if you remove the headers being sent | ||
| // above. When I do nothing else but change the expected value below to | ||
| // have two "Origin"s suddenly Vary only has one. It is somehow always the | ||
| // opposite of whatever I put for the expected. So, reluctantly, remove | ||
| // duplicate "Origin" values. | ||
| vardeduped []string | ||
| varaddedOriginbool | ||
| for_,value:=rangeresp.Header.Values("Vary") { | ||
| ifvalue!="Origin"||!addedOrigin { | ||
| ifvalue=="Origin" { | ||
| addedOrigin=true | ||
| } | ||
| deduped=append(deduped,value) | ||
| } | ||
| } |
code-asherJun 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I must be going mad, I have no idea how the response headers could change in perfect opposition to what I pass into therequire.Equal for the expected value.
Examples of it swapping:
https://github.com/coder/coder/actions/runs/5284785388/jobs/9562577501
https://github.com/coder/coder/actions/runs/5284691553/jobs/9562383538
And all I did was add"Origin", to the check.
It happens consistently despite multiple attempts, always exactly the opposite. Could be a flake and just a coincidence that it always lines up with me changing the expected value...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Headers should be case insensitive anyway.Vary: origin,Origin should be equal toVary: origin. So this is fine imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oddly it was twoOrigin strings, same capitalization. It seems so bizarre that the headers returned could change based on how I change completely unrelated code, so bizarre that I feel I must be doing something dunderheaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we need to change this section in our docs?
Applications can set their own headers which will override the defaults but this will only apply to non-preflight requests. Preflight requests through the dashboard are never sent to applications and thus cannot be modified by them. Read more about the difference between simple requests and requests that trigger preflightshere.
Yup, this PR also makes changes to this section. Let me know if you have any thoughts on the wording!https://github.com/coder/coder/pull/8057/files#diff-fee7817d909d710ad6c6a59e4a5fc65ea0b7608949e74c45e3465b1f51e5cc99R127-R132 |
The problem is that the headers get doubled up (not overwritten) andbrowsers do not like multiple values for the allowed origin even thoughit appears the spec allows for it.We could prefer the application's headers instead of ours but since wecontrol OPTIONS I think preferring ours will by the more consistentexperience and also aligns with the original RFC.
9aa2e85 to4fabc54CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM 👍
| varies:=r.Header.Values(httpmw.VaryHeader) | ||
| r.Header.Del(httpmw.VaryHeader) | ||
| forbiddenVary:= []string{ | ||
| httpmw.OriginHeader, | ||
| httpmw.AccessControlRequestMethodsHeader, | ||
| httpmw.AccessControlRequestHeadersHeader, | ||
| } | ||
| for_,value:=rangevaries { | ||
| if!slice.ContainsCompare(forbiddenVary,value,strings.EqualFold) { | ||
| r.Header.Add(httpmw.VaryHeader,value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is this part doing?
code-asherJun 20, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I figure an application mightVary based on headers other than CORS-related headers, so I am adding back the ones that are not related to CORS. So if you haveVary: Access-Control-Request-Methods andVary: X-Foobar the former gets removed while the latter gets kept.
Edit: I said the wrong thing initially, the latter gets kept not axed.
| // Somehow there are two "Origin"s in Vary even though there should only be | ||
| // one (from the CORS middleware), even if you remove the headers being sent | ||
| // above. When I do nothing else but change the expected value below to | ||
| // have two "Origin"s suddenly Vary only has one. It is somehow always the | ||
| // opposite of whatever I put for the expected. So, reluctantly, remove | ||
| // duplicate "Origin" values. | ||
| vardeduped []string | ||
| varaddedOriginbool | ||
| for_,value:=rangeresp.Header.Values("Vary") { | ||
| ifvalue!="Origin"||!addedOrigin { | ||
| ifvalue=="Origin" { | ||
| addedOrigin=true | ||
| } | ||
| deduped=append(deduped,value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Headers should be case insensitive anyway.Vary: origin,Origin should be equal toVary: origin. So this is fine imo.
Somehow I totally missed the docs changes when I initially reviewed. All LGTM. |
Uh oh!
There was an error while loading.Please reload this page.
This is a followup to#7688
The problem is that if applications send their own CORS headers the headers get doubled up (not overwritten) and browsers do not like multiple values for the allowed origin even though it appears the spec allows for it.
We could prefer the application's headers instead of ours but since we control OPTIONS I think preferring ours will by the more consistent experience across the board and also aligns with the original RFC.
Closes#8010.