core icon indicating copy to clipboard operation
core copied to clipboard

fix: phishing detector validation to drop invalid configs from detector

Open AugmentedMode opened this issue 1 year ago • 1 comments

Explanation

We need to ensure that only valid configurations are processed by the phishing detector. Currently, if one configuration fails validation, it causes all configurations to fail, preventing the detector from receiving updates.

The validation process needs to be updated so that invalid configurations are filtered out, while valid configurations are still passed to the detector.

Solution:

The processConfigs function has been introduced to:

  • Filter out invalid configurations using the validateConfig function.
  • Log any validation errors encountered during this filtering process.

This ensures that any configuration passed to the phishing detector is both valid and properly formatted. Invalid configurations are logged and discarded.

References

Changelog

@metamask/phishing-controller

  • CHANGED: processConfigs function to filter and validate phishing detector configurations, discarding invalid ones.
  • CHANGED: Instead of throwing an error and failing to update the detector when validation failed on any one config, invalid configurations are now logged via console.error and skipped.

Checklist

  • [x] I've updated the test suite for new or updated code as appropriate
  • [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

AugmentedMode avatar Oct 21 '24 04:10 AugmentedMode

I think it might be worth a discussion to see if I should update the validation to be more strict in this PR or not

for example enforcing name to be in the config as so

  if (
    !('name' in config) ||
    typeof config.name !== 'string' ||
    !(config.name in ListNames)
  ) {
    throw new Error(
      "Invalid config parameter: 'name'. Must be a valid string and part of ListNames enum.",
    );
  }

AugmentedMode avatar Oct 21 '24 04:10 AugmentedMode