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

MM-11160 Adding proper CORS support. #9152

Merged
merged 2 commits into from Jul 26, 2018

Conversation

@crspeller
Copy link
Member

crspeller commented Jul 24, 2018

Adds proper support for CORS. This should support embedding cases even when the requester needs credentials.

Good resource to understand what this is all about: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS

New settings:

  • CorsExposedHeaders: Whitelist of server headers to be exposed to the client.
  • CorsAllowCredentials: CORS requests that pass validation will include the Access-Control-Allow-Credentials header.
  • CorsDebug: Prints messages to the logs to help an integration developer debug CORS issues.

Resolves: mattermost/mattermost-redux#557 , #8687
All test cases in the above issues pass with the proper configuration.

req.Header.Set("Origin", "http://mattermost.com")
},
func(t *testing.T, resp *http.Response) {
for name, headers := range resp.Header {

This comment has been minimized.

@lieut-data

lieut-data Jul 24, 2018

Member

Should this t.Log block just be part of the t.Run?

This comment has been minimized.

@crspeller

crspeller Jul 24, 2018

Author Member

No that was just me debugging that test. Will remove.

assert.Equal(t, http.StatusOK, resp.StatusCode)
assert.Equal(t, "*", resp.Header.Get(acAllowOrigin))
assert.Equal(t, "", resp.Header.Get(acExposeHeaders))
assert.Equal(t, "", resp.Header.Get(acAllowCredentials))

This comment has been minimized.

@lieut-data

lieut-data Jul 24, 2018

Member

nit: Any reason why we're not testing all the headers in each test case?

This comment has been minimized.

@crspeller

crspeller Jul 24, 2018

Author Member

Because those headers aren't appropriate in that situation. Wanted to avoid unnecessary checks.

This comment has been minimized.

@lieut-data

lieut-data Jul 24, 2018

Member

That's fair, but it's not obvious why we're checking them at all in the first place. Take, for example, acMaxAge: we only check that it's empty twice. Why do we need to assert that?

This comment has been minimized.

@crspeller

crspeller Jul 24, 2018

Author Member

Paranoia. Really we don't. It doesn't matter if that's set because it's only for preflight requests.

This comment has been minimized.

@lieut-data

lieut-data Jul 24, 2018

Member

2/5, but I think we should either check the "expected" empty value for paranoia reasons in all cases, or not at all. Its special status here is confusing :)

This comment has been minimized.

@crspeller

crspeller Jul 25, 2018

Author Member

OK I fixed it.

@crspeller crspeller force-pushed the mm-11160 branch from 12b92da to d1ecc91 Jul 24, 2018

@crspeller crspeller merged commit bae26ec into master Jul 26, 2018

3 checks passed

build.mattermost.com Successful Build
Details
codecov/project 65.73% (+0.07%) compared to 8387d92
Details
mattermost/serverless-ops/github PR is up-to-date.

@crspeller crspeller deleted the mm-11160 branch Jul 26, 2018

@jasonblais jasonblais self-assigned this Aug 3, 2018

@jasonblais jasonblais added Docs: Done and removed Docs: Needed labels Aug 3, 2018

@iampeter

This comment has been minimized.

Copy link

iampeter commented Feb 6, 2019

@crspeller @lieut-data thanks for this update, but with 5.7.0 I am still having issues with Client4 on Firefox, namely getting CORS header 'Access-Control-Allow-Origin' missing -> explained here

@crspeller

This comment has been minimized.

Copy link
Member Author

crspeller commented Feb 6, 2019

@iampeter Have you set the appropriate config settings? AllowCorsFrom? You can also try enabling CorsDebug and the server will print some useful debugging to the logs.

@iampeter

This comment has been minimized.

Copy link

iampeter commented Feb 12, 2019

here's my setup:

"AllowCorsFrom": "http://localhost:8065 ws://localhost:8065 http://localhost:8081 ws://localhost:8081 http://localhost:8002 ws://localhost:8002",
"CorsAllowCredentials": true, 
"CorsDebug": true,

and an excerpt from the log:

{"level":"info","ts":1549963720.8639622,"caller":"cors/cors.go:198","msg":"Handler: Actual request","source":"cors"}
{"level":"info","ts":1549963720.8640494,"caller":"cors/cors.go:311","msg":"Actual request no headers added: origin '*' not allowed","source":"cors"}
{"level":"error","ts":1549963720.8642828,"caller":"api4/websocket.go:28","msg":"websocket connect err: websocket: request origin not allowed by Upgrader.CheckOrigin"}
{"level":"error","ts":1549963720.8643298,"caller":"web/context.go:60","msg":"Failed to upgrade websocket connection","path":"/api/v4/websocket","request_id":"po5macx69pn4me4gmjdx5o69pa","ip_addr":"127.0.0.1","user_id":"5dcqz5ozmfnq788d5axbhbjfjo","method":"GET","err_where":"connect","http_code":500,"err_details":""}
{"level":"info","ts":1549963720.8643599,"caller":"http/server.go:1102","msg":"http: multiple response.WriteHeader calls","source":"httpserver"}

Chrome is fine - this happens in Firefox.

@crspeller

This comment has been minimized.

Copy link
Member Author

crspeller commented Feb 12, 2019

@iampeter Hmm. Your going to need to explain in more detail what you are trying to accomplish. Can we take this conversation over to our community server? The peer-to-peer-help channel would be good. Otherwise I am going to miss your messages.

@iampeter

This comment has been minimized.

Copy link

iampeter commented Feb 13, 2019

@crspeller sure let's do this. what timezone are you in?

@crspeller

This comment has been minimized.

Copy link
Member Author

crspeller commented Feb 13, 2019

@iampeter Vancouver time (PST)

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