prazdevs / pinia-plugin-persistedstate

💾 Configurable persistence and rehydration of Pinia stores.
https://prazdevs.github.io/pinia-plugin-persistedstate/
MIT License
2.13k stars 121 forks source link

Critical security vulnerability? #284

Closed SossenSystems closed 8 months ago

SossenSystems commented 8 months ago

Describe the bug

I discovered a critical security vulnerability in Nuxt 3 with your plugin, but I don't know how. I can only guess that it has to do with the SSR and the cookie method, that they are not tailored to each user.

I have noticed that when a user saves something via the Pinia Store, another user (different browser, different device, etc.) can see the data.

Do you have an idea why this is the case?

Reproduction

.

System Info

System:
    OS: macOS 14.3.1
    CPU: (8) arm64 Apple M1
    Memory: 70.86 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 9.6.6 - /opt/homebrew/bin/npm
  Browsers:
    Brave Browser: 107.1.45.113
    Chrome: 122.0.6261.94
    Safari: 17.3.1

Used Package Manager

npm

Validations

SossenSystems commented 8 months ago

I made an interesting discovery.

This code can be used to reproduce my problem:

const defaultState = {
  user: {
    deliveryAddress: {}
  }
}

export const useUserStore = defineStore('user', {
  state: () => ({ ...defaultState }),
  getters: {
    getDeliveryAddress: state => state.user.deliveryAddress
  },
  actions: {
    updateDeliveryAddress (options: {}) {
      this.user.deliveryAddress = options
    }
  },
  persist: true
})

And with this code the problem is no longer obviously there:

export const useUserStore = defineStore('user', {
  state: () => ({
    user: {
      deliveryAddress: {}
    }
  }),
  getters: {
    getDeliveryAddress: state => state.user.deliveryAddress
  },
  actions: {
    updateDeliveryAddress (options: {}) {
      this.user.deliveryAddress = options
    }
  },
  persist: true
})
sanjacob commented 8 months ago

Aren't you basically sharing a reference to defaultState? This isn't a bug with the library

SossenSystems commented 8 months ago

The reference to defaultState is the error; this appears to save the data globally, which leads to a data protection violation.

sanjacob commented 8 months ago

@SossenSystems I suggest you look into how arrow functions work in javascript. The one you describe as part of state contains a reference to a global state. What else did you expect? This is a feature of JavaScript. Just make the empty dict part of the state or use a setup store if that is more convenient.

prazdevs commented 8 months ago

As stated above, it doesnt seem like a security issue, and even less within the plugin. It's more of a way you define your store and data, and how JavaScript works (when the initialState object is instantiated/when the store state is created) :)

telmaantunes commented 6 months ago

@SossenSystems The issue is not the arrow function, it's the global variable defaultState that gets stored on the server and then changed by the cookies. I had the same issue, fix it by changing it to a function or by using lodash cloneDeep instead of the object destructuring, because the first one creates a deep copy and the second one only creates a shallow copy.

const defaultState = {
  user: {
    deliveryAddress: {}
  }
}

export const useUserStore = defineStore('user', {
  state: () => ({ ...defaultState }),
  ...
})

to


function getDefaultState() {
  return  {
    user: {
      deliveryAddress: {}
    }
  }
}

export const useUserStore = defineStore('user', {
  state: () => { 
    return getDefaultState();
  },
  ...
})

Hope this helps someone in the future!