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: use hostname to check against allowlist #4645

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Conversation

AugmentedMode
Copy link
Contributor

@AugmentedMode AugmentedMode commented Aug 29, 2024

Explanation

The current implementation of the PhishingController uses the full origin (Punycode encoded) to check against the allowlist. This approach has limitations when dealing with subdomains or variations in the URL structure, which could lead to inconsistent results or false negatives.

The changes introduced in this PR address these issues by extracting the hostname from the origin and checking the allowlist against this hostname instead of the full origin. This ensures that variations in the URL structure do not prevent legitimate domains from being correctly recognized as safe.

Specifically, this PR:

  • Modifies the test, isBlockedRequest, and bypass methods to use the hostname for allowlist checks.
  • Adds a new utility function, getHostnameFromUrl, which extracts the hostname from a given URL. This function is now used across the PhishingController to standardize how hostnames are derived from origins.

These changes ensure that domains are properly validated against the allowlist regardless of URL variations.

References

Changelog

@metamask/phishing-controller

  • CHANGED: Updated test, isBlockedRequest, and bypass methods to use the hostname for allowlist checks instead of the full origin.
  • ADDED: Introduced getHostnameFromUrl utility function to standardize hostname extraction from URLs.

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

@AugmentedMode AugmentedMode requested a review from a team as a code owner August 29, 2024 03:56
@AugmentedMode AugmentedMode self-assigned this Aug 29, 2024
@AugmentedMode AugmentedMode added the team-product-safety Push issues to Product Safety team label Aug 29, 2024
@AugmentedMode AugmentedMode merged commit cee5c9e into main Aug 29, 2024
116 checks passed
@AugmentedMode AugmentedMode deleted the fix/allowlist branch August 29, 2024 13:44
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.

2 participants