tech story: [M3-8385] - Replace lodash set utility function to handle prototype pollution security threat

[coliu-akamai]

Description 📝

The lodash.set package causes a security concern for prototype pollution. This PR aims to resolve the security concern by creating our own version of set (specific to our use cases, not a rehash of lodash's set) that is also prototype pollution safe -- see internal ticket for full details on reasoning behind this approach, but the tldr:

• Although the security issue is fixed in lodash v4.17.19+, we've already moved away from using the entire lodash package so it didn't really make sense to add it back in
• we're already using the most updated version of lodash.set (4.3.2) that has this concern
• so I tried to recreate lodash's set, but it isn't a complete recreation of it - there's a lot of edge cases (see skipped tests). we've made a simpler version of set that is specific to our use case (for example, while lodash's set might accept paths in array format, we only accept paths in string format)

Changes 🔄

• remove lodash.set dependency
• create our version of set
• add a bunch of tests
• removed case of importing omit from the lodash package + replaced with our already existing omit function

Target release date 🗓️

?

How to test 🧪

Verification steps

• confirm yarn test formikErrorUtils.test.ts and that there are no UI changes for places that use getFormikErrorsFromAPIErrors (specifically PlacementGroups stuff!)
• confirm that yarn test utilities/set.test.ts passes if you switch out the new version of set for lodash.set (removing because this is no longer a direct copy for lodash's set)
• spotcheck the Linode Create v2 flow, since I exchanged a function there (but the one I exchanged it for is a direct replacement so it should be fine ✌️)

As an Author I have considered 🤔

Check all that apply

• 👀 Doing a self review
• ❔ Our contribution guidelines
• 🤏 Splitting feature into small PRs
• ➕ Adding a changeset
• 🧪 Providing/Improving test coverage
• 🔐 Removing all sensitive information from the code and PR description
• 🚩 Using a feature flag to protect the release
• 👣 Providing comprehensive reproduction steps
• 📑 Providing or updating our documentation
• 🕛 Scheduling a pair reviewing session
• 📱 Providing mobile support
• ♿ Providing accessibility support

[coliu-akamai]

honestly seems like we're still able to import from lodash even after removing the dependency earlier? based on how this snuck in

[abailly-akamai]

right, cause Formik has it as a dependency... 😢

still i created M3-8489 to prevent direct imports cause we shouldn't

[coliu-akamai]

omitProps is a direct replacement for lodash's omit

should we rename it to omit as well?

[abailly-akamai]

yes this change is fine. I just realized lodash is still in out bundle because of formik, but we don't import it directly (in other words this import would break the build if we were to get rid of formik for instance). I am going to add an eslint rule to prevent that from happening in the future

[coliu-akamai]

these are just eslint changes for this test

(note - the specific method these tests are for may be able to be removed in favor of using getFormikErrorsFromAPIErrors actually - planning to look into this!)

[abailly-akamai]

Thanks for tackling this, and nice writing! Going to spend a little bit more time with this one. My initial feedback is that in general, writing a suite of utils to serve only one case should be avoided, that being said I am aware this isn't a simple instance. Wondering if there's a way to satisfy the existing test cases for formikErrorUtils without having to re-create a set function. Happy to go with this solution if not, but worth a bit more digging

[coliu-akamai]

thanks for the feedback Alban - will also think about this more too!

[abailly-akamai]

@coliu-akamai came up with this smaller footprint solution which seems to satisfy the existing getFormikErrorsFromAPIErrors test suite. It may need to be tweaked and tested (also typed better) but this may reduce complexity overall
I dunno if it is as robust as your solution, but again, do we need a fully fledge util for this once case?

// local set util (only serving getFormikErrorsFromAPIErrors since that's our only candidate)

const set = (obj: any, path: string, value: any): any => {
  // Split the path into parts, handling both dot notation and array indices
  const [head, ...rest] = path.split(/\.|\[|\]/).filter(Boolean);

  // Since this is recursive, if there are no more parts in the path, set the value and return
  if (rest.length === 0) {
    return { ...obj, [head]: value };
  }

  // Handle array indices
  if (head.match(/^\d+$/)) {
    const index = parseInt(head, 10);
    // Copy the existing one or create a new empty one
    const newArray = Array.isArray(obj) ? [...obj] : [];
    // Recursively set the value at the specified index
    newArray[index] = set(newArray[index] || {}, rest.join('.'), value);

    return newArray;
  }

  // Handle nested objects
  return {
    ...obj,
    // Recursively set the value for the nested path
    [head]: set(obj[head] || {}, rest.join('.'), value),
  };
};

// getFormikErrorsFromAPIErrors

export const getFormikErrorsFromAPIErrors = <T>(
  errors: APIError[],
  prefixToRemoveFromFields?: string
): FormikErrors<T> => {
  return errors.reduce((acc: FormikErrors<T>, error: APIError) => {
    if (error.field) {
      const field = prefixToRemoveFromFields
        ? error.field.replace(prefixToRemoveFromFields, '')
        : error.field;

      return set(acc, field, error.reason); // only difference is needing to return the accumulated result here
    }
    return acc;
  }, {});
};

[coliu-akamai]

thanks Alban! Been poking around your solution - added the prototype pollution check to it + ran it against my test cases from earlier in an offshoot branch (see test PR I opened up here). IMO the main test case failing that may be a cause for concern is this one (the commented object is what's returned), but we could argue that here, having an object instead of an array makes sense. Maybe this one as well, but I think it would be quick to fix it.

One of my main questions is - while we're only using lodash's set once right now, are there potential other use cases in the future where we'd want to use set again + have lodash's flexibility when using it? If not, then I agree with switching to the less complex version and will change to it

ty for your help + feedback! 😆

[abailly-akamai]

@coliu-akamai

while we're only using lodash's set once right now, are there potential other use cases in the future where we'd want to use set again + have lodash's flexibility when using it

This is a valid question, but also a premature optimization? All together your version is still valid - my primary concern is for the script to perhaps not exactly replicate lodash.set functionalities but still set (pun intended) that expectation. In the end, up to you, I won't block that PR cause it does what we want and has enough comments to guard users against its limitations.

if you wanna pursue the smaller/localized footprint route, this updated version could get you a little closer and is safer against prototype pollution (duh,, this was the whole point 🤦 ) but still unsure it'll satisfy all your requirements.

this mutates the input, but i believe lodash.set does too. could also considering making copies at each level to avoid that.

again, splitting hairs a bit here but worth considering either option

const set = (obj: any, path: string, value: any): any => {
  // Safeguard against prototype pollution
  if (path === '__proto__' || path === 'constructor' || path === 'prototype') {
    return obj;
  }

  const parts = path.split(/\.|\[|\]/).filter(Boolean);

  return parts.reduce((acc: any, part: string, index: number) => {
    if (index === parts.length - 1) {
      // Last part, set the value
      acc[part] = value;
    } else if (part.match(/^\d+$/)) {
      // Handle array indices
      const arrayIndex = parseInt(part, 10);
      acc[arrayIndex] =
        acc[arrayIndex] || (parts[index + 1].match(/^\d+$/) ? [] : {});
    } else {
      // Handle nested objects
      acc[part] = acc[part] || (parts[index + 1].match(/^\d+$/) ? [] : {});
    }
    return acc[part];
  }, obj);
};

[coliu-akamai]

thanks Alban will take another look!

[coliu-akamai]

@abailly-akamai tested it in a another branch (put up another draft PR here to see changes). In short, with a few slight tweaks the behavior actually really closely matches the set here 👀 (there are some differences but for the functionality we want but imo they wouldn't be important for what we're currently using set for).

Planning to clean up + switch to this version then! 😆 Would you recommend still creating a separate file and tests for the new set? Or just keep it as an untested helper function in the formikErrorUtils file?

Also, another question - in general, is there a rule of thumb for when we should try to optimize/consider wider use cases vs making something more specific to what we currently have?

[abailly-akamai]

@coliu-akamai

I would keep it as a helper function in formikErrorUtils. Your tests look quite great so i would port the ones you feel are important to the existing formikErrorUtils.test.ts

in general, is there a rule of thumb for when we should try to optimize/consider wider use cases vs making something more specific to what we currently have?

External utils are meant to be shared, portable and reusable. If we're supporting only one function, it's fair not to optimize for something we don't know we would ever need. Those mutation patterns should be seldomly used anyway. Always go small, avoid a package if we can code it simply enough and nae things well so people can find the util if they are thinking of reusing it in the future 👍

[github-actions]

Coverage Report: ✅
Base Coverage: 86.04%
Current Coverage: 86.13%

[abailly-akamai]

Looks great, thx for coping with me 🤣

Approving pending a couple type improvements

[abailly-akamai]

right, cause Formik has it as a dependency... 😢

still i created M3-8489 to prevent direct imports cause we shouldn't

[bnussman-akamai]

Thanks for doing this!

When I delete Linode Create v1, I'll be able to remove the last two lodash dependencies (lodash.curry and lodash.clonedeep) 🎉