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

Rewrite the CORS middleware #237

Merged
merged 12 commits into from
Apr 21, 2022
Merged

Rewrite the CORS middleware #237

merged 12 commits into from
Apr 21, 2022

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Mar 21, 2022

Closes #194

Motivation

The CORS middleware could use some streamlining internally and also doesn't offer all of the functionality that people want from it.

Solution

This PR basically rewrites how the cors::ResponseFuture is built (but changes very little about how its structure and Future impl). It builds on my previous refactorings, but still amounts to a very large diff (mostly due to reorganizing how header values are obtained). It might be easier to read the new module entirely than to look at the diff

Notes

This is a work in progress, I still need to

  • Add missing documentation for the new types
  • Add more examples to the method documentation
  • Add more constructors for the various new types (AllowOrigin, ExposeHeaders and such)
  • Fix Vary header application (currently there's just a hardcoded list of headers there, which seems wrong)
  • Add a new constructor that's more permissive than Cors::permissive() (potential names would be permissive_with_credentials, very_permissive, extremely_permissive, anything_goes)
  • Create internal sub-modules?
  • Panic if AllowCredentials::yes() is combined with Any headers / origin / ...
  • Remove allow-credentials from permissive()
  • Add #[must_use] to most / all of the types in the cors module
  • Fix broken intra-doc links

@jplatte jplatte marked this pull request as draft March 21, 2022 11:52
@jplatte jplatte force-pushed the jplatte/cors-rewrite branch 5 times, most recently from bb6071e to 663dd1d Compare March 26, 2022 14:28
@jplatte jplatte marked this pull request as ready for review March 26, 2022 14:28
@jplatte
Copy link
Collaborator Author

jplatte commented Mar 26, 2022

Still one last thing left to do but if CI passes there should be no more rebases (the last change will be a separate commit).

@jplatte
Copy link
Collaborator Author

jplatte commented Apr 7, 2022

Hello, #237 looks great ! I'm also waiting for this PR to solve the issue apollographql/router#792

@bnjjj in #244 (comment)

I just wanted to highlight that this PR as-is does not change what happens when there is no origin.

I read that it's a best practice to include Vary: Origin in the response headers when there is no origin header in the request so I wanted to look into that, but I don't really get why full CORS headers should be returned to requests without an origin, because the origin header is a critical part of what makes a request a CORS request. Can you explain your use case?

@jplatte
Copy link
Collaborator Author

jplatte commented Apr 7, 2022

Well, scratch that. The origin is now no longer required for CORS stuff to be sent, since I didn't really see the downside to changing it that way and it even simplified the code.

The closure CORS settings constructors still have an origin parameter though, and will not run if there is no origin. Instead, when a closure is configured, we just don't set the corresponding header. I think this should be sufficient for all the use cases I've heard of so far, but with the new design it should be backwards-compatible to introduce a new closure variant without the origin and stuff like that anyways.

This is now finally ready for review! 🎉

@jplatte jplatte requested a review from davidpdrsn April 7, 2022 19:38
pan93412 added a commit to UnblockNeteaseMusic/server-rust that referenced this pull request Apr 18, 2022
Note that CORS layer can't work since
tower-rs/tower-http#237
has not been released yet.
@jplatte jplatte added the breaking change A PR that makes a breaking change. label Apr 19, 2022
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I think this looks great! Thanks for working on it.

Wanna add a changelog entry as well?

@jplatte
Copy link
Collaborator Author

jplatte commented Apr 21, 2022

But why have only one changelog entry when you can have five? 😄

@davidpdrsn davidpdrsn enabled auto-merge (squash) April 21, 2022 19:39
@davidpdrsn davidpdrsn merged commit 9299ba5 into master Apr 21, 2022
@davidpdrsn davidpdrsn deleted the jplatte/cors-rewrite branch April 21, 2022 19:41
@davidpdrsn davidpdrsn mentioned this pull request Apr 21, 2022
4 tasks
@bnjjj
Copy link

bnjjj commented Apr 25, 2022

@davidpdrsn Just out of curiosity, I need this new cors middleware in Axum, do you plan to release a new version soon ?

@davidpdrsn
Copy link
Member

I do yes. Once #250 has landed I'll prepare a release. Until then you can depend on tower-http via a git dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preflight response access control headers should match request
3 participants