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 Dec 20, 2019
@Akryum Akryum self-assigned this Dec 20, 2019
packages/@vue/cli/lib/ui.js Outdated Show resolved Hide resolved
@sodatea
Copy link
Member

@sodatea 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 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 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 Akryum commented Dec 30, 2019

Good catch!

@sodatea
Copy link
Member

@sodatea 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 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
5 of 8 checks passed
@sodatea sodatea deleted the vue-ui-cors branch Feb 3, 2020
sodatea added a commit that referenced this issue Feb 4, 2020
…5142)

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

* fix: match whole string
mactanxin added a commit to mactanxin/vue-cli that referenced this issue 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 added a commit to mactanxin/vue-cli that referenced this issue 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants