Improve CORS Method Middleware by fharding1 · Pull Request #477 · gorilla/mux

fharding1 · 2019-05-27T02:30:18Z

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 · 2019-05-27T02:43:39Z

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

fharding1 · 2019-05-27T17:19:35Z

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 · 2019-05-27T21:02:28Z

Good move - no disagreement from me. Will review in full this week.

…

On Mon, May 27, 2019 at 12:19 PM Franklin Harding ***@***.***> wrote: 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. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#477?email_source=notifications&email_token=AAAEQ4C6RRVUZZ66QCIBUEDPXQJ2PA5CNFSM4HPYDHU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWKHU5I#issuecomment-496269941>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAEQ4BL2FB3QNDRFGCKQMDPXQJ2PANCNFSM4HPYDHUQ> .

elithrar · 2019-06-17T16:09:45Z

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

fharding1 · 2019-06-17T16:14:40Z

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

elithrar

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

README.md

example_cors_method_middleware_test.go

middleware.go

* Only sets Access-Control-Allow-Methods on valid preflight requests * 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 * Adds tests for the listed behavior

…hodMiddleware

fharding1 · 2019-06-29T07:29:12Z

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 · 2019-06-29T16:54:27Z

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

elithrar · 2019-06-29T19:19:50Z

See https://github.com/gorilla/.github/blob/master/config.yml.tmpl as the canonical example!

…

On Sat, Jun 29, 2019 at 12:16 PM Franklin Harding ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .circleci/config.yml <#477 (comment)>: > @@ -11,8 +11,11 @@ jobs: - checkout - run: go version - run: go get -t -v ./... - - run: diff -u <(echo -n) <(gofmt -d .) - - run: if [[ "$LATEST" = true ]]; then go vet -v .; fi + - run: > + if [[ "$LATEST" = true ]]; then + diff -u <(echo -n) <(gofmt -d .) Good call — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#477?email_source=notifications&email_token=AAAEQ4GHOKIY3FHVWVF4IE3P46YIJA5CNFSM4HPYDHU2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5BPVSQ#discussion_r298808793>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAEQ4FM24YN4C275OTHZXTP46YIJANCNFSM4HPYDHUQ> .

elithrar · 2019-06-29T20:52:43Z

Thanks again @fharding1! 👍

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>

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

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>

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>

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

sam-github · 2020-01-18T04:55:27Z

README.md

+> 
+< HTTP/1.1 200 OK
+< Access-Control-Allow-Methods: GET,PUT,PATCH,OPTIONS
+< Access-Control-Allow-Origin: *


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.

fharding1 mentioned this pull request May 27, 2019

Use subtests for middleware tests #478

Merged

fharding1 commented May 27, 2019

View reviewed changes

middleware.go Outdated Show resolved Hide resolved

fharding1 force-pushed the improve-cors-method-middleware branch from 21fd551 to 485bbf3 Compare May 27, 2019 03:27

fharding1 mentioned this pull request May 27, 2019

CORSMethodMiddleware removing unexpected behaviour #459

Closed

fharding1 closed this May 27, 2019

fharding1 reopened this May 27, 2019

fharding1 force-pushed the improve-cors-method-middleware branch from 12176f8 to ff3f999 Compare May 27, 2019 17:14

elithrar self-assigned this Jun 17, 2019

elithrar self-requested a review June 17, 2019 15:55

elithrar added enhancement minor-rev-change needs review labels Jun 17, 2019

elithrar suggested changes Jun 28, 2019

View reviewed changes

fharding1 force-pushed the improve-cors-method-middleware branch from c79f90b to 38f5d51 Compare June 28, 2019 19:51

fharding1 and others added 11 commits June 29, 2019 00:19

Add example for CORSMethodMiddleware

26bc453

Do not check for preflight and add documentation to the README

3148d83

Use http.MethodOptions instead of "OPTIONS"

b6e908d

Add link to CORSMethodMiddleware section to readme

4675152

Add test for unmatching route methods

80c843e

Rename CORS Method Middleware to Handling CORS Requests in README

5f61438

Link CORSMethodMiddleware in README to godoc

6eb5e65

Break CORSMethodMiddleware doc into bullets for readability

091bfb2

Add comment about specifying OPTIONS to example in README for CORSMet…

1f56cda

…hodMiddleware

Document cURL command used for testing CORS Method Middleware

1e92c8a

Franklin Harding added 2 commits June 29, 2019 00:20

Update comment in example to "Handle the request"

10c79f3

Add explicit comment about OPTIONS matchers to CORSMethodMiddleware doc

794440e

fharding1 force-pushed the improve-cors-method-middleware branch from 7ca8d93 to 794440e Compare June 29, 2019 07:21

Update circleci config to only check gofmt diff on latest go version

5931853

elithrar reviewed Jun 29, 2019

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

fharding1 added 2 commits June 29, 2019 12:16

Break up gofmt and go vet checks into separate steps.

65ad3ea

Merge branch 'master' into improve-cors-method-middleware

20a68cc

Use canonical circleci config

f5356bb

elithrar removed the needs review label Jun 29, 2019

elithrar approved these changes Jun 29, 2019

View reviewed changes

elithrar merged commit 0534769 into gorilla:master Jun 29, 2019

thaJeztah mentioned this pull request Oct 6, 2019

Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux moby/moby#40045

Merged

thaJeztah mentioned this pull request Oct 23, 2019

Bump various dependencies docker/cli#2152

Merged

sam-github reviewed Jan 18, 2020

View reviewed changes

dependabot bot mentioned this pull request Mar 11, 2021

Bump github.com/gorilla/mux from 1.7.0 to 1.8.0 awslabs/amazon-ecs-local-container-endpoints#55

Closed

Conversation

fharding1 commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fharding1 commented May 27, 2019

Uh oh!

Uh oh!

fharding1 commented May 27, 2019

Uh oh!

elithrar commented May 27, 2019 via email

Uh oh!

elithrar commented Jun 17, 2019

Uh oh!

fharding1 commented Jun 17, 2019

Uh oh!

elithrar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fharding1 commented Jun 29, 2019

Uh oh!

elithrar commented Jun 29, 2019

Uh oh!

Uh oh!

elithrar commented Jun 29, 2019 via email

Uh oh!

elithrar commented Jun 29, 2019

Uh oh!

sam-github Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fharding1 commented May 27, 2019 •

edited

Loading