From 08416a1d784280fc2ac0fca83f0504599d99a6d0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Sat, 29 Aug 2026 01:32:38 -0400 Subject: [PATCH 1/2] Add extension overlay to `deeplink-handler` The skill shipped a `repos/` directory with only `metamask-mobile.md`, so the overlay gate skipped it for `metamask-extension` entirely. Extension authors a deep link as a `Route` in `shared/lib/deep-links/routes/`, which the mobile `SUPPORTED_ACTIONS` handler pattern does not describe. The overlay draws its security model from ADR-0011 and ADR-0020, and the recurring defects from review on #38003, #40995 and #45504. It names the CODEOWNERS split so a feature team can tell which paths are theirs. Description rewritten from 36 chars, which could not match a request. --- .../repos/metamask-extension.md | 124 ++++++++++++++++++ .../coding/skills/deeplink-handler/skill.md | 26 +++- 2 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 domains/coding/skills/deeplink-handler/repos/metamask-extension.md diff --git a/domains/coding/skills/deeplink-handler/repos/metamask-extension.md b/domains/coding/skills/deeplink-handler/repos/metamask-extension.md new file mode 100644 index 00000000..90a970ab --- /dev/null +++ b/domains/coding/skills/deeplink-handler/repos/metamask-extension.md @@ -0,0 +1,124 @@ +--- +repo: metamask-extension +parent: deeplink-handler +--- + +# Authoring a Deep Link Route (Extension) + +Extension deep links are `Route` objects in `shared/lib/deep-links/routes/`, parsed and +verified by a security boundary that a feature team does not own. This file covers adding a +route. It does not cover changing how links are parsed, verified, or gated — that is a +different task with a different reviewer. + +## What you own, and what pulls in Security + +`.github/CODEOWNERS` draws the line, and it is mechanical rather than a judgement call: + +| Path | Owner | +|---|---| +| `shared/lib/deep-links/routes/.ts` | you | +| `shared/lib/deep-links/routes/index.ts` | you (registration only) | +| `shared/lib/deep-links/routes/route*` | `@MetaMask/extension-security-team` | +| `shared/lib/deep-links/parse*`, `verify*`, `utils*`, `security-policy*` | `@MetaMask/extension-security-team` | +| `app/scripts/lib/deep-links/deep-link-router.ts` | `@MetaMask/extension-security-team` | +| `ui/helpers/utils/resolve-deep-link-href*`, `ui/pages/deep-link/` | `@MetaMask/extension-security-team` | + +If your change reaches any owned path, you are no longer adding a route — stop and read the +next section before writing the diff. + +## The interstitial is not yours to weaken + +`AGENTS.md` rule 17, verbatim: + +> **DEEPLINK INTERSTITIAL SECURITY — EXTREMELY HIGH RISK:** Before implementing any change +> that can cause fewer deep links to show the security interstitial, agents **MUST stop and +> obtain explicit, documented consent from `@MetaMask/extension-security-team`**. Without +> documented Security approval, do not make the change—even when it appears necessary to +> complete another feature, migration, refactor, or test fix. + +`security-policy.ts` carries the same instruction in its header, addressed to agents +specifically: *"Do not add bypasses, route or asset allowlists, remote lookups, or broader +trusted sources in pursuit of another task."* + +Treat both as hard stops. "The feature needs it" is the case they were written for. + +## Security model + +Authority is [ADR-0011 (Deep Linking Into Wallet)][adr11] and [ADR-0020 (Shared Deeplink +Registry)][adr20]. Three facts a route author has to hold: + +**Signature is a trust signal, not an authorization.** Both clients verify link signatures. +`SignatureStatus` is `VALID`, `INVALID`, or `MISSING`. A valid signature does not change the +route contract; it changes whether the interstitial shows. Signing happens in the internal +signer service behind privileged Okta — never reimplement it. + +**A trusted origin bypasses the interstitial before the signature is consulted.** +`shouldShowDeepLinkInterstitial` returns `false` for a request origin in +`TRUSTED_WEB_ORIGINS` — today exactly `https://metamask.io` and `https://app.metamask.io` — +ahead of any signature check. So a link initiated from those origins reaches your +destination unsigned and unwarned. **Registering a route inherits this.** You have not +changed it, but your destination is now reachable that way. + +**Unsigned links forward every parameter.** `canonicalize` keeps only the `sig_params` +allowlist for signed links; with no `sig_params` it takes a backward-compatibility branch and +forwards every param except `sig`. Your handler must assume hostile input on the unsigned +path. + +## Adding a route + +**1. Write the route file.** `shared/lib/deep-links/routes/.ts`: + +```ts +import { Route, SETTINGS_ROUTE, SHIELD_PLAN_ROUTE } from './route'; + +export const shield = new Route({ + pathname: '/shield', + getTitle: (_: URLSearchParams) => 'deepLink_theTransactionShieldPage', + handler: function handler(params: URLSearchParams) { + return { path: SHIELD_PLAN_ROUTE, query: params }; + }, +}); +``` + +`getTitle` returns an **i18n message key**, not a display string. `handler` returns a +`Destination` — either `{ path, query }` or `{ redirectTo: URL }` — and may throw if the +params cannot be processed. + +**2. Register it** in `shared/lib/deep-links/routes/index.ts`. Import and add to the exported +map. This is the line that makes the destination reachable. + +**3. Add an E2E test.** Not optional. From review on +[#38003](https://github.com/MetaMask/metamask-extension/pull/38003): *"We always need e2e +tests for these routes (I'm sure some teams are slipping by without adding them, but they +aren't supposed to!)"* + +**4. Add the CODEOWNERS entry** for your route file if your team owns the surface, following +`routes/perps.ts @MetaMask/perps`. + +## Defects that recur in review + +**An allowlist must not be a plain object literal.** A lookup against `{}` resolves inherited +keys, so `?setting=constructor` returns `function Object() { [native code] }` and the +`?? DEFAULT` fallback never fires. Use a `Set`, a `Map`, or `Object.create(null)`. + +*And the negative test must use an inherited key.* `'not-a-setting'` is `undefined` on the +prototype chain too, so it passes against a broken implementation and a correct one alike — +it cannot tell them apart. Test `'constructor'` or `'__proto__'`. + +**Use the validated parameter.** Validating `type` and then hardcoding the path means the +validation changes nothing. If a param is worth checking, the destination must depend on it. + +**Decide about dropped query params, and say so.** A handler that silently discards `params` +is a finding. Forward them or state why not. + +**Do not reintroduce `handlerSearchParams`.** The field existed and was reverted; `main` +carries zero occurrences. Routes default to canonical param handling, which removes unsigned +params for signed links. Restoring per-route control reopens what the reverts closed. + +**A new class of destination is a security question even when the code is routine.** Content +pages and a settings surface carrying consent toggles are not the same risk. If your +destination changes state, grants a permission, or exposes a toggle, raise it with the +`@MetaMask/extension-security-team` before it lands rather than after. + +[adr11]: https://github.com/MetaMask/decisions/blob/main/decisions/core/0011-deep-linking-into-wallet.md +[adr20]: https://github.com/MetaMask/decisions/pull/149 diff --git a/domains/coding/skills/deeplink-handler/skill.md b/domains/coding/skills/deeplink-handler/skill.md index 84437b18..14bdc191 100644 --- a/domains/coding/skills/deeplink-handler/skill.md +++ b/domains/coding/skills/deeplink-handler/skill.md @@ -1,4 +1,28 @@ --- name: deeplink-handler -description: Deeplink handler creation guidelines +description: Add or change a deep link route in a MetaMask client — the route file, its registration, the required E2E test, and the security boundary a feature team does not own. Use when exposing a new deep link, pointing one at a new destination, or when review raises the interstitial, signature verification, or param canonicalization. Names which paths belong to the extension security team and which are yours, plus the defects that recur in review — allowlists written as plain object literals, which admit every prototype key; validated params that never reach the destination; and silently dropped query strings. --- + +# Deep Link Routes + +A deep link is an external entry point into the wallet. The route you add is reachable by +anyone who can construct the URL, so the parts that decide whether a link is trusted are +owned separately from the parts that decide where it goes. + +## When To Use + +- Exposing a new deep link for a feature +- Pointing an existing deep link at a different destination +- Review has raised the security interstitial, signature verification, or param handling +- A deep link reaches a destination that changes state or grants a permission + +Not for changing how links are parsed, verified, or gated. That is a security-boundary change +with a different owner — see the repo overlay. + +## Workflow + +1. Read the repo overlay for this client. The implementations differ substantially. +2. Confirm which files your change touches, and whether any are security-owned. +3. Write the route or handler, register it, and add the E2E test. +4. If the destination changes state or grants a permission, raise it with the security owners + before the PR lands. From 7d665f3f10f6461e92be1731b8b8bae4ae9d113b Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 1 Sep 2026 14:20:52 -0400 Subject: [PATCH 2/2] Move the cross-client security model into the shared skill The security model and the recurring review defects hold in both clients, so they belong in `skill.md` rather than in one client's section. The symbols that implement them do not: `TRUSTED_WEB_ORIGINS` and `shouldShowDeepLinkInterstitial` appear nowhere in `metamask-mobile`, so the split is by abstraction level, not by section. Neither deeplink ADR is accepted. `0011-deep-linking-into-wallet` is on `main`; `0020-shared-deeplink-registry` is an open draft whose own non-goals exclude implementing the package. The skill now says which is which instead of calling both authority, and cites them by path, since `decisions/core/` already holds two files numbered 0020. The extension section becomes a pointer to `docs/deeplink-route-authoring.md` in `metamask-extension`, where its audience reads. `handlerSearchParams` is dropped: a reverted field nobody outside the authoring session can resolve. --- .../repos/metamask-extension.md | 125 ++---------------- .../coding/skills/deeplink-handler/skill.md | 79 +++++++++-- 2 files changed, 74 insertions(+), 130 deletions(-) diff --git a/domains/coding/skills/deeplink-handler/repos/metamask-extension.md b/domains/coding/skills/deeplink-handler/repos/metamask-extension.md index 90a970ab..045efc8c 100644 --- a/domains/coding/skills/deeplink-handler/repos/metamask-extension.md +++ b/domains/coding/skills/deeplink-handler/repos/metamask-extension.md @@ -3,122 +3,13 @@ repo: metamask-extension parent: deeplink-handler --- -# Authoring a Deep Link Route (Extension) +# Deeplink Routes (Extension) -Extension deep links are `Route` objects in `shared/lib/deep-links/routes/`, parsed and -verified by a security boundary that a feature team does not own. This file covers adding a -route. It does not cover changing how links are parsed, verified, or gated — that is a -different task with a different reviewer. +Authoring and reviewing extension deeplink routes is documented in the extension repository, +at `docs/deeplink-route-authoring.md`. It covers the ownership split with +`@MetaMask/extension-security-team`, the interstitial rule, the extension's own trusted +origins and canonicalization, and the four steps of adding a route. -## What you own, and what pulls in Security - -`.github/CODEOWNERS` draws the line, and it is mechanical rather than a judgement call: - -| Path | Owner | -|---|---| -| `shared/lib/deep-links/routes/.ts` | you | -| `shared/lib/deep-links/routes/index.ts` | you (registration only) | -| `shared/lib/deep-links/routes/route*` | `@MetaMask/extension-security-team` | -| `shared/lib/deep-links/parse*`, `verify*`, `utils*`, `security-policy*` | `@MetaMask/extension-security-team` | -| `app/scripts/lib/deep-links/deep-link-router.ts` | `@MetaMask/extension-security-team` | -| `ui/helpers/utils/resolve-deep-link-href*`, `ui/pages/deep-link/` | `@MetaMask/extension-security-team` | - -If your change reaches any owned path, you are no longer adding a route — stop and read the -next section before writing the diff. - -## The interstitial is not yours to weaken - -`AGENTS.md` rule 17, verbatim: - -> **DEEPLINK INTERSTITIAL SECURITY — EXTREMELY HIGH RISK:** Before implementing any change -> that can cause fewer deep links to show the security interstitial, agents **MUST stop and -> obtain explicit, documented consent from `@MetaMask/extension-security-team`**. Without -> documented Security approval, do not make the change—even when it appears necessary to -> complete another feature, migration, refactor, or test fix. - -`security-policy.ts` carries the same instruction in its header, addressed to agents -specifically: *"Do not add bypasses, route or asset allowlists, remote lookups, or broader -trusted sources in pursuit of another task."* - -Treat both as hard stops. "The feature needs it" is the case they were written for. - -## Security model - -Authority is [ADR-0011 (Deep Linking Into Wallet)][adr11] and [ADR-0020 (Shared Deeplink -Registry)][adr20]. Three facts a route author has to hold: - -**Signature is a trust signal, not an authorization.** Both clients verify link signatures. -`SignatureStatus` is `VALID`, `INVALID`, or `MISSING`. A valid signature does not change the -route contract; it changes whether the interstitial shows. Signing happens in the internal -signer service behind privileged Okta — never reimplement it. - -**A trusted origin bypasses the interstitial before the signature is consulted.** -`shouldShowDeepLinkInterstitial` returns `false` for a request origin in -`TRUSTED_WEB_ORIGINS` — today exactly `https://metamask.io` and `https://app.metamask.io` — -ahead of any signature check. So a link initiated from those origins reaches your -destination unsigned and unwarned. **Registering a route inherits this.** You have not -changed it, but your destination is now reachable that way. - -**Unsigned links forward every parameter.** `canonicalize` keeps only the `sig_params` -allowlist for signed links; with no `sig_params` it takes a backward-compatibility branch and -forwards every param except `sig`. Your handler must assume hostile input on the unsigned -path. - -## Adding a route - -**1. Write the route file.** `shared/lib/deep-links/routes/.ts`: - -```ts -import { Route, SETTINGS_ROUTE, SHIELD_PLAN_ROUTE } from './route'; - -export const shield = new Route({ - pathname: '/shield', - getTitle: (_: URLSearchParams) => 'deepLink_theTransactionShieldPage', - handler: function handler(params: URLSearchParams) { - return { path: SHIELD_PLAN_ROUTE, query: params }; - }, -}); -``` - -`getTitle` returns an **i18n message key**, not a display string. `handler` returns a -`Destination` — either `{ path, query }` or `{ redirectTo: URL }` — and may throw if the -params cannot be processed. - -**2. Register it** in `shared/lib/deep-links/routes/index.ts`. Import and add to the exported -map. This is the line that makes the destination reachable. - -**3. Add an E2E test.** Not optional. From review on -[#38003](https://github.com/MetaMask/metamask-extension/pull/38003): *"We always need e2e -tests for these routes (I'm sure some teams are slipping by without adding them, but they -aren't supposed to!)"* - -**4. Add the CODEOWNERS entry** for your route file if your team owns the surface, following -`routes/perps.ts @MetaMask/perps`. - -## Defects that recur in review - -**An allowlist must not be a plain object literal.** A lookup against `{}` resolves inherited -keys, so `?setting=constructor` returns `function Object() { [native code] }` and the -`?? DEFAULT` fallback never fires. Use a `Set`, a `Map`, or `Object.create(null)`. - -*And the negative test must use an inherited key.* `'not-a-setting'` is `undefined` on the -prototype chain too, so it passes against a broken implementation and a correct one alike — -it cannot tell them apart. Test `'constructor'` or `'__proto__'`. - -**Use the validated parameter.** Validating `type` and then hardcoding the path means the -validation changes nothing. If a param is worth checking, the destination must depend on it. - -**Decide about dropped query params, and say so.** A handler that silently discards `params` -is a finding. Forward them or state why not. - -**Do not reintroduce `handlerSearchParams`.** The field existed and was reverted; `main` -carries zero occurrences. Routes default to canonical param handling, which removes unsigned -params for signed links. Restoring per-route control reopens what the reverts closed. - -**A new class of destination is a security question even when the code is routine.** Content -pages and a settings surface carrying consent toggles are not the same risk. If your -destination changes state, grants a permission, or exposes a toggle, raise it with the -`@MetaMask/extension-security-team` before it lands rather than after. - -[adr11]: https://github.com/MetaMask/decisions/blob/main/decisions/core/0011-deep-linking-into-wallet.md -[adr20]: https://github.com/MetaMask/decisions/pull/149 +It lives there rather than here because its audience is an extension engineer reading the +repository, not only an agent with this skill installed. This file is a pointer so the two do +not diverge. diff --git a/domains/coding/skills/deeplink-handler/skill.md b/domains/coding/skills/deeplink-handler/skill.md index 14bdc191..cd5ba145 100644 --- a/domains/coding/skills/deeplink-handler/skill.md +++ b/domains/coding/skills/deeplink-handler/skill.md @@ -1,28 +1,81 @@ --- name: deeplink-handler -description: Add or change a deep link route in a MetaMask client — the route file, its registration, the required E2E test, and the security boundary a feature team does not own. Use when exposing a new deep link, pointing one at a new destination, or when review raises the interstitial, signature verification, or param canonicalization. Names which paths belong to the extension security team and which are yours, plus the defects that recur in review — allowlists written as plain object literals, which admit every prototype key; validated params that never reach the destination; and silently dropped query strings. +description: Add or change a deeplink route in a MetaMask client, covering the route file, its registration, the required E2E test, and the security boundary a feature team does not own. Use when exposing a new deeplink, pointing one at a new destination, or when review raises the interstitial, signature verification, or param canonicalization. Carries the cross-client security model, defers to the client section for the per-client symbols and paths, and lists the defects that recur in review: validated params that never reach the destination, silently dropped query strings, and allowlists written as plain object literals, which admit every prototype key. --- -# Deep Link Routes +# Deeplink Routes -A deep link is an external entry point into the wallet. The route you add is reachable by -anyone who can construct the URL, so the parts that decide whether a link is trusted are -owned separately from the parts that decide where it goes. +A deeplink is an external entry point into the wallet. A registered route is reachable by +anyone who can construct the URL, so the parts that decide whether a link is trusted are owned +separately from the parts that decide where it goes. + +Installing this skill for a repository appends that repository's section to this file, below +the shared material. Everything client-specific lives there. In this repository the source +is `repos/.md` beside this file. ## When To Use -- Exposing a new deep link for a feature -- Pointing an existing deep link at a different destination +- Exposing a new deeplink for a feature +- Pointing an existing deeplink at a different destination - Review has raised the security interstitial, signature verification, or param handling -- A deep link reaches a destination that changes state or grants a permission +- A deeplink reaches a destination that changes state or grants a permission Not for changing how links are parsed, verified, or gated. That is a security-boundary change -with a different owner — see the repo overlay. +with a different owner, named in the client section. ## Workflow -1. Read the repo overlay for this client. The implementations differ substantially. -2. Confirm which files your change touches, and whether any are security-owned. +1. Read the client section below. The implementations differ substantially, and it names the + files, symbols and owners this workflow refers to. +2. Confirm which files the change touches, and whether any are security-owned. 3. Write the route or handler, register it, and add the E2E test. -4. If the destination changes state or grants a permission, raise it with the security owners - before the PR lands. +4. If the destination changes state or grants a permission, raise it with the client's + security owners before the PR lands. + +## Security model + +`decisions/core/0011-deep-linking-into-wallet.md` is on `main` in `MetaMask/decisions` and is +the record that applies. `decisions/core/0020-shared-deeplink-registry.md` (proposed, +MetaMask/decisions#149) is an open draft. Its own Status section reads `Proposed`, it is not +on `main`, and it lists implementing the package among its non-goals, so nothing in it +describes how either client works today. It proposes moving the public route contract into a +shared package and dispatching to client-owned handlers keyed by a shared `RouteId`, which +would replace per-client route registration in both clients. + +Each fact below holds in both clients. The symbols, origin lists, and file paths that +implement them are per-client and are named in the client section. + +**A signature is a trust signal, not an authorization.** Both clients verify link signatures. +`SignatureStatus` is `VALID`, `INVALID`, or `MISSING`. A valid signature does not change the +route contract. It changes whether the interstitial shows. Signing happens in the internal +signer service behind privileged Okta and is never reimplemented. + +**A trusted origin bypasses the interstitial before the signature is consulted.** A request +from such an origin reaches the destination unsigned and unwarned. Registering a route +inherits this. The bypass itself is unchanged, but the new destination is now reachable that +way. Which origins qualify, and the function that decides, are per-client and are named in the +client section. + +**Unsigned links forward every parameter.** A handler assumes hostile input on the unsigned +path. + +## Defects that recur in review + +**An allowlist must not be a plain object literal.** A lookup against `{}` resolves inherited +keys, so `?param=constructor` returns `function Object() { [native code] }` and the +`?? DEFAULT` fallback never fires. Use a `Set`, a `Map`, or `Object.create(null)`. + +*And the negative test must use an inherited key.* `'not-a-setting'` is `undefined` on the +prototype chain too, so it passes against a broken implementation and a correct one alike. It +cannot tell them apart. Test `'constructor'` or `'__proto__'`. + +**Use the validated parameter.** Validating a param and then hardcoding the path means the +validation changes nothing. If a param is worth checking, the destination must depend on it. + +**Decide about dropped query params, and say so.** A handler that silently discards the params +is a finding. Forward them or state why not. + +**A new class of destination is a security question even when the code is routine.** Content +pages and a settings surface carrying consent toggles are not the same risk. If the +destination changes state, grants a permission, or exposes a toggle, raise it with the +client's security owners before it lands rather than after.