Potential XSS when using with SSR about vuex-router-sync HOT 17 CLOSED

This audit report might be related.
https://www.npmjs.com/advisories/814

from vuex-router-sync.

@JohannesLamberts I just wrote to them again as the author of this package, hopefully they'll respond. And thanks for digging into this!

from vuex-router-sync.

Update: npm has removed the vulnerability flag on vuex-router-sync.

from vuex-router-sync.

I dug deeper into this to understand why our application using vue-router-sync was not affected.
It seems to me, that the issue is not in vuex-router-sync, but essentially in the state-serialization via @nuxt/devalue (essentially making every plugin / application code storing untrusted data in the store [added: as a key] a potential vulnerbility).

Vue SSR uses serialize-javascript per default (https://github.com/vuejs/vue/blob/b65f6d78e0e480601b0042b1b5e8259343b629fb/src/server/template-renderer/index.js#L213).

Nuxt.js uses @nuxt/devalue (https://github.com/nuxt/nuxt.js/blob/b978a3761d8dd18f26bf59a7b901e4741ae3ee41/packages/vue-renderer/src/renderer.js#L406).

This codesandbox compares both functions: https://codesandbox.io/s/5x2wpo27k4.

The underlying issue is that @nuxt/devalue uses JSON.stringify to sanitize a key, but JSON.stringify does not escape linebreaks unsafe chars: (https://github.com/nuxt/devalue/blob/650239dc86b37dbd87f997b7fb545fb29013d70c/src/index.ts#L250).

I think it should be safe to use the same approach as in serialize-javascript to replace unsafe characters: https://github.com/yahoo/serialize-javascript/blob/35f64803a3a67662e16ad5260901d4e291260989/index.js#L126

from vuex-router-sync.

What would be a URI exposing such XSS? Double quotes seem to be correctly escaped by Nuxt

from vuex-router-sync.

Code and Repro URL

Tested in Chrome with --disable-xss-auditor

from vuex-router-sync.

Can you share just the URI?

from vuex-router-sync.

Yes, looks like sandbox stops my instance as soon as I leave it. Here you go:

https://y25wmk1vq1.sse.codesandbox.io/?test%0A%2f%2f%3C%2fstYle%2f%3C%2ftitLe%2f%3C%2fteXtarEa%2f%3C%2fscRipt%2f!%3E%5Cx3csVg%2f%3CsVg%2foNloAd%3dalert%28document.domain%29%2f%2f%3E%5Cx3e

from vuex-router-sync.

@Atinux Do you think it's better to enable this only on client side or does removing the route state from the store makes more sense

// plugins/vuex-router-sync.js
import { sync } from 'vuex-router-sync'

export default ({ app, store }) => {
  sync(store, app.router)
}

// nuxt.config.js
plugins: [{ src: '~/plugins/vue-router-sync', ssr: false }]

In both scenario we could export a plugin/module for nuxt to prevent this problem

from vuex-router-sync.

@posva - If I may have a vote here, making the plugin client-only will definitely break our code: the initial route info is being used during SSR.

from vuex-router-sync.

yeah, you are right, we cannot make it client-side only because any action, mutation using the store would not work anymore

from vuex-router-sync.

So, my quick hack was to add the following Nuxt module:

module.exports = function(){
  this.nuxt.hook( 'render:routeContext', context => {
    if( context && context.state && context.state.route ){
      context.state.route = undefined;
    }
  });
};

Another approach might be escaping all strings (both names and values) before adding them to the store.

from vuex-router-sync.

Hi @grinnery

Thank you for this bug report, indeed, the issue comes from this plugin directly, they have to escape the query right here: https://github.com/vuejs/vuex-router-sync/blob/master/src/index.js#L65

@posva will work on a fix I am sure 💪

from vuex-router-sync.

@nuxt/devalue fixed this issue in v1.2.3.

Safe URL:

https://1vxxq979q.sse.codesandbox.io/?test%0A%2F%2F<%2FstYle%2F<%2FtitLe%2F<%2FteXtarEa%2F<%2FscRipt%2F%21>%5Cx3csVg%2F<sVg%2FoNloAd%3Dalert%28document.domain%29%2F%2F>%5Cx3e

Users will force update @nuxt/devalue to the latest patch by next patch release of nuxt.

from vuex-router-sync.

Will we get a patch for this plugin in the next days? (not urging you of course)
This plugin has been flagged as vulnerable through npm, and because of that our pipeline that relies on npm audit breaks.

from vuex-router-sync.

@WilliamDASILVA as explained in the comment above the vulnerability isn't from this plugin. The npm report should be closed but not sure where to file it :/

from vuex-router-sync.

@WilliamDASILVA I've already written to [email protected] around 48 hours ago, asking to close the report for vuex-router-sync and to add issues for devalue and @nuxt/devalue but got no response so far :/.

from vuex-router-sync.