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

fix(CORS): only allow connections from the designated host #4985

Merged
merged 6 commits into from Feb 3, 2020
Merged

Conversation

Akryum
Copy link
Member

@Akryum Akryum commented Dec 20, 2019

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Underlying tools
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Other information:
For security reasons, only allow connection from localhost.

@Akryum Akryum requested a review from sodatea December 20, 2019 18:48
@Akryum Akryum self-assigned this Dec 20, 2019
packages/@vue/cli/lib/ui.js Outdated Show resolved Hide resolved
@sodatea
Copy link
Member

sodatea commented Dec 26, 2019

I'm not sure if I was testing it incorrectly, but as far as I tried out, this change seems does not prevent other domains from accessing the GraphQL interface.

@sodatea
Copy link
Member

sodatea commented Dec 26, 2019

I tested this by running another instance of the cli-ui frontend and point the VUE_APP_CLI_UI_URL variable to the modified server instance.

@sodatea
Copy link
Member

sodatea commented Dec 26, 2019

Got it.

The origin check should be placed in the WebSocket server, that is, the apolloServerOptions.subscriptions.onConnect method.

It's because

a CORS policy cannot be inserted into an HTTP Upgrade response

@Akryum
Copy link
Member Author

Akryum commented Dec 30, 2019

Good catch!

@sodatea
Copy link
Member

sodatea commented Jan 8, 2020

I got it fixed locally, by adding the following lines in vue-cli-plugin-apollo/graphql-server/index.js:

httpServer.on('upgrade', (req, socket) => {
  const { origin } = req.headers
  if (!origin || !/https?:\/\/localhost/.test(origin)) {
    socket.destroy()
  }
})

But since the httpServer instance is not exported from the plugin, I can't do the fix on the Vue CLI side.
Could you please add it to the plugin API?

@sodatea sodatea changed the title fix(cors): only allow localhost fix(CORS): only allow connections from the designated host Feb 3, 2020
@sodatea
Copy link
Member

sodatea commented Feb 3, 2020

I've migrated to a custom version of the graphql-server to work around this issue for now.

Should check it again after the apollo plugin adds the needed API.

@sodatea sodatea merged commit da43343 into dev Feb 3, 2020
@sodatea sodatea deleted the vue-ui-cors branch February 3, 2020 11:35
sodatea added a commit that referenced this pull request Feb 4, 2020
…5142)

* fix: followup of #4985, allow same-site ws requests of any domain

* fix: match whole string
mactanxin pushed a commit to mactanxin/vue-cli that referenced this pull request Feb 11, 2020
* fix(cors): only allow localhost

* fix: use host so it's configurable

* fix: use cors options object

* feat: use a custom graphql-server instead of the one from apollo plugin

exports the httpServer instance

* fix: add CORS validation in the http upgrade request

Co-authored-by: Haoqun Jiang <haoqunjiang@gmail.com>
mactanxin pushed a commit to mactanxin/vue-cli that referenced this pull request Feb 11, 2020
…ain (vuejs#5142)

* fix: followup of vuejs#4985, allow same-site ws requests of any domain

* fix: match whole string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants