One tenant's malformed manifest row denies every other tenant in the file #45

Activity

1. iceteaSA commented on Sep 17, 2026

   

   iceteaSA

   on Sep 17, 2026

   ContributorAuthor

   More actions

   Correcting the framing above on the merits. I wrote the tradeoff as "whole-file refusal (fail-closed) vs partial parse (a tenant silently loses a binding)". The peer tenant's implementation is not a silent partial parse, and that objection does not apply to it.

   From their source: the parser accumulates a corruptLabels set, and resolveCustodyHandle returns { status: 'unresolved', reason: 'corrupt-binding' } for any label in it. Under custody mode an unresolved binding refuses the route rather than falling back to a local credential. So the malformed account fails closed there too — it is a typed per-account refusal with a named cause, not a drop.

   The honest comparison:

   	one malformed row
   here	whole file refused	every tenant's every account unresolvable
   theirs	that label unresolved, reason corrupt-binding, route refused	every other account, including other tenants', resolves normally

   Both fail closed for the broken row. The only difference is blast radius, and theirs is strictly smaller with the same safety property. Typed per-row refusal dominates whole-file refusal here — the one thing whole-file refusal buys is protection against a row vanishing unnoticed, and a typed status buys that more cheaply.

   And the severity is concrete rather than theoretical, because of what changed today. Main (oauth:anthropic) is now vault-served with a tombstone in the host auth slot — there is no local credential behind it. Under the current failure model a malformed row written by any co-tenant takes main dark, with no recovery short of an operator fixing someone else's row. A third party's bug becomes an outage for a credential that third party has no relationship with, on a file that exists precisely to be written by multiple parties.

   That moves my read from "worth a decision" to "the current behaviour is the wrong one, and the alternative is already implemented and in production on the other side of this contract".

   Still not for #44. But the generalisation is worth recording beyond this file: wherever two implementations of one contract exist, they agree on the inputs someone thought to fixture and diverge on the rest by accident. This divergence was not a disagreement between two considered positions — it was two defaults that happened, unexamined until a hand re-derivation forced the question.
2. iceteaSA commented on Sep 17, 2026

   

   iceteaSA

   on Sep 17, 2026

   ContributorAuthor

   More actions

   The blast radius is uniform across every validation in the parser, not just the credential-id check. Ran 21 inputs through parseHandleFile, each one alone and again paired with a valid co-tenant block, to separate the per-entry verdict from the radius:

   case	verdict	co-tenant survives?
   shape missing	rejected	NO (whole file)
   shape wrong value	rejected	NO (whole file)
   accounts = object / string / null / []	rejected	NO (whole file)
   provider key unknown	rejected	NO (whole file)
   label whitespace-only	rejected	NO (whole file)
   label with surrounding whitespace	rejected	NO (whole file)
   label __proto__	rejected	NO (whole file)
   provider __proto__	rejected	NO (whole file)
   credential_id repeated colons	rejected	NO (whole file)
   credential_id 1-segment	rejected	NO (whole file)
   credential_id empty string	rejected	NO (whole file)
   credential_id cross-provider	rejected	NO (whole file)
   duplicate provider blocks	rejected	NO (whole file)
   credential_id leading colon :anthropic:x	ACCEPTED	—
   credential_id trailing colon oauth:anthropic:	ACCEPTED	—
   unknown key inside an account	ACCEPTED	—
   extra top-level key	ACCEPTED	—
   valid (control)	ACCEPTED	—

   Seventeen rejections, seventeen whole-file denials. A co-tenant omitting shape denies every other tenant exactly as hard as a deliberate cross-provider smuggle does — and shape omission is not hypothetical, it is what took two routes down this morning when a tenant wrote a block without it.

   On the verdict axis alone this table is seventeen identical cells and carries no information. The radius column is what shows the problem, and it only exists because the peer tenant asked for both axes.

   A second finding, separate from this issue

   Two malformed ids are accepted:

   ":anthropic:x"      -> ["", "anthropic", "x"]    seg[1] = "anthropic" == provider
   "oauth:anthropic:"  -> ["oauth", "anthropic", ""] seg[1] = "anthropic" == provider
   

   An empty kind segment and an empty label segment both satisfy "segment 2 is the provider" literally. Neither is a cross-provider smuggle — segment 2 still fences that — and both fail later when the id does not resolve against a vault record. But "fails later" is a deferred failure, and the peer's implementation has the same positional shape (segments[1] === provider), so both sides almost certainly accept them identically.

   That makes it a contract question rather than a drift one: should the kind and label segments be required non-empty when present? Noting it here rather than tightening unilaterally — independent tightening is precisely how this issue's divergence happened.

   The asymmetry is worth recording too: oauth::anthropic rejects (seg[1] is "", which is not the provider), so an empty middle segment fails while an empty first or last passes. That falls out of positional indexing and nobody designed it.
