From cfd6a37944c511ce3033d8cbbc81b41dcd8e0e1f Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:03:07 -0400 Subject: [PATCH 1/7] feat(security): add security domain with supply-chain-audit and lavamoat-policy-diligence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two composing skills for "is this dependency change safe to take". `supply-chain-audit` is the breadth pass: Socket findings, `yarn npm audit` advisories, lockfile and manifest diffs, and the fronts no upstream scanner sees because they are things the repo does to its dependencies afterwards — yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags, and yarn plugins that execute at install. `lavamoat-policy-diligence` is the depth pass it delegates capability containment to. Because a LavaMoat policy is generated from a real run, every grant has a call site by construction — so "each addition is justified" is a tautology, not a finding. It instead reads each grant's use at the installed version to find its gate, and sorts into removable / removable-at-a-cost / load-bearing. Neither renders an accept/reject verdict; disposition belongs to the people who own the dependency. Adds a CODEOWNERS entry for the new domain, defaulted to the platform teams. --- .github/CODEOWNERS | 1 + .../scripts/policy-audit.py | 60 +++++ .../skills/lavamoat-policy-diligence/skill.md | 231 ++++++++++++++++++ .../skills/supply-chain-audit/skill.md | 158 ++++++++++++ 4 files changed, 450 insertions(+) create mode 100644 domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py create mode 100644 domains/security/skills/lavamoat-policy-diligence/skill.md create mode 100644 domains/security/skills/supply-chain-audit/skill.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f156c522..cfd79699 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -18,6 +18,7 @@ /domains/performance/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/perps/ @MetaMask/perps /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/security/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/swaps/ @MetaMask/swaps-engineers /domains/testing/ @MetaMask/qa /domains/ui/ @MetaMask/design-system-engineers diff --git a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py new file mode 100644 index 00000000..6c7331e8 --- /dev/null +++ b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 +"""Turn a LavaMoat policy base/head pair into a per-grant justification worklist. + +Detection is LavaMoat's job: `@metamaskbot update-policies` regenerates the policy from a +real run of the code and CI fails on drift. This script does NOT re-derive or classify that +diff — it enumerates every capability newly granted so each can be JUSTIFIED with a permalink +to the dependency's own source (accept), or REJECTED where no call site uses it. + +Usage: policy-audit.py +Falsifier: a listed grant for which no upstream call site can be found. +""" +import json +import sys + + +def resources(path): + with open(path) as f: + return json.load(f).get("resources", {}) + + +def newly_granted(head, base): + """Every (pkg, kind, capability) that is true in head and absent/false in base.""" + out = [] + for pkg, cfg in head.items(): + for kind in ("globals", "builtins", "packages"): + for cap, val in (cfg.get(kind) or {}).items(): + if val and not ((base.get(pkg, {}).get(kind) or {}).get(cap)): + out.append((pkg, kind, cap)) + return out + + +def main(): + if len(sys.argv) != 3: + sys.exit("usage: policy-audit.py ") + base = resources(sys.argv[1]) + head = resources(sys.argv[2]) + grants = sorted(newly_granted(head, base)) + + print("PER-GRANT JUSTIFICATION WORKLIST") + print("=" * 74) + print("Detection is LavaMoat's; each row below needs a REASON, not a category.") + print("Justify with a permalink to the dependency's source at the installed version") + print("(accept), or reject where no call site uses the capability.\n") + + if not grants: + print(" (no new grants between base and head — nothing to justify)") + return + + for pkg, kind, cap in grants: + print( + f" [ ] {pkg[:38]:40s} {kind[:3]}:{cap:22s}" + " reason: verdict: accept|REJECT" + ) + + print(f"\n {len(grants)} grant(s) to justify.") + print(" A grant with no locatable call site is the finding — reject it.") + + +if __name__ == "__main__": + main() diff --git a/domains/security/skills/lavamoat-policy-diligence/skill.md b/domains/security/skills/lavamoat-policy-diligence/skill.md new file mode 100644 index 00000000..a1e3491f --- /dev/null +++ b/domains/security/skills/lavamoat-policy-diligence/skill.md @@ -0,0 +1,231 @@ +--- +name: lavamoat-policy-diligence +description: Triage a LavaMoat policy change for least privilege — which newly granted capabilities can be dropped without breaking anything. Detection is delegated to `@metamaskbot update-policies` plus CI, and because the policy is generated from a real run, every grant has a call site by construction, so "each addition is justified" is a tautology and not the deliverable. Instead read each grant's use at the installed version to find its gate — a config flag nobody sets, an API nobody calls, a branch our payloads never take, an error-only path — and sort into removable / removable-at-a-cost / load-bearing, with the removal test (drop it, rebuild, run e2e) proposed for the policy owners to run. Lead with removal candidates and anything the reading turned up that bears on security; never render an accept/reject verdict, that call is the reviewer's. Hand it over untagged while the workflow is in trial. Triggers on /lavamoat-policy-diligence, or when asked about a LavaMoat policy grant, policy.json diff, capability containment, scuttling, allowScripts, or why a package needs a global/builtin. The specialized engine behind `supply-chain-audit`'s capability-containment lane. +maturity: experimental +--- + +# /lavamoat-policy-diligence + +Detection is not the job. LavaMoat already tells you which capabilities a dependency change +grants: `@metamaskbot update-policies` regenerates the `policy.json` files from a real run of +the code, and CI's `validate-lavamoat-policies` fails the build if the committed policy drifts +from that regeneration. Re-deriving the diff by hand, or sorting the grants into +network/DOM/red-flag buckets, only re-does a machine that is already trusted. + +**Finding the call site is not the job either — that search cannot fail.** The policy is +*generated from a real run*, so every grant in it corresponds to something the bundled code +did. "Each addition has a call site, therefore each is justified" is a tautology dressed as an +audit; it will report 11 of 11 justified every time, and a check that cannot come back negative +carries no information. + +**The job is least privilege: which of these grants can be dropped without breaking +anything?** A grant exists because an identifier appears in bundled source. That is *not* the +same as a reachable path needing it — a capability read behind a config flag nobody sets, or in +a branch our usage never takes, is removable. So for each grant, ask what executes it under +*our* usage, and sort: + +| | | +|---|---| +| **removable** | nothing on our path executes the read → candidate; propose the test | +| **removable at a cost** | only a convenience or error-detail path executes it → name the cost | +| **load-bearing** | our usage genuinely needs it → say so briefly and move on | + +The lead is the first two rows plus anything the reading turned up that bears on security. +Load-bearing grants still each get a row in the capability → call-site table (step 5) — they just +don't get paragraphs. + +> **Falsifier.** A grant you called load-bearing that a build with it removed still passes. +> The test is cheap and it is the only thing that settles the question: drop the grant from the +> resource, rebuild, run the relevant e2e. Propose it; the policy owners run it. + +**Corollary — the reading is where the real findings come from.** Locating each call site means +reading the code that uses the capability, and that is when genuine issues surface: unbounded work +on remote input, a decode path with no size cap, a feature-detection fallback that makes a grant +droppable. Those observations are worth more than the grant inventory. Lead with them. + +## Method + +1. **Take the diff from LavaMoat; don't re-derive it.** The bot's `update-policies` run + produces the authoritative delta and CI enforces it. Your input is the list of + newly-`true` grants per package, not a hand-rolled scan. (`scripts/policy-audit.py` turns a + base/head policy pair into that list as a worklist — it enumerates, it does not classify.) + + **When no current CI policy exists, regenerate locally — that is the fallback, not a + competing method.** The bot hasn't run, the branch is unpushed, or a variant CI didn't + cover: `yarn webpack:lavamoat:policy:build` (`:mv2` / `:mv3`) produces the same base/head + pair to feed step 2. Note what changes and what doesn't: the *worklist* is equally valid, + but its **provenance is weaker** — it reflects your node version, OS, and lockfile + resolution rather than CI's. Say which source the diff came from when handing the + justification over, and re-check against the bot's policy once it runs. Never regenerate + locally *in preference to* an available CI policy; that re-does a trusted machine and + substitutes a less reproducible artifact for a more reproducible one. + +2. **Know what denial actually does before you reason about it.** An ungranted global is **absent + from the package's endowments and reads as `undefined`** — it does not throw. + `getEndowmentsForConfig` collects `whitelistedReads`, `makeMinimalViewOfRef` builds an object + holding only those, and a `false` value simply keeps the path out of that list + ([endowmentsToolkit.js](https://github.com/LavaMoat/LavaMoat/blob/f5e52ab457c16c3aea72cc8a9dd0833547dd7d2c/packages/core/src/endowmentsToolkit.js#L101-L162)). + This is the whole basis of the analysis, so get it right: **do not confuse per-package + `globals` policy with scuttling**, which is a separate root-realm mechanism. Asserting the + wrong one to the LavaMoat maintainer got the reply "This is jibberish" (#45024, 2026-07-30). + + The consequence is what makes denial testable: a package that reads a global behind a + `globalThis.X || ` guard **keeps working when denied**, because the read yields + `undefined` and the fallback engages. A feature-detection shim is therefore evidence *for* + removability, not against it. + +3. **Read each grant's call site — then ask what executes it under our usage.** Read the + dependency's code *at the version being installed*. Locating the use is the start, not the + answer; the question the reading has to settle is whether anything on our path runs it. Look + for the gate: a config flag (`BigNumber.set({CRYPTO:true})`), an API we never call + (`.random()`), a feature-detection fallback, a branch keyed on a payload type we never send + (`data instanceof Blob` — note this one *does* break when denied, since `instanceof undefined` + throws), an error-only path. A gated read whose gate we never open is a removal candidate. + + Check reachability from *our* side too, not just the dependency's: does our code subscribe to + the feed, import the subpath, take that option? A capability behind a feature we don't use is + the cleanest removal there is — and a capability behind one we *do* use is load-bearing, which + is worth one line and no more. + +4. **Cite it at a pinned tag, not a branch head.** A permalink to `…/blob//#Ln` is + immutable; a branch-head link drifts out from under the citation. The permalink *is* the + evidence — a reader clicks it and lands on the code, convinced without re-running anything. + "It needs X" retyped into a table proves nothing about provenance. + +5. **Lead with removal candidates and anything security-relevant — then give the full table.** + Open on what can be dropped and what the reading turned up, not on an inventory. But every + grant still gets its own row in the capability → call-site table, load-bearing ones included: + that mapping is what a reviewer came for, and a load-bearing row is one short row, not a + reason to merge it into prose with its neighbours. **Removed grants get their names only** + (`WebSocket` and `CustomEvent` are removed by this bump) — no table, no justification column, + since a removal reduces capability and needs no defence. The exception is a removal that is + itself interesting: one that was load-bearing implies a behaviour change worth a sentence. + + Target a few hundred words of *prose*; the table does not count against that and must not be + compressed to hit it. (Violated on extension#45024, 2026-07-30 — a trim pass dissolved the + table into paragraphs and destroyed the comment's key content.) + + **The accept/reject call belongs to the human reviewer; never write it.** No `accept` + column, no `REJECT`, no "Verdict: safe to take", no ✅/❌. Those words do the reviewer's + deciding for them and anchor the judgment before they have read the evidence — and if the + call is wrong, it is wrong in a document that looks authoritative. Describing *risk* is in + scope where it is a fact about the capability ("this reads the global on every exception + path", "these are decode and timer primitives, no filesystem or subprocess reach"); the + disposition is not. State findings and open questions, and let the reviewer conclude. + (Violated on extension#45024, 2026-07-30 — 11 `accept` cells and a "Verdict" section.) + + **A grant with no locatable call site is a real finding, and rare.** On a generated policy it + usually means the identifier is present but the generator saw it in a path you haven't found + — say what you searched rather than implying nothing uses it. + +6. **Put it where the policy is reviewed — but do not tag anyone.** Post the justification as a + comment on the PR carrying the `policy.json` change, so it lands in front of the people who + own the policy rather than standing as a unilateral assertion elsewhere. + + **Show the full comment body in the response before running `gh pr comment`.** The permission + prompt renders the command, not the `--body-file` contents, so approving it blind is approving + unseen text published under the user's name. Paste the table and prose inline first, then post. + Naming the scratchpad path instead of showing the text is the same failure. + + **Do not add `cc @MetaMask/policy-reviewers` — or any `@`-mention — unless the user asks for + it in this session.** Authorization to post a comment is not authorization to notify a team, + and this step being written into the skill does not supply that authorization; editing the + comment afterwards does not un-send the ping. Draft without the tag, post, then offer the cc + line as a separate ready-to-paste suggestion. + + **Status as of 2026-07-30: hold the cc — this workflow is in a trial phase.** The tag is + expected to become standard once the output has proven itself; it is being withheld for now, + not forbidden on principle. So the rule is about *timing being the user's call*, not about + tagging being wrong. Re-confirm before assuming trial phase still applies — and even after it + ends, the tag goes in because the user says so, not because this line stops saying "hold". + (Violated on extension#45024, 2026-07-30.) + +7. **One reason covers the variants.** Extension builds carry several policy files + (`lavamoat/webpack/{mv2,mv3}/{beta,experimental,flask,main}/policy.json`). When the grant + delta is identical across them, a single justification covers all — confirm the identity + once. A grant that appears in one variant and not others is itself a question. + +## Output + +**The capability → call-site table is the deliverable. Never dissolve it into prose.** +One row per grant, every grant, with its permalink and its removability in the row. A reviewer +scans the column, not paragraphs — 11 rows is denser and faster to read than three paragraphs +carrying the same 11 facts, so the table *is* the trimmed form. Prose around it is what gets cut. + +``` +LavaMoat grants — -> + + + +| capability | package | call site | can it go? | +|---|---|---|---| +| | | | yes — | +| | | · | at a cost — | +| | | | no | +…every grant gets a row… + +Test for the "yes" rows: drop the grant, rebuild, run . +Note on : +Removed: , . ← names only, no table, no justification column +Loose ends: + +``` + +Order is the point: the lead frames the question, the table answers it, and a reviewer hits the +removable rows immediately. Post it on the PR carrying the `policy.json` change, untagged. + +**Runtime claims need a runtime artifact.** "Byte-identical across all 8 policy files" is an +observation, not something a `/blob/` link witnesses — publish the check output (JSON) and link +it. The `pr-evidence-gate` hook enforces this and will block the post otherwise; it is right to. + +## Worked example — extension#42867 (@sentry/browser 8.33.1 → 10.38.0) + +The bump added grants across the `@sentry/*` subtree; the bot produced the diff and CI +enforced it — detection was never in question. On `mv2/main/policy.json` a reviewer questioned +two grants: *"I wonder what it's using this for. Likewise for `importScripts`."* Each was +answered with the upstream line, pinned to `10.38.0`: + +- **`WebAssembly`** → the event builder's `isWebAssemblyException` check, which runs on *every* + exception (defined L165, called on the exception path L186 and L203): + `https://github.com/getsentry/sentry-javascript/blob/10.38.0/packages/browser/src/eventbuilder.ts#L163-L168` +- **`importScripts`** → the profiling utils' main-thread detection at module scope + (`typeof importScripts === 'undefined'`): + `https://github.com/getsentry/sentry-javascript/blob/10.38.0/packages/browser/src/profiling/utils.ts#L33-L34` + +Both run unconditionally — the exception path and module scope — and under scuttling the read +itself throws unless excepted, so both are load-bearing with no gate to close. That is the +useful conclusion: *not* "each grant has a reason" (it always will) but "neither is removable, +and here is the unconditional path that makes it so." + +**Counter-example from extension#45024, which the first pass got wrong.** That comment reported +"11 additions, 11 reasons, each resolving to a line" as its headline. Tautological — the policy +is generated from a run, so the count was guaranteed. Reading for *gates* instead surfaced the +actual findings: `crypto` on `bignumber.js` is reachable only via `BigNumber.set({CRYPTO:true})` +or `.random()`, neither of which the consumer calls, so it is a removal candidate; and the +`DecompressionStream` grant sits on a `fastAssetCtxs` decode path that inflates remote input +with no size cap. Same reading, same permalinks — the first framing hid both. + +## Scope — what this skill is NOT + +This covers **capability containment only**: what a dependency is *permitted to reach* under +LavaMoat, and whether each new permission has a reason. It says nothing about whether the +dependency is *known-vulnerable* or *behaving maliciously* — those are different questions with +different detectors and different falsifiers, and they live in `supply-chain-audit`: + +| question | detector | skill | +|---|---|---| +| does this dep now reach a capability it didn't? | LavaMoat policy diff | **this skill** | +| is this dep version known-vulnerable? | `yarn npm audit`, advisories | `supply-chain-audit` | +| is this package behaving maliciously / newly-authored / install-scripted? | Socket Security | `supply-chain-audit` | + +A clean policy diff does not mean a safe dependency, and a known CVE does not show up as a new +grant. Run the umbrella skill when the question is "is this bump safe"; run this one when the +question is "why does it need that". + +## Called by supply-chain-audit and pr-validate + +`supply-chain-audit` delegates its capability-containment lane here. pr-validate keeps +**supply-chain** as an evidence category and packages the per-grant justification (accept / +reject, each with its permalink) posted where the policy is reviewed. Engine helper: +`scripts/policy-audit.py`. Usable standalone whenever a policy grant needs a reason. diff --git a/domains/security/skills/supply-chain-audit/skill.md b/domains/security/skills/supply-chain-audit/skill.md new file mode 100644 index 00000000..32dbffd2 --- /dev/null +++ b/domains/security/skills/supply-chain-audit/skill.md @@ -0,0 +1,158 @@ +--- +name: supply-chain-audit +description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy-diligence`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by pr-validate as the engine behind its supply-chain evidence category. +maturity: experimental +--- + +# /supply-chain-audit + +**"Is this bump safe?" is not one question.** A dependency can be free of known CVEs and still +reach `child_process` for the first time. It can have a clean policy diff and ship a +newly-added install script from a maintainer who joined last week. Each detector answers a +different question and is blind to the others, so a single green check is never the answer. + +> **Falsifier.** Any lane's finding left unaccounted for: a Socket alert nobody dispositioned, +> an advisory with no upgrade path or accepted-risk note, a capability grant with no call site. +> An unexplained finding is the output, not a nit to wave through. + +## Lanes + +| question | detector | disposition | +|---|---|---| +| what actually changed? | lockfile / `package.json` diff | direct vs transitive; resolution swaps; version range widening | +| known-vulnerable? | `yarn npm audit`, GitHub advisories, Dependabot | fixed-in version, or an explicit accepted-risk with reachability | +| behaving maliciously or anomalously? | **Socket Security** | per-alert disposition — see below | +| new capability reached? | **LavaMoat** policy diff | **delegate to `lavamoat-policy-diligence`** | +| install-time code execution? | `allowScripts` in `package.json` (`@lavamoat/allow-scripts`) | a newly-`true` entry is a finding in its own right | +| **is dependency source modified in-repo?** | **`.yarn/patches/*.patch`** | read the diff — see below | +| **is a version being forced?** | **`resolutions`** in `package.json` | pinned below a fix? stubbed out? | +| **are findings being suppressed?** | **`npmAuditIgnoreAdvisories`** in `.yarnrc.yml` | every entry needs a reason and a re-check date | +| **does untrusted code run in CI?** | **`uses:` pinning** in `.github/workflows` | third-party actions pinned to a full SHA, not a mutable tag | +| **does untrusted code run at install?** | **`.yarn/plugins/*.cjs`** + their `spec:` URLs | committed bundle reviewed; spec pinned, not `main` | + +## The fronts that no scanner covers + +Socket, `audit`, and LavaMoat all examine the dependency **as published**. The last five lanes +above are things *your own repo, or your CI,* does around dependencies afterwards, so no +upstream scanner sees them. Measured on `metamask-extension` today, to show these aren't hypothetical: + +- **Yarn patches — 53 of them.** A patch is arbitrary modification of a dependency's source, + applied at install, living in your repo. It is the single most direct injection point in the + list and the least watched: the package can be clean at every scanner and still execute your + patch. **Read every patch diff on change**, the same way you'd read a diff to `app/`. A patch + that grows beyond its stated purpose, or touches a file unrelated to the bug it works around, + is the finding. Record why each patch exists and what removes it (upstream fix, version bump) + — an unattributed patch is technical debt with a security surface. + +- **`resolutions` — 149 entries.** Forcing a version across the tree. Two failure modes: a pin + that holds a transitive *below* the version that fixed an advisory (audit may not flag it, + because the range resolves), and outright substitution — this repo maps several packages to + `npm:npm-empty-package@1.0.0` to neutralize them. Substitution is legitimate and deliberate, + but it means **"same version range" does not imply "same code"**, so treat a resolution + change as a dependency change and re-run the lanes on it. + +- **`npmAuditIgnoreAdvisories` — a suppression list.** Entries here are accepted risks by + definition, and this repo's numeric IDs carry no inline reason (its deprecation entries do). + This directly contradicts this skill's own falsifier: an unaccounted finding is the output. + Each entry wants a reason, an owner, and a condition that retires it. An ignore list nobody + revisits converts a finding into silence. + +- **CI action pinning — 7 of 47 third-party `uses:` are SHA-pinned.** The rest ride mutable + tags (`actions/checkout@v6`, `actions/github-script@v9`). A tag can be repointed by its owner + or by anyone who compromises that account, and CI holds secrets — this is the + `tj-actions/changed-files` failure mode. Pin third-party actions to a full 40-char commit + SHA. First-party (`MetaMask/*`, 25 here) is lower risk but the same mechanism. + +- **Yarn plugins execute at install with full privilege.** Three `.cjs` bundles are committed + (good — the committed bytes are what runs), but their `spec:` URLs point at + `raw.githubusercontent.com/.../main/...`, a moving branch. Re-importing pulls whatever `main` + holds that day. Pin the spec to a tag or SHA, and review the bundle diff when it changes. + +**Also consider `enableHardenedMode`** (Yarn 4) — not currently set here. It validates +resolutions and checksums against the registry, and is designed for exactly the untrusted-PR +case. And leave `checksumBehavior` at its default (`throw`): a checksum mismatch means the +registry served different bytes for a version you already resolved, which is a signal, not a +nuisance. + +Run the lanes the change actually touches. A lockfile-only bump of a build-time dev dependency +does not need the same treatment as a new runtime dependency in the wallet's hot path — but +say which lanes you ran and which you skipped, and why. + +## Method + +1. **Establish what changed before assessing it.** Direct bump, transitive pull-through, or a + *resolution swap* (same range, different resolved package)? The last is the easiest to miss + and the most interesting: an identifier substitution in a policy or lockfile + (`pkgC>name` replacing `pkgB>pkgA>name`) can mean the dependency was replaced rather than + updated. + +2. **Take each tool's findings as the worklist; don't re-derive them.** Socket, audit, and + LavaMoat all run in CI and are trusted machines. Re-implementing their detection by hand + re-does work and produces a less reproducible artifact. Your input is their output. + +3. **Prefer the CI-generated artifact over a local regeneration.** Where a bot regenerates + something (policies especially) and CI enforces drift, that committed artifact is + authoritative. Regenerate locally only as a **fallback** — bot hasn't run, branch unpushed, + variant not covered — and note that the provenance is weaker (your node version, OS, and + lockfile resolution, not CI's). Re-check against the bot's artifact once it runs. + +4. **Disposition every finding; the reason is the deliverable.** For each alert or advisory: + what the tool flagged, whether it is reachable from how *this* project uses the package, and + the outcome — fixed by upgrading to X, accepted with a stated reason, or blocking. Socket's + common alert classes need different reasoning: `install scripts` (what does it run, at whose + trust level), `new author` / `low download count` (typosquat and takeover surface), + `network access` / `filesystem access` in a package with no business doing either, + `obfuscated code`, `protestware`. A tool's severity is an input to that judgment, not a + substitute for it. + +5. **Cite at a pinned version, not a branch head.** Every claim about what a dependency does + resolves to a permalink at the version being installed. A branch-head link drifts out from + under the citation; the permalink *is* the evidence, because a reader clicks it and is + convinced without re-running anything. + +6. **Hand it to the owners.** Post the disposition where the people who own the dependency + read it, not as a unilateral assertion. This skill produces a justification for humans to + act on; it does not approve anything. + +## Capability containment → `lavamoat-policy-diligence` + +LavaMoat policy grants are a specialized lane with their own method and tooling. **Delegate to +`lavamoat-policy-diligence`** and fold its result in as this audit's capability-containment lane. + +Do not restate that lane's question as "does each new capability have a call site" — the policy +is generated from a real run, so it always does, and that check cannot fail. The lane's actual +output is a least-privilege triage: which grants are **removable** (their gate is never opened by +our usage), which are removable at a stated cost, which are load-bearing, plus anything the +reading turned up bearing on security. Carry those findings through; do not compress them to a +pass/fail. + +Keep the boundary straight in the writeup: **a clean policy diff does not mean a safe +dependency, and a known CVE does not appear as a new grant.** They are independent. + +## Output + +``` +Supply-chain assessment — -> () + lockfile/manifest + advisories → fixed in | accepted: | none + Socket → | no alerts + install scripts | unchanged + patches <.yarn/patches touched> → diff read: | unchanged + resolutions | unchanged + audit ignores → reason + retire-when | unchanged + ci actions → SHA-pinned? | unchanged + capabilities → lavamoat-policy-diligence: | no policy change + lanes skipped — +Unresolved: — | none +``` + +Lead with whatever is actionable — an unresolved finding, a removable capability, a patch whose +scope exceeded its purpose. Lanes that came back clean are a compact line each, not sections. +**No overall accept/reject verdict and no `@`-mentions**: the disposition belongs to the people +who own the dependency, and tagging them is the user's call, not this skill's. Close on what is +unresolved and what would settle it. + +## Related + +- `lavamoat-policy-diligence` — the capability-containment engine this skill delegates to. +- `pr-validate` — packages this skill's output as its supply-chain evidence category. From d9c6231a11ff782ecf0b8c54978cad03124a9856 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 10:26:20 -0400 Subject: [PATCH 2/7] Update `pr-validate` references to `evidence` in the security skills `pr-validate` was renamed to `evidence`; both skills still named the old one, in a section heading, prose, and a `## Related` entry. `supply-chain-audit` now also links its evidence category in the catalog rather than naming it bare. --- domains/security/skills/lavamoat-policy-diligence/skill.md | 4 ++-- domains/security/skills/supply-chain-audit/skill.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/domains/security/skills/lavamoat-policy-diligence/skill.md b/domains/security/skills/lavamoat-policy-diligence/skill.md index a1e3491f..4a86838d 100644 --- a/domains/security/skills/lavamoat-policy-diligence/skill.md +++ b/domains/security/skills/lavamoat-policy-diligence/skill.md @@ -223,9 +223,9 @@ A clean policy diff does not mean a safe dependency, and a known CVE does not sh grant. Run the umbrella skill when the question is "is this bump safe"; run this one when the question is "why does it need that". -## Called by supply-chain-audit and pr-validate +## Called by supply-chain-audit and evidence -`supply-chain-audit` delegates its capability-containment lane here. pr-validate keeps +`supply-chain-audit` delegates its capability-containment lane here. `evidence` keeps **supply-chain** as an evidence category and packages the per-grant justification (accept / reject, each with its permalink) posted where the policy is reviewed. Engine helper: `scripts/policy-audit.py`. Usable standalone whenever a policy grant needs a reason. diff --git a/domains/security/skills/supply-chain-audit/skill.md b/domains/security/skills/supply-chain-audit/skill.md index 32dbffd2..8a8cb91c 100644 --- a/domains/security/skills/supply-chain-audit/skill.md +++ b/domains/security/skills/supply-chain-audit/skill.md @@ -1,6 +1,6 @@ --- name: supply-chain-audit -description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy-diligence`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by pr-validate as the engine behind its supply-chain evidence category. +description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy-diligence`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by `evidence` as its supply-chain engine. maturity: experimental --- @@ -155,4 +155,4 @@ unresolved and what would settle it. ## Related - `lavamoat-policy-diligence` — the capability-containment engine this skill delegates to. -- `pr-validate` — packages this skill's output as its supply-chain evidence category. +- `evidence` — packages this skill's output as its [supply-chain evidence category](https://github.com/MetaMask/skills/blob/main/domains/pr-workflow/skills/evidence/references/evidence-catalog.md). From 73d4b1734148e454eafb23581ea58ead8ef62ae0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Sat, 1 Aug 2026 10:56:01 -0400 Subject: [PATCH 3/7] Audit override scope, and escalate rather than rule on the critical grants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `--override`: overrides are where containment is widened by a person rather than observed by the toolchain, and the decision persists across regenerations, so it outlives the reason for it. On the extension's mv3/main policy: 42 tightened, 83 persisting, 9 narrowable, 2 write. Three things came from reading lavamoat-core/src/mergePolicy.js rather than inferring them, each of which had been wrong: - The effective policy is mergePolicy(generated, override). The two files are DESIGNED not to align — the generated one is regenerated on dependency updates while the override persists — so an override entry absent from the generated policy is the normal case. Calling that "never observed" was alarmist and wrong; 83 of 87 entries are in that state by construction. - `validateHierarchy` throws when both `X` and `X.y` are present, so the first version of the narrowing suggestion would have produced a policy that fails to build. The documented form denies the parent: `"X": false, "X.y": true`. - Escalation flagged every global in any package matching a name hint, labelling `Array` and `Object` as "critical class". Intrinsics are now excluded and the reason string is true of the row it appears on — an escalation list that is mostly noise trains its reader to skip it. Critical grants and write access get RAISE WITH A HUMAN and no verdict. Correctness there depends on intent and threat model, neither of which is in the policy files, and an audit that silently resolves them has substituted a guess for the thing it was asked to check. --- .../scripts/policy-audit.py | 183 ++++++++++++++++-- 1 file changed, 170 insertions(+), 13 deletions(-) diff --git a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py index 6c7331e8..c6a39221 100644 --- a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py +++ b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py @@ -1,17 +1,56 @@ #!/usr/bin/env python3 -"""Turn a LavaMoat policy base/head pair into a per-grant justification worklist. +"""Turn a LavaMoat policy base/head pair into a per-grant justification worklist, +and audit hand-written overrides for scope. Detection is LavaMoat's job: `@metamaskbot update-policies` regenerates the policy from a real run of the code and CI fails on drift. This script does NOT re-derive or classify that diff — it enumerates every capability newly granted so each can be JUSTIFIED with a permalink to the dependency's own source (accept), or REJECTED where no call site uses it. -Usage: policy-audit.py +With --override it also audits `policy-override.json`. Per lavamoat-core/src/mergePolicy.js the +effective policy is `mergePolicy(generated, override)`, with priority to stricter decisions in +the override. The two files are DESIGNED not to align: the generated policy is regenerated on +dependency updates while the override persists, which is the whole point of separating them. +So an override entry absent from the generated policy is the normal case, not a finding. + +What the audit reports is therefore scope, not divergence: which entries persist without a +recorded reason, which are broader than the generated policy shows a need for, and which grant +write. Narrowing a whole-object grant requires setting the parent to `false` — `validateHierarchy` +throws if `X` and `X.y` are both present. + +Usage: + policy-audit.py + [--override ] [--generated ] + Falsifier: a listed grant for which no upstream call site can be found. """ import json import sys +# Capability classes where a wrong call is not recoverable by a follow-up PR. The script +# refuses a verdict on these by design — it reports and escalates. Naming them explicitly +# rather than scoring them keeps the escalation auditable. +CRITICAL = { + "child_process", "fs", "vm", "worker_threads", "module", "process", + "eval", "Function", "WebAssembly", "importScripts", "SharedArrayBuffer", + "fetch", "XMLHttpRequest", "WebSocket", "crypto", "indexedDB", + "localStorage", "sessionStorage", "chrome", "browser", +} +CRITICAL_PKG_HINTS = ("keyring", "vault", "snap", "lavamoat", "seed", "wallet") + +# ECMAScript and DOM intrinsics. Granting these is unremarkable — a package that renders +# anything touches Element and Object — so they never escalate on their own. Without this +# a sensitive-package hint floods the list with `Object`, `String`, `Array`, and an +# escalation list that is mostly noise trains its reader to skip it. +BENIGN = { + "Array", "Object", "String", "Number", "Boolean", "Symbol", "BigInt", "Math", + "JSON", "Date", "RegExp", "Map", "Set", "WeakMap", "WeakSet", "Weakmap", + "Promise", "Error", "TypeError", "Proxy", "Reflect", "Intl", + "Document", "DocumentFragment", "Element", "Event", "EventTarget", "Node", + "NavigateEvent", "NavigationDestination", "Clipboard", "CSS", "Text", + "console", "queueMicrotask", "structuredClone", +} + def resources(path): with open(path) as f: @@ -29,11 +68,78 @@ def newly_granted(head, base): return out +def root_of(cap): + return cap.split(".", 1)[0] + + +def criticality(pkg, cap): + """Return a reason string, or None. The reason is reported verbatim, so it must be + true of THIS row — a package-level hint is not a claim about the capability.""" + root = root_of(cap) + if root in CRITICAL: + return f"critical capability: {root}" + if root in BENIGN: + return None + if any(h in pkg.lower() for h in CRITICAL_PKG_HINTS): + return "non-intrinsic grant in a security-sensitive package" + return None + + +def is_critical(pkg, cap): + return criticality(pkg, cap) is not None + + +def audit_overrides(over, gen): + """Classify each override entry against what the generated policy observed. + + widened granted here, not observed by the toolchain — a human decision needing a reason + tightened explicit false over an observed grant — containment narrowed, no action + broad whole-object grant where only specific members were observed — narrowable + write write access; read may suffice, and only call sites can settle it + """ + widened, tightened, broad, write = [], [], [], [] + for pkg, cfg in over.items(): + gpkg = gen.get(pkg, {}) + for kind, caps in cfg.items(): + if not isinstance(caps, dict): + continue + gcaps = gpkg.get(kind) or {} + for cap, val in caps.items(): + if val == "write": + write.append((pkg, kind, cap)) + continue + if val is False: + if gcaps.get(cap): + tightened.append((pkg, kind, cap)) + continue + if not gcaps.get(cap): + # NOT "never observed" — the generated policy is regenerated + # independently, so absence here is expected. This is only a list of + # entries whose justification lives outside both files. + widened.append((pkg, kind, cap)) + if "." not in cap: + members = sorted( + c for c in gcaps if c.startswith(cap + ".") and gcaps.get(c) + ) + if members and not gcaps.get(cap): + broad.append((pkg, kind, cap, tuple(members))) + return widened, tightened, broad, write + + def main(): - if len(sys.argv) != 3: - sys.exit("usage: policy-audit.py ") - base = resources(sys.argv[1]) - head = resources(sys.argv[2]) + args = sys.argv[1:] + if len(args) < 2: + sys.exit("usage: policy-audit.py " + "[--override ] [--generated ]") + base_p, head_p = args[0], args[1] + over_p = gen_p = None + for i, a in enumerate(args): + if a == "--override" and i + 1 < len(args): + over_p = args[i + 1] + if a == "--generated" and i + 1 < len(args): + gen_p = args[i + 1] + + base, head = resources(base_p), resources(head_p) grants = sorted(newly_granted(head, base)) print("PER-GRANT JUSTIFICATION WORKLIST") @@ -44,16 +150,67 @@ def main(): if not grants: print(" (no new grants between base and head — nothing to justify)") + else: + for pkg, kind, cap in grants: + mark = "!" if is_critical(pkg, cap) else " " + print(f" {mark}[ ] {pkg[:38]:40s} {kind[:3]}:{cap:22s}" + " reason: verdict: accept|REJECT") + crit = [g for g in grants if is_critical(g[0], g[2])] + print(f"\n {len(grants)} grant(s) to justify" + + (f"; {len(crit)} marked ! for escalation." if crit else ".")) + print(" A grant with no locatable call site is the finding — reject it.") + + if not over_p: return + gen = resources(gen_p) if gen_p else head + over = resources(over_p) + widened, tightened, broad, write = audit_overrides(over, gen) + + print("\n\nOVERRIDE SCOPE AUDIT") + print("=" * 74) + print("Effective policy = mergePolicy(generated, override), stricter decisions winning.") + print("The files are meant to differ: the generated one is regenerated on dependency") + print("updates while the override persists. Entries below are scoped, not \"unobserved\".") + print(f"{len(over)} package(s) overridden.\n") + print(f" tightened {len(tightened):3d} explicit false over an observed grant — containment narrowed") + print(f" persisting {len(widened):3d} in the override only — expected, but each needs a standing reason") + print(f" broad {len(broad):3d} whole-object grant where only members were observed") + print(f" write {len(write):3d} write access — read may suffice") - for pkg, kind, cap in grants: - print( - f" [ ] {pkg[:38]:40s} {kind[:3]}:{cap:22s}" - " reason: verdict: accept|REJECT" - ) + if broad: + print("\n\nSUGGESTED TIGHTENINGS — evidence-based, functionality-preserving") + print("-" * 74) + print("A whole-object grant where the generated policy shows only members in use.") + print("`validateHierarchy` REJECTS a policy containing both `X` and `X.y`, so narrowing") + print("requires denying the parent explicitly — that is LavaMoat's documented form:") + print(" \"You could set the parent to false if you intended a less permissive policy.\"") + print("Regenerate and re-run the app after applying; an over-narrowed grant fails loudly.\n") + for pkg, kind, cap, members in sorted(broad)[:20]: + print(f" {pkg}") + narrowed = {cap: False} + narrowed.update({m: True for m in members}) + print(f" now: \"{cap}\": true") + print(f" → " + json.dumps(narrowed)[1:-1]) + if len(broad) > 20: + print(f"\n … {len(broad) - 20} further narrowable grant(s).") - print(f"\n {len(grants)} grant(s) to justify.") - print(" A grant with no locatable call site is the finding — reject it.") + escalate = sorted(set( + [(p, k, c) for p, k, c in widened if is_critical(p, c)] + + [(p, k, c) for p, k, c in write] + )) + if escalate: + write_set = set(write) + print("\n\nRAISE WITH A HUMAN — no verdict offered") + print("-" * 74) + print("These widen a capability class where a wrong call is not recoverable by a") + print("follow-up PR, or grant write where read may suffice. Whether each is correct") + print("depends on intent and threat model, neither of which is in the policy files.") + print("This script stops here deliberately rather than guessing.\n") + for pkg, kind, cap in escalate: + why = "write access" if (pkg, kind, cap) in write_set else criticality(pkg, cap) + print(f" [?] {pkg[:42]:44s} {kind[:3]}:{cap:20s} {why}") + print(f"\n {len(escalate)} decision(s) for a human. An audit that silently resolves") + print(" these has substituted a guess for the thing it was asked to check.") if __name__ == "__main__": From f0eaf4111c3a3eec9cfe286f6a9dfe28be79b429 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Sat, 1 Aug 2026 12:39:16 -0400 Subject: [PATCH 4/7] Separate the grants someone chose from the ones a new package brought MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single escalation list mixes two different questions. A capability granted to a package the base policy did not contain arrives because the package arrived — the question there is whether the package belongs in the bundle. A capability newly granted to a package already contained is somebody's decision about that capability. Mixed together on one mv3 policy, 14 of the 24 rows were the first kind, and a list that is mostly not actionable teaches its reader to skim. Write access is exempt from the row cap: it is the smallest and highest-signal category, and truncating it hides the row that most needed a reader. --- .../scripts/policy-audit.py | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py index c6a39221..e87e45d7 100644 --- a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py +++ b/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py @@ -126,6 +126,21 @@ def audit_overrides(over, gen): return widened, tightened, broad, write +def partition_escalations(rows, base, write_set): + """Split a human's worklist into decisions and consequences. + + A capability granted to a package the base policy did not contain at all arrives + because the package arrived — a bundle-graph change, not a choice anyone made about + that capability. A capability newly granted to a package already contained is + somebody's decision. Under one heading the second is buried in the first: a list + where 23 of 24 rows are not actionable teaches its reader to skim past the one that is. + """ + chosen, inherited = [], [] + for pkg, kind, cap in rows: + (inherited if pkg not in base else chosen).append((pkg, kind, cap)) + return chosen, inherited + + def main(): args = sys.argv[1:] if len(args) < 2: @@ -200,15 +215,40 @@ def main(): )) if escalate: write_set = set(write) + chosen, inherited = partition_escalations(escalate, base, write_set) print("\n\nRAISE WITH A HUMAN — no verdict offered") print("-" * 74) print("These widen a capability class where a wrong call is not recoverable by a") print("follow-up PR, or grant write where read may suffice. Whether each is correct") print("depends on intent and threat model, neither of which is in the policy files.") print("This script stops here deliberately rather than guessing.\n") - for pkg, kind, cap in escalate: + def row(pkg, kind, cap): why = "write access" if (pkg, kind, cap) in write_set else criticality(pkg, cap) print(f" [?] {pkg[:42]:44s} {kind[:3]}:{cap:20s} {why}") + + print(f"\nCHOSEN HERE — {len(chosen)} row(s), on a package the base already contained") + if chosen: + for pkg, kind, cap in chosen: + row(pkg, kind, cap) + else: + print(" (none — every escalation below arrived with a new package)") + + if inherited: + print(f"\nARRIVES WITH A NEWLY-CONTAINED PACKAGE — {len(inherited)} row(s)") + print(" The package is new to this policy, so the grant follows from containing") + print(" it. The question is whether the package belongs in this bundle, not") + print(" whether the capability was correctly chosen.") + # Write access is never truncated. It is the smallest and highest-signal + # category, and a cap that hides it turns the section into a list whose + # most important row is the one the reader cannot see. + w = [r for r in inherited if r in write_set] + rest = [r for r in inherited if r not in write_set] + for pkg, kind, cap in w: + row(pkg, kind, cap) + for pkg, kind, cap in rest[:10]: + row(pkg, kind, cap) + if len(rest) > 10: + print(f" … {len(rest) - 10} further read-only row(s) of the same kind.") print(f"\n {len(escalate)} decision(s) for a human. An audit that silently resolves") print(" these has substituted a guess for the thing it was asked to check.") From af1a58fdd2991927455b30a15d800da5600a1afe Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 05:39:54 -0400 Subject: [PATCH 5/7] Rename `lavamoat-policy-diligence` to `lavamoat-policy` and restore the breadth step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installs as `mms-lavamoat-policy`. A trim pass had deleted the step that asks how wide each grant is versus what the call actually uses, leaving only the reachability question. That step is what produces findings on grants that are reached and not stale — a bare `node:url` standing in for `fileURLToPath` and `pathToFileURL` clears every other check. Also corrects the stated mechanism. Policy is generated by static analysis (`lavamoat-tofu`, `@babel/parser`), not by observing a run, and the difference matters: a statically detected grant says nothing about whether our usage reaches it, which is where removal candidates come from. And scopes the tautology claim to the *existence* of a call site rather than its content — the policy records `"crypto": true` and discards what a reviewer needs, so recovering that is the job rather than something to skip. --- .../scripts/policy-audit.py | 0 .../skill.md | 86 +++++++++++++------ .../skills/supply-chain-audit/skill.md | 12 +-- 3 files changed, 68 insertions(+), 30 deletions(-) rename domains/security/skills/{lavamoat-policy-diligence => lavamoat-policy}/scripts/policy-audit.py (100%) rename domains/security/skills/{lavamoat-policy-diligence => lavamoat-policy}/skill.md (70%) diff --git a/domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py b/domains/security/skills/lavamoat-policy/scripts/policy-audit.py similarity index 100% rename from domains/security/skills/lavamoat-policy-diligence/scripts/policy-audit.py rename to domains/security/skills/lavamoat-policy/scripts/policy-audit.py diff --git a/domains/security/skills/lavamoat-policy-diligence/skill.md b/domains/security/skills/lavamoat-policy/skill.md similarity index 70% rename from domains/security/skills/lavamoat-policy-diligence/skill.md rename to domains/security/skills/lavamoat-policy/skill.md index 4a86838d..9f4634ac 100644 --- a/domains/security/skills/lavamoat-policy-diligence/skill.md +++ b/domains/security/skills/lavamoat-policy/skill.md @@ -1,22 +1,31 @@ --- -name: lavamoat-policy-diligence -description: Triage a LavaMoat policy change for least privilege — which newly granted capabilities can be dropped without breaking anything. Detection is delegated to `@metamaskbot update-policies` plus CI, and because the policy is generated from a real run, every grant has a call site by construction, so "each addition is justified" is a tautology and not the deliverable. Instead read each grant's use at the installed version to find its gate — a config flag nobody sets, an API nobody calls, a branch our payloads never take, an error-only path — and sort into removable / removable-at-a-cost / load-bearing, with the removal test (drop it, rebuild, run e2e) proposed for the policy owners to run. Lead with removal candidates and anything the reading turned up that bears on security; never render an accept/reject verdict, that call is the reviewer's. Hand it over untagged while the workflow is in trial. Triggers on /lavamoat-policy-diligence, or when asked about a LavaMoat policy grant, policy.json diff, capability containment, scuttling, allowScripts, or why a package needs a global/builtin. The specialized engine behind `supply-chain-audit`'s capability-containment lane. +name: lavamoat-policy +description: Triage a LavaMoat policy change for least privilege — which newly granted capabilities can be dropped without breaking anything. Detection is delegated to `@metamaskbot update-policies` plus CI, and because the policy is generated by static analysis, every grant has a call site by construction, so reporting that one exists is a tautology and not the deliverable. `policy.json` records `"crypto": true` and nothing about what the code does with it or how much of the module it touches — recovering that discarded half is the job. Read each grant's use at the installed version to find its gate (a config flag nobody sets, an API nobody calls, a branch our payloads never take, an error-only path) and its breadth (a bare global standing in for one property is a finding even when reached), with the removal or narrowing test proposed for the policy owners to run. Lead with removal candidates and anything the reading turned up that bears on security; never render an accept/reject verdict, that call is the reviewer's. Hand it over untagged while the workflow is in trial. Triggers on /mms-lavamoat-policy, or when asked about a LavaMoat policy grant, policy.json diff, capability containment, scuttling, allowScripts, or why a package needs a global/builtin. The specialized engine behind `supply-chain-audit`'s capability-containment lane. maturity: experimental --- -# /lavamoat-policy-diligence +# /lavamoat-policy Detection is not the job. LavaMoat already tells you which capabilities a dependency change -grants: `@metamaskbot update-policies` regenerates the `policy.json` files from a real run of -the code, and CI's `validate-lavamoat-policies` fails the build if the committed policy drifts -from that regeneration. Re-deriving the diff by hand, or sorting the grants into -network/DOM/red-flag buckets, only re-does a machine that is already trusted. - -**Finding the call site is not the job either — that search cannot fail.** The policy is -*generated from a real run*, so every grant in it corresponds to something the bundled code -did. "Each addition has a call site, therefore each is justified" is a tautology dressed as an -audit; it will report 11 of 11 justified every time, and a check that cannot come back negative -carries no information. +grants: `@metamaskbot update-policies` regenerates the `policy.json` files, and CI's +`validate-lavamoat-policies` fails the build if the committed policy drifts from that +regeneration. Re-deriving the diff by hand, or sorting the grants into network/DOM/red-flag +buckets, only re-does a machine that is already trusted. + +**Reporting that a call site exists is not the job — that search cannot fail.** Policy is +generated by **static analysis**, not by observing a run: `lavamoat-tofu` parses each module with +`@babel/parser` and records global and builtin references (`inspectGlobals` / `inspectImports`, +[generatePolicy.js](https://github.com/LavaMoat/LavaMoat/blob/main/packages/core/src/generatePolicy.js)). +A grant means the identifier is textually present in a parsed module, so searching for it must +succeed. "Each addition has a call site, therefore each is justified" reports 11 of 11 every +time, and a check that cannot come back negative carries no information. + +**Finding the call site is the entire job — its existence is just the part not worth printing.** +`policy.json` records `"crypto": true` and stops. It does not record what the code does with the +capability, or how much of the module it touches, and that discarded half is what a reviewer +needs. Recovering it is what this skill is for. Static detection is also why removal candidates +are common: an identifier surviving into the bundle says nothing about whether our usage reaches +it — which a runtime trace would have. **The job is least privilege: which of these grants can be dropped without breaking anything?** A grant exists because an identifier appears in bundled source. That is *not* the @@ -31,7 +40,7 @@ a branch our usage never takes, is removable. So for each grant, ask what execut | **load-bearing** | our usage genuinely needs it → say so briefly and move on | The lead is the first two rows plus anything the reading turned up that bears on security. -Load-bearing grants still each get a row in the capability → call-site table (step 5) — they just +Load-bearing grants still each get a row in the capability → call-site table (step 6) — they just don't get paragraphs. > **Falsifier.** A grant you called load-bearing that a build with it removed still passes. @@ -87,12 +96,36 @@ droppable. Those observations are worth more than the grant inventory. Lead with the cleanest removal there is — and a capability behind one we *do* use is load-bearing, which is worth one line and no more. -4. **Cite it at a pinned tag, not a branch head.** A permalink to `…/blob//#Ln` is +4. **Record what each grant reaches versus what it uses — breadth is the usual finding.** + Reachability and width are independent. "Nothing calls it" finds removable grants and says + nothing about a grant that *is* called and hands over far more than the call needs. LavaMoat + grants whatever path you name, and + [dotted sub-paths are supported](https://github.com/LavaMoat/LavaMoat/blob/f5e52ab457c16c3aea72cc8a9dd0833547dd7d2c/packages/core/src/endowmentsToolkit.js#L101-L162) + for globals and builtins alike — the extension's own override already uses + `document.visibilityState`. So put each grant's used surface next to its call site, and when a + grant is wider than its use, name the narrowest path that covers every call: `crypto.getRandomValues` + not `crypto`, `node:url.fileURLToPath` not `node:url`, `document.visibilityState` not `document`. + A bare global whose only use is one property is a finding even though it is reached. + + Name the exploit-relevant grants present — code loading (`eval`, `Function`, `importScripts`, + `WebAssembly`, `Worker`, `Blob`+`createObjectURL`), network (`fetch`, `XMLHttpRequest`, + `WebSocket`, `sendBeacon`, `postMessage`), crypto/storage (bare `crypto`, `indexedDB`, + `localStorage`, `chrome.storage`), UI/navigation (`clipboard`, `window.open`, `location`, + `document`, `chrome.tabs`), Node builtins (`child_process`, `fs`, `net`, `http`, `vm`) — and + **hand them over without assessing them**. Do not explain what an attacker could do, do not + rank them. That is threat-model work owned by someone else. + + Do not inflate benign grants to look like findings. `clearTimeout` takes a timer id and cancels + it; it confers no reach. (On extension#45024 the `crypto` grant was caught only because it was + *unreachable* — had `random()` been called it would have been filed as needed and its `subtle` + breadth never mentioned. That is the gap this step closes.) + +5. **Cite it at a pinned tag, not a branch head.** A permalink to `…/blob//#Ln` is immutable; a branch-head link drifts out from under the citation. The permalink *is* the evidence — a reader clicks it and lands on the code, convinced without re-running anything. "It needs X" retyped into a table proves nothing about provenance. -5. **Lead with removal candidates and anything security-relevant — then give the full table.** +6. **Lead with removal candidates and anything security-relevant — then give the full table.** Open on what can be dropped and what the reading turned up, not on an inventory. But every grant still gets its own row in the capability → call-site table, load-bearing ones included: that mapping is what a reviewer came for, and a load-bearing row is one short row, not a @@ -118,7 +151,7 @@ droppable. Those observations are worth more than the grant inventory. Lead with usually means the identifier is present but the generator saw it in a path you haven't found — say what you searched rather than implying nothing uses it. -6. **Put it where the policy is reviewed — but do not tag anyone.** Post the justification as a +7. **Put it where the policy is reviewed — but do not tag anyone.** Post the justification as a comment on the PR carrying the `policy.json` change, so it lands in front of the people who own the policy rather than standing as a unilateral assertion elsewhere. @@ -140,7 +173,7 @@ droppable. Those observations are worth more than the grant inventory. Lead with ends, the tag goes in because the user says so, not because this line stops saying "hold". (Violated on extension#45024, 2026-07-30.) -7. **One reason covers the variants.** Extension builds carry several policy files +8. **One reason covers the variants.** Extension builds carry several policy files (`lavamoat/webpack/{mv2,mv3}/{beta,experimental,flask,main}/policy.json`). When the grant delta is identical across them, a single justification covers all — confirm the identity once. A grant that appears in one variant and not others is itself a question. @@ -148,17 +181,22 @@ droppable. Those observations are worth more than the grant inventory. Lead with ## Output **The capability → call-site table is the deliverable. Never dissolve it into prose.** -One row per grant, every grant, with its permalink and its removability in the row. A reviewer -scans the column, not paragraphs — 11 rows is denser and faster to read than three paragraphs -carrying the same 11 facts, so the table *is* the trimmed form. Prose around it is what gets cut. +One row per grant, every grant, with its permalink, its used surface and its breadth in the row. +A reviewer scans the column, not paragraphs — 11 rows is denser and faster to read than three +paragraphs carrying the same 11 facts, so the table *is* the trimmed form. Prose around it is +what gets cut. + +**Present findings; do not explain the mechanism.** Reviewers here know what a LavaMoat policy is, +what the bot does and what CI enforces. Restating it spends their attention on what they already +know. No paragraph on why call-site search is tautological, no methodology section — open on what +was found. ``` LavaMoat grants — -> - + -| capability | package | call site | can it go? | +| capability | surface used | what it does | breadth | |---|---|---|---| | | | | yes — | | | | · | at a cost — | diff --git a/domains/security/skills/supply-chain-audit/skill.md b/domains/security/skills/supply-chain-audit/skill.md index 8a8cb91c..1401095b 100644 --- a/domains/security/skills/supply-chain-audit/skill.md +++ b/domains/security/skills/supply-chain-audit/skill.md @@ -1,6 +1,6 @@ --- name: supply-chain-audit -description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy-diligence`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by `evidence` as its supply-chain engine. +description: Assess whether a dependency change is safe to take, across every detector that answers a different part of that question — Socket Security (malicious/anomalous package behavior, install scripts, new maintainers), `yarn npm audit` and advisories (known vulnerabilities), lockfile and manifest diffs (what actually changed, including transitive and resolution swaps), and LavaMoat policy grants (new capabilities, delegated to `lavamoat-policy`). Also covers the fronts no upstream scanner sees because they are things your own repo does to dependencies afterwards: yarn patches that modify dependency source at install, `resolutions` that force or stub versions, `npmAuditIgnoreAdvisories` suppression lists, CI actions riding mutable tags instead of pinned SHAs, and yarn plugins that execute at install. The falsifier is a lane whose finding is unaccounted for — a flagged package, an unresolved advisory, or a grant with no call site. Detection belongs to the tools; the job is disposition, and handing it to the humans who own the dependency. Triggers on /mms-supply-chain-audit, or when asked whether a dependency bump is safe, to review a lockfile or package.json change, to triage a Socket or audit finding, or to assess supply-chain risk of a change. Callable by `evidence` as its supply-chain engine. maturity: experimental --- @@ -22,7 +22,7 @@ different question and is blind to the others, so a single green check is never | what actually changed? | lockfile / `package.json` diff | direct vs transitive; resolution swaps; version range widening | | known-vulnerable? | `yarn npm audit`, GitHub advisories, Dependabot | fixed-in version, or an explicit accepted-risk with reachability | | behaving maliciously or anomalously? | **Socket Security** | per-alert disposition — see below | -| new capability reached? | **LavaMoat** policy diff | **delegate to `lavamoat-policy-diligence`** | +| new capability reached? | **LavaMoat** policy diff | **delegate to `lavamoat-policy`** | | install-time code execution? | `allowScripts` in `package.json` (`@lavamoat/allow-scripts`) | a newly-`true` entry is a finding in its own right | | **is dependency source modified in-repo?** | **`.yarn/patches/*.patch`** | read the diff — see below | | **is a version being forced?** | **`resolutions`** in `package.json` | pinned below a fix? stubbed out? | @@ -114,10 +114,10 @@ say which lanes you ran and which you skipped, and why. read it, not as a unilateral assertion. This skill produces a justification for humans to act on; it does not approve anything. -## Capability containment → `lavamoat-policy-diligence` +## Capability containment → `lavamoat-policy` LavaMoat policy grants are a specialized lane with their own method and tooling. **Delegate to -`lavamoat-policy-diligence`** and fold its result in as this audit's capability-containment lane. +`lavamoat-policy`** and fold its result in as this audit's capability-containment lane. Do not restate that lane's question as "does each new capability have a call site" — the policy is generated from a real run, so it always does, and that check cannot fail. The lane's actual @@ -141,7 +141,7 @@ Supply-chain assessment — -> ( | unchanged audit ignores → reason + retire-when | unchanged ci actions → SHA-pinned? | unchanged - capabilities → lavamoat-policy-diligence: | no policy change + capabilities → lavamoat-policy: | no policy change lanes skipped — Unresolved: — | none ``` @@ -154,5 +154,5 @@ unresolved and what would settle it. ## Related -- `lavamoat-policy-diligence` — the capability-containment engine this skill delegates to. +- `lavamoat-policy` — the capability-containment engine this skill delegates to. - `evidence` — packages this skill's output as its [supply-chain evidence category](https://github.com/MetaMask/skills/blob/main/domains/pr-workflow/skills/evidence/references/evidence-catalog.md). From 609dd9c40600475b2aab664dc5af14e92f4fed22 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 05:49:40 -0400 Subject: [PATCH 6/7] End the diligence comment with an applicable override diff, and define its marker pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additions to the output contract. A fenced `diff` against `lavamoat/webpack//policy-override.json` for every narrowing or removal in the table. Prose like "could be narrowed to `node:url.fileURLToPath`" makes the reviewer translate it into JSON; a diff makes it a decision. The override is the file a human edits — the generated `policy.json` is regenerated and would lose the change. Dotted paths already work there: `copy-webpack-plugin>serialize-javascript` is granted `crypto.getRandomValues`, so the precedent is cited rather than the support asserted. A marker pair, `LAVAMOAT_DILIGENCE_START`/`_END`. `evidence/skill.md` already described this contract as having "its own header and marker pair" and no such pair was ever defined, so a re-run appended a second comment. Deliberately not `VALIDATION_RUN_*` — sharing that region would let an evidence re-run silently eat a diligence comment. And a sharper rule on runtime claims. A permalink witnesses a line; it does not witness what the author ran. An `npm pack` result, a grep over a tarball, a byte-comparison across policy files — those read as properties of the package and are properties of an unwitnessed local run. State them as the search or publish the output. Bare integers in prose fall under the same rule. --- .../security/skills/lavamoat-policy/skill.md | 56 +++++++++++++++---- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/domains/security/skills/lavamoat-policy/skill.md b/domains/security/skills/lavamoat-policy/skill.md index 9f4634ac..bc0981a9 100644 --- a/domains/security/skills/lavamoat-policy/skill.md +++ b/domains/security/skills/lavamoat-policy/skill.md @@ -192,30 +192,62 @@ know. No paragraph on why call-site search is tautological, no methodology secti was found. ``` + +> + LavaMoat grants — -> | capability | surface used | what it does | breadth | |---|---|---|---| -| | | | yes — | -| | | · | at a cost — | -| | | | no | +| | | | wider than its use — | +| | | | | +| | not referenced at any spelling | — | 🔍 candidate — | …every grant gets a row… -Test for the "yes" rows: drop the grant, rebuild, run . -Note on : + + +Test: . +Loose ends: Removed: , . ← names only, no table, no justification column -Loose ends: - + +``` + +The marker pair is not decoration: a re-run replaces the region between them instead of appending +a second comment. It is deliberately *not* `VALIDATION_RUN_*` — this is a diligence artifact with +no verdict, and sharing that region would let an evidence re-run silently eat it. + +Order is the point: the lead names the findings, the table carries them, the diff makes them +actionable. Post it on the PR carrying the `policy.json` change, untagged. + +**End with a diff a reviewer can apply, not a description of one.** Every narrowing or removal in +the table gets a fenced `diff` block against the matching +`lavamoat/webpack//policy-override.json`, because that is the file a human edits — the +generated `policy.json` is regenerated and would lose the change. Prose like "could be narrowed to +`node:url.fileURLToPath`" makes the reviewer translate; a diff makes it a decision. + +```diff + "resources": { ++ "sass-loader": { ++ "builtin": { ++ "node:url.fileURLToPath": true, ++ "node:url.pathToFileURL": true, ++ "node:url": false ++ } ++ }, ``` -Order is the point: the lead frames the question, the table answers it, and a reviewer hits the -removable rows immediately. Post it on the PR carrying the `policy.json` change, untagged. +Dotted paths already work in these files — `copy-webpack-plugin>serialize-javascript` is granted +`crypto.getRandomValues`, not bare `crypto`. Cite that precedent rather than asserting support. -**Runtime claims need a runtime artifact.** "Byte-identical across all 8 policy files" is an -observation, not something a `/blob/` link witnesses — publish the check output (JSON) and link -it. The `pr-evidence-gate` hook enforces this and will block the post otherwise; it is right to. +**Runtime claims need a runtime artifact.** A permalink witnesses what a line says; it does not +witness what *you ran*. "The tarball's complete specifier set is X", "byte-identical across all 8 +policy files", any grep or `npm pack` result — those are claims about a local run, and the reader +cannot check them. Publish the output and link it, or state the claim as the search it was +("searched N files, found no match") rather than as a property of the package. Bare integers in +prose need the same treatment: a reader who cannot trace "14 call sites" to something shown is +being asked to take it on trust. ## Worked example — extension#42867 (@sentry/browser 8.33.1 → 10.38.0) From bbf746e3baba2c9cbcda3df3927f2893c3f3b5ca Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 07:59:35 -0400 Subject: [PATCH 7/7] Require the removal test to exercise the capability, not the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A three-arm build on extension#44727 established nothing. Denying sass-loader every URL and path builtin still compiled the extension, exactly as granting all of them did — so "it still builds" was the expected result either way and carried no information. Most grants are not on the startup path; that is usually why they look removable. So a build or a boot with the grant removed shows only that startup did not need it. The test has to name the scenario that actually executes the read — the error path formatting a span URL, the source-map write, the importer resolving a relative `@use`, the flag that turns the feature on — and run that. Two preconditions before either arm is believed, both cheap and both skipped on that run: confirm the effective policy really changed by merging the override into the base and printing the resource, and confirm a fully-denied arm actually fails. If denying everything passes, the grant is not enforced on that path, and "this suite does not arbitrate this grant" is the finding. --- .../security/skills/lavamoat-policy/skill.md | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/domains/security/skills/lavamoat-policy/skill.md b/domains/security/skills/lavamoat-policy/skill.md index bc0981a9..851a1d53 100644 --- a/domains/security/skills/lavamoat-policy/skill.md +++ b/domains/security/skills/lavamoat-policy/skill.md @@ -45,7 +45,32 @@ don't get paragraphs. > **Falsifier.** A grant you called load-bearing that a build with it removed still passes. > The test is cheap and it is the only thing that settles the question: drop the grant from the -> resource, rebuild, run the relevant e2e. Propose it; the policy owners run it. +> resource, rebuild, run the relevant e2e. +> +> **Run it yourself whenever the steps are clear enough to run.** A removal candidate handed +> over untested asks the reviewer to do the work that would settle it, and most of them will not +> — so the finding sits. A build that passes with the capability removed converts `🔍 candidate` +> into a demonstrated non-breaking reduction, and that is a different object: it can be merged +> rather than considered. Publish the run as the evidence, not the conclusion drawn from it. +> +> Hand the test over only when you genuinely cannot run it — a variant needing credentials you +> do not have, an e2e suite the environment cannot host, a policy whose regeneration needs CI's +> toolchain. Say which of those it is; "propose the test" as a default is the failure mode this +> replaces. +> +> **Exercise the capability, not the app.** A build that compiles, or an app that boots, with the +> grant removed shows only that nothing on the startup path needed it. Most grants are not on the +> startup path — that is usually *why* they look removable — so "it still builds" is the null you +> should expect either way, and it is not evidence. Name the scenario that would actually execute +> the read: the error path that formats a `span.url`, the source-map write, the importer resolving +> a relative `@use`, the config flag that turns the feature on. Then run that. +> +> **And prove the arms differ before believing either.** Two things, both cheap. That the +> effective policy really changed — merge the override into the base and print the resource, +> rather than assuming an edit took. And that a fully-denied arm actually *fails*. If denying +> everything still passes, the grant is not enforced on that path, and no arrangement of arms on +> that path can tell you anything. Report that: "this suite does not arbitrate this grant" is a +> real finding, and more useful than a green you cannot cash. **Corollary — the reading is where the real findings come from.** Locating each call site means reading the code that uses the capability, and that is when genuine issues surface: unbounded work