From 3fd9bdac212281cf4412b7ac9f1ecc81154e7329 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 14:52:07 -0400 Subject: [PATCH 1/4] Add `privacy-egress-diligence` skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `app/scripts/constants/sentry-state.ts` decides what user data leaves the machine. It carries 116 fields currently set to `true` — meaning the real value is copied and sent — and it is edited inside ordinary feature PRs (onboarding, swaps, rewards, the analytics controller) with no CODEOWNERS entry, so no privacy reviewer is automatically tagged. Same shape as `lavamoat-policy-diligence`: the diff is mechanical, the judgement is what each grant means. `git diff` finds every newly-`true` field exactly, so the deliverable is not "is it listed" but what the field holds at runtime — a mask path cannot distinguish `selectedTab` from `selectedAddress`. Sorts findings into safe / needs-narrowing / must-not-egress with the evidence and a proposed mask for each, and renders no accept verdict: that call belongs to privacy and legal, and a confident reviewer "this is fine" is precisely what lets an unreviewed field through. Also covers the sibling pipes a PR widens at the same time — new MetaMetrics or Segment properties, and error strings that interpolate runtime values, both of which egress regardless of the mask. --- .../skills/privacy-egress-diligence/skill.md | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 domains/security/skills/privacy-egress-diligence/skill.md diff --git a/domains/security/skills/privacy-egress-diligence/skill.md b/domains/security/skills/privacy-egress-diligence/skill.md new file mode 100644 index 00000000..507e82ad --- /dev/null +++ b/domains/security/skills/privacy-egress-diligence/skill.md @@ -0,0 +1,113 @@ +--- +name: privacy-egress-diligence +maturity: experimental +description: >- + Triage a change to what user data leaves the device — the Sentry state masks in + `app/scripts/constants/sentry-state.ts`, new MetaMetrics/Segment event properties, and + error or breadcrumb strings that interpolate user values. Detection is mechanical (the + mask diff), so the deliverable is not "is each field listed" but what each newly + unmasked field actually holds at runtime: a bounded enum is not an account address, and + the mask cannot tell them apart. Sorts findings into safe / needs-narrowing / + must-not-egress with the evidence for each, and hands the accept decision to the people + who own it. Use when a PR touches sentry-state, adds a tracked event or property, or + widens what an error message includes. +--- + +# Privacy egress diligence + +`app/scripts/constants/sentry-state.ts` decides what leaves the user's machine. It is +~11.7KB of per-controller masks, **116 fields currently set to `true`**, and it is edited +inside ordinary feature PRs — onboarding, swaps, rewards, the analytics controller — with +**no CODEOWNERS entry**, so no privacy reviewer is automatically tagged. + +This skill reviews that egress surface the way `lavamoat-policy-diligence` reviews +capability grants: the diff is mechanical, the judgement is what each grant *means*. + +## When to use + +- A PR touches `app/scripts/constants/sentry-state.ts` (either mask). +- A PR adds or widens a MetaMetrics / Segment event property. +- A PR adds an error message, breadcrumb, or log line that interpolates runtime values. +- A new controller lands and its state gets a mask entry. + +## Do not use when + +- The change only *removes* fields or narrows a mask — that shrinks egress; note it and move on. +- The PR is a pure rename with no change to which values are copied. + +## The mechanic, and the trap + +`maskObject` (`shared/lib/object.utils.ts`) walks the mask: + +| Mask value | Effect on the field | +|---|---| +| `true` | **the real value is copied and sent** | +| `false`, `[]`, absent | leaf replaced with its `typeof` string | +| nested object | recurse | +| `[AllProperties]` | applies to dynamic keys — **the field names are not known at review time** | + +Unlisted is safe by default, so the risk direction is one-way: **a field promoted to +`true`.** That is the whole review surface, and `git diff` finds it exactly. + +**The trap:** "the field is in the mask, so someone decided it was fine." The mask *is* the +decision — appearing in it is not evidence that anyone weighed it. Every `true` was typed by +someone, usually while shipping an unrelated feature. Presence proves authorship, not review. + +## Procedure + +1. **Extract the newly-`true` set.** + + ```bash + git diff origin/main...HEAD -- app/scripts/constants/sentry-state.ts | grep -E '^\+.*:\s*true' + ``` + + Also flag any new `[AllProperties]`, which admits keys nobody has seen. + +2. **Resolve each field to its runtime type.** The mask names a path, not a type. Find the + controller field and read what it actually holds — the declaration, and a real value if + the state fixtures have one: + + ```bash + grep -rn "" app/scripts/controllers/ shared/ --include=*.ts + grep -rn "" test/e2e/default-fixture.js app/scripts/../test/**/mock-state.json 2>/dev/null + ``` + +3. **Sort into three buckets.** This is the deliverable. + + | Bucket | What it looks like | Action | + |---|---|---| + | **Safe** | bounded enum, boolean, count, duration, feature-flag name, error code | note the type that makes it safe | + | **Needs narrowing** | object whose *shape* is useful but whose *leaves* are not — a tx object, a quote, a network config | propose the nested mask that keeps the shape and drops the values | + | **Must not egress** | account address, ENS name, balance, token amount, private RPC URL, free text a user typed, anything keyed by address | propose `false`, or a derived non-identifying substitute (a count, a boolean, a hash) | + +4. **Check the sibling surfaces** the same PR may have widened: + - **Event properties** — a new MetaMetrics/Segment property carrying an address or amount + is the same defect on a different pipe. `analytics-instrumentation` covers whether the + event is *correct*; this covers whether its payload is *sendable*. + - **Error strings** — `throw new Error(\`... ${someValue}\`)` reaches Sentry as the message. + Interpolating an address or a balance leaks it regardless of the mask. + +5. **Report, do not rule.** Give each field its bucket, the evidence (declaration site, an + observed value), and a proposed mask. **Do not render an accept/reject verdict** — whether + a given field is acceptable to collect is a call for the privacy and legal owners, and a + confident-sounding "this is fine" from a reviewer is exactly the artifact that lets an + unreviewed field through. + +## Common pitfalls + +| Mistake | Correct approach | +|---|---| +| Treating mask membership as review | Presence proves someone typed it, not that anyone weighed it | +| Reading the mask path as a type | Resolve the field to its declaration; `selectedAddress` and `selectedTab` look alike in a mask | +| Ignoring `[AllProperties]` | Dynamic keys are unreviewable by construction — say so, and ask what generates them | +| Approving because a value is "usually" small | Report the worst case the type admits, not the common case | +| Rendering a verdict | Sort and evidence; the accept decision belongs to privacy/legal | +| Reviewing only `sentry-state.ts` | Event properties and interpolated error strings egress on other pipes | + +## Related + +- `lavamoat-policy-diligence` — same shape for capability grants; read it for the + diff-is-mechanical-judgement-is-not pattern. +- `analytics-instrumentation` — whether an event is correctly *identified* and *gated* + (`isOptIn`, `metaMetricsId`). This skill is about whether its payload is *sendable*. +- `supply-chain-audit` — the third diligence lane, for dependency capability. From 477009f50f7fc6a4cb92449ab0b41516de5dc6fd Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Sat, 1 Aug 2026 11:12:31 -0400 Subject: [PATCH 2/4] =?UTF-8?q?Add=20`egress-delta.py`=20=E2=80=94=20what?= =?UTF-8?q?=20a=20diff=20newly=20exposes,=20and=20what=20it=20stopped=20pr?= =?UTF-8?q?otecting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves. INTRODUCED lists added lines that send, store, or log off-device, tagged by the sensitivity of the payload near the call. WORSENED lists removed consent gates, sanitisers, sampling gates, and validations — and that half is the reason the script exists, because a deleted protection adds no code and so is invisible to anything that scans what a diff introduces. Verified against two real PRs. On extension#42519 it ranks the compliance call first of sixteen egress sites, tagged `identifier` — the same site found by hand, found here without being told where to look. On extension#43869 it reports five removed consent gates including `if (!canSubmitAnalytics(...))`. Two defects the runs exposed, both of which had it reporting nothing: - Payload sensitivity was read from the egress line alone, but a call and its argument sit on different lines — `submitRequestToBackground(` then `[addresses]`. Now read from a window around the call site. - `\baddress\b` does not match `addresses`, nor `\btoken\b` match `tokenList`. Identifiers appear pluralised and camel-cased far more often than bare, so a closing word boundary missed the entire payload on real call sites. Guards removed and re-added in the same diff are excluded as refactors. No verdict is offered: whether a flow is acceptable depends on disclosure, jurisdiction, and intent, and a screening check a user can decline screens nobody — so an absent consent gate is not automatically a defect. Anything sensitive belongs in a private tracker rather than a public comment. --- .../scripts/egress-delta.py | 183 ++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 domains/security/skills/privacy-egress-diligence/scripts/egress-delta.py diff --git a/domains/security/skills/privacy-egress-diligence/scripts/egress-delta.py b/domains/security/skills/privacy-egress-diligence/scripts/egress-delta.py new file mode 100644 index 00000000..3b9df8e6 --- /dev/null +++ b/domains/security/skills/privacy-egress-diligence/scripts/egress-delta.py @@ -0,0 +1,183 @@ +#!/usr/bin/env python3 +"""Security and privacy surface delta for a diff — what this change newly exposes, +and what protection it removed. + +Two halves, and the second is the one nothing else catches: + + INTRODUCED added lines that send, store, or log something that leaves the + device or outlives the session + WORSENED removed lines that were a consent gate, a sanitiser, a redaction, + or a validation — a protection deleted is a regression that no + "scan the new code" pass can see, because there is no new code + +Reports and escalates. It does not rule on whether a flow is acceptable: that +depends on disclosure, jurisdiction, and intent, none of which are in a diff. +A finding here is a question for a human, and for anything sensitive it belongs +in a private tracker rather than a public comment. + +Usage: egress-delta.py [--context ] +Falsifier: an added egress site with no corresponding gate anywhere in its call path. +""" +import re +import sys +from collections import defaultdict + +# Things that move data off-device or persist it beyond the session. +EGRESS = [ + (r"\bfetch\s*\(", "network: fetch"), + (r"\bXMLHttpRequest\b", "network: XHR"), + (r"\bnew WebSocket\s*\(", "network: websocket"), + (r"\bsendBeacon\s*\(", "network: beacon"), + (r"\baxios\.\w+\s*\(", "network: axios"), + (r"\bsubmitRequestToBackground\s*(<[^>]*>)?\s*\(", "background RPC"), + (r"\btrackEvent\s*\(", "telemetry: event"), + (r"\bcaptureException\s*\(|\bcaptureMessage\s*\(", "telemetry: sentry"), + (r"\baddBreadcrumb\s*\(", "telemetry: breadcrumb"), + (r"\bstartSpan\w*\s*\(|\btrace\s*\(\s*\{", "telemetry: span"), + (r"\blocalStorage\.setItem\s*\(|\bsessionStorage\.setItem\s*\(", "storage: web"), + (r"\bchrome\.storage\.\w+\.set\s*\(", "storage: extension"), + (r"\bindexedDB\.open\s*\(", "storage: indexeddb"), + (r"\bconsole\.(log|info|warn|error)\s*\(", "log: console"), +] + +# Identifier-shaped payloads. Presence on an egress line is what makes it interesting. +# Trailing \w* rather than \b: identifiers appear pluralised and camel-cased far more +# often than bare — `addresses`, `accountIds`, `tokenList`. A closing \b silently misses +# every one of those, which is the whole payload on a real call site. +SENSITIVE = [ + (r"\bprivate\w*[Kk]ey|\bmnemonic\w*|\bseed\w*[Pp]hrase", "SECRET"), + (r"\bvault\w*|\bkeyring\w*|\bencryptionKey\w*", "SECRET"), + (r"\baddress\w*|\baccount\w*|\bpublicKey\w*", "identifier"), + (r"\bemail\w*|\bipAddress\w*|\buserId\w*|\bdeviceId\w*", "identifier"), + (r"\bjwt\w*|\btoken\w*|\bbearer\w*|\bapiKey\w*|\bsecret\w*", "credential"), + (r"\bbalance\w*|\btxHash\w*|\btransaction\w*", "activity"), +] + +# Protections whose REMOVAL is the finding. +GUARDS = [ + (r"\buseExternalServices\b|\bbasicFunctionality\b", "basic-functionality gate"), + (r"\bparticipateInMetaMetrics\b|\bcanSubmitAnalytics\b|\boptedIn\b|\boptIn\b", "consent gate"), + (r"\bisEnabled\b|\bfeatureFlag\w*\b|\bremoteFeatureFlags\b", "feature gate"), + (r"\bsanitiz|\bredact|\bmask\b|\bscrub\b|\banonymi", "sanitiser"), + (r"\bvalidate\w*\s*\(|\bassert\w*\s*\(|\bisValid\w*\s*\(", "validation"), + (r"\bbeforeSend\b|\btracesSampleRate\b|\bsampleRate\b", "sampling gate"), + (r"\bencrypt\w*\s*\(|\bhash\w*\s*\(", "encryption/hashing"), +] + +HOST = re.compile(r"https?://([A-Za-z0-9.\-]+)") + + +def parse(patch_path): + """Yield (file, sign, text) for +/- lines, tracking the current file.""" + cur = None + with open(patch_path, errors="replace") as f: + for line in f: + if line.startswith("+++ b/"): + cur = line[6:].strip() + continue + if line.startswith("--- ") or line.startswith("+++ "): + continue + if line.startswith("+") or line.startswith("-"): + yield cur, line[0], line[1:].rstrip("\n") + + +def classify(text, table): + return [label for pat, label in table if re.search(pat, text)] + + +def main(): + if len(sys.argv) < 2: + sys.exit("usage: egress-delta.py ") + + introduced = [] # (file, kind, sensitivity, host, text) + worsened = defaultdict(list) # guard -> [(file, text)] + removed_egress = 0 + + # An egress call and its payload are usually on different lines — + # `submitRequestToBackground(` on one, `[addresses]` on the next. Matching a + # single line therefore misses precisely the argument that makes the call + # interesting, so sensitivity is read from a window around the call site. + WINDOW = 3 + rows = [(p, sg, t) for p, sg, t in parse(sys.argv[1]) + if p and not p.endswith((".md", ".json", ".lock", ".snap"))] + + for i, (path, sign, text) in enumerate(rows): + stripped = text.strip() + if not stripped or stripped.startswith(("//", "*", "/*")): + continue + + kinds = classify(text, EGRESS) + if sign == "+" and kinds: + lo, hi = max(0, i - WINDOW), min(len(rows), i + WINDOW + 1) + near = " ".join(t for p2, sg2, t in rows[lo:hi] if p2 == path and sg2 == "+") + sens = classify(near, SENSITIVE) + host = HOST.search(near) + introduced.append((path, kinds[0], sens, host.group(1) if host else None, stripped)) + elif sign == "-" and kinds: + removed_egress += 1 + + if sign == "-": + for g in classify(text, GUARDS): + worsened[g].append((path, stripped)) + + # A guard removed in the same hunk that re-adds it is a refactor, not a regression. + readded = set() + for path, sign, text in parse(sys.argv[1]): + if sign == "+": + for g in classify(text, GUARDS): + readded.add((path, g)) + worsened = {g: [(p, t) for p, t in v if (p, g) not in readded] + for g, v in worsened.items()} + worsened = {g: v for g, v in worsened.items() if v} + + print("SECURITY / PRIVACY SURFACE DELTA") + print("=" * 74) + print("Introduced = added lines that send, store, or log off-device.") + print("Worsened = removed protections. Nothing that scans new code can see these,") + print(" because a deleted guard adds no code.\n") + + if not introduced and not worsened: + print(" (no egress added and no protection removed in this diff)") + return + + if introduced: + print(f"INTRODUCED — {len(introduced)} site(s)") + print("-" * 74) + sens_first = sorted(introduced, key=lambda r: (not r[2], r[0])) + for path, kind, sens, host, text in sens_first[:25]: + tag = ("/".join(sens) if sens else "—") + print(f" [{tag:>12}] {kind:<22} {path}") + print(f" {text[:96]}") + if host: + print(f" → host: {host}") + if len(introduced) > 25: + print(f"\n … {len(introduced) - 25} further site(s).") + print() + + if worsened: + n = sum(len(v) for v in worsened.values()) + print(f"WORSENED — {n} protection(s) removed and not re-added in this diff") + print("-" * 74) + for guard, rows in sorted(worsened.items()): + print(f" {guard} ({len(rows)})") + for path, text in rows[:4]: + print(f" {path}") + print(f" - {text[:92]}") + print() + + if removed_egress: + print(f" ({removed_egress} egress line(s) also removed — a move or a deletion, " + "check before reading the counts above as net-new.)\n") + + print("RAISE WITH A HUMAN — no verdict offered") + print("-" * 74) + print("Whether a flow is acceptable depends on disclosure, jurisdiction, and intent,") + print("none of which are in a diff. A screening check that a user can decline screens") + print("nobody, so an absent consent gate is not automatically a defect — and that is") + print("exactly the judgement this script must not make.") + print() + print("Anything sensitive here belongs in a private tracker, not a public comment.") + + +if __name__ == "__main__": + main() From c30e57582c37e1c7923c725b0b23ea00944b0bb0 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 3 Aug 2026 07:57:53 -0400 Subject: [PATCH 3/4] Say what this skill does not cover It reviews what a field carries and not whether the send is permitted, and a reader who runs it has covered half the question while believing otherwise. An unstated scope on a review is the same defect as an unstated scope on a search: the negative reads wider than the thing that produced it. The other axis lives in the private repo, because naming the conditions under which a control does not run is a different kind of document from describing the control. --- .../security/skills/privacy-egress-diligence/skill.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/domains/security/skills/privacy-egress-diligence/skill.md b/domains/security/skills/privacy-egress-diligence/skill.md index 507e82ad..7dbb3b89 100644 --- a/domains/security/skills/privacy-egress-diligence/skill.md +++ b/domains/security/skills/privacy-egress-diligence/skill.md @@ -93,6 +93,17 @@ someone, usually while shipping an unrelated feature. Presence proves authorship confident-sounding "this is fine" from a reviewer is exactly the artifact that lets an unreviewed field through. +## What this does not cover + +This reviews **what** a field carries. It does not review whether the send is permitted at all — +consent state, basic functionality, compliance and region gates, and what happens to data +buffered before a user decided. A correctly masked field sent without consent is the worse +failure of the two, and nothing here can see it. + +That axis is reviewed separately, in the private skills repo, because naming the conditions +under which a control does not run is a different kind of document from describing the control. +Run both on a change that adds collection; either alone leaves half the question open. + ## Common pitfalls | Mistake | Correct approach | From 1706f0f90d2123ccec96245df8ae7eb691787aa6 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 05:40:15 -0400 Subject: [PATCH 4/4] Point references at the renamed `lavamoat-policy` skill Renamed on the security-domain branch; installs as `mms-lavamoat-policy`. --- domains/security/skills/privacy-egress-diligence/skill.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/domains/security/skills/privacy-egress-diligence/skill.md b/domains/security/skills/privacy-egress-diligence/skill.md index 7dbb3b89..023651ef 100644 --- a/domains/security/skills/privacy-egress-diligence/skill.md +++ b/domains/security/skills/privacy-egress-diligence/skill.md @@ -20,7 +20,7 @@ description: >- inside ordinary feature PRs — onboarding, swaps, rewards, the analytics controller — with **no CODEOWNERS entry**, so no privacy reviewer is automatically tagged. -This skill reviews that egress surface the way `lavamoat-policy-diligence` reviews +This skill reviews that egress surface the way `lavamoat-policy` reviews capability grants: the diff is mechanical, the judgement is what each grant *means*. ## When to use @@ -117,7 +117,7 @@ Run both on a change that adds collection; either alone leaves half the question ## Related -- `lavamoat-policy-diligence` — same shape for capability grants; read it for the +- `lavamoat-policy` — same shape for capability grants; read it for the diff-is-mechanical-judgement-is-not pattern. - `analytics-instrumentation` — whether an event is correctly *identified* and *gated* (`isOptIn`, `metaMetricsId`). This skill is about whether its payload is *sendable*.