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

Deny CORS Requests by Default #158

Merged
merged 3 commits into from
Feb 12, 2019
Merged

Conversation

jonny-improbable
Copy link
Contributor

Change the default implementation of grpcweb.WrapServer so it denies requests from foreign origins by default to avoid circumventing the browser security model.

See discussion in #61.

@mwitkow
Copy link

mwitkow commented Apr 1, 2018

Cool :)
CORS issues are hard to debug if one's just starting. Can you please update a mention of it in the top level doc.go of:
https://github.com/improbable-eng/grpc-web/tree/master/go/grpcweb

Copy link
Contributor

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just one point re origins checking in the proxy.

go/grpcweb/wrapper_test.go Outdated Show resolved Hide resolved
if *flagCorsAllowAllOrigins {
return true
}
for _, allowedOrigin := range *flagCorsAllowOrigins {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looping over all the flags will cause this function to run each time a request is received. Perhaps we can do a map lookup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the latest round.

@petomalina
Copy link

Hi guys, any update on this please?

@easyCZ
Copy link
Contributor

easyCZ commented Sep 7, 2018

@jonny-improbable Could you please rebase the PR and look into the comments?

@johanbrandhorst johanbrandhorst added the waiting-on-submitter Waiting on original poster action label Sep 8, 2018
@johanbrandhorst
Copy link
Contributor

We really should ship this 😄

@jonny-improbable jonny-improbable force-pushed the feature/deny-cors-by-default branch 3 times, most recently from 6cf2de4 to a6dc50d Compare February 11, 2019 16:20
@jonny-improbable jonny-improbable added breaking-change Will require a major version increment and removed waiting-on-submitter Waiting on original poster action labels Feb 11, 2019
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Nice to finally have this almost ready!

go/grpcweb/helpers.go Show resolved Hide resolved
go/grpcweb/helpers.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
…s requests from foreign origins by default to avoid circumventing the browser security model.

See discussion in #61.
- Simplify `makeHttpOriginFunc` logic
- Improve `--allowed_origins` help text
- Improve error message string formatting
@jonny-improbable jonny-improbable force-pushed the feature/deny-cors-by-default branch from beb3805 to eabca78 Compare February 12, 2019 10:16
@jonny-improbable
Copy link
Contributor Author

@johanbrandhorst All tests are passing, bar Safari 6 which is a Platform Error with Saucelabs (ie: not a test failure).

@johanbrandhorst
Copy link
Contributor

LGTM! Can we override the test failure?

@jonny-improbable jonny-improbable merged commit ebef475 into master Feb 12, 2019
@jonny-improbable
Copy link
Contributor Author

Can we override the test failure?

By the power of grayskull

@jonny-improbable jonny-improbable deleted the feature/deny-cors-by-default branch February 12, 2019 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Will require a major version increment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants