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

Cors handling middleware improved to handle preflight cors requests #110

Merged
merged 3 commits into from Jan 22, 2018

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Jan 22, 2018

No description provided.

return isOptionsMethod && containsAccessControlRequestHeader && containsAccessControlRequestMethod
}

func applyPreflightCorsResponse(resp http.ResponseWriter) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 22, 2018

Member

Why conditional check is done?
As i understand, later both applyPreflightCorsResponse() and allowAllCorsActions() does the same

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 22, 2018

Author Member

Actually there are two cases:

  1. If it's preflight request - add headers and return EMPTY response
  2. Any other case - simply add cors headers.
    I wanted to show intent that it's separate case - return empty response with headers. Maybe renaming function to generatePreflightCorsResponse will express intetion better.

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

What about this?

allowAllCorsActions(resp)
if isPreflightCorsRequest(req) {
  return
}
wrapper.originalHandler.ServeHTTP(resp, req)

This shows that same headers are applied always and that empty response is returned in case it's preflight request.

@@ -7,12 +7,32 @@ type corsHandler struct {
}

func (wrapper corsHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
resp.Header().Set("Access-Control-Allow-Origin", "*")
resp.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE")
if isPreflightCorsRequest(req) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 22, 2018

Member

Idea - would not it be easier to connect middleware for each route, we are registering?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 22, 2018

Author Member

It would make sense if we really gonna use CORS for more grained control checking in our api. At the moment - it is simplier to handle all cors (allow all actions) in single place which naturaly is single http handling function.

@shroomist

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

Options request now is accepted, but I'm still getting Request header field Content-Type is not allowed by Access-Control-Allow-Headers in preflight response

@donce

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

For those who review and doesn't know about preflight requests (like me):
https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request

@donce
Copy link
Contributor

left a comment

Doesn't work, because content-type is returned.

func allowAllCorsActions(resp http.ResponseWriter) {
resp.Header().Set("Access-Control-Allow-Origin", "*")
resp.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS, PUT, DELETE")
resp.Header().Set("Access-Control-Allow-Headers", "Content-type")

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 22, 2018

Contributor

Access-Control-Allow-Headers", "Content-type" fixes preflights for me.

return isOptionsMethod && containsAccessControlRequestHeader && containsAccessControlRequestMethod
}

func applyPreflightCorsResponse(resp http.ResponseWriter) {

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

What about this?

allowAllCorsActions(resp)
if isPreflightCorsRequest(req) {
  return
}
wrapper.originalHandler.ServeHTTP(resp, req)

This shows that same headers are applied always and that empty response is returned in case it's preflight request.

assert.NoError(t, err)

req.Header.Add("Access-Control-Request-Method", "DELETE")
req.Header.Add("Access-Control-Request-Headers", "origin, x-requested-with")

This comment has been minimized.

Copy link
@donce

donce Jan 22, 2018

Contributor

Can we add content-type header here, to check that it's allowed? (which was why it was failing before).

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 22, 2018

Author Member

Refactored handler now simply allows all headers defined in request

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2018

Refactored to allow any headers specified by request
@donce It works now, and I think your idea was good, but not relevant anymore. Please recheck

@tadovas tadovas force-pushed the bug/MYST-262-add-cors-preflight-handling branch from 44b105a to c81c048 Jan 22, 2018

@Waldz

Waldz approved these changes Jan 22, 2018

@donce

donce approved these changes Jan 22, 2018

@tadovas tadovas merged commit 9f53d31 into master Jan 22, 2018

@tadovas tadovas deleted the bug/MYST-262-add-cors-preflight-handling branch Jan 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.