GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix: strip CORS headers from applications by code-asher · Pull Request #8057 · coder/coder

Conversation

code-asher commented Jun 15, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

code-asher changed the title ~~Strip CORS headers from applications~~ fix: strip CORS headers from applications

Jun 15, 2023

github-actions Bot assigned code-asher

Jun 15, 2023

code-asher force-pushed the asher/override-cors branch 16 times, most recently from e84d992 to 9aa2e85 Compare

June 16, 2023 00:54

code-asher commented

Jun 16, 2023

View reviewed changes

Comment thread

coderd/workspaceapps/apptest/apptest.go Outdated

Comment on lines 1301 to 1316

code-asher Jun 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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 the require.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...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Emyrk Jun 20, 2023

Copy link

Copy Markdown

Member

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 to Vary: origin. So this is fine imo.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

code-asher Jun 20, 2023

Copy link

Copy Markdown

Member Author

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 two Origin 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

code-asher marked this pull request as ready for review

June 16, 2023 01:21

code-asher requested a review from Emyrk

June 16, 2023 01:21

bpmct reviewed

Jun 16, 2023

View reviewed changes

bpmct left a comment

Copy link

Copy Markdown

Member

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 preflights here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

code-asher commented Jun 20, 2023

Copy link

Copy Markdown

Member Author

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 preflights here.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Strip CORS headers from applications

4fabc54

The problem is that 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 and also aligns with the original RFC.

code-asher force-pushed the asher/override-cors branch from 9aa2e85 to 4fabc54 Compare

June 20, 2023 15:54

Emyrk approved these changes

Jun 20, 2023

View reviewed changes

Emyrk left a comment

Copy link

Copy Markdown

Member

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.

LGTM 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

coderd/workspaceapps/proxy.go

Comment on lines +550 to +561

		varies := r.Header.Values(httpmw.VaryHeader)
		r.Header.Del(httpmw.VaryHeader)
		forbiddenVary := []string{
		httpmw.OriginHeader,
		httpmw.AccessControlRequestMethodsHeader,
		httpmw.AccessControlRequestHeadersHeader,
		}
		for _, value := range varies {
		if !slice.ContainsCompare(forbiddenVary, value, strings.EqualFold) {
		r.Header.Add(httpmw.VaryHeader, value)
		}
		}

Emyrk Jun 20, 2023

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

code-asher Jun 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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 might Vary based on headers other than CORS-related headers, so I am adding back the ones that are not related to CORS. So if you have Vary: Access-Control-Request-Methods and Vary: X-Foobar the former gets removed while the latter gets kept.

Edit: I said the wrong thing initially, the latter gets kept not axed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Emyrk reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

coderd/workspaceapps/apptest/apptest.go Outdated

Comment on lines 1301 to 1316

Emyrk Jun 20, 2023

Copy link

Copy Markdown

Member

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 to Vary: origin. So this is fine imo.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bpmct commented Jun 21, 2023

Copy link

Copy Markdown

Member

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 preflights here.

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

Somehow I totally missed the docs changes when I initially reviewed. All LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

code-asher merged commit 96f9e61 into main

Jun 21, 2023

code-asher deleted the asher/override-cors branch

June 21, 2023 21:41

github-actions Bot locked and limited conversation to collaborators

Jun 21, 2023

Reviewers

bpmct

bpmct left review comments

Emyrk

Emyrk approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

code-asher

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle case where application sends conflicting CORS headers

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

code-asher commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-asher Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

code-asher Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

bpmct left a comment

Choose a reason for hiding this comment

Uh oh!

code-asher commented Jun 20, 2023

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Emyrk Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

code-asher Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk Jun 20, 2023

Choose a reason for hiding this comment

Uh oh!

bpmct commented Jun 21, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

code-asher commented Jun 15, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

code-asher Jun 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

code-asher Jun 20, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.