Repository navigation
feat: attribute dapp-scanning URL requests - #10357
Conversation
imblue-dabadee
changed the title
Add request-source attribution to the two dapp-scanning URL scan endpoints so phishing-detection service metrics can attribute scan volume to the client flow that caused it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13adf1f to
c8e925e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c8e925e. Configure here.
|
This depends on wallet-guard/phishing-detection-service#276 right? That PR's allowlist is still the provisional mobile-browser / mobile-other / extension list, so I thinkeverything here except |
| - Add request-source attribution to dapp-scanning URL scans, emitted as an `x-request-source` header ([#10357](https://github.com/MetaMask/core/pull/10357)) | ||
| - Add an optional `platform` constructor option and an optional `flow` parameter to `scanUrl` and `bulkScanUrls`. The header value is composed as `<platform>-<flow>`. | ||
| - Export the `RequestSourcePlatform` and `RequestSourceFlow` enums, the `RequestSource` type, the `REQUEST_SOURCE_HEADER` and `UNKNOWN_REQUEST_SOURCE` constants, and the `buildRequestSource` helper. | ||
| - `RequestSourcePlatform` is `Extension` or `Mobile`. `RequestSourceFlow` is one of `dapp-connection`, `browser`, `rpc-trust-signals`, `confirmations`, `reveal-srp`, or `nft-detection`. |
There was a problem hiding this comment.
This reads like the values are Extension / Mobile, but they're extension / mobile now. Worth listing the emitted values.
There was a problem hiding this comment.
I've instead opted to reduce the amount of language used in the CHANGELOG.
There was a problem hiding this comment.
as in you're pushing another commit to reduce the amount of changelog lines?
There was a problem hiding this comment.
OK for some reason github wasn't showing that you'd pushed another commit. Looks good
This PR does not depend on the the PDS PR. The PDS will need to be updated to accommodate for the decisions made here. Concerning the ordering, it doesn't particularly matter; I chose to do the core PR and get this across first since changes here and in clients take much longer to propagate and deploy than server side changes. |
## Explanation This release candidate publishes: - `@metamask/phishing-controller` `18.2.0` - `@metamask/assets-controllers` `112.1.0` `@metamask/phishing-controller` adds optional request-source attribution for URL scans. `NftController` in `@metamask/assets-controllers` adopts that API for NFT metadata scans using the `nft-detection` source. The assets-controllers minor release also includes its existing additive consumer-facing changes, including optional RWA token security metadata, as documented in its changelog. No breaking API changes are included. The assets-controllers dependency range is updated to consume `@metamask/phishing-controller@^18.2.0`. ## References - Related to MetaMask#10357 - Related to MetaMask#10542 ## 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 communicated my changes to consumers by updating changelogs for packages I've changed - [ ] I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Release-only dependency and version updates with additive, optional phishing attribution APIs and no breaking changes called out in this PR. > > **Overview** > This PR cuts **monorepo release 1307.0.0** by publishing **`@metamask/phishing-controller@18.2.0`** and **`@metamask/assets-controllers@112.1.0`**, then wiring dependents to those versions. > > **`@metamask/phishing-controller@18.2.0`** (documented in its changelog) adds optional **request-source** parameters on `scanUrl` / `bulkScanUrls`, which set an **`x-request-source`** header on phishing URL scans. > > **`@metamask/assets-controllers@112.1.0`** rolls that dependency forward and records **`NftController`** passing the **`nft-detection`** source on **`PhishingController:bulkScanUrls`** so NFT metadata URL checks are tagged separately in service metrics. The same release line also captures already-shipped additive API work (e.g. optional RWA **`securityData`** when **`includeTokenSecurityData`** is used). > > **`@metamask/assets-controller`** and **`@metamask/bridge-controller`** bump their **`@metamask/assets-controllers`** and **`@metamask/phishing-controller`** ranges; **`yarn.lock`** and changelog compare links are updated accordingly. No application source changes appear in this diff—only versioning, changelogs, and lockfile alignment. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6a4ff12. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
3 participants
Add request-source attribution to dapp-scanning URL requests so phishing-detection metrics can identify the MetaMask platform and flow that initiated each scan.
Explanation
Phishing-detection URL-scan metrics currently cannot distinguish the flows that produce them. This makes it difficult to understand traffic patterns and prioritize work around scan volume.
This PR adds a bounded
x-request-sourceheader to both dapp-scanning URL endpoints:scanUrl(v2/scan)bulkScanUrls(bulk-scan)The header composes a controller-level platform (
extensionormobile) with a per-scan flow, such asextension-confirmationsormobile-nft-detection. The new public request-source types validate values at runtime and degrade invalid or missing attribution to anunknownsentinel, so attribution can never cause a scan to fail.Request-source taxonomy
Core defines the shared request-source vocabulary; Extension and Mobile adopt the values relevant to their flows.
dapp-connectionextension-dapp-connectionmobile-dapp-connectionbrowsermobile-browserrpc-trust-signalsextension-rpc-trust-signalsmobile-rpc-trust-signalsconfirmationsextension-confirmationsmobile-confirmationsreveal-srpextension-reveal-srpnft-detectionextension-nft-detectionmobile-nft-detectionunknown,extension-unknown, andmobile-unknownare fallback values for callers without valid attribution.NftControllernow labels its bulk scans asnft-detection. The new API parameters remain optional, preserving behavior for existing consumers until they adopt attribution.Tests cover header emission for both endpoints, valid source composition, and missing or invalid source values.
References
Checklist
Note
Low Risk
Additive optional API and telemetry header only; scan and blocking behavior are unchanged, with safe fallbacks when attribution is missing or invalid.
Overview
Adds request-source attribution for phishing URL scans so the detection service can split metrics by MetaMask client and product flow.
PhishingControllernow accepts an optionalplatform(extension/mobile) and optionalflowarguments onscanUrlandbulkScanUrls. Actual network scans (not cache hits) send anx-request-sourceheader built bybuildRequestSource, with shared enums/types exported from a newrequest-sourcemodule. Unknown or invalid platform/flow values degrade tounknown,extension-unknown, ormobile-unknownwithout failing the scan.NftControlleris updated to passRequestSourceFlow.NftDetectiononPhishingController:bulkScanUrlswhen checking NFT metadata URLs. Other callers can adopt flows later; the new parameters stay optional.Reviewed by Cursor Bugbot for commit 71b93b4. Bugbot is set up for automated code reviews on this repo. Configure here.