fix(sync): key the device, span and trace digests - #931
Conversation
|
Converted to draft: this was opened with only a self-review by the model that wrote it, which is not the review bar this batch is held to. Independent review by two other reviewers is running now; I will mark it ready once both have passed and any findings are applied. Apologies for the noise. |
Ports three upstream commits this branch never received: the attribution feature (1bf7206), the review hardening on top of it (ccee28a), and the security follow-up that closed credential-leak paths and added session retraction (50c8251). They are ported as an end state rather than in sequence. Two and three revise one, so replaying them in order would have introduced the very issues they fix and then removed them again — and anything missed in the third pass would have shipped a feature with a reopened hole, which is the specific way this port could have gone wrong. The credential-leak paths that commit closes are enumerated and checked off individually against the result. One correction to an earlier draft of this message, which claimed no new unkeyed digest is introduced. That was wrong: stateHash in sync/otlp.ts is a new unkeyed sha256, and it feeds deriveSpanId, so it is an input to a value that goes on the wire. It is not a D1 violation — D1 governs core's fingerprint module and its caller-supplied key, while stateHash is a local ledger discriminator computed over data that is itself sent in cleartext, so it hides nothing and leaks nothing. But the sentence was false and is worth correcting rather than quietly dropping. That matters for merge order. getagentseal#931 rewrites the id derivations in this same file, and this commit adds a new digest plus two new consumers of deriveSpanId and deriveTraceId that getagentseal#931 was not written against. Land getagentseal#931 first, or reconcile the attribution call sites into it.
|
Cross-PR note from an independent review of #933: two decoders — That does not weaken this change — keying is exactly what makes it safe to hash a path. But it means the two PRs are coupled: if #933 ends up fingerprinting |
… keys Three contract defects in the published observation surface, and a fourth that the first attempt at fixing them created. **`model` and `pricingModel` were unbounded free text** — `z.string().min(1)`, no max, no pattern — inside a schema whose docstring states that no field can hold free text. For vercel-gateway the value arrives from a fetched report. The first attempt bounded them with a pattern. Review proved that wrong: real providers emit display names, not identifiers. Antigravity reads `payload.model.display_name` directly, Warp's alias map is a closed ten-entry list that returns anything unmapped verbatim, and devin emits the same shape. A pattern would have rejected the envelope for those providers, and the next Warp model to ship would have broken decoding — a breaking narrowing of a contract already published at 0.9.20. So the value is normalized at the observation boundary instead, which is what this package already does for tool names, rather than validated and rejected. **The smuggling test asserted the leak survives** — it planted a prompt in `model` and checked it was still there. Inverting that assertion exposed the real carrier: `dedupKey` is built from the model string, so bounding the model while the key is derived from it is half a fix. The secret rode through the key. **`sourceRef` was documented as an opaque fingerprint while every caller passed a raw absolute path**, and three decoders folded it straight into dedup keys — which are observation output. A reviewer produced a schema-valid envelope containing a real home directory path. Rather than rewriting the contract text to match the leak, the decoders now fingerprint it: codebuff, lingtai-tui and zerostack all route through the keyed helper. That also matters for getagentseal#931, which derives sync span ids from these keys. The codebuff golden pinned the raw-path form. It is updated with the derivation shown, and a comment recording that the raw path was the defect — the same trap the kiro golden set in a sibling PR.
026fc8b to
a063ccb
Compare
… keys Three contract defects in the published observation surface, and a fourth that the first attempt at fixing them created. **`model` and `pricingModel` were unbounded free text** — `z.string().min(1)`, no max, no pattern — inside a schema whose docstring states that no field can hold free text. For vercel-gateway the value arrives from a fetched report. The first attempt bounded them with a pattern. Review proved that wrong: real providers emit display names, not identifiers. Antigravity reads `payload.model.display_name` directly, Warp's alias map is a closed ten-entry list that returns anything unmapped verbatim, and devin emits the same shape. A pattern would have rejected the envelope for those providers, and the next Warp model to ship would have broken decoding — a breaking narrowing of a contract already published at 0.9.20. So the value is normalized at the observation boundary instead, which is what this package already does for tool names, rather than validated and rejected. **The smuggling test asserted the leak survives** — it planted a prompt in `model` and checked it was still there. Inverting that assertion exposed the real carrier: `dedupKey` is built from the model string, so bounding the model while the key is derived from it is half a fix. The secret rode through the key. **`sourceRef` was documented as an opaque fingerprint while every caller passed a raw absolute path**, and three decoders folded it straight into dedup keys — which are observation output. A reviewer produced a schema-valid envelope containing a real home directory path. Rather than rewriting the contract text to match the leak, the decoders now fingerprint it: codebuff, lingtai-tui and zerostack all route through the keyed helper. That also matters for getagentseal#931, which derives sync span ids from these keys. The codebuff golden pinned the raw-path form. It is updated with the derivation shown, and a comment recording that the raw path was the defect — the same trap the kiro golden set in a sibling PR.
The sync path derived three identifiers with bare SHA-256 and sent them to a configured endpoint. `deriveDeviceId` hashed `hostname:username` and truncated to 64 bits, commented "pseudonymous, stable". An unkeyed digest of a host and username pair is not pseudonymous against anyone who can guess plausible values: hash the guess, compare, done. `deriveSpanId` hashed the dedup key — and for pi, zerostack, lingtai-tui and codebuff that key embeds the raw absolute source path, home directory included, because the bridge passes `source.path` straight through. Guess a plausible home and project name and the same confirmation works. This is the project's own standard, not an outside opinion. Decision D1 requires a caller-supplied HMAC key for fingerprints precisely so digests of paths cannot be dictionary-attacked, and core's fingerprint module throws on an empty key to enforce it. The sync path bypassed the primitive entirely. It also contradicted the project's own user-facing guarantee: docs/sync/README.md promises that code, file contents, diffs and PATHS stay local, and the unkeyed span id shipped absolute paths (for the four providers above) in a form confirmable by anyone with a plausible guess. All three ids are now HMAC-SHA256 under the per-install privacy key — the same key core's fingerprints use — with domain prefixes so one value in two positions never yields the same digest, and composite inputs joined with the same ASCII Unit Separator (0x1f) core/fingerprint.ts uses so a value containing ':' cannot forge a field boundary. The derive functions throw on an empty key rather than degrading. The payload builder obtains the key itself, so the decode path, which runs with an empty key by design, never reaches it. Sync now REQUIRES the persisted key: privacy-key.ts exposes a strict variant that aborts the push instead of falling back to per-process randomness when the config dir is unwritable, and refuses to silently regenerate a key file that fails validation (truncated by a full disk, a partial write). Cross-process id stability is load-bearing — partially rejected batches are not ledgered precisely because deterministic span ids make full-batch retry safe — so a per-process fallback key would emit fresh ids on every retry and let the backend double-count accepted spans, and a silent re-key would orphan everything already pushed. The fingerprint consumers keep the tolerant fallback: they only need per-process stability. The refusal is now complete, and enforced for every corrupt shape: "no file at all" is the only state a first use may create. A file that exists but is unreadable, zero-byte or whitespace-only (a partial write), or fails hex validation aborts the push and is left untouched — treating those as MISSING would silently regenerate the file and re-key every id, which is exactly the case the strict path exists to refuse. First creation is also exclusive (O_CREAT|O_EXCL): when two processes race the first use, the loser re-reads and adopts the winner's key, so concurrent pushes can never mint different keys and mix cached device ids with spans derived from the other. Scope, stated honestly: sync is opt-in and needs an endpoint plus credentials, the digests are of identifiers rather than prompts or file contents, and this predates the extraction. It is not an active leak of user content. It is a weak construction the project already knows how to do properly. This change narrows the exposure rather than closing it: ai.project still ships a project name in the clear, and in one Claude fallback path that name is a dash-encoded absolute path. Blast radius: every id is re-keyed once at upgrade, so anything already pushed stops correlating with new sends and the backend sees a fresh device identity. Ids stay stable afterwards unless the key file is lost. The host-side sent ledger keys off the raw dedup key and is unaffected, so re-push filtering keeps working.
a063ccb to
c467548
Compare
… keys Three contract defects in the published observation surface, and a fourth that the first attempt at fixing them created. **`model` and `pricingModel` were unbounded free text** — `z.string().min(1)`, no max, no pattern — inside a schema whose docstring states that no field can hold free text. For vercel-gateway the value arrives from a fetched report. The first attempt bounded them with a pattern. Review proved that wrong: real providers emit display names, not identifiers. Antigravity reads `payload.model.display_name` directly, Warp's alias map is a closed ten-entry list that returns anything unmapped verbatim, and devin emits the same shape. A pattern would have rejected the envelope for those providers, and the next Warp model to ship would have broken decoding — a breaking narrowing of a contract already published at 0.9.20. So the value is normalized at the observation boundary instead, which is what this package already does for tool names, rather than validated and rejected. **The smuggling test asserted the leak survives** — it planted a prompt in `model` and checked it was still there. Inverting that assertion exposed the real carrier: `dedupKey` is built from the model string, so bounding the model while the key is derived from it is half a fix. The secret rode through the key. **`sourceRef` was documented as an opaque fingerprint while every caller passed a raw absolute path**, and three decoders folded it straight into dedup keys — which are observation output. A reviewer produced a schema-valid envelope containing a real home directory path. Rather than rewriting the contract text to match the leak, the decoders now fingerprint it: codebuff, lingtai-tui and zerostack all route through the keyed helper. That also matters for getagentseal#931, which derives sync span ids from these keys. The codebuff golden pinned the raw-path form. It is updated with the derivation shown, and a comment recording that the raw path was the defect — the same trap the kiro golden set in a sibling PR.
… keys Three contract defects in the published observation surface, and a fourth that the first attempt at fixing them created. **`model` and `pricingModel` were unbounded free text** — `z.string().min(1)`, no max, no pattern — inside a schema whose docstring states that no field can hold free text. For vercel-gateway the value arrives from a fetched report. The first attempt bounded them with a pattern. Review proved that wrong: real providers emit display names, not identifiers. Antigravity reads `payload.model.display_name` directly, Warp's alias map is a closed ten-entry list that returns anything unmapped verbatim, and devin emits the same shape. A pattern would have rejected the envelope for those providers, and the next Warp model to ship would have broken decoding — a breaking narrowing of a contract already published at 0.9.20. So the value is normalized at the observation boundary instead, which is what this package already does for tool names, rather than validated and rejected. **The smuggling test asserted the leak survives** — it planted a prompt in `model` and checked it was still there. Inverting that assertion exposed the real carrier: `dedupKey` is built from the model string, so bounding the model while the key is derived from it is half a fix. The secret rode through the key. **`sourceRef` was documented as an opaque fingerprint while every caller passed a raw absolute path**, and five decoders folded it straight into dedup keys — which are observation output. A reviewer produced a schema-valid envelope containing a real home directory path. Rather than rewriting the contract text to match the leak, the decoders now fingerprint it: codebuff, lingtai-tui, zerostack, pi/omp and grok all route through the keyed helper. That also matters for getagentseal#931, which derives sync span ids from these keys. **Cross-PR review: the fingerprint was keyed NOWHERE on the production path.** The first version of this change added `sourceRefFingerprint` with an unkeyed SHA-256 fallback, justified by the CLI bridge's old comment that the rich decoder "never consumes" the privacy key. That comment was overtaken by this very change — the decoders consume the key precisely via the dedup keys being fingerprinted here, so the fallback shipped dictionary-attackable digests of absolute paths in the same dedup keys, one layer down from the raw-path leak it replaced. The bridge now threads the host privacy key (getHostPrivacyKey, per-install stable, already used by the optimize detectors) into the rich decode, and `sourceRefFingerprint` now requires a key and throws on an empty one (decision D1, like every other fingerprint in the module) — an empty key can no longer silently degrade to an unkeyed digest. The bridge parity goldens derive their expected dedup keys under the same host key, and the session / daily cache bumps below still fire exactly once: the only released key shape is the raw-path one, so the re-parse drops those keys and lands the keyed fingerprint shape in the same pass. The codebuff golden pinned the raw-path form. It is updated with the derivation shown, and a comment recording that the raw path was the defect — the same trap the kiro golden set in a sibling PR.
The sync path derived three identifiers with bare SHA-256 and sent them to a configured endpoint.
What was being sent
deriveDeviceIdhashedhostname:username, truncated to 64 bits, commented "pseudonymous, stable". An unkeyed digest of a host/username pair is not pseudonymous against anyone who can guess plausible values — hash the guess, compare, done.deriveSpanIdhashed the dedup key. For pi, zerostack, lingtai-tui and codebuff that key embeds the raw absolute source path, home directory included, because the bridge passessource.pathstraight through to the decoders. Guess a plausible home and project name and the same confirmation works.Why this is the project's own standard
Decision D1 requires a caller-supplied HMAC key for fingerprints precisely so that digests of paths cannot be dictionary-attacked, and
packages/core/src/fingerprint.tsthrows on an empty key to enforce it. Every fingerprint that goes through core obeys this. The sync path bypassed the primitive and calledcreateHashdirectly.The fix
All three ids are now HMAC-SHA256 under the per-install privacy key — the same key core's fingerprints use — with domain prefixes (
sync-span:,sync-trace:,sync-device:) so one value appearing in two positions never yields the same digest. The derive functions throw on an empty key rather than degrading to unkeyed. The payload builder obtains the key itself, so the decode path — which runs with an empty key by design — never reaches them.Determinism is preserved: the key is stable per install, so re-sends stay byte-identical and server-side dedup still holds.
Scope, stated honestly
Sync is opt-in and needs an endpoint plus credentials. The digests are of identifiers, not prompts or file contents. This predates the extraction. It is not an active leak of user content — it is a weak construction in a project that already knows how to do this properly, on the one surface that skipped the primitive.
Blast radius
Every id is re-keyed once at upgrade, so anything already pushed stops correlating with new sends and the backend sees a fresh device identity per install. Ids are stable afterwards unless the key file is lost, which re-keys again. The host-side sent-ledger keys off the raw dedup key and is unaffected, so re-push filtering keeps working. First sync push creates the key file if the optimize detectors have not already.