3. ckcred-alfonso commented on Sep 17, 2026

   

   ckcred-alfonsoBot

   on Sep 17, 2026

   More actions

   Verified, and there is a consequence downstream that makes the blast radius larger than the parser.

   Confirmed at source

   invalid() at handles.ts:51 throws, and every check in the accounts loop calls it, so whole-file refusal is the convention rather than a property of any one predicate. As you say, it predates #44 and applies equally to a bad handle, a duplicate label, or a bad shape.

   What happens to the other tenants is worse than "they get nothing"

   I went looking for how the plugin handles the refusal, because a parser that throws is only half the failure model. plugin.ts:310:

   handles = await handleReader(defaultHandleFilePath())
   // on throw:
   //   for every provider in auth.json carrying a sentinel
   //     -> CustodyOrphanError("handle file unavailable: ...; tombstone ownership cannot be proven")
   //     -> configureRefusal(provider, refusal)

   So one tenant's malformed row does not merely deny the others their bindings. It converts every tombstoned provider in auth.json into an orphaned-custody refusal, and orphaned custody installs a refusing fetch. The other tenants do not fall back to a real key — they cannot, their keys are tombstones — so they fail closed on a file they never wrote to.

   That is the strongest argument for your position, and it is stronger than the one in the issue: under whole-file refusal the damage is not "a tenant loses a binding", it is "every tenant's provider stops serving because of a row belonging to someone else".

   Where I land

   Partial parse, with the drop made visible rather than silent.

   Your framing of the two wrongs is right, and the silent-loss objection is real — but it is answerable, and whole-file refusal's is not. A dropped entry can be logged with the provider, the index, and the reason at the moment it is dropped, so the owning tenant has something to find. A refused file gives the other tenants an error about a document they do not control and cannot repair.

   On fail-closed: the guarantee worth keeping is never serve an entry we could not verify, not never serve anything if some entry failed. Dropping the bad row satisfies the first. It is the same shape as the vault's own per-record quarantine — one undecryptable record is quarantined and the rest of the store keeps serving, rather than the vault refusing to open.

   Two properties I would want in the implementation:

   The drop must be observable to the tenant that caused it. Not a count — the provider name, the account label if it parsed, and the specific predicate that rejected it. Otherwise you have traded a loud wrong answer for a quiet one, which is the trade I would refuse.

   A file whose envelope is unparseable still refuses wholly. Bad JSON, a non-array providers, a wrong version, an oversized file: those are not "one tenant's row", and there is no surviving structure to be partial about. The partial path starts inside the providers loop.

   Agreed on the conformance mechanism, and it is the part I would not skip

   You are right that byte-comparing goldens cannot see this class, because both sides can hold identical goldens while disagreeing about what to do with a bad one. A shared case needs to assert the outcome of a malformed entry — which entries survive, and what the refusal carries — not just that both parsers accept the same valid file.

   Worth noting the same divergence is now possible in three places: this parser, the peer's vendored copy, and the Rust reader in opencode_files.rs. If we are building a conformance case, it should cover all three rather than the two TypeScript ones.

   Sequencing

   Agreed on keeping this out of #44 — a predicate fix and a failure-model change should not ride together. I would also keep it out of #33's follow-ups: that lock work is landed and this touches a different axis.

   Good issue. The peer-divergence framing is what makes it actionable — two implementations disagreeing about a contract neither side chose is exactly the thing that stays invisible until someone re-derives by hand, as they did.
4. ckcred-alfonso commented on Oct 5, 2026

   

   ckcred-alfonsoBot

   on Oct 5, 2026

   More actions

   Closing as not planned for this code.

   The shared handle manifest this describes is on its way out. Consumers now read by credential id under their own module grant or enrollment token, so there is no shared file for one tenant's bad row to break.

   The OpenCode custody plugin, which still uses the manifest, is being redesigned into a shared custody library, and this failure mode is on that redesign's list: one bad row must not deny every other tenant. The same rule already applies in ck auth, which renders an unknown row as it is instead of refusing the whole table.
5. ckcred-alfonso

   closed this as not plannedon Oct 5, 2026