From 1b3c0dfb293252a533b0f03c1e926a090602226d Mon Sep 17 00:00:00 2001 From: Gregor MacLennan Date: Tue, 7 Jul 2026 21:42:55 +0100 Subject: [PATCH] feat(sentry): redact bare rootkey-shaped tokens with an exact-shape base64-22 rule Enables the detection deliberately disabled in #111. The rootkey crosses the wire as strict base64 of 16 bytes (backend/index.js enforces /^[A-Za-z0-9+/]{22}==$/), and any valid 16-byte base64 ends its 22nd char in [AQgw], so the rule matches exactly 22 base64/base64url chars ending there, optionally ==-padded, bounded by non-base64 chars. Padded matches always redact; unpadded ones also need mixed case plus a digit/symbol and must not be pure hex, which spares 32-hex trace ids, 16-hex span ids, PascalCase exception type names, ERR_* codes, error_class tag values, and base64-ish ids of other lengths. Applied identically to both hand-mirrored scrubber copies and to the forbidden-metric tag-value gate; shared cases in test-support/scrubber-cases.js cover both suites. Closes #77 --- backend/before-send.js | 61 +++++++++++++++++++++++----- backend/lib/metrics.test.mjs | 6 ++- docs/sentry-integration.md | 29 ++++++++++--- src/__tests__/sentry-metrics.test.js | 6 +-- src/__tests__/sentry.test.js | 3 +- src/sentry-scrub.ts | 61 ++++++++++++++++++++++------ test-support/scrubber-cases.js | 29 ++++++++++--- 7 files changed, 154 insertions(+), 41 deletions(-) diff --git a/backend/before-send.js b/backend/before-send.js index e04ddb2..69fe2a4 100644 --- a/backend/before-send.js +++ b/backend/before-send.js @@ -8,14 +8,12 @@ // the same scrubbing + drop behaviour runs on Node-side events before // they leave the FGS. // -// Mirrored from `src/sentry-scrub.ts`. The broad base64-22 token rule (to -// catch bare rootKeys / public keys / project ids) is intentionally NOT -// enabled here either — it over-matched Sentry's own 32-hex trace_ids, -// PascalCase exception type names, and error_class metric tags, redacting -// data we need. Pending a narrower design agreed with the team; bare tokens -// are unscrubbed until then. Object fields keyed -// lat/lng/latitude/longitude are redacted regardless of value type; lat/lng -// markers redact the trailing number; HTTP breadcrumb URLs reduce to host-only. +// Mirrored from `src/sentry-scrub.ts`. Bare (unmarked) rootkey-shaped +// tokens are redacted by the exact-length base64-22 rule — see +// BARE_KEY_TOKEN_PATTERN below for the design and its known limits. +// Object fields keyed lat/lng/latitude/longitude are redacted regardless +// of value type; lat/lng markers redact the trailing number; HTTP +// breadcrumb URLs reduce to host-only. const REDACTED = "[redacted]"; @@ -29,6 +27,36 @@ const SCRUB_PATTERNS = [ /\b(?:latitude|longitude|lat|lng|lon)\b\s*["']?\s*[:=]\s*-?\d+(?:\.\d+)?/gi, ]; +/** + * Bare rootkey-shaped token: exactly 22 base64/base64url chars whose last + * data char is [AQgw] (any valid 16-byte base64 ends there — its final 4 + * bits are padding zeros), optional `==`, bounded by non-base64 chars. + * This exact-length + final-char anchor replaces the disabled "any + * 22+-char base64 run" rule that over-matched 32-hex trace_ids, PascalCase + * exception type names, and error_class metric tags. Matches still pass + * through isBareKeyShaped before redaction. + */ +const BARE_KEY_TOKEN_PATTERN = + /(? + isBareKeyShaped(match) ? REDACTED : match, + ); } /** Reduce an HTTP(S) URL to scheme + host. @param {string} url */ @@ -241,6 +281,7 @@ export function isForbiddenMetric(name, attributes) { for (const pattern of FORBIDDEN_METRIC_VALUE_PATTERNS) { if (pattern.test(tagValue)) return true; } + if (containsBareKeyToken(tagValue)) return true; } } return false; diff --git a/backend/lib/metrics.test.mjs b/backend/lib/metrics.test.mjs index 1798161..0deb457 100644 --- a/backend/lib/metrics.test.mjs +++ b/backend/lib/metrics.test.mjs @@ -145,8 +145,10 @@ test("before_metric_send drops forbidden tag VALUES (lat/lng shape)", () => { 0, "metric carrying a lat/lng tag value must be dropped", ); - // A normal bucket value passes. (The broad base64-22 value rule is disabled - // pending a narrower design — a bare token would also pass now.) + // A bare rootkey-shaped value is also dropped. + metrics.storageSizeBucket("bm90LWEtcmVhbC1rZXktMQ"); + assert.equal(calls.count.length, 0); + // A normal bucket value passes. metrics.storageSizeBucket("<10MB"); assert.equal(calls.count.length, 1); }); diff --git a/docs/sentry-integration.md b/docs/sentry-integration.md index 10fb7fe..46735c2 100644 --- a/docs/sentry-integration.md +++ b/docs/sentry-integration.md @@ -1287,12 +1287,29 @@ Node; shared test cases in `test-support/scrubber-cases.js` keep the two copies from drifting): it walks the event tree — message, exception values, extra, contexts, breadcrumbs, structured logs — redacting `rootKey`-marked values and coordinate markers (`lat`, `lng`, `lon`, -`latitude`, `longitude`, in key/value and JSON-serialized forms). A -broad base64-22-char rule (to catch *bare* unmarked rootkeys) is -deliberately **not** enabled: it also matched trace IDs and exception -type names; a narrower design is pending, and until then bare unmarked -tokens pass through. This is belt-and-suspenders — the fix is always at -the capture site, but the scrubber catches mistakes before they ship. +`latitude`, `longitude`, in key/value and JSON-serialized forms). + +*Bare* (unmarked) rootkey-shaped tokens are caught by an exact-shape +rule rather than the originally proposed "any 22+-char base64 run" +(which over-matched 32-hex trace IDs, PascalCase exception type names, +and `error_class` metric tags). The rootkey crosses the wire as strict +base64 of 16 bytes — `backend/index.js` enforces +`/^[A-Za-z0-9+/]{22}==$/` — and any valid 16-byte base64 ends its 22nd +char in `A`/`Q`/`g`/`w` (the final 4 bits are padding zeros). So the +rule matches exactly 22 base64/base64url chars ending in `[AQgw]`, +optionally `==`-padded, bounded by non-base64 chars; a padded match is +the exact wire shape and always redacts, while an unpadded match must +also show a random-key character mix (both cases plus a digit or +symbol, and not pure hex). That structurally spares trace/span IDs +(wrong length, hex), type names and `ERR_*` codes (letters-only or +single-case), and base64-ish IDs of other lengths (43-char public keys, +52-char project IDs). Known limits: roughly 1% of random rootkeys are +all-letters and would slip through in *unpadded* form (the padded wire +form still redacts), and a 22-char mixed-case identifier containing a +digit that happens to end in `[AQgw]` is falsely redacted — accepted, +since false negatives are worse here. This is belt-and-suspenders — the +fix is always at the capture site, but the scrubber catches mistakes +before they ship. --- diff --git a/src/__tests__/sentry-metrics.test.js b/src/__tests__/sentry-metrics.test.js index 97321fd..021b374 100644 --- a/src/__tests__/sentry-metrics.test.js +++ b/src/__tests__/sentry-metrics.test.js @@ -93,11 +93,11 @@ describe("sentry-metrics", () => { expect(calls).toHaveLength(1); }); - // The broad base64-22 value rule is disabled pending a narrower design - // (see sentry-scrub.ts), so a bare token tag no longer drops the metric. - test("bare base64 tag values pass through while the broad rule is disabled", () => { + test("bare rootkey-shaped tag values drop the metric; error_class survives", () => { const { __metricsInternals } = require("../sentry-metrics"); __metricsInternals.count("comapeo.x", { bucket: "bm90LWEtcmVhbC1rZXktMQ" }); + expect(calls).toHaveLength(0); + __metricsInternals.count("comapeo.rpc.errors", { error_class: "NotFoundError" }); expect(calls).toHaveLength(1); }); diff --git a/src/__tests__/sentry.test.js b/src/__tests__/sentry.test.js index be0c269..14129bb 100644 --- a/src/__tests__/sentry.test.js +++ b/src/__tests__/sentry.test.js @@ -310,8 +310,7 @@ describe("initSentry", () => { expect(payload).toContain("[redacted]"); expect(payload).not.toContain("aGVsbG8td29ybGQtMTIzNA"); // rootKey value gone expect(payload).not.toContain("-12.34"); // lat/lng gone - // Broad base64-22 rule disabled: a bare token in `extra` currently survives. - expect(payload).toContain("bm90LWEtcmVhbC1rZXktMQ"); + expect(payload).not.toContain("bm90LWEtcmVhbC1rZXktMQ"); // bare rootkey-shaped token gone }); test("beforeBreadcrumb reduces HTTP URLs to host-only", () => { diff --git a/src/sentry-scrub.ts b/src/sentry-scrub.ts index 86f6a4f..605e30c 100644 --- a/src/sentry-scrub.ts +++ b/src/sentry-scrub.ts @@ -11,10 +11,13 @@ * buggy host (and our own mistakes) before a payload leaves the device. * * What it redacts (and the false-positive trade-off): - * - Explicit `rootKey=…` markers (key=value / json / prose). A broad - * "any 22+-char base64url run" rule is deliberately NOT enabled — see - * the SCRUB_PATTERNS note below — so bare rootKeys/keys/project-ids - * with no marker are currently unscrubbed. + * - Explicit `rootKey=…` markers (key=value / json / prose). + * - Bare (unmarked) rootkey-shaped tokens: exactly 22 base64 chars + * ending in [AQgw] — the only shape a 16-byte base64 rootkey can take + * (`backend/index.js` enforces /^[A-Za-z0-9+/]{22}==$/ on the wire). + * Padded (`==`) matches always redact; unpadded ones also need a + * character-mix check — see BARE_KEY_TOKEN_PATTERN below for the rule + * and its known limits. * - Object fields whose KEY is lat/lng/latitude/longitude are redacted * regardless of value type — a numeric `{latitude: 12.3}` is the most * likely capture shape and value-only scrubbing would miss it. @@ -39,18 +42,40 @@ const SCRUB_PATTERNS: RegExp[] = [ // field delimiter (whitespace, `,;&`, quote) so co-located fields in a // compact string like `rootKey=abc,method=x` survive. /\broot[_-]?key\b\s*["']?\s*[:=]\s*[^\s,;&"']+/gi, - // NOTE: a broad 22+-char URL-safe base64 rule (to catch bare rootKeys at - // 22, public keys at 43, project ids at ~52) is intentionally NOT enabled - // — it also matched Sentry's own 32-hex trace_ids, PascalCase exception - // type names, and error_class metric tags, redacting data we need. Pending - // a narrower design agreed with the team; bare tokens are unscrubbed until - // then. // Latitude / longitude markers followed by a number. `lon` is the field // name @comapeo/schema observations actually use. Optional quote between // key and separator so JSON-serialized coordinates (`"lat":-12.3`) match. /\b(?:latitude|longitude|lat|lng|lon)\b\s*["']?\s*[:=]\s*-?\d+(?:\.\d+)?/gi, ]; +/** + * Bare rootkey-shaped token: exactly 22 base64/base64url chars whose last + * data char is [AQgw] (any valid 16-byte base64 ends there — its final 4 + * bits are padding zeros), optional `==`, bounded by non-base64 chars. + * This exact-length + final-char anchor replaces the disabled "any + * 22+-char base64 run" rule that over-matched 32-hex trace_ids, PascalCase + * exception type names, and error_class metric tags. Matches still pass + * through isBareKeyShaped before redaction. + */ +const BARE_KEY_TOKEN_PATTERN = + /(? + isBareKeyShaped(match) ? REDACTED : match, + ); } /** Reduce an HTTP(S) URL to scheme + host, dropping path/query/fragment. */ @@ -261,6 +295,7 @@ export function isForbiddenMetric( for (const pattern of FORBIDDEN_METRIC_VALUE_PATTERNS) { if (pattern.test(tagValue)) return true; } + if (containsBareKeyToken(tagValue)) return true; } } return false; diff --git a/test-support/scrubber-cases.js b/test-support/scrubber-cases.js index 14031e6..5fcee52 100644 --- a/test-support/scrubber-cases.js +++ b/test-support/scrubber-cases.js @@ -11,6 +11,9 @@ export const BASE64_43 = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8"; // ~52-char z-base-32 project id. export const ZBASE32_52 = "ybybybybybybybybybybybybybybybybybybybybybybybybybyb"; +// 16-byte rootkey in the wire format `backend/index.js` enforces: +// 22 base64 chars + `==` (base64 of the ASCII bytes "0123456789abcdef"). +export const ROOTKEY_PADDED = "MDEyMzQ1Njc4OWFiY2RlZg=="; /** `scrubString(input)` must equal `expect`. */ export const scrubStringCases = [ @@ -26,8 +29,21 @@ export const scrubStringCases = [ // co-located fields in a compact string survive. { name: "rootKey value stops at comma delimiter", input: "rootKey=abc,method=obs.create,code=500", expect: "[redacted],method=obs.create,code=500" }, { name: "plain sentence untouched", input: "hello world", expect: "hello world" }, - // Broad base64-22 rule is intentionally disabled — bare tokens pass through. - { name: "bare base64 token passes through", input: "token bm90LWEtcmVhbC1rZXktMQ done", expect: "token bm90LWEtcmVhbC1rZXktMQ done" }, + // Bare (unmarked) rootkey-shaped tokens: exact-length base64-22 rule. + { name: "bare padded rootkey (wire format) redacted", input: `boot got key ${ROOTKEY_PADDED} from native`, expect: "boot got key [redacted] from native" }, + { name: "bare unpadded rootkey-shaped token redacted", input: "token bm90LWEtcmVhbC1rZXktMQ done", expect: "token [redacted] done" }, + // The marker rule can't reach a JSON-quoted value (quote delimits it); + // the bare rule catches the token inside the quotes. + { name: "JSON-quoted rootkey value redacted by bare rule", input: `{"rootKey":"${ROOTKEY_PADDED}"}`, expect: '{"rootKey":"[redacted]"}' }, + // Shapes the disabled broad rule over-matched — must all survive. + { name: "32-hex trace id and 16-hex span id survive", input: "trace 4bf92f3577b34da6a3ce929d0e0e4736 span 00f067aa0ba902b7", expect: "trace 4bf92f3577b34da6a3ce929d0e0e4736 span 00f067aa0ba902b7" }, + { name: "exception type names survive", input: "NotFoundError: caused by TypeError", expect: "NotFoundError: caused by TypeError" }, + // 22 letters ending in [AQgw] — only the character-mix check spares it. + { name: "22-char PascalCase identifier survives", input: "handled ProjectInviteTokenView event", expect: "handled ProjectInviteTokenView event" }, + // 22 chars, right final char, mixed case + digits — the hex check spares it. + { name: "22-char hex fragment survives", input: "id deadbeefDEADBEEF0011AA end", expect: "id deadbeefDEADBEEF0011AA end" }, + { name: "22-char ERR_* code survives", input: "ERR_INVALID_URL_SCHEME", expect: "ERR_INVALID_URL_SCHEME" }, + // Exact-length rule ignores base64-ish tokens of other lengths. { name: "43-char public key passes through", input: BASE64_43, expect: BASE64_43 }, { name: "52-char project id passes through", input: ZBASE32_52, expect: ZBASE32_52 }, ]; @@ -49,8 +65,11 @@ export const forbiddenMetricCases = [ { name: "forbidden metric name", metricName: "project_id", attributes: { platform: "ios" }, expect: true }, { name: "lat/lng-shaped tag value", metricName: "comapeo.x", attributes: { coord: "lat=12.34" }, expect: true }, { name: "lon-shaped tag value", metricName: "comapeo.x", attributes: { coord: "lon=-55.12" }, expect: true }, - // Broad base64-22 value rule disabled — bare tokens no longer drop a metric. - { name: "43-char token value allowed (broad rule off)", metricName: "comapeo.x", attributes: { bucket: BASE64_43 }, expect: false }, - { name: "52-char token value allowed (broad rule off)", metricName: "comapeo.x", attributes: { bucket: ZBASE32_52 }, expect: false }, + { name: "bare rootkey-shaped tag value drops the metric", metricName: "comapeo.x", attributes: { note: `key ${ROOTKEY_PADDED}` }, expect: true }, + { name: "error_class tag value allowed", metricName: "comapeo.rpc.errors", attributes: { error_class: "NotFoundError" }, expect: false }, + { name: "trace-id tag value allowed", metricName: "comapeo.x", attributes: { trace: "4bf92f3577b34da6a3ce929d0e0e4736" }, expect: false }, + // Exact-length rule ignores base64-ish tokens of other lengths. + { name: "43-char token value allowed", metricName: "comapeo.x", attributes: { bucket: BASE64_43 }, expect: false }, + { name: "52-char token value allowed", metricName: "comapeo.x", attributes: { bucket: ZBASE32_52 }, expect: false }, { name: "ordinary rpc tags allowed", metricName: "comapeo.rpc.client.duration_ms", attributes: { method: "read.doc", status: "ok", platform: "ios" }, expect: false }, ];