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

Implemented CORS header handler middleware #262

Merged
merged 22 commits into from
Jun 6, 2019

Conversation

k-nasa
Copy link
Contributor

@k-nasa k-nasa commented May 25, 2019

Description

Implemented CORS header handler middleware.

Motivation and Context

close: #215

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Small nit, but this is really good!

Copy link
Contributor

@prasannavl prasannavl left a comment

Choose a reason for hiding this comment

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

Thanks @k-nasa!

A few more bits:

  • One of the reasons I layed down the current differences in implementation here (Implement a way to handle CORS headers #215 (comment)) was to help deciding which way to approach from.
  • If we just want to start without validations, then I think it makes sense to start with this implementation. However I think we should leave out the features that don't make sense without proper validations entirely rather than try to cover them partially which ends up in unexpected or confusing behaviour.
  • Any lack in feature set of the middleware during the initial phases, can always easily be compensated by directly adding the headers.

Copy link
Contributor

@prasannavl prasannavl 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 now! A few trivial bits.

@k-nasa
Copy link
Contributor Author

k-nasa commented May 28, 2019

@yoshuawuyts @prasannavl I fixed all! Please re review 🙇

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This is looking really good!

@yoshuawuyts yoshuawuyts requested a review from prasannavl May 28, 2019 15:23
@fairingrey
Copy link
Contributor

Looks great, thanks for your contribution! 👍

Copy link
Contributor

@prasannavl prasannavl left a comment

Choose a reason for hiding this comment

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

Okay, so just went through the spec - while many production cases are likely constrained to single origin where should work, I think this is impossible to get right for without validation for atleast origin to support the case of multiple origins.

  • Anything other than a single origin, we need to validate and send back the current one only or fail with 400 or 401.
  • When the requests have credentials, or when allow-credentials is true, origin of * will fail according to the spec.

Given this, I think the simplest way to make this work is to add origin validation to each and get the input instead as an enum Origin of Any, Exact(String/HeaderValue) or List([String]/[HeaderValue]) and then validate the origin on each request matching on the these, and sending back the actual origin directly for the Any case or sending the matched origin case otherwise, or fail with 400/401.

Alternatively, we need to parse the string header, and do the classification manually which looks a bit fragile, even though it's likely just a split on , (comma). With the enum route, we can even expand it later to support regexes or wildcarded subdomains. Ideally, we probably need to do this with a builder pattern, say CorsBuilder and get the middleware out of it, but we could do this later.

Approaching from this route, I think if a simple origin check can be done as above, we'll have a fully working middleware and we can slowly expand it to more sophisticated server side validations similar to the actix middleware.

@k-nasa - let me know if this some thing you'd like to do!

/cc @gruberb

@k-nasa
Copy link
Contributor Author

k-nasa commented May 29, 2019

@prasannavl I think it's good! A lot of things to do!

@k-nasa
Copy link
Contributor Author

k-nasa commented Jun 1, 2019

Should these extensions be done with this PR?

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

LGTM

@yoshuawuyts yoshuawuyts dismissed prasannavl’s stale review June 6, 2019 12:10

All outstanding issues have been addressed!

@yoshuawuyts
Copy link
Member

I say we go ahead and merge this! -- thanks heaps @k-nasa!

@yoshuawuyts yoshuawuyts merged commit 1fb4ab3 into http-rs:master Jun 6, 2019
@k-nasa k-nasa deleted the feature_cors branch June 6, 2019 12:33
@k-nasa k-nasa mentioned this pull request Jun 12, 2019
8 tasks
yoshuawuyts added a commit that referenced this pull request Nov 3, 2019
Signed-off-by: Yoshua Wuyts <[email protected]>
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.

Implement a way to handle CORS headers
5 participants