GeezerVison

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

GeezerVison is showing github.com — feat: bypass built-in CORS handling for workspace apps by dannykopping · Pull Request #15669 · coder/coder

Conversation

dannykopping commented Nov 27, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

NOTE: this PR is blocked; we want to first introduce this feature to port shares, as it has transpired that that use-case is far more common. It would be awkward to get this PR into the next release but not for port shares, so we're keeping this one on hold.

This PR introduces a partial fix for #15096; it allows operators to specify the CORS behavior on a per-coder_app level.

This change requires cors_behavior to be set on a given coder_app; coder/terraform-provider-coder#309 introduces that new attribute.

coderd currently handles CORS automatically by handling preflight requests and stripping CORS headers from upstream coder_app responses.

Two CORS behaviors are defined in this PR:

simple: the current behavior of handling CORS within coderd
passthru: new behavior which transparently bypasses our CORS handling in coderd so the coder_app service is then fully responsible for handling CORS

We plan to further add this behavior to port shares.

The sharing level (owner, authenticated, public) is still respected, regardless of CORS behavior.

Sorry, something went wrong.

Uh oh!

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

All reactions

github-actions Bot assigned dannykopping

Nov 27, 2024

dannykopping commented

Nov 27, 2024

View reviewed changes

Comment thread

coderd/workspaceapps/proxy.go

		return
		}

		// Use the passed in app middlewares before checking authentication and

dannykopping Nov 27, 2024

Copy link

Copy Markdown

Contributor 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.

Note to reviewers: this is the meat of the change.
I had to move some things around here since the token was only being created after CORS was handled, and the defined behavior is stored in the token.

I moved the CORS middleware invocation into determineCORSBehavior because we needed to conditionally invoke it if behavior != passthru, only.

Sorry, something went wrong.

Uh oh!

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

All reactions

dannykopping force-pushed the dk/cors branch 2 times, most recently from c5c74d1 to ec3bac6 Compare

November 27, 2024 12:11

dannykopping mentioned this pull request

Nov 27, 2024

Workspace applications CORS handling prevents external requests to apps #15096

Closed

dannykopping force-pushed the dk/cors branch 2 times, most recently from 4c6beb5 to f91f220 Compare

November 28, 2024 11:12

dannykopping added 9 commits

November 28, 2024 12:04


          Working implementation

0dc7ca3

Signed-off-by: Danny Kopping <[email protected]>


          Rename CorsBehavior to CORSBehavior

3c0e33a

Signed-off-by: Danny Kopping <[email protected]>


          Hard & simplify CORORS handling logic

a4d3c8d

Signed-off-by: Danny Kopping <[email protected]>


          Minor fixes

873c3e4

Signed-off-by: Danny Kopping <[email protected]>


          Appeasing the linter

65f984f

Signed-off-by: Danny Kopping <[email protected]>


          Move type def to codersdk

1f0b34c

Signed-off-by: Danny Kopping <[email protected]>


          Appease the linter, again

5be5477

Signed-off-by: Danny Kopping <[email protected]>


          Fix migration num

ddbbaa9

Signed-off-by: Danny Kopping <[email protected]>


          Removing unnecessary header check

63c1852

Signed-off-by: Danny Kopping <[email protected]>

dannykopping force-pushed the dk/cors branch from 79ce356 to 63c1852 Compare

November 28, 2024 14:21

dannykopping changed the title ~~feat: bypass built-in CORS handling~~ feat: bypass built-in CORS handling for workspace apps

Nov 28, 2024

dannykopping added 3 commits

November 28, 2024 14:56


          Improve tests

976cd78

Signed-off-by: Danny Kopping <[email protected]>


          Merge branch 'main' of https://github.com/coder/coder into dk/cors

9f9068f


          Comprehensive CORS test suite for both request & response

3dc00e1

Signed-off-by: Danny Kopping <[email protected]>

This was referenced Nov 29, 2024

feat: bypass built-in CORS handling for shared ports #15684

Closed

chore: add tests for default CORS behavior #15685

Closed

github-actions Bot added the stale This issue is like stale bread. label

Dec 6, 2024

github-actions Bot closed this

Dec 9, 2024

dannykopping commented Dec 9, 2024

Copy link

Copy Markdown

Contributor Author

Reopening; this has been deprioritized slightly but not forgotten.

All reactions

Sorry, something went wrong.

Uh oh!

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

dannykopping reopened this

Dec 9, 2024

github-actions Bot removed the stale This issue is like stale bread. label

Dec 10, 2024

github-actions Bot added the stale This issue is like stale bread. label

Dec 19, 2024

github-actions Bot closed this

Dec 22, 2024

Emyrk mentioned this pull request

Apr 28, 2025

feat: bypass built-in CORS handling for workspace apps #17596

Closed

github-actions Bot deleted the dk/cors branch

May 30, 2025 00:07

cstyan mentioned this pull request

Jul 9, 2025

feat: allow bypassing current CORS magic based on template config #18706

Merged

cstyan added a commit that referenced this pull request

Jul 30, 2025


          feat: allow bypassing current CORS magic based on template config (#1…

ffbfaf2

…8706)

Solves #15096

This is a slight rework/refactor of the earlier PRs from @dannykopping
and @Emyrk:
- #15669
- #15684
- #17596

Rather than having a per-app CORS behaviour setting and additionally a
template level setting for ports, this PR adds a single template level
CORS behaviour setting that is then used by all apps/ports for
workspaces created from that template.

The main changes are in `proxy.go` and `request.go` to:
a) get the CORS behaviour setting from the template
b) have `HandleSubdomain` bypass the CORS middleware handler if the
selected behaviour is `passthru`
c) in `proxyWorkspaceApp`, do not modify the response if the selected
behaviour is `passthru`

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added support for configuring CORS behavior ("simple" or "passthru")
at the template level for all shared ports.
* Introduced a new "CORS Behavior" setting in the template creation and
settings forms.
* API endpoints and responses now include the optional `cors_behavior`
property for templates.
* Workspace apps and proxy now honor the specified CORS behavior,
enabling conditional CORS middleware application.
* Enhanced workspace app tests with comprehensive scenarios covering
CORS behaviors and authentication states.

* **Bug Fixes**
  * None.

* **Documentation**
* Updated API and admin documentation to describe the new
`cors_behavior` property and its usage.
* Added examples and schema references for CORS behavior in relevant API
docs.

* **Tests**
* Extended automated tests to cover different CORS behavior scenarios
for templates and workspace apps.

* **Chores**
* Updated audit logging to track changes to the `cors_behavior` field on
templates.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Callum Styan <[email protected]>

Reviewers

spikecurtis

Awaiting requested review from spikecurtis

spikecurtis will be requested when the pull request is marked ready for review

spikecurtis is a code owner

mafredri

Awaiting requested review from mafredri

mafredri will be requested when the pull request is marked ready for review

mafredri is a code owner

Uh oh!

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

Assignees

dannykopping

Labels

stale

This issue is like stale bread.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

1 participant

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

dannykopping commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannykopping Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

dannykopping commented Dec 9, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dannykopping commented Nov 27, 2024 •

edited

Loading

Uh oh!

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