Skip to content

DRAFT: contracts 0.10.0 upgrade — deny-path fix BLOCKED (verified.agent absent on success path) + upgrade breaks check:contracts - #199

Draft
andrei-hasna wants to merge 1 commit into
mainfrom
fix/14471bf9-contracts-0.10.0-deny-path
Draft

DRAFT: contracts 0.10.0 upgrade — deny-path fix BLOCKED (verified.agent absent on success path) + upgrade breaks check:contracts#199
andrei-hasna wants to merge 1 commit into
mainfrom
fix/14471bf9-contracts-0.10.0-deny-path

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

DRAFT — NOT LANDABLE AS-IS. Checks are red on purpose. Refs todos 14471bf9 (upgrade lane) and 65b71f2e (the defect).

This lane was to upgrade @hasna/contracts 0.5.2 -> 0.10.0 and land the one-line deny-path fix. The upgrade is here. The fix is not, and the reason is a measured contradiction of the remedy itself.

1. The briefed remedy does not exist on the success path

The instruction was to read verified.agent instead of verified.claims.agent at src/lib/auth/tenant-auth.ts:113, with an explicit stop condition: confirm the field is present in the installed artefact; if it is absent, stop and report rather than hand-rolling a guard.

Measured in the installed node_modules/@hasna/contracts 0.10.0 — not the changelog. agent is on the failure branch only:

export type ApiKeyVerifyResult = {
    ok: true;  claims; kid; app;  tid: string | null;          <-- no agent
} | {
    ok: false; reason; message; kid?; tid?;  agent?: string | null;
};

dist/auth/index.js computes exactly the right guard and attaches it only to denials:

const agent = Object.hasOwn(claims, "agent") && typeof claims.agent === "string" ? claims.agent : null;
...
return { ok: false, reason: "insufficient_scope", ..., agent };  // failure: carries it
return { ok: true, claims, kid: claims.kid, app, tid };          // success: does not

Line 113 sits after if (!verified.ok) return ..., so verified is narrowed to the one branch with no agent.

Two-sided runtime probe against the installed package:

SUCCESS(no agent claim)     ok=true  keys=["ok","claims","kid","app","tid"] hasOwn(agent)=false value=undefined
SUCCESS(with agent claim)   ok=true  keys=["ok","claims","kid","app","tid"] hasOwn(agent)=false value=undefined
CONTROL FAILURE(with agent) ok=false reason=insufficient_scope hasOwn(agent)=true  value=principal-a
CONTROL FAILURE(no agent)   ok=false reason=insufficient_scope hasOwn(agent)=true  value=null
NAIVE `verified.agent` on success path === 'principal-a' ? false
POLLUTED verified.agent on success path = principal-a

The control arm reads the same field with the same expression where the type says it is present, and finds it — so "absent on success" is a fact about the package, not a broken probe.

The briefed one-liner would fail in both directions, and the second is the serious one:

  1. It denies all legitimate traffic. On success verified.agent is undefined, so undefined !== row.principal_id is always true and every valid request becomes key_record_mismatch.
  2. It does not stop the attack. Because agent is not an own property of the success object either, verified.agent also falls through to Object.prototype — last probe line above. Under the exact pollution this lane exists to close, the "fixed" expression returns the attacker's value and the deny still does not fire. It swaps one prototype-readable expression for another.

So the stop condition fired and no guard was invented here. The real remedy is a decision for the defect's owner: either @hasna/contracts wires the existing line-237 guard into the success result (and into the principal at line 637, which a parallel census found still unguarded, plus five unguarded audit emit calls at 598/611/620/629/641), or open-loops applies its own Object.hasOwn guard at the call site — which is exactly what the brief told me not to do unilaterally.

2. The upgrade is not additive — it breaks check:contracts

gate rc result
bun run typecheck 0 clean
bun test --timeout 120000 (full) 1 1151 pass, 57 skip, 3 fail — 1211 tests, 76 files, 553s
bun run check:contracts 1 manifest validation fails

Failures:

(fail) Loops repository contract conformance > passes official bin conformance without a loops-api compatibility waiver
(fail) Loops repository contract conformance > leaves an unwaived extra package bin fatal
(fail) tenant API authentication > a polluted Object.prototype.agent cannot satisfy the principal binding

The third is the new regression test and is expected to fail here — the fix is absent. The first two are caused by the upgrade, established with a controlled pair: a second worktree at the same base sha 8691b69, differing only in the dependency version.

BASELINE (0.5.2)  check-contract-conformance.test.mjs -> rc=0   5 pass  0 fail
BASELINE (0.5.2)  bun run check:contracts             -> rc=0
UPGRADED (0.10.0) same file                            -> 2 fail
UPGRADED (0.10.0) bun run check:contracts             -> rc=1

Gate output:

Service contract manifest failed validation: storage.backend Required;
storage Unrecognized key(s) in object: 'mode';
serviceSurfaces.0 Unrecognized key(s) in object: 'deploymentModes';
<root> Unrecognized key(s) in object: 'deploymentModes'

This is the deployment-modes removal landing: 0.10.0 is the hotfix that makes a manifest carrying deploymentModes fail validation. hasna.contract.json here still has the old shape. The migration is mechanical — enum read from the installed artefact, SERVER_DATA_BACKENDS = ["sqlite","postgresql"]:

  1. drop root deploymentModes
  2. storage.mode: "local" -> storage.backend: "sqlite" (keep sqlitePath; required when backend is sqlite)
  3. drop serviceSurfaces[0].deploymentModes

I did not make that change. It is a semantic migration of a published contract manifest, governed by its own directive, and landing it inside a security lane whose fix is blocked would put an unreviewed vocabulary migration under a misleading title. It wants its own row.

What is in this branch

  • @hasna/contracts 0.5.2 -> 0.10.0 (package.json, bun.lock)
  • two tests in src/lib/auth/tenant-auth.test.ts: the pollution reproduction (currently failing — that is the open vulnerability) and an other-direction guard asserting a real mismatch still denies and a real match still allows, which already passes and would catch a fix that denied everything

The reproduction was verified to fail before any change, on unmodified main@8691b69: expect(decision.ok).toBe(false) / Expected: false, Received: true. A test that only passes afterwards would prove nothing here.

Severity

Confirmed but not currently reachable in the loops verifier: no global prototype-pollution sink exists in that process and bun audit reports no such advisory. This is a latent weakening of defence-in-depth, not a live break — priority should reflect that.

Quarantine untouched; no exclude list edited. @hasna/contracts@0.10.0 installs cleanly through the arborist/bun path.

Agent: Polybius


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…ssion for the deny-path prototype read

WIP / NOT LANDABLE AS-IS. Two blockers, both measured; see the PR body.

Carries:
  - @hasna/contracts 0.5.2 -> 0.10.0
  - the prototype-pollution regression test for the `key_record_mismatch`
    deny path (tenant-auth.ts:113), which FAILS on this branch because the
    fix is NOT here

Deliberately NOT carried:
  - the deny-path fix. The briefed remedy was "read verified.agent instead
    of verified.claims.agent". Measured against the installed 0.10.0
    artefact, `agent` is on the FAILURE branch of ApiKeyVerifyResult only;
    the ok:true branch is {ok, claims, kid, app, tid}. The brief's own stop
    condition ("if it is absent after the upgrade, STOP and report rather
    than hand-rolling a guard") therefore fires, so no guard was invented.
  - the hasna.contract.json migration that 0.10.0 now forces. That is the
    deployment-modes removal landing in this repo and it wants its own row.

Refs: 14471bf9, 65b71f2e

Agent: Polybius
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant