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 default configuration #40

Closed
o0Ignition0o opened this issue Oct 19, 2021 · 9 comments · Fixed by #759
Closed

CORS default configuration #40

o0Ignition0o opened this issue Oct 19, 2021 · 9 comments · Fixed by #759
Assignees

Comments

@o0Ignition0o
Copy link
Contributor

o0Ignition0o commented Oct 19, 2021

Followup to the previous work on 5b36237

Comments on the introspection efforts raised the fact we're not quite aligned on what we would like the cors default setting to be, and what we would like to customize it wit.
[Allow any origin might ease use and setup but it might not be considered as the safe default option

Let's write our expected CORS behavior down, so I can add unit tests. We can of course revisit it anytime

@o0Ignition0o o0Ignition0o self-assigned this Oct 19, 2021
@cecton
Copy link
Contributor

cecton commented Oct 20, 2021

Hmm yeah this definitely encourages security holes. I understand it was the default on gateway but that seems like something to fix, not to keep. I did not review @o0Ignition0o's code after he changed it because I did not expect he would pick true for the default 😖 this is counter-intuitive.

There is a third solution: requiring the user to specify allow_any_origin in the config. This is what @o0Ignition0o implemented in the first place. I said we should make it optional because it's more of a "debug/test" feature rather than a "user preference". But in this case it's kind of a middle ground.

To summarize:

  1. Default is false: secure but doesn't match gateway
  2. Default is true: insecure but matches gateway
  3. The field is required

I don't think option 2 is acceptable at all but feel free to challenge me.

@BrynCooke
Copy link
Contributor

Agree 100%. The absence of config should be secure by default. Would vote for 1 with the our example config explicitly configured for studio.

@o0Ignition0o
Copy link
Contributor Author

o0Ignition0o commented Oct 20, 2021

I vote for 1 as well (it was what I went with intuitively)

@o0Ignition0o
Copy link
Contributor Author

Followup to our last huddle discussion:

It looks like we're trying to place ourselves on a tradeoff between hardening (security) and ease of (first) use / integration.

Here are the knobs we can act on:

Configuration and defaults:

  • There is a default fallback configuration in rust we can act on, that will be used if no configuration section has been declared.
  • a Cors section in configuration.yaml will override the defaults, and allow people to tailor CORS to their production needs.
  • Maybe eventually allow setting up the router configuration through a rover command?

Runtime behavior:

  • We could decide to refuse a configuration that doesn't include a valid CORS configuration (panic/make the configuration mandatory).
  • We could be noisy if CORS isn't configured, by logging the lack of configuration and printing a warning and linking to guidelines and best practices on how to set up cors on the router.

Given we would like people to be able to explore the router functionality in a seamless manner, yet eventually prevent unsecure settings in production, we are currently leaning into:

  • Not allowing any origin by default
  • Defaulting origin to the studio (in rust)
  • Printing a message at startup that lets people know of the CORS configuration and how to tailor it to their needs

Did I forget anything?

@BrynCooke
Copy link
Contributor

BrynCooke commented Oct 21, 2021

Assuming the above is implemented.
Is there even a reason to have allow any origin in our config?

@o0Ignition0o
Copy link
Contributor Author

allow any origin is the only way for us to accept Origin: null (which isnt a valid origin scheme), and some playgrounds send this graphql/graphql-playground#296 :/

@Geal
Copy link
Contributor

Geal commented Oct 22, 2021

this looks like something that should be fixed in graphql-playground, not the router

@o0Ignition0o o0Ignition0o transferred this issue from another repository Nov 4, 2021
@o0Ignition0o
Copy link
Contributor Author

linking to #127 , that provides insights on nice headers to add to the defaults!

@o0Ignition0o
Copy link
Contributor Author

As discussed, we want allow_any_origin = false by default, and origins: ["https://studio.apollographql.com"].

Both are configurable by yaml regardless

o0Ignition0o added a commit that referenced this issue Mar 30, 2022
Fixes :#40

The Router now allows only the `https://studio.apollographql.com` origin by default, instead of any origin.
o0Ignition0o added a commit that referenced this issue Mar 30, 2022
Fixes :#40

The Router now allows only the `https://studio.apollographql.com` origin by default, instead of any origin.
o0Ignition0o added a commit that referenced this issue Mar 30, 2022
Fixes :#40

The Router now allows only the `https://studio.apollographql.com` origin by default, instead of any origin.
@BrynCooke BrynCooke linked a pull request Mar 30, 2022 that will close this issue
@Geal Geal closed this as completed Mar 31, 2022
BrynCooke pushed a commit that referenced this issue Apr 12, 2022
Update to GA release
Bump node-fetch from 2.6.0 to 2.6.1 (#40)

Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
- [Release notes](https://github.com/bitinn/node-fetch/releases)
- [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
- [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Add operation name to subqueries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants