From 06badeef8232d14601e8d0e552eac17936dfb3f0 Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:18:01 +0200 Subject: [PATCH] opencode: scope a manifest credential id to its provider segment The check was non-empty-string only, so a binding carrying `oauth:openai` parsed cleanly inside an `anthropic` provider block and every tenant reading the shared manifest would have honoured it. Two peer tenants found the same hole in their own parsers independently, which is the argument for fixing it in the shared parser rather than three times. Segment 2 must BE the provider block it sits in. Scoped to segment 2 only, and the two exclusions are deliberate rather than unfinished: segment 1 (kind) an OPEN SET -- oauth:, chatgpt:, antigravity:, apikey: are all live in this deployment, so a kind allowlist refuses real ids segment 3+ (label) operator-chosen and optional -- main is the 2-segment `oauth:anthropic`, fallbacks are 3-segment Constraining either would reject the deployment this contract describes. The predicate moved to packages/client since this was first written: handles.ts in packages/opencode is now a re-export shim, so the fix lands in the shared parser and both tenants inherit it. Mutation-proved: restoring the weak predicate fails `rejects a credential id scoped to another provider` and `rejects a credential id without a provider segment`. --- .../src/bin/cli_support/opencode_files.rs | 22 ++- .../credentials-module/tests/cli_opencode.rs | 133 ++++++++++++++++++ docs/opencode-custody-design.md | 42 ++++++ packages/client/src/handles.ts | 40 +++++- packages/opencode/README.md | 5 + packages/opencode/src/tests/contracts.test.ts | 69 +++++++++ scripts/gate.sh | 4 +- 7 files changed, 311 insertions(+), 4 deletions(-) diff --git a/crates/credentials-module/src/bin/cli_support/opencode_files.rs b/crates/credentials-module/src/bin/cli_support/opencode_files.rs index 48de6de..8a366ca 100644 --- a/crates/credentials-module/src/bin/cli_support/opencode_files.rs +++ b/crates/credentials-module/src/bin/cli_support/opencode_files.rs @@ -869,7 +869,27 @@ fn validate_handle_file(file: &HandleFile) -> Result<(), OpenCodeFilesError> { account.label ))); } - if account.credential_id.is_empty() { + // Must match `parseHandleFile` in packages/client/src/handles.ts. THIS IS A + // WRITER: `validate_handle_file` runs from `write_handle_file_for_tenant` and + // `verify_handle_written`, so a rule missing here lets `ck auth` ORIGINATE a + // row the TypeScript reader refuses -- and that reader refuses the whole + // document, so one bad row written here denies every tenant in the file. + // + // Until this commit the check was emptiness only, which let `ck auth` write + // `oauth:openai` into an `anthropic` block: the exact cross-provider smuggle + // the TypeScript side was tightened to reject. Two implementations of one + // predicate in one repo, diverging because the fix landed on the reader. + // + // Segment 2 must BE the provider block; NO segment may be empty. Segment 1 + // (kind) is an open set -- oauth, chatgpt, antigravity, apikey are all live -- + // and segment 3+ (label) is operator-chosen and optional, so neither is + // constrained beyond non-emptiness. `:anthropic:x` and `oauth:anthropic:` + // satisfy the provider rule literally while naming ids that cannot exist. + let segments: Vec<&str> = account.credential_id.split(':').collect(); + if account.credential_id.is_empty() + || segments.get(1) != Some(&provider.provider.as_str()) + || segments.iter().any(|segment| segment.is_empty()) + { return Err(OpenCodeFilesError::Invalid(format!( "provider {index} account {} has invalid credential id", account.label diff --git a/crates/credentials-module/tests/cli_opencode.rs b/crates/credentials-module/tests/cli_opencode.rs index c419e35..dabdba2 100644 --- a/crates/credentials-module/tests/cli_opencode.rs +++ b/crates/credentials-module/tests/cli_opencode.rs @@ -263,6 +263,66 @@ fn hostile_provider_ids_and_account_labels_are_refused_by_the_rust_handle_valida } } +/// Holds the WRITE path specifically. The sibling arms parse raw fixtures and would stay +/// green if `validate_handle_file` were dropped from `write_handle_file_for_tenant`, and +/// that is the direction that matters: a writer without the rule ORIGINATES a row the +/// TypeScript reader refuses wholesale, denying every tenant in the shared manifest. +/// +/// Constructed in memory rather than parsed, because the point is that a caller already +/// holding a `HandleFile` cannot persist an invalid one -- no deserialization step stands +/// between this value and the disk. +/// +/// THE WRITER VALIDATES TWICE -- once on the caller's value and once on the merged result +/// after the tenant block is folded in -- so REMOVING EITHER ONE ALONE LEAVES THIS ARM +/// GREEN. That was measured, not assumed: deleting only the entry check kept all 71 tests +/// passing, and the arm reddens only when both go. So this holds the WRITE PATH as a +/// whole and does NOT pin either call site individually; a refactor that drops one of the +/// two will not be caught here. Stated because the alternative is a reader inferring +/// coverage from the name, which is how the gap this arm closes was created. +#[test] +fn an_invalid_handle_file_is_refused_at_the_write_path() { + let root = tmp_root("write-path-validation"); + let path = root.path().join("opencode-handles.json"); + + let invalid = opencode_files::HandleFile { + version: 1, + providers: vec![opencode_files::HandleProvider { + provider: "deepseek".into(), + shape: opencode_files::HandleShape::Api, + serve: "opencode-claustrum".into(), + accounts: vec![opencode_files::HandleAccount { + label: "main".into(), + handle: "ckh_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".into(), + // Scoped to another provider: the smuggle the TypeScript reader rejects. + credential_id: "oauth:anthropic".into(), + superseded: Vec::new(), + }], + }], + }; + + let err = opencode_files::write_handle_file(&path, &invalid) + .expect_err("the write path must refuse a cross-provider credential id"); + assert!( + err.to_string().contains("invalid credential id"), + "unexpected error: {err}" + ); + + // Refused BEFORE touching disk. A writer that validates after creating the file + // leaves a partial artifact for the next reader, and "it returned an error" does not + // distinguish the two. + assert!( + !path.exists(), + "a refused write must not leave a file behind" + ); + + // Positive control: the same shape with a correctly scoped id must persist, so the + // refusal above is the predicate acting rather than the writer refusing everything. + let mut valid = invalid; + valid.providers[0].accounts[0].credential_id = "apikey:deepseek:main".into(); + opencode_files::write_handle_file(&path, &valid).expect("a valid file must persist"); + assert!(path.exists(), "the valid write must produce a file"); +} + #[test] fn handle_file_debug_redacts_live_and_superseded_capabilities() { let file = opencode_files::HandleFile { @@ -349,6 +409,79 @@ fn a_handle_file_with_an_empty_credential_id_is_refused() { ); } +/// The Rust validator runs on the WRITE path (`write_handle_file_for_tenant`, +/// `verify_handle_written`), so a rule it lacks lets `ck auth` ORIGINATE a row the +/// TypeScript reader refuses -- and that reader refuses the whole document, denying +/// every tenant in a shared file. These arms pin the two sides to one predicate. +/// +/// Each case is also asserted in `packages/opencode/src/tests/contracts.test.ts`. The +/// duplication is forced -- two languages, one contract -- so it is marked here rather +/// than left to look like an independent local rule. +/// +/// THE ARMS BELOW GO THROUGH `read_handle_file`, WHICH IS THE READ PATH. They pin the +/// predicate but NOT the claim in the paragraph above: deleting `validate_handle_file` +/// from the writer leaves every one of them green, because a raw fixture never reaches +/// the writer at all. `an_invalid_handle_file_is_refused_at_the_write_path` is the arm +/// that holds the writer, and it is separate for exactly that reason -- a comment +/// asserting coverage its arms do not have is the defect this file keeps finding +/// elsewhere. +#[test] +fn a_handle_file_with_a_cross_provider_credential_id_is_refused() { + let err = read_raw_handle_fixture( + "cross-provider-credential-id", + r#"{"version":1,"providers":[{"provider":"deepseek","shape":"api","serve":"opencode-claustrum","accounts":[{"label":"main","handle":"ckh_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","credential_id":"oauth:anthropic"}]}]}"#, + ) + .expect_err("a credential id scoped to another provider refuses"); + + assert!( + err.to_string().contains("invalid credential id"), + "unexpected error: {err}" + ); +} + +#[test] +fn a_handle_file_with_an_empty_credential_id_segment_is_refused() { + for (name, credential_id) in [ + ("empty-kind-segment", ":deepseek:main"), + ("empty-label-segment", "apikey:deepseek:"), + ("empty-middle-segment", "apikey::deepseek"), + ] { + let err = read_raw_handle_fixture( + name, + &format!( + r#"{{"version":1,"providers":[{{"provider":"deepseek","shape":"api","serve":"opencode-claustrum","accounts":[{{"label":"main","handle":"ckh_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","credential_id":"{credential_id}"}}]}}]}}"# + ), + ) + .unwrap_err(); + + assert!( + err.to_string().contains("invalid credential id"), + "{name}: unexpected error: {err}" + ); + } +} + +#[test] +fn a_handle_file_with_live_credential_id_shapes_is_accepted() { + // Positive control against the real deployment: a tightening that refuses a live id + // is worse than the gap it closes, and every one of these is in the vault today. + for (provider, credential_id) in [ + ("deepseek", "apikey:deepseek:main"), + ("anthropic", "oauth:anthropic"), + ("anthropic", "oauth:anthropic:work-alt"), + ("openai", "chatgpt:openai"), + ("google", "antigravity:google"), + ] { + read_raw_handle_fixture( + &format!("live-shape-{credential_id}"), + &format!( + r#"{{"version":1,"providers":[{{"provider":"{provider}","shape":"api","serve":"opencode-claustrum","accounts":[{{"label":"main","handle":"ckh_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","credential_id":"{credential_id}"}}]}}]}}"# + ), + ) + .unwrap_or_else(|err| panic!("{credential_id} must parse: {err}")); + } +} + #[test] fn a_handle_file_with_a_malformed_superseded_capability_is_refused() { let err = read_raw_handle_fixture( diff --git a/docs/opencode-custody-design.md b/docs/opencode-custody-design.md index c1b13b6..b680a28 100644 --- a/docs/opencode-custody-design.md +++ b/docs/opencode-custody-design.md @@ -34,6 +34,48 @@ provider with in-request failover for providers the generic plugin serves. | Multi-account | One vault record per key/account; ordered priority list per provider | | Dedicated-plugin providers | Served by THEIR plugin consuming this client + handle file + tombstone convention; never by the generic closure | +### Handle-manifest credential-id scope + +The handle manifest is multi-tenant: each tenant owns only its `provider` + `serve` blocks. +Within a block for provider `P`, `credential_id.split(':')[1] === P` is the whole id-level +scope check. Segment 1 is the credential kind and is an OPEN SET: `antigravity`, `apikey`, +`chatgpt`, and `oauth` exist today, and new kinds are expected. Do not allowlist or infer the +kind. Segment 3+ is a label convention only and is never consulted for provider scoping; +provider scoping and label derivation are different properties, and an unlabelled +`oauth:anthropic` is valid for `anthropic` just as `oauth:anthropic:any-label` is. + +| expectation | provider | account label | `credential_id` | reason | +|---|---|---|---|---| +| MUST RESOLVE | `openai` | `main` | `chatgpt:openai` | A kind-prefix rule would reject this live OpenAI shape. | +| MUST RESOLVE | `google` | `main` | `antigravity:google` | A kind-prefix rule would reject this live Google shape. | +| MUST RESOLVE | `anthropic` | `work-alt` | `oauth:anthropic:something-else` | The label must not be derived or consulted. | +| MUST REJECT | `anthropic` | `main` | `chatgpt:openai` | A real cross-tenant id must not parse in another provider's block. | + +Provider-segment validation is a SHAPE check, not an existence check: only the runtime fence, +which compares `credential_id` with what `credential.get` returns for the bound handle, proves +that a binding names a real record. This rule is only a cheap pre-filter for cross-tenant +smuggling. Tenant fixtures must therefore use the REAL vault credential id and say why; a tidier +plausible id can pass every row above and still fail the runtime fence. Credential ids are +operator-chosen and cannot be derived from the provider, account label, or record kind: +`chatgpt:openai` is live even though its kind segment is `chatgpt` while the record kind is +`oauth`; those are unrelated. + +**"The runtime fence catches it" is only a valid justification for a consumer that CAN read vault +ground truth, and today that is a property of the transport a tenant happened to choose.** A tenant +with its own transport reads `credential_id` off the `credential.get` reply and can refuse on +mismatch. A tenant vendoring `@cortexkit/claustrum-client` CANNOT: `ServedCredential` is +`{material, recordVersion, expiresAtMs}` and has never carried `credential_id` or `account_id` at +any ref, so the client discards five of the eight non-secret fields the wire sends before a +consumer sees them. This repo's own custody plugin is in that position — `packages/opencode/src/serve.ts` +logs the manifest's `credential_id` beside the vault's `record_version`, two values from different +sources that read as corroboration. + +That matters because relaxing a parse-time constraint is licensed by the runtime fence existing. +That license was extended to three tenants while only two could exercise it. Until the client +carries the served metadata, treat the fence as a per-tenant capability rather than a contract-level +guarantee, and do not justify a parse-time relaxation by it without checking that the tenant in +question can actually perform the comparison. + ### Seam boundary This is a config-hook/fetch-seam integration, **not provider-universal custody**. The generic diff --git a/packages/client/src/handles.ts b/packages/client/src/handles.ts index 836e93b..d6ca605 100644 --- a/packages/client/src/handles.ts +++ b/packages/client/src/handles.ts @@ -76,7 +76,45 @@ export function parseHandleFile(value: unknown): OpenCodeHandleFileV1 { if (labels.has(account.label)) invalid(`provider ${index} duplicates account label ${account.label}`) labels.add(account.label) if (!handleIsValid(account.handle)) invalid(`provider ${index} account ${account.label} has invalid handle`) - if (!account.credential_id) invalid(`provider ${index} account ${account.label} has invalid credential id`) + // Segment 2 of the credential id must BE the provider block it sits in. Without + // this the check was non-empty-string only, so an `oauth:openai` binding parsed + // cleanly inside an `anthropic` block -- a cross-provider smuggle that every + // tenant reading this manifest would have honoured. Two peer tenants found the + // same hole in their own parsers independently. + // + // SCOPED TO SEGMENT 2 ONLY, deliberately. Segment 1 (the kind) is an OPEN SET -- + // `oauth:`, `chatgpt:`, `antigravity:`, `apikey:` are all live in this vault today + // -- so a kind allowlist would refuse real ids. Segment 3+ (the label) is + // operator-chosen and may be absent: main is the 2-segment `oauth:anthropic`, + // fallbacks are 3-segment. Constraining either would reject the deployment this + // contract describes. + // + // NO SEGMENT MAY BE EMPTY. Segment 2 alone is what fences the provider, but a + // position-1 check ignores the rest of the string, and that left two ids passing + // that name credentials which cannot exist: `:anthropic:x` (empty kind) and + // `oauth:anthropic:` (empty label) both satisfy "segment 2 is the provider" + // literally. Neither is a smuggle; both defer a GUARANTEED resolve-time failure + // past the door, and under custody a resolve-time failure on a tombstoned account + // is a dark route rather than a refused row. + // + // It also removes an asymmetry nobody designed and everyone would read as a bug: + // `oauth::anthropic` rejected while `:anthropic:x` passed, purely because the + // check indexed position 1 and ignored positions 0 and 2. Agreed with the peer + // tenant and mirrored on their side, so this is chosen rather than defaulted -- + // the previous behaviour was two independent defaults that happened to differ. + // + // The emptiness guard is ALSO explicit rather than a consequence: `''.split(':')` + // yields `['']`, whose `[1]` is `undefined` and cannot equal a provider string, + // so an empty id would reject anyway -- but that is a coincidence doing + // load-bearing work, and the check this replaced (`!account.credential_id`) was + // the emptiness guard. NO TEST DISTINGUISHES THAT ONE (empty rejects with or + // without it, verified by removal), so it is kept for a future reader who loosens + // the comparison, not for an arm it could never redden. The non-empty-SEGMENT + // rule below is different: it reddens, and is pinned. + const segments = account.credential_id.split(':') + if (!account.credential_id || segments[1] !== item.provider || segments.some((segment) => segment.length === 0)) { + invalid(`provider ${index} account ${account.label} has invalid credential id`) + } if (account.superseded?.some((handle) => !handleIsValid(handle))) { invalid(`provider ${index} account ${account.label} has invalid superseded handle`) } diff --git a/packages/opencode/README.md b/packages/opencode/README.md index d6636ca..e13285a 100644 --- a/packages/opencode/README.md +++ b/packages/opencode/README.md @@ -38,6 +38,11 @@ The plugin never reads this table: only the CLI that creates tombstones does. The handle file comes from `CLAUSTRUM_OPENCODE_HANDLES`, or from `${XDG_CONFIG_HOME:-$HOME/.config}/cortexkit/opencode-handles.json`. It must be a regular file owned by the current user with mode `0600`; symlinks are refused. Provider ids and account labels must match `^[a-z0-9][a-z0-9._-]{0,63}$` and cannot be `__proto__`, `constructor`, or `prototype`. OpenCode auth is read from `OPENCODE_AUTH_CONTENT` when it is set, otherwise from `${XDG_DATA_HOME:-$HOME/.local/share}/opencode/auth.json`. +The canonical handle-manifest credential-id scope contract and its conformance table live in +`docs/opencode-custody-design.md` under “Handle-manifest credential-id scope”. In short, the +second colon-separated segment must equal the block provider; the kind is open and labels are not +used for validation. + If the selected auth source cannot be parsed or validated, the plugin scans it in bounded chunks for self-describing tombstones and refuses the named providers. No scan hit leaves a never-migrated oversized auth source alone. A raw scan does not recognize JSON-escaped sentinel bytes; that hand-edit/foreign-writer limitation shares the same no-hit branch, so changing either behavior requires deciding both. OpenCode's provider API and UI serialize `Provider.Info.key`, so a tombstone can look like a configured credential. It is non-secret and does not grant access; custody still refuses when ownership cannot be proven. diff --git a/packages/opencode/src/tests/contracts.test.ts b/packages/opencode/src/tests/contracts.test.ts index 1992d5c..5227c6a 100644 --- a/packages/opencode/src/tests/contracts.test.ts +++ b/packages/opencode/src/tests/contracts.test.ts @@ -8,6 +8,19 @@ import goldenHandles from "../../golden/handles.json"; import goldenTombstoneJson from "../../golden/tombstone.json"; const goldenTombstone = goldenTombstoneJson as GoldenTombstone; +const validHandle = `ckh_${"a".repeat(43)}`; + +function manifestWithCredential(provider: string, label: string, credentialId: string) { + return { + version: 1, + providers: [{ + provider, + shape: "api", + serve: "opencode-claustrum", + accounts: [{ label, handle: validHandle, credential_id: credentialId }], + }], + }; +} describe("custody wire contracts", () => { test("loads the canonical tombstone golden rather than a copied fixture", () => { @@ -77,4 +90,60 @@ describe("custody wire contracts", () => { }), ).toThrow("invalid handle"); }); + + // Discriminator: an oauth-only kind rule rejects this live OpenAI shape, so it cannot prove provider scoping. + test("accepts chatgpt:openai for the openai provider", () => { + expect(() => parseHandleFile(manifestWithCredential("openai", "main", "chatgpt:openai"))).not.toThrow(); + }); + + // Discriminator: an oauth-only kind rule rejects this live Google shape, so it cannot prove provider scoping. + test("accepts antigravity:google for the google provider", () => { + expect(() => parseHandleFile(manifestWithCredential("google", "main", "antigravity:google"))).not.toThrow(); + }); + + test("accepts a provider-scoped id whose label does not match the account label", () => { + expect(() => + parseHandleFile(manifestWithCredential("anthropic", "work-alt", "oauth:anthropic:something-else")), + ).not.toThrow(); + }); + + test("rejects a credential id scoped to another provider", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "chatgpt:openai"))).toThrow("credential id"); + }); + + test("accepts the existing apikey provider-scoped live shape", () => { + expect(() => parseHandleFile(manifestWithCredential("deepseek", "main", "apikey:deepseek:main"))).not.toThrow(); + }); + + test("accepts the existing unlabelled oauth provider-scoped live shape", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "oauth:anthropic"))).not.toThrow(); + }); + + test("rejects a credential id without a provider segment", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "oauth"))).toThrow("credential id"); + }); + + // Both of these satisfy "segment 2 is the provider" literally, so a position-1 check + // alone accepts them -- and both name credentials that cannot exist, deferring a + // guaranteed resolve-time failure past the door. They also fixed an asymmetry that + // read as a bug: `oauth::anthropic` rejected (empty at position 1) while these passed + // (empty at positions 0 and 2). Agreed with the anthropic-auth tenant and mirrored + // there, so the rule is chosen on both sides rather than defaulted on either. + test("rejects a credential id with an empty kind segment", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", ":anthropic:x"))).toThrow("credential id"); + }); + + test("rejects a credential id with an empty label segment", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "oauth:anthropic:"))).toThrow( + "credential id", + ); + }); + + test("rejects a credential id with an empty provider segment", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "oauth::main"))).toThrow("credential id"); + }); + + test("rejects a credential id whose provider segment differs only by case", () => { + expect(() => parseHandleFile(manifestWithCredential("anthropic", "main", "oauth:Anthropic:main"))).toThrow("credential id"); + }); }); diff --git a/scripts/gate.sh b/scripts/gate.sh index f89f78c..e96b990 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -235,7 +235,7 @@ stream and pass the arm without ever seeing it skip." # follows it), and any gap between the floor and the real count is how many can go # before anyone is told. Measured 402 across the workspace's suites at the time of # writing; an earlier floor of 200 left a third of them free to disappear. -# The current measured total is 608 (debug profile, the same command this arm +# The current measured total is 614 (debug profile, the same command this arm # runs). It covers master's resolved-credential-id pins and RAII temp-dir lifecycle, plus this # branch's Rust manifest-lock tests: the ABA observation that cannot rename a replacement, one # quarantine directory per stale owner, unknown and malformed owner fields tolerated but still @@ -254,7 +254,7 @@ stream and pass the arm without ever seeing it skip." # # Raise this when tests are added. A failure here is normally that, not a defect -- # but it should be a deliberate edit rather than a number nobody revisits. -run_expect 610 "workspace unit + integration" \ +run_expect 614 "workspace unit + integration" \ cargo test --locked --workspace --features credentials-core/test-support # Two independent defences, because each catches what the other misses: