Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: phishing detector validation to drop invalid configs from detector #4820

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AugmentedMode
Copy link
Contributor

@AugmentedMode AugmentedMode commented Oct 21, 2024

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

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

@AugmentedMode AugmentedMode requested a review from a team as a code owner October 21, 2024 04:27
@AugmentedMode AugmentedMode self-assigned this Oct 21, 2024
@AugmentedMode AugmentedMode added the team-product-safety Push issues to Product Safety team label Oct 21, 2024
@AugmentedMode AugmentedMode marked this pull request as draft October 21, 2024 04:27
@AugmentedMode AugmentedMode marked this pull request as ready for review October 21, 2024 04:38
@AugmentedMode
Copy link
Contributor Author

AugmentedMode commented Oct 21, 2024

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.",
    );
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-product-safety Push issues to Product Safety team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant