Potential prototype pollution

[quinnturner]

I haven't tested this, so I might be completely wrong. However, it might be worth testing whether this hook is vulnerable to prototype pollution. Consider reviewing https://codeburst.io/what-is-prototype-pollution-49482fc4b638

[michalsvorc]

Thanks for opening this issue. Here is a link to free article instead of non-free Medium platform. Keep the knowledge free.

As I understand it, attackers can tamper with methods on prototype chain which can lead to remote code execution. These tampered objects can be sent to un-sanitized inputs, or can be present in dependency libraries.

This hook is complementary to react-hook-form library, it reads values from react-hook-form and serializes them to Web storage API.

Hence react-hook-form is responsible for reading user input and sanitizing values returned from inputs.

As for dependencies, there are none, only React useEffect hook and global window storage: index.js

To prevent this vulnerability, I suggest updating dependencies to newest React (17.X), otherwise there is nothing much to do.

[quinnturner]

The prototype pollution comes from the storage rather than the form. Upon reading from the storage, it assigns the keys to an empty object. If the keys include things like constructor or __proto__ then it might be vulnerable. I have not tested this and may be completely off, but it's something that should be investigated. I have to revisit rules for whether they can exist on an object like this or if it has to be a class instance. Either way, it might be worth it to include the risk mitigation techniques in the snippet below. The snippet is a mini-fork of mine that is in TypeScript and includes item expirations. Once I test it and use it I can create a PR.

  useEffect(() => {
    const storageItem = storage.getItem(storageKey);

    if (storageItem === null) return;

    const item = JSON.parse(storageItem) as Record<
      string | number | symbol,
      unknown
    >;

    const now = new Date();

    // A null or empty expiry means no expiry
    const expiry = typeof item.expiry === 'number' ? item.expiry : null;

    if (expiry && now.getTime() > expiry) {
      // Has expired
      storage.removeItem(storageKey);
      return;
    }

    const { values } = item;
    if (typeof values !== 'object' || !values) {
      return;
    }

    const dataRestored: Partial<Record<FieldName<T>, unknown>> = {};
    Object.keys(values).forEach((key) => {
      // TODO: revisit useFormPersist prototype pollution
      // Prototype pollution mitigation taken from Lodash.
      // Consider this as being potentially incomplete.
      if (key === 'constructor' && typeof values[key] === 'function') {
        return;
      }
      if (key === '__proto__') {
        return;
      }
      // TODO: Fix typing on useFormPersist
      // @ts-ignore
      const value = values[key];
      // @ts-ignore
      dataRestored[key] = value;
      // @ts-ignore
      setValue(key, value);
    });

    if (onDataRestored) {
      onDataRestored(dataRestored);
    }
  }, [storage, storageKey, onDataRestored, setValue]);