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

Improve CORS Method Middleware #477

Merged
merged 17 commits into from Jun 29, 2019

Conversation

fharding1
Copy link
Member

@fharding1 fharding1 commented May 27, 2019

This PR implements the behavior outlined in #476:

  • Sets Access-Control-Allow-Methods on all requests to a route with an OPTIONS method matcher
  • Does not return after setting the Access-Control-Allow-Methods header
  • Does not append OPTIONS header to Access-Control-Allow-Methods regardless of whether there is an OPTIONS method matcher. Only returns OPTIONS if there is an actual matcher
  • Adds tests for the above behavior

Closes #476

@fharding1
Copy link
Member Author

fharding1 commented May 27, 2019

It looks like CI is failing on Go < 1.11 due to diff -u <(echo -n) <(gofmt -d .). Likely related to golang/go#26228. Not sure what I should do about this. Should CI be modified to only check the gofmt diff on Go >= 1.11?

middleware.go Outdated Show resolved Hide resolved
@fharding1
Copy link
Member Author

fharding1 commented May 27, 2019

I've modified the travis CI config to only check the gofmt diff on the latest Go version, let me know if that is undesirable for some reason.

@elithrar
Copy link
Member

elithrar commented May 27, 2019

@elithrar
Copy link
Member

elithrar commented Jun 17, 2019

Can you summarize the breaking changes / major behavioral changes (same thing) for the change-log, too?

@fharding1
Copy link
Member Author

fharding1 commented Jun 17, 2019

Sure! I think these are all of them:

Breaking changes to CORSMethodMiddleware:

  • Only sets the Access-Control-Allow-Methods header if an OPTIONS method matcher exists for the route
  • Sets Access-Control-Allow-Methods on all requests to a route using CORSMethodMiddleware that have an OPTIONS method matcher, not just OPTIONS/preflight requests
  • Does not return after setting the header, continues to request handler
  • Does not append OPTIONS to allowed methods. The header will be set to the list of method matchers for a route, that's all

Copy link
Member

@elithrar elithrar left a comment

See comments.

As I've reviewed this, my overall feedback is that the .Methods(http.MethodOptions) requirement will continue to be a sharp edge for users, and we should document it clearly if we are not automatically handling it for them.

.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example_cors_method_middleware_test.go Outdated Show resolved Hide resolved
middleware.go Outdated Show resolved Hide resolved
@fharding1 fharding1 force-pushed the improve-cors-method-middleware branch from c79f90b to 38f5d51 Compare Jun 28, 2019
@fharding1 fharding1 force-pushed the improve-cors-method-middleware branch from 7ca8d93 to 794440e Compare Jun 29, 2019
@fharding1
Copy link
Member Author

fharding1 commented Jun 29, 2019

Updated the circleci config to only check the gofmt diff on the latest go version like I did for travis originally. Also that github integration with circleci is clean 🙌

@elithrar
Copy link
Member

elithrar commented Jun 29, 2019

Thanks! I’ll update the base CI template to do that + run lint where possible too^

^ A couple of Gorilla packages fail golint due to Http vs. HTTP casing.

.circleci/config.yml Show resolved Hide resolved
@elithrar
Copy link
Member

elithrar commented Jun 29, 2019

@elithrar elithrar merged commit 0534769 into gorilla:master Jun 29, 2019
@elithrar
Copy link
Member

elithrar commented Jun 29, 2019

Thanks again @fharding1! 👍

thaJeztah added a commit to thaJeztah/docker that referenced this pull request Oct 6, 2019
full diff: gorilla/mux@ed099d4...00bdffe

changes included:

- gorilla/mux#477 Improve CORS Method Middleware
    - implements gorilla/mux#477 Make CORSMethodMiddleware actually make sense
- gorilla/mux#489 Fix nil panic in authentication middleware example

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Oct 7, 2019
full diff: gorilla/mux@ed099d4...00bdffe

changes included:

- gorilla/mux#477 Improve CORS Method Middleware
    - implements gorilla/mux#477 Make CORSMethodMiddleware actually make sense
- gorilla/mux#489 Fix nil panic in authentication middleware example

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: bb5650619e26cf0b79f30bafc551e5120a0643a4
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this pull request Oct 21, 2019
full diff: gorilla/mux@ed099d4...00bdffe

changes included:

- gorilla/mux#477 Improve CORS Method Middleware
    - implements gorilla/mux#477 Make CORSMethodMiddleware actually make sense
- gorilla/mux#489 Fix nil panic in authentication middleware example

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: zach <Zachary.Joyner@linux.com>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Oct 23, 2019
full diff: gorilla/mux@ed099d4...00bdffe

changes included:

- gorilla/mux#477 Improve CORS Method Middleware
    - implements gorilla/mux#477 Make CORSMethodMiddleware actually make sense
- gorilla/mux#489 Fix nil panic in authentication middleware example

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker/docker-ce that referenced this pull request Dec 4, 2019
full diff: gorilla/mux@ed099d4...00bdffe

changes included:

- gorilla/mux#477 Improve CORS Method Middleware
    - implements gorilla/mux#477 Make CORSMethodMiddleware actually make sense
- gorilla/mux#489 Fix nil panic in authentication middleware example

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 4b5ecc982a441b26846c4240c30b03f940950b31
Component: cli
>
< HTTP/1.1 200 OK
< Access-Control-Allow-Methods: GET,PUT,PATCH,OPTIONS
< Access-Control-Allow-Origin: *

Choose a reason for hiding this comment

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

The docs added above said

You will still need to use your own CORS handler to set the other CORS headers such as Access-Control-Allow-Origin

But the example shows that header is added, which is it?

And what's the relationship of this middleware with github.com/gorilla/handlers/CORS?

I'm confused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CORSMethodMiddleware actually make sense
3 participants