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.

Sign up for GitHub

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

Jump to bottom

MM-11160 Adding proper CORS support. #9152

Merged
merged 2 commits into from
Jul 26, 2018
Merged

MM-11160 Adding proper CORS support. #9152

merged 2 commits into from
Jul 26, 2018

Conversation

crspeller
Copy link
Member

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.

@crspeller crspeller added the 2: Dev Review Requires review by a developer label Jul 24, 2018
@crspeller crspeller mentioned this pull request Jul 24, 2018
Merged
lieut-data
lieut-data approved these changes Jul 24, 2018
View reviewed changes
api4/cors_test.go Outdated
req.Header.Set("Origin", "http://mattermost.com")
},
func(t *testing.T, resp *http.Response) {
for name, headers := range resp.Header {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

api4/cors_test.go Outdated
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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@lieut-data lieut-data Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I fixed it.

@crspeller crspeller merged commit bae26ec into master Jul 26, 2018
@crspeller crspeller deleted the mm-11160 branch July 26, 2018 15:31
@amyblais amyblais added Docs/Needed Requires documentation Changelog/Done Required changelog entry has been written labels Jul 27, 2018
@jasonblais jasonblais mentioned this pull request Jul 31, 2018
Merged
@jasonblais jasonblais self-assigned this Aug 3, 2018
@jasonblais jasonblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Aug 3, 2018
@lindalumitchell lindalumitchell added the Tests/Not Needed New release tests not required label Aug 7, 2018
@iampeter
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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

@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
Copy link

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

@crspeller
Copy link
Member Author

@iampeter Vancouver time (PST)

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

@lieut-data lieut-data lieut-data approved these changes

@grundleborg grundleborg grundleborg approved these changes

@csduarte csduarte Awaiting requested review from csduarte

Assignees

@jasonblais jasonblais

Labels
2: Dev Review Requires review by a developer Changelog/Done Required changelog entry has been written Docs/Done Required documentation has been written Tests/Not Needed New release tests not required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@crspeller @iampeter @lieut-data @grundleborg @jasonblais @lindalumitchell @amyblais