feat: Adds security domain - /supply-chain-audit, /lavamoat-policy, /privacy-egress-diligence skills - #83
Open
MajorLift wants to merge 10 commits into
Open
feat: Adds security domain - /supply-chain-audit, /lavamoat-policy, /privacy-egress-diligence skills#83MajorLift wants to merge 10 commits into
security domain - /supply-chain-audit, /lavamoat-policy, /privacy-egress-diligence skills#83MajorLift wants to merge 10 commits into
Conversation
…oat-policy-diligence 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.
MajorLift
changed the title
MajorLift
changed the title
supply-chain-audit and lavamoat-policy-diligence skills
MajorLift
marked this pull request as draft
July 30, 2026 14:03
MajorLift
marked this pull request as ready for review
July 30, 2026 18:15
Contributor
Author
Context budgetWhat this PR costs an agent, measured from an install rather than read from the diff. Three tiers, and only the first is unavoidable.
Frontmatter is the only tier paid unconditionally — every agent loads it on every run once the skill is installed, used or not, because it is what the agent reads to decide relevance. The 28 skills across the eleven open skill PRs sit at a median of ~1,716 tokens selected and ~1,860 with references followed. All are within the 1,536-character description budget. Selected is paid only when the agent picks the skill. + refs & knowledge is the ceiling if every bundled reference is then read; it is a worst case, not an expectation. Method
These figures are pinned to the commit above and drift on every push; #96 tracks automating them. |
`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.
This was referenced Jul 31, 2026
Merged
…rants 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.
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.
This was referenced Aug 3, 2026
…he breadth step 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.
7 tasks
…e its marker pair Three additions to the output contract. A fenced `diff` against `lavamoat/webpack/<variant>/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.
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.
This was referenced Aug 6, 2026
MajorLift
changed the title
supply-chain-audit and lavamoat-policy-diligence skillssupply-chain-audit and lavamoat-policy skills
This was referenced Aug 13, 2026
…main Containment reviews keep hitting the same architectural facts, and the intuitive answer is reliably wrong: whether a page can reach a background listener without `externally_connectable`, whether a port-based content script is in a `runtime.onMessage` sender set, whether an ISOLATED content script carries the LavaMoat runtime, and whether scuttling can be disabled per chunk. Also records the failure a sender check cannot catch — an open shadow root lets the page drive the content script's own handler with an untrusted event, forging nothing. Cross-linked from `lavamoat-policy` and `supply-chain-audit`.
This was referenced Aug 14, 2026
Merged
7 tasks
7 tasks
MajorLift
changed the title
supply-chain-audit and lavamoat-policy skillssecurity domain - supply chain audit, LavaMoat policy skills
MajorLift
changed the title
security domain - supply chain audit, LavaMoat policy skillssecurity domain - /supply-chain-audit, /lavamoat-policy skills
## Summary - Adds `privacy-egress-diligence` to `security` — review for changes to what user data leaves the device. - Fills the one proof-standard domain with no lane today. Raised during [ADR-0058 review](MetaMask/decisions#173): security, observability/data, and ci/devex/build all have coverage; compliance/privacy had none. ## Why this surface `app/scripts/constants/sentry-state.ts` decides what egresses. Measured on `metamask-extension` at the current head: - **116 fields set to `true`** — the real value is copied and sent to Sentry. - Edited inside **ordinary feature PRs** — recent changes ride onboarding, swaps, rewards, and analytics-controller work. - **No CODEOWNERS entry**, so no privacy reviewer is automatically tagged. A file governing data egress, changed casually, reviewed by whoever happened to review the feature. ## Shape Deliberately the same as `lavamoat-policy`: detection is mechanical, judgement is not. `maskObject` (`shared/lib/object.utils.ts`) makes the risk one-way — unlisted fields degrade to a `typeof` string, so only a field promoted to `true` widens egress, and `git diff` finds those exactly. **The trap it names:** "the field is in the mask, so someone decided it was fine." The mask *is* the decision. Presence proves authorship, not review. So the deliverable is what each field holds at runtime — a mask path cannot distinguish `selectedTab` from `selectedAddress` — sorted into: | Bucket | Action | |---|---| | Safe | note the type that makes it safe (bounded enum, boolean, count) | | Needs narrowing | propose a nested mask keeping the shape, dropping the leaves | | Must not egress | propose `false` or a non-identifying substitute | It also covers the sibling pipes a PR widens at the same time: new MetaMetrics/Segment properties, and error strings interpolating runtime values, both of which leak regardless of the mask. **No accept/reject verdict.** Whether a field is acceptable to collect belongs to privacy and legal; a confident reviewer "this is fine" is exactly the artifact that lets an unreviewed field through. ## Notes - Experimental. One skill, no knowledge files. - Lands in `security` rather than a new `privacy` domain — a one-skill domain is premature, and the diligence pattern already lives there. Say the word if you'd rather it were separate. - Complements `instrumentation` (#76), which covers whether an event is correctly *identified* and consent-*gated*. This covers whether its payload is *sendable*. - No `CHANGELOG.md` entry: skill-only, no CLI change. --------- Co-authored-by: abretonc7s <107169956+abretonc7s@users.noreply.github.com> Co-authored-by: behroozreview <behrooz.aghakhanian@gmail.com>
MajorLift
changed the title
security domain - /supply-chain-audit, /lavamoat-policy skillssecurity domain - /supply-chain-audit, /lavamoat-policy, /privacy-egress-diligence skills
A deep link is an external entry point into the wallet, and the parts that decide whether a link is trusted — the interstitial, signature verification, param canonicalization — are owned by the extension security team rather than by the feature team adding a route. That is a security boundary, so the skill belongs beside the other two that guard one. Files are byte-identical to #139, relocated from `domains/coding/`. Both repo overlays come with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds a
securitydomain, four experimental skills: what enters the codebase, what leaves the device, what reaches in.supply-chain-audit— is this dependency change safe to take?: Socket,yarn npm audit, lockfile and manifest diffs, and LavaMoat grants, whichlavamoat-policytakes in depth. Neither renders an accept/reject verdict: disposition, not detection.privacy-egress-diligence— what is leaving that was not before: Sentry masks, new MetaMetrics/Segment properties, any outbound field.deeplink-handler— the inbound edge, moved fromcoding: the parts deciding whether a link is trusted — interstitial, signature verification, param canonicalization — belong to the extension security team, not the feature team, so the skill says which half is the author's. Three defects, each seen repeatedly in review: allowlists as plain object literals, admitting every prototype key; validated params that never reach the destination; silently dropped query strings.Needs a decision:
/domains/security/CODEOWNERS defaults to the*fallback's platform teams; reassign if security should own it.Motivation
Socket and
yarn npm auditsee a dependency as published, never what this repo does to it afterwards:patches/,resolutions,npmAuditIgnoreAdvisories, mutable CI action tags, yarn plugins.A LavaMoat policy is generated from a real run, so every grant has a call site by construction — "each addition is justified" is a tautology, not a deliverable. The deliverable is each grant's gate: a config flag nobody sets, an API nobody calls, a branch our payloads never take, sorted at the installed version into removable / removable-at-a-cost / load-bearing.
Showcase
lavamoat-policyon extension#42867: both queried@sentry/browsergrants are load-bearing on unconditional paths —WebAssembly,importScripts, so neither has a gate to close. #45024's first pass is the counter-example: 11 additions, 11 reasons was guaranteed.supply-chain-auditon extension#44865, from amainaudit: four advisories, two againstpostcss8 (GHSA-6g55-p6wh-862q, GHSA-r28c-9q8g-f849); upgradestylelint, owning 8 of 11 postcss-7 paths; patch the two that cannot move; reject the blanketresolutions.postcssbreaking five^7consumers (#44860, closed); track the rest.Trial runs, merged PRs nobody flagged:
supply-chain-audit@sentry/browsergainsfetch,importScripts,WebAssemblylavamoat-policywritesupply-chain-auditsass-embeddedbump grantsworker_threads+fssupply-chain-audittar; delta comes fromstreamx