Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use cors module instead of custom cors logic #2823

Merged
merged 2 commits into from
Mar 26, 2021
Merged

Use cors module instead of custom cors logic #2823

merged 2 commits into from
Mar 26, 2021

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Mar 21, 2021

fixes #2762

  • adds support for regex new regex env variable COMPANION_CLIENT_ORIGINS_REGEX
  • corresponding option for the companion object corsOrigins which equals to cors origin config
  • encapsulate custom cors headers merge logic in own middleware
  • pull out non cors logic from middleware
  • unit test the cors middleware

NOTE: that returned headers are probably not exactly the same as before (there were no unit tests for cors), but it will be according to the cors module, so it should be a more "standard" behaviour.

- adds support for regex COMPANION_CLIENT_ORIGINS_REGEX
- encapsulate custom cors header merge logic in own middleware
- pull out non cors logic from middleware
- unit test the cors middleware
Copy link
Contributor

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, and great that you were explicit where we needed to diverge from default cors behavior

packages/@uppy/companion/src/server/middlewares.js Outdated Show resolved Hide resolved
Co-authored-by: Julian Gruber <julian@juliangruber.com>
Copy link
Contributor

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great, hard to predict if it will have any breaking consequences so I guess there's one way to find out 😇

@arturi
Copy link
Contributor

arturi commented Mar 25, 2021

that returned headers are probably not exactly the same as before

Can this break if someone is running Uppy 1.3 from 1 year ago and Companion upgrades on Transloadit servers?

@mifi
Copy link
Contributor Author

mifi commented Mar 26, 2021

Is there anything special about uppy 1.3? I can't really guarantee that nothing will break, but I'm using the cors module in a pretty standard way, and that module is used by thousands of devs with I imagine millions of users, so it should work according to normal CORS behavior. Whether there have been some historic quirks/hacks throughout the history of uppy, that could be affected by this, is beyond me.

Differences in behaviour that I can think of are:

@goto-bus-stop
Copy link
Contributor

goto-bus-stop commented Mar 26, 2021

I think uppy 1.3 was just a random example of an old version. People might still have them if they started using the CDN packages for example and never updated the URLs.
Those differences do all sound like they should be compatible.

@arturi
Copy link
Contributor

arturi commented Mar 26, 2021

Whether there have been some historic quirks/hacks throughout the history of uppy, that could be affected by this, is beyond me.

Yeah, I understand. I did mean 1.3 or 1.5 as an arbitrary example of old versions.

What I can remember was backwards-compat breaking is this: #1564

@arturi arturi merged commit 415681f into master Mar 26, 2021
@arturi arturi deleted the cors-rewrite branch March 26, 2021 19:38
@arturi
Copy link
Contributor

arturi commented Mar 26, 2021

Here goes! Let's keep an eye out for potential issues. Thanks, Mikael!

HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
* use cors module instead of custom cors logic transloadit#2762

- adds support for regex COMPANION_CLIENT_ORIGINS_REGEX
- encapsulate custom cors header merge logic in own middleware
- pull out non cors logic from middleware
- unit test the cors middleware

* fix capitalization

Co-authored-by: Julian Gruber <julian@juliangruber.com>

Co-authored-by: Julian Gruber <julian@juliangruber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow regex CORS config
4 participants