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

[WIP] Add a fairing for CORS support #930

Closed
wants to merge 2 commits into from

Conversation

richard-uk1
Copy link

This commit adds a fairing to support CORS.

It works by adding CORS headers to all responses, and also catching 404 OPTIONS requests - returning a 200 response with the correct cors headers as per configuration.

It probably needs some work on the docs before it is merged, and also I didn't realise that my text editor runs cargo fmt by default, so either a separate commit needs to go in to just run cargo fmt on the existing codebase, or I need to revert all those changes :'(.

I'm opening it now to see how you feel about the approach. It works well for the project I'm using it in, but I'm interested to see if that is replicated in other projects.

@jebrosen
Copy link
Collaborator

This commit adds a fairing to support CORS.

It works by adding CORS headers to all responses, and also catching 404 OPTIONS requests - returning a 200 response with the correct cors headers as per configuration.

I haven't looked much at the implementation yet, partly because of the large diff. Have you seen https://github.com/lawliet89/rocket_cors or @SergioBenitez's Responder example in #25 (comment)? How does this compare to those? I assume from the documentation in this PR that it essentially only operates in the "Fairing" mode of rocket_cors.

Personally, I think it would be reasonable to provide a "works for the majority" CORS implementation in contrib, and let other crates such as rocket_cors provide more advanced functionality or unusual scenarios. I don't know well enough, however, where the line should be drawn between "works for the majority" vs "unusual scenarios".

I didn't realise that my text editor runs cargo fmt by default, so either a separate commit needs to go in to just run cargo fmt on the existing codebase, or I need to revert all those changes :'(.

That is unfortunate. It shouldn't be too hard to "undo" entire trees that had format-only chages (e.g. core/, contrib/codegen), which should cover most of the noise. I'd be happy to give further advice or assistance with this effort.

@SergioBenitez
Copy link
Member

SergioBenitez commented May 11, 2019

Closing due to inactivity (but this is something that we would like, heeding @jebrosen's commentary above).

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label May 11, 2019
@richard-uk1
Copy link
Author

I've been working on another project, but now I'm back to rust web :).

It may not, however, abort or respond directly to the request; these issues are better handled via request guards or via response callbacks.

Does this mean that CORS should be implemented as a RequestGuard? The above statement seems to indicate yes, however you probably want the guard on all requests, so it seems safer to apply it as a blanket rather than on route callbacks where they could potentially be forgotten?

I don't know the right answer so interested to hear thoughts.

@richard-uk1
Copy link
Author

I've thought about this some more, and I think Fairings should be allowed to interrupt the usual flow and respond in the request stage of the middleware traversal. I know there are times that a guard is more appropriate, but CORS is something that in most cases works the same for the whole website, so implementing it at the fairing level seems to make sense, at least to me.

This would require having a way to say "stop and return a response now" in the request part of the fairing.

@SergioBenitez
Copy link
Member

@derekdreery I agree, and it's planned, but doing it the way other frameworks do it is not a path we'd like to follow for many reasons. See my commentary in #749 (comment) for more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants