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: