fix(daemon): fail closed when the auth hook is silent about tenant

[thymikee]

Summary

An auth hook that ran but returned no tenantId opted the deployment into tenant attestation. Both HTTP surfaces the daemon exposes tenant scoping on — /rpc (via body meta.tenantId/flags.tenant) and the auxiliary routes (/upload, /artifacts/*, request diagnostics, via the x-agent-device-tenant header) — silently fell back to trusting the client's own claim, or admitted the request unscoped when the client declared nothing at all. A holder of one valid shared token could declare any tenant id and be treated as that tenant, or omit one entirely and read a tenant-owned session/artifact by naming it directly.

resolveTrustedTenant() (src/daemon/server/tenant-trust.ts) is the single seam both surfaces go through before a request is admitted:

• Hook attests a tenant → use it (unchanged); this also overwrites any mismatched client-declared meta.tenantId/flags.tenant, so neither can survive as a second route to identity.
• No hook configured → keep today's client-declared behavior (loopback/dev unchanged).
• Hook configured but does not attest a tenant → refuse the request outright (401 / JSON-RPC -32001), regardless of what the client does or doesn't declare. A hook that wants any request admitted must attest tenantId on it.

Per the issue's triage comment, no trustClientDeclaredTenant escape hatch was added.

Review history

This went through several rounds of maintainer review that materially changed the design from its first version:

• The trust seam originally only inspected meta.tenantId; a real bypass via flags.tenant (independently read by scopeRequestSession/leaseScopeFromRequest) was found and closed — the flag is now derived and overwritten alongside meta.tenantId.
• The posture originally admitted a request unscoped when the client declared no tenant at all under a silent hook. A real P1 was found: request-diagnostics-http.ts's ownership check and request-admission.ts's lease-field matching only enforce tenant ownership when a tenant is present, so an unscoped caller could read a tenant-owned session/request by naming it directly and omitting any claim. The posture is now unconditional: a hook that doesn't attest means the request is refused, period — not just when the client also makes a claim.
• Comment-policy cleanup (removed narrative comments, test-tour banners, and obvious JSDoc per this repo's convention), operator-facing doc/CHANGELOG wording tightened twice for precision, and a CodeQL code-construction-from-a-variable flag on a test fixture was fixed.

Test plan

• src/daemon/__tests__/http-server-tenant-trust.test.ts: both surfaces (RPC command, RPC lease, aux request-diagnostics + upload routes) across every hook posture — attested, silent-with-a-claim (meta, flags, header), and silent-with-no-claim-at-all — plus regressions proving the no-hook path is unchanged.
• Every rejection case was planted red against the pre-fix code before implementing its fix (TDD), including the flags.tenant bypass and the omitted-tenant/unscoped-access bypass found in later review rounds.
• Full unit-core + fuzz-worker + provider-integration suites pass; tsc and oxlint clean; test/wire-compat ledger updated with a compatibleChanges entry (the changed function reuses the same 401 {ok:false,error,code} shape already sent by this route's other rejection paths, so this is additive, not a protocol bump).
• Ran an 8-angle adversarial code review against the initial diff in addition to the maintainer's own multi-round review.

Fixes #2095

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	2.48 MB	2.48 MB	-272 B
JS gzip	835.0 kB	835.0 kB	+2 B
npm tarball	958.5 kB	958.5 kB	+41 B
npm unpacked	3.32 MB	3.32 MB	-272 B

npm unpacked components

Component	Base	Current	Diff
JS / dist source	2.64 MB	2.64 MB	-272 B
Apple runner source/project	581.1 kB	581.1 kB	0 B
macOS helper source	54.8 kB	54.8 kB	0 B
Android helper artifacts	0 B	0 B	0 B
Other package files	45.4 kB	45.4 kB	0 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	29.8 ms	29.4 ms	-0.4 ms
CLI --help	84.2 ms	84.5 ms	+0.4 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/internal/daemon.js	+649 B	+231 B
dist/src/perf-runtime-plan.js	-648 B	-149 B
dist/src/session2.js	-214 B	-45 B
dist/src/session-store.js	0 B	-5 B
dist/src/registry.js	0 B	+1 B

Top changed packed files

Packed file	Base	Current	Diff
dist/src/internal/daemon.js	107.3 kB	107.9 kB	+649 B
dist/src/perf-runtime-plan.js	64.2 kB	63.6 kB	-648 B
dist/src/session2.js	216.3 kB	216.0 kB	-214 B
dist/src/device-claim-inspection.js	4.2 kB	4.1 kB	-64 B
dist/src/runtime.js	64.8 kB	64.8 kB	+5 B

[thymikee]

Not ready. The fail-closed posture is correct, but the claimed trust seam returns only a boolean, leaving both HTTP callers to promote authResult.tenantId ?? clientDeclaredTenant independently; the whitespace RPC case can still deliver the raw client value downstream. Return the normalized trusted identity/discriminated decision and strip raw client tenant metadata before dispatch, with a regression. Add the required auth-hook migration/CHANGELOG note and remove issue/test-tour narration. Coverage and iOS were still pending during review.

[thymikee]

Addressed:

• Trust seam now returns the resolved identity, not just a boolean. resolveTrustedTenant() is back to a discriminated {trusted: true; tenantId} | {trusted: false}, and both the /rpc handler and authorizeAuxiliaryHttpRequest now assign/return tenantTrust.tenantId directly instead of each re-deriving authResult.tenantId ?? clientDeclaredTenant independently.
• Fixed the whitespace-RPC leak. The gap was real: daemonRequest.meta.tenantId was only ever overwritten when a hook attested a tenant, so a blank/whitespace client-declared value on the "hook configured but silent" path survived into the dispatched request untouched instead of being cleared. The RPC handler now always assigns daemonRequest.meta.tenantId = tenantTrust.tenantId, so the seam's decision is enforced on every trusted path, not just the attested one. Added a regression test (RPC: a whitespace-only meta.tenantId is admitted untenanted, not forwarded raw, under a silent hook) that asserts meta.tenantId is undefined, not the raw ' ' — it was red against the previous code.
• Docs/CHANGELOG. Added an Unreleased CHANGELOG entry and a paragraph in website/docs/docs/security-trust.md describing the new fail-closed posture and what a hook author needs to do (attest tenantId explicitly) if they relied on the old fallback.
• Removed narration. Dropped the file-header docblock in the new test file and the multi-line design-rationale comment on the trust-seam type; the invariant now lives in the function/type names and the tests, per AGENTS.md.
• CodeQL "improper code sanitization" on the attesting-hook test fixture: it built the hook module's source via template-string interpolation of a variable. Both call sites passed the same literal, so I dropped the parameter entirely and hardcoded the value — no more code construction from a variable.

test/wire-compat/ledger.json updated for authorizeAuxiliaryHttpRequest's new digest (this is the second update in this PR since the function's body changed again for the fix above). Full unit-core/fuzz-worker suite (8159 tests), tsc, and oxlint all pass locally.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-28 09:50 UTC

[thymikee]

Comments removed:

• `HttpAuthHookContext.daemonRequest` — dropped the long inline note.
• Test section banners (`// --- RPC surface ---`, etc.) — removed; the test names already carry the grouping.
• `restoreEnv` JSDoc in `src/tests/test-utils/env.ts` — dropped, the signature says it all.

No other new comments in the diff (double-checked with a diff-only scan of every touched file). Digests in `test/wire-compat/ledger.json` are unaffected since comment-only edits don't move them.

Pushed at 3b468c0. Watching CI now.

[thymikee]

The trust seam and regressions are now sound: normalized trusted identity is returned centrally and raw client tenant metadata is overwritten/removed before handling. Still not ready. security-trust.md overstates behavior by saying a configured silent hook refuses the request; code intentionally permits an unscoped request and refuses only a client-declared tenant scope. Correct that operator-facing contract. Remove remaining test-tour banners/obvious helper narration under current comment policy. Exact-head CI is still incomplete.

[thymikee]

The comment-policy cleanup is complete at 3b468c04 without changing the trust semantics or weakening regressions. One blocker remains: security-trust.md still says a configured silent hook refuses the request, but code intentionally admits unscoped requests and refuses only a client-declared tenant scope. Correct that operator-facing contract. Exact-head CI also restarted and is still incomplete. Not ready yet.

[thymikee]

All checks now pass (the iOS smoke-test failure was the dropped-keystroke XCTest flake — confirmed by rerun with no code changes; TEXT_INPUT_COMMIT_NOT_OBSERVED, dispatch chars=17 for a 17-char string but only the first character committed). PR is clean and mergeable at 3b468c04bc.

[thymikee]

Rebased onto latest `main` (was 4454aef, now on 2e87347 — clean rebase, no conflicts, same 8-file diff).

Also fixed the operator-facing contract I'd missed: `security-trust.md` and the CHANGELOG entry both said "a configured silent hook refuses the request," which overstates it. The actual posture is narrower — a request that also declares a client tenant is refused (401); a request that declares none proceeds unscoped, same as before. Reworded both to say that precisely:

a request that also declares a tenant is refused (401) rather than run as whichever tenant the caller claimed, while a request that declares none proceeds unscoped

Pushed at `d49e7e0609`. Watching CI now.

[thymikee]

Not ready at d49e7e0 — P0 tenant-auth bypass remains. The HTTP trust resolver only treats daemonRequest.meta.tenantId as the client tenant claim, but the unchanged request continues downstream and leaseScopeFromRequest() falls back to flags.tenant. With a configured shared-token hook that authenticates but returns no tenantId, a caller can send RPC flags: { tenant: "victim", sessionIsolation: "tenant" }; the request is accepted and executes in the claimed tenant. An isolated exact-head probe expecting 401 instead received 200.

Derive the untrusted tenant claim from both RPC meta and flags.tenant; when the hook is silent, reject either client claim and ensure the flag cannot reintroduce tenant identity downstream. Add a planted red/green regression proving this request is rejected before handleRequest. The current focused suite is green but does not cover the flag path.

[thymikee]

Confirmed and fixed — this was a real bypass. Root cause: the trust seam only ever inspected daemonRequest.meta.tenantId, but scopeRequestSession() (src/daemon/request-admission.ts:24, called on every request via createRequestExecutionScope) and leaseScopeFromRequest() (src/core/lease-scope.ts:80) both independently resolve tenant identity as meta.tenantId ?? flags.tenant. Clearing meta.tenantId to undefined on the "hook silent, no real claim" path did nothing to stop a client from putting the claim in flags.tenant instead — it would sail through the OR-fallback untouched and scopeRequestSession would scope the request as that tenant.

Fix, at src/daemon/server/http-server.ts's RPC handler:

• clientDeclaredTenant is now daemonRequest.meta?.tenantId ?? daemonRequest.flags?.tenant — the exact same precedence the two downstream readers use, so detection can never diverge from what they'd actually resolve.
• After a trusted decision, daemonRequest.flags.tenant (when present) is overwritten to the resolved tenantTrust.tenantId, not just meta.tenantId — so the flag can't carry a different, unattested value past the seam into either downstream reader.

Added a planted red/green regression (RPC: a hook configured but silent on tenant refuses a client-declared flags.tenant) reproducing your exact probe — flags: { tenant: "victim", sessionIsolation: "tenant" } against a silent hook — confirmed 200/admitted before the fix, 401 after. Also added: a hook-attests case proving flags.tenant gets overwritten to the attested tenant (not just meta.tenantId), and a no-hook regression proving the legitimate local-CLI --tenant flag path (flags.tenant with no meta.tenantId) is unaffected.

Aux routes (upload/download/diagnostics) don't carry a flags object in their synthetic request at all, so this specific channel doesn't apply there — only the RPC surface was exposed.

Pushed at e4d23217e0. Full suite green locally (tsc, oxlint, wire-compat ledger unaffected since this touches the RPC dispatcher's anonymous handler body, not a listed declaration). Watching CI now.

[thymikee]

Re-reviewed e4d2321. The confirmed flags.tenant trust-boundary bypass is fixed: silent-hook claims are rejected before dispatch, and attested identity replaces both metadata and an existing tenant flag. Exact-head CI is green; I found no remaining code/security blocker. One P2 remains before readiness: CHANGELOG.md and website/docs/docs/security-trust.md describe the blocked RPC claim only as meta.tenantId, but this head specifically closes the flags.tenant route too. Please name both forms so the operator-facing security/migration contract matches enforcement.

[thymikee]

Fixed — both docs now name `flags.tenant` alongside `meta.tenantId`:

RPC body `meta.tenantId` or `flags.tenant`, or the `x-agent-device-tenant` header on the upload/artifact-download/diagnostics routes ... a request that also declares a tenant through any of those is refused

Pushed at `e4d9a2fe5a`. Watching CI now.

[thymikee]

Re-reviewed e4d9a2fe. The prior documentation P2 is fully fixed: CHANGELOG and security-trust docs now name meta.tenantId, flags.tenant, and the auxiliary header, matching the unchanged enforcement and planted regression. I found no remaining code/docs finding. Exact-head static, compatibility, integration, coverage, CodeQL, size, Linux, macOS, and iOS checks pass. Android Smoke failed in the unchanged system-automation wait for “Alert result: cancelled”; the documentation-only delta has no causal path and the code-identical prior head passed, but this exact head still needs a green rerun before merge readiness.

[thymikee]

Android Smoke rerun is green (98797116829, 6m51s). All checks pass at e4d9a2fe5a, PR is clean and mergeable.

[thymikee]

[P1] A configured-but-tenant-silent auth hook is still bypassable by omitting the tenant claim. resolveTrustedTenant returns trusted/unscoped for that shape (tenant-trust.ts:14-17). Auxiliary diagnostics then receives tenantId: undefined, and request-diagnostics-http.ts:119-129 skips ownership enforcement because it only checks a truthy tenant; a caller with the shared token can omit x-agent-device-tenant and request a known victim-owned session/request path. RPC has the analogous gap: an omitted request tenant leaves session access unscoped, and request-admission.ts:129-155 treats missing request lease fields as matching an owned session. The new tests prove declared-tenant rejection but do not plant omitted-tenant access to owned diagnostics/session red. Fail closed when a configured hook does not attest a tenant, or prove/enforce a public/unowned scope at every route, and add both end-to-end regressions. All checks being green does not make this head ready.

[thymikee]

Confirmed and fixed — you're right that "declares none proceeds unscoped" was itself the hole, exactly as described:

• `request-diagnostics-http.ts:119` (`if (auth.tenantId && !isTenantOwnedSessionName(...))`) skips ownership enforcement entirely when `tenantId` is falsy, so an unscoped caller could read a tenant-owned session's diagnostics by naming it directly, no claim needed.
• `request-admission.ts`'s `assertMatchingLeaseField` (`if (!requestValue || !sessionValue || requestValue === sessionValue) return;`) treats a missing request-side lease field as matching whatever the session already has, so an unscoped RPC request against an existing tenant-owned session's lease fields sails through the same way.

I took the "fail closed when a configured hook does not attest a tenant" option rather than auditing/hardening every downstream ownership check individually — it's the smaller, single-point fix that closes both named paths at the actual trust seam instead of chasing every place that conditions enforcement on a truthy tenant:

`resolveTrustedTenant()` no longer has an "admit as unscoped" branch. The posture collapses to three cases: hook attests → use it; no hook → keep today's client-declared behavior (unchanged); hook configured and doesn't attest → refuse (401), full stop, independent of whatever the client does or doesn't declare. `clientDeclaredTenant` is now only consulted in the no-hook branch.

Added two planted red/green regressions matching your exact examples:

• RPC `agent_device.command` with no `meta.tenantId` and no `flags.tenant` at all, under a silent hook → confirmed 200/admitted before, 401 after.
• Diagnostics read of a known `victim-tenant:default` session with the `x-agent-device-tenant` header omitted entirely, under a silent hook → confirmed 200/leaked before, 401 after.

One existing test (`RPC: a whitespace-only meta.tenantId is admitted untenanted...`) asserted the old "admit unscoped" outcome for a blank claim; updated to expect 401, since that outcome no longer exists under a silent hook. Full suite (14/14 in the tenant-trust file, all other daemon/wire-compat/provider-integration suites) green; `tsc`/`oxlint` clean.

Updated CHANGELOG and security-trust.md to state the posture is now unconditional (hook must attest `tenantId` on every request it wants admitted), and updated the PR description to walk through the full review-driven design history for anyone picking this up cold.

Pushed at `423cd18232`. Watching CI now.