fix(security): honor the SQL permission denial processAST computes by dawsontoth · Pull Request #2202 · HarperFast/harper

dawsontoth · 2026-08-18T16:56:02Z

Split out of #2173 at a reviewer's request — the defect is pre-existing and affects all SQL authorization, not just that feature.

The bug

sqlTranslator/index.ts guarded its permission check with:

let permissionsCheck = checkASTPermissions(jsonMessage, parsedSqlObject);
if (permissionsCheck && permissionsCheck.length > 0) {

checkASTPermissions returns either null or a PermissionResponseObject. That class has error, unauthorized_access, and invalid_schema_items — no length. So the condition evaluated undefined > 0, was always false, and the denial was computed correctly and then dropped on the floor.

Why nobody noticed

This branch is normally the second permission check, not the first. A direct sql call reaches processAST with permissions_checked already true, set by chooseOperation — whose own consumer of the same function uses a correct bare truthiness test (server/serverHelpers/serverUtilities.ts). So in the common path the branch never runs.

It runs when something re-parses a statement: a job dispatches its nested search_operation, which carries no parsed_sql_object, so evaluateSQL re-parses with permissions_checked === false. That is precisely the path with no outer gate behind it.

The fix

A bare truthiness test, matching the identical consumer in serverUtilities.ts.

Tests

unitTests/sqlTranslator/processASTPermissions.test.js drives processAST directly rather than asserting that a denial is merely computed — which is how a dead consumer went unnoticed in the first place. Four cases:

a denied statement comes back 403 with the permission response
the response object survives intact for the caller to render
an allowed statement is still allowed (this must not start denying what was always permitted)
an already-checked statement still skips the branch

The two negative cases were confirmed to fail against the old guard before being kept (2 passing / 2 failing when reverted, 4 passing with the fix).

Scope

No behavior change for any statement that was correctly authorized. The change is that a denial computed on the re-parse path is now acted upon instead of ignored.

🤖 Generated with Claude Code

`processAST` guarded its permission check with `permissionsCheck && permissionsCheck.length > 0`, but `checkASTPermissions` returns either null or a `PermissionResponseObject` — a class with no `length`. So the test evaluated `undefined > 0`, was always false, and every denial reaching that branch was computed correctly and then discarded. This survived because the branch is normally the second check rather than the first: a direct `sql` call arrives with `permissions_checked` already true from `chooseOperation`, whose own guard (`if (astPermCheck)`) is correct. The branch only executes when something re-parses a statement — a job dispatching its nested `search_operation` — which is exactly the path with no outer gate behind it. Now a bare truthiness test, matching the identical consumer in serverUtilities.ts. Tests drive processAST directly, including the already-checked and allowed paths so this cannot start denying statements that were always permitted; the two negative cases were confirmed to fail against the old guard. Found while reviewing #2173, which does not depend on this: its own gate refuses the export/write combination at the front door. Split out because the defect is pre-existing and affects all SQL authorization, not just that feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

A reviewer pointed out the dead `permissionsCheck.length > 0` guard is pre-existing and affects all SQL authorization, not just this feature, and asked for it as its own change with its own coverage rather than bolted onto an auth PR. Agreed — it is now #2202, with tests that drive processAST directly and cover the allowed and already-checked paths too, so it cannot start denying statements that were always permitted. This PR does not depend on it. The outer gate in serverUtilities refuses an out-of-scope job operation and sqlWriteScopeDenial refuses write SQL, both through correct truthiness tests, so export_local + DELETE is already refused at the front door. Left a note at the call site pointing at #2202 so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request fixes a critical authorization bypass bug in sqlTranslator/index.ts where permission denials were being discarded because the code checked for a .length property on a PermissionResponseObject (which does not exist, causing the check to always evaluate to false). The fix replaces this check with a simple truthiness check on permissionsCheck. Additionally, a comprehensive suite of unit tests has been added in unitTests/sqlTranslator/processASTPermissions.test.js to verify the correct behavior of processAST under various permission scenarios. There are no review comments, and we have no additional feedback to provide as the changes are correct and well-tested.

claude · 2026-08-18T16:58:12Z

Reviewed; no blockers found.

…body `checkASTPermissions` resolved the token-scope operation as `jsonMessage.api_operation ?? jsonMessage.operation`. On the direct-SQL path `jsonMessage` IS the client's request body, and that check is the ONLY gate there — the `sql` branch of chooseOperation is mutually exclusive with its verifyPerms call. So a caller could send `{operation: 'sql', sql: '...', api_operation: '<whatever their scope allows>'}` and run arbitrary SQL under it. Reproduced against this branch; the regression test was confirmed to fail before the fix. I introduced this in 11f2280, carrying a job's real operation to the nested check on a request property. That is reverted. The operation now comes from an explicit argument or the dispatched `json.operation`, never from a field on the message — chooseOperation passes the operation it already resolved. Stripping `api_operation` at the ingress points was the first fix I tried, and it is the wrong shape: it leaves the check trusting a body property and makes safety depend on every current and future entry point remembering to strip. The property is gone instead. The trade is that a job's SQL is checked as `sql` rather than as `export_local`. That changes no outcome today, because the branch in processAST that would act on the denial is dead — PermissionResponseObject has no `length`, so its guard never fires (#2202). When #2202 makes that branch live it needs a carrier for the job's operation that a client cannot forge; a request property is not one, however carefully it is stripped. Recorded at both sites so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

dawsontoth · 2026-08-18T17:07:36Z

Moving this to draft. It is a real pre-existing defect and the fix stands, but it touches shared SQL authorization for every caller, not just the OIDC feature, so it needs the full harper-engineering-guidelines treatment on its own — cross-model reviews with the coverage reported in the description — rather than inheriting #2173's. That will be picked up separately.

Nothing in #2173 depends on this landing: its own gate refuses the export/write combination at the front door, and it no longer attempts to carry a job's operation into the branch this fixes.

One thing for whoever picks this up: making the processAST branch live means a job's SQL will be authorized as sql rather than as its own operation (export_local), so a token scoped only to the job operation would be denied by its own job. Carrying the real operation on the request body was tried in #2173 and reverted — on the direct-SQL path that body is client-supplied and this check is the only gate, so any property it consults is forgeable. That carrier needs to be something a client cannot set.

🤖 Generated with Claude Code

Reverting the processAST guard to #2202 removed the one test that asserted this invariant is ENFORCED rather than merely computed, and the safety argument now rests entirely on chooseOperation's front-door gate — which had no enforcement test of its own. The rest of the scope suite only checks that verifyPermsAST returns a denial object, which is exactly how a dead consumer goes unnoticed. Three cases on the real dispatch path: an export job carrying nested write SQL outside the scope throws 403, an export whose own operation is outside the scope throws 403, and an in-scope export still runs — the last so this cannot pass by refusing everything. Confirmed they fail when the front-door gate is given the same dead-guard shape (`astPermCheck && astPermCheck.length > 0`) that made the inner branch a no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…g in a comment The interaction between this PR and #2202 was documented only in prose, and the two can merge in either order. Removing the forgeable operation carrier leaves checkASTPermissions falling back to jsonMessage.operation, which at the processAST call site is the nested search_operation's own `sql` — so once #2202 makes that branch live, an export_local-scoped token 403s on its own export job. Added a tripwire that drives evaluateSQL with the exact shape export.ts:363 dispatches and asserts an in-scope export is not refused by the permission gate. It passes today and was confirmed to fail with #2202's one-line change applied on top, so whichever PR lands second turns CI red rather than shipping a silently broken feature. The comment on it says what to do when it fires — supply the job's real operation through a carrier a client cannot set, rather than relaxing the scope check. Preferred this over making apiOperation a required parameter: that turns the missing carrier into a compile error the next author satisfies by passing jsonMessage.operation, which is the wrong value and compiles clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

dawsontoth · 2026-08-18T18:04:45Z

Ordering note for whoever picks this up — #2173 now carries a test that will go red when this lands, deliberately.

Making the processAST branch live means a job's SQL is authorized as sql rather than as its own operation, because checkASTPermissions falls back to jsonMessage.operation and at that call site jsonMessage is the nested search_operation. So a token scoped to export_local would be denied by its own export job. Verified by applying this PR's diff on top of #2173 and rebuilding.

The failing test is admits an in-scope export job through the path export.ts actually dispatches in unitTests/security/tokenOperationScope.test.js. The fix is not to relax the scope check or delete the test — it is to give the job's real operation a carrier a client cannot set. A request-body property is not one: that was tried in #2173 and reverted, because on the direct-SQL path the body is client-supplied and that check is the only gate, so any property it consults is forgeable.

The two PRs can merge in either order; whichever is second goes red rather than silently shipping an export-scoped token that 403s on its own export.

🤖 Generated with Claude Code

The tripwire compared against 403, which is UNAUTHORIZED_RESPONSE in the very file it exists to watch — so changing that constant would leave it green while the refusal it guards against still happened. A tripwire must not depend on a constant its own target owns. Now asserted by shape: the permission path is the only one that calls back with a bare numeric status, while every other failure forwards an Error. evaluateSQL drops the second callback argument on error, so the denial object never reaches the test and the number is the whole signal — which also rules out asserting on the PermissionResponseObject shape directly. Verified across four states: passes today, fails with #2202's guard applied, still fails with #2202 applied AND the status changed to 401 (the case the old assertion missed), and passes again restored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

A reviewer pointed out the dead `permissionsCheck.length > 0` guard is pre-existing and affects all SQL authorization, not just this feature, and asked for it as its own change with its own coverage rather than bolted onto an auth PR. Agreed — it is now #2202, with tests that drive processAST directly and cover the allowed and already-checked paths too, so it cannot start denying statements that were always permitted. This PR does not depend on it. The outer gate in serverUtilities refuses an out-of-scope job operation and sqlWriteScopeDenial refuses write SQL, both through correct truthiness tests, so export_local + DELETE is already refused at the front door. Left a note at the call site pointing at #2202 so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…body `checkASTPermissions` resolved the token-scope operation as `jsonMessage.api_operation ?? jsonMessage.operation`. On the direct-SQL path `jsonMessage` IS the client's request body, and that check is the ONLY gate there — the `sql` branch of chooseOperation is mutually exclusive with its verifyPerms call. So a caller could send `{operation: 'sql', sql: '...', api_operation: '<whatever their scope allows>'}` and run arbitrary SQL under it. Reproduced against this branch; the regression test was confirmed to fail before the fix. I introduced this in 11f2280, carrying a job's real operation to the nested check on a request property. That is reverted. The operation now comes from an explicit argument or the dispatched `json.operation`, never from a field on the message — chooseOperation passes the operation it already resolved. Stripping `api_operation` at the ingress points was the first fix I tried, and it is the wrong shape: it leaves the check trusting a body property and makes safety depend on every current and future entry point remembering to strip. The property is gone instead. The trade is that a job's SQL is checked as `sql` rather than as `export_local`. That changes no outcome today, because the branch in processAST that would act on the denial is dead — PermissionResponseObject has no `length`, so its guard never fires (#2202). When #2202 makes that branch live it needs a carrier for the job's operation that a client cannot forge; a request property is not one, however carefully it is stripped. Recorded at both sites so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Reverting the processAST guard to #2202 removed the one test that asserted this invariant is ENFORCED rather than merely computed, and the safety argument now rests entirely on chooseOperation's front-door gate — which had no enforcement test of its own. The rest of the scope suite only checks that verifyPermsAST returns a denial object, which is exactly how a dead consumer goes unnoticed. Three cases on the real dispatch path: an export job carrying nested write SQL outside the scope throws 403, an export whose own operation is outside the scope throws 403, and an in-scope export still runs — the last so this cannot pass by refusing everything. Confirmed they fail when the front-door gate is given the same dead-guard shape (`astPermCheck && astPermCheck.length > 0`) that made the inner branch a no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…g in a comment The interaction between this PR and #2202 was documented only in prose, and the two can merge in either order. Removing the forgeable operation carrier leaves checkASTPermissions falling back to jsonMessage.operation, which at the processAST call site is the nested search_operation's own `sql` — so once #2202 makes that branch live, an export_local-scoped token 403s on its own export job. Added a tripwire that drives evaluateSQL with the exact shape export.ts:363 dispatches and asserts an in-scope export is not refused by the permission gate. It passes today and was confirmed to fail with #2202's one-line change applied on top, so whichever PR lands second turns CI red rather than shipping a silently broken feature. The comment on it says what to do when it fires — supply the job's real operation through a carrier a client cannot set, rather than relaxing the scope check. Preferred this over making apiOperation a required parameter: that turns the missing carrier into a compile error the next author satisfies by passing jsonMessage.operation, which is the wrong value and compiles clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

The tripwire compared against 403, which is UNAUTHORIZED_RESPONSE in the very file it exists to watch — so changing that constant would leave it green while the refusal it guards against still happened. A tripwire must not depend on a constant its own target owns. Now asserted by shape: the permission path is the only one that calls back with a bare numeric status, while every other failure forwards an Error. evaluateSQL drops the second callback argument on error, so the denial object never reaches the test and the number is the whole signal — which also rules out asserting on the PermissionResponseObject shape directly. Verified across four states: passes today, fails with #2202's guard applied, still fails with #2202 applied AND the status changed to 401 (the case the old assertion missed), and passes again restored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ial (#2173) * feat(security): OIDC identity token verification for trusted publishing Core of #2171: verify a CI identity token against an issuer's published signing keys and match it to a trust policy's claim constraints. No storage or operation wiring yet — this is the layer those sit on. security/oidcTrust/claims.ts is pure: claim normalization (deriving workflow_path from workflow_ref so a tag release can pin the workflow file without knowing the tag), exact/any-of matching that denies on an absent claim, and write-time validation requiring a repository pin, a workflow pin, and a ref-or-environment gate. That last requirement is what npm's repository+filename model lacks: without it, anyone who can push a branch can add the trusted workflow to it and mint a token. security/oidcTrust/jwks.ts fetches keys with the conservatism the unauthenticated exchange endpoint demands: https only, bounded body and time, discovery-issuer cross-check, asymmetric keys only, concurrent loads collapsed, and a rate limit on the refetch an unrecognized kid triggers. The rate-limit clock is kept outside the cache entry so a successful fetch does not reset it — and so a genuine key rotation is picked up on first use rather than after the window. security/oidcTrust/index.ts verifies signature, issuer, and audience, and additionally requires exp, a bounded lifetime, and jti (a token that cannot be identified cannot be replay-protected). Rejection reasons go to the log, never to the caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(security): hdb_oidc_trust table and trust policy operations Storage and administration for #2171: add_oidc_trust / list_oidc_trust / drop_oidc_trust over a new system.hdb_oidc_trust table, following the three-touchpoint pattern DESIGN.md documents for a new system table (schema entry, SYSTEM_TABLE_NAMES, upgrade directive). The directive is tagged 5.3.0 to match the release that ships these operations — a later tag would never fire on the upgrade path and leave the table missing. A policy names a Harper user and deliberately carries no operation allowlist of its own: least privilege is that user's role, and a second authorization mechanism running alongside roles is one more place for the two to disagree. Notes for review: - add_oidc_trust rejects an issuer's default audience (https://github.com/<owner>), which every repository under an owner shares. Accepting it is the one configuration mistake that makes the audience check meaningless. - User existence is checked against the users cache, not findAndValidateUser: with validatePassword false that returns a bare { username } for an unknown user, so it cannot answer the question. - Handlers enforce super_user directly as well as via requiredPermissions, matching secretOperations — a role's `operations` allowlist can otherwise delegate an SU-only operation. - Naming a super_user returns a warning rather than an error. An admin may mean it; it just should not be silent. - The ops join secrets in the MCP DEFAULT_EXCLUDED set. list_oidc_trust matches the `list_*` glob, and the policy set names exactly which repository and workflow are worth compromising. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(security): exchange_oidc_token — mint a token from a CI identity Completes the server half of #2171. A runner posts its provider's identity token; if it verifies against an enabled trust policy, Harper returns a one-hour operation token for the user that policy names. The operation is unauthenticated because it *is* the authentication, the same way create_authentication_tokens is against a password. createOperationToken is new in tokenAuthentication.ts because createTokens could not be used: it overwrites hdb_user.refresh_token as a side effect, so minting for CI would silently revoke whatever credential that user already held (#2018) — the exact problem this feature exists to remove. Notes for review: - Every rejection returns the same message and status; the reason goes to the log. The endpoint is unauthenticated, so a caller told which check failed can enumerate a policy one claim at a time. - Replay: hdb_oidc_token_use records issuer|jti with expiresAt set past the token's own expiry, so it stays proportional to in-flight tokens. The get-then-put is not atomic and does not pretend to be — see the comment on getTokenUseTable for why the concurrent race is tolerable (it is not a privilege escalation) and what it does stop. - The use is recorded *before* minting. A failure after recording costs a CI re-run; the reverse ordering would leave a spendable token behind. - The user is resolved before the token is spent, so a policy naming a deleted or deactivated user fails without burning a token the runner cannot re-mint. Deactivating a user stops its workflows. - Policy selection iterates enabled policies for the token's issuer, first match by id. Signature verification is memoized per audience so N policies sharing one audience cost one verification. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(cli): exchange a CI identity for a Harper token automatically Completes #2171. On a runner that offers an OIDC identity token, the CLI asks the provider for one addressed to this instance and trades it via exchange_oidc_token — so a GitHub Actions deploy needs `id-token: write` and a target URL, and no secret at all. Ranked below every configured credential (env tokens, saved login), not above. Adding `id-token: write` to a workflow that still sets HARPER_CLI_REFRESH_TOKEN must not silently change which identity deploys; the ambient credential is the fallback, not the override. Notes for review: - The audience sent to GitHub is the resolved target, not the provider default — that default is shared by every repository under an owner, and is what makes a token replayable at an unrelated service. - Detection requires BOTH ACTIONS_ID_TOKEN_REQUEST_URL and _TOKEN. Their absence means the workflow did not grant `id-token: write`, which is an answer rather than a failure to report. - Failures are reported and swallowed. This is the last credential source before the request goes out unauthenticated, and the resulting 401 says nothing useful, so a 401 from the exchange prints what the operator can actually inspect (list_oidc_trust, the audience). - Local (no-target) operations never reach the exchange, same as the env-var tokens: bypassLocalAuth only applies with no Authorization header, so attaching one opts out of the domain socket's trust. DESIGN.md gains a section on the layering and the three constraints that look like choices but are not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(security): cut comments the code can carry itself Self-audit pass. No behavior change — 196 tests unchanged and green. Structural, not just prose: - claims.ts folded three copy-pasted structural checks into one STRUCTURAL_REQUIREMENTS table. The table shows the symmetry that a paragraph previously had to assert, and the three exported constant arrays it replaces were exported but imported nowhere, not even by tests. - Deleted describeUnpinned(), a wrapper around Array#join whose name described something it did not do. - listOidcTrust and loadEnabledPolicies were the same scan-toRecord-sort with one filter differing; both now call readPolicies(includeDisabled). - rejectToken moved to identityToken.ts and is shared with the exchange, replacing a second near-identical rejectExchange — and with it the fourth copy of the "reason goes to the log, not the caller" rationale. - findMatchingPolicy lost a try/catch and a `void error` to a .catch(). - security/oidcTrust/index.ts is now identityToken.ts. It was never a barrel, so `from './index.ts'` misdescribed what siblings were importing; the name now matches its test file. Three rationales were each told in three or four files (audience must be instance-specific, rejection reasons stay in the log, least privilege is the named user's role). Each now has one canonical site with the code that enforces it, and pointers elsewhere. Across security/oidcTrust/ plus bin/ciIdentityToken.ts: 1040 -> 939 lines, of which comment lines 310 -> 225 (29% -> 23%). What stayed is the non-obvious: why splitting on `@refs/` rather than `@`, why the rate-limit clock sits outside the cache entry, why createTokens cannot be used here, and why ref_type is not a ref gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): keep identity and refresh tokens out of the operations log processLocalTransaction logs every operation body at INFO — a common default level — after stripping a fixed field list. `token` was not on it, so exchange_oidc_token wrote the raw CI identity JWT verbatim on every call. The log happens before the handler, so a *rejected* attempt logged an unspent, still-usable credential. Caught in review by claude[bot] on #2173. Two adjacent fields had the same gap and are fixed here too, since it is the same list and the same class of bug: - `token` also carries the login-purpose token (login, #1876). - `refresh_token` carries the 30-day credential (refresh_operation_token) — pre-existing, and the longest-lived of the three. The inline rest-destructure became `redactForOperationLog` + `UNLOGGABLE_OPERATION_FIELDS`. That is not tidying: `operationLog` is built from mainLogger at module load, so the logged body cannot be intercepted after the fact, and the existing redaction test guards on `if (info_log_stub.called)` — which is never true in the unit environment, so it has been passing vacuously. Exporting the list and the function makes the contract directly testable, and drops an eslint-disable for unused vars along the way. Five tests cover it, including one pinning each credential-bearing field in the list so a refactor cannot quietly drop one the way harper#1527 did for set_env_value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(security): make the OIDC core issuer-agnostic, GitHub a profile Addresses @heskew's structural review on #2173. No change to the hardening; this is about where the issuer-specific parts live so the layer can seed an authn core rather than a GitHub feature. - security/oidcTrust/ -> security/authn/oidc/, and the CLI's ciIdentityToken.ts -> workloadIdentity.ts, structured as a provider list (GitHub Actions is entry one; a Kubernetes entry is available() testing for a projected token path and requestToken() reading it). - providers/githubActions.ts now owns everything GitHub-shaped: the three pin requirements, workflow_path derivation, the shared-default audience regex, and principal description. Nothing else in the module says GitHub. The ref-gate rule — the part flagged as most likely to be wrong — is right for GitHub and now cannot constrain any other issuer. - providers/generic.ts is the fallback for unregistered issuers, and is strict rather than permissive: the policy must pin `sub`. That makes Kubernetes service accounts, GCP service accounts, and SPIFFE SVIDs work with zero provider code, all of which have stable canonical subjects. GitHub needs a profile precisely because its `sub` is the one claim not to pin. - The GitHub profile default-denies `pull_request_target` unless a policy constrains event_name, closing the fork callout in #2171. A plain pull_request run from a fork already cannot mint (no id-token: write); pull_request_target can. - Replay is keyed on SHA-256 of the token rather than issuer|jti, and verifyIdentityToken no longer requires jti. Azure emits `uti` and others omit it; a replayed token is byte-identical by definition, so this is strictly more general. Hashed, so the table never holds a credential. - Exchanges now emit AuthAuditLog on success and failure, through the same stream and the same logging.auditAuthEvents switches as every other authentication event. serverHandlers already injects baseRequest for NO_AUTH_OPERATIONS, so ip/method/path are available — the TODO is gone rather than deferred. claims.ts keeps only issuer-agnostic matching and constraint-shape validation; validateTrustPolicyClaims split into that plus the profile's assertPolicyIsSpecific. Tests restructured to match: provider profiles get their own suites, claims.test.js uses a deliberately non-GitHub token, and the exchange suite gains a second issuer with no profile to prove the zero-provider- code path end to end. 438 green across the touched suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: remove the JWT keys the exchange suite writes, and share the helper tokenExchange.test.js wrote real signing keys into the test base path and never removed them. That path is shared: unitTests/utility/install/checkJWTTokensExist.test.js asserts those files are ABSENT (its happy path expects accessSync to throw ENOENT), and mocha runs every file in one process against one base path — so whichever ran first decided whether the other passed. It has been latent here. It surfaced on the stacked branch (#2174) when a second file started writing the same keys, failing that suite on all three Node versions; the same landmine was already sitting on this branch waiting for a file-order change. The fix is a testUtils.installTestJwtKeys() that returns a cleanup function, so the next test needing signing keys gets the removal for free rather than copying the setup and not the teardown. Verified by running the exchange suite and checkJWTTokensExist together, which failed before and passes now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(security): narrow a minted token to a subset of its user's operations Stacked on feat/oidc-trusted-publishing. Explores the per-policy operation scoping Kris asked about — with the constraint that makes it safe, which is the reason it is a separate PR rather than part of #2173. An OIDC trust policy may carry `operations`. The exchanged token then carries that list as a claim, and verifyPerms intersects it with the user's role. One Harper user can back several workflows, each holding a credential narrower than the user itself. It can only ever subtract. Two things make that true, and both are the whole point: 1. The check is the FIRST authorization step in verifyPerms. Both the super_user bypass and the `operations` gate-2 grant return null early, so a narrowing check after either would be bypassable by exactly the identities it most needs to constrain. Tested directly: a super_user token scoped to get_status cannot insert. 2. The scope is never merged into role.permission.operations. That field is not purely narrowing — gate 2 treats an explicit listing of an SU-only operation as a deliberate grant — so merging into it could widen instead of narrow. It travels on the user as `tokenOperations` and is intersected separately. Absent claim means today's behavior exactly, so every existing token and every unscoped policy is unaffected. Operation names are validated at write time against OPERATIONS_ENUM (groups expanded first): a typo would otherwise fail closed at request time, in CI, with nothing to point at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): carry an empty operation scope instead of dropping it Review feedback from gemini-code-assist on #2174; all four points were correct. The one that mattered: `createOperationToken` gated the claim on `user.operations?.length`, so an EMPTY scope — meaning "no operations" — was omitted from the payload entirely. The minted token then looked unscoped, verifyPerms skipped narrowing, and the holder got everything its role allowed. A security control failing open, and in the one direction that matters. add_oidc_trust rejects an empty array (Joi .min(1)), so this is not reachable through the documented API. It is reachable by a row arriving through replication from a peer, which is the same path matchTrustPolicyClaims already backstops against — a control must fail closed regardless of how the input got there. Also: - verifyPerms used `!== undefined` where an unscoped policy stores `operations: null`; expanding null would throw rather than fall through to the role. Now `!= null`, which is also the repo's documented idiom (.gemini/styleguide.md). - Operation-name validation delegates to validateOperations instead of a local OPERATIONS_ENUM check. That helper also accepts operations registered at runtime via server.registerOperation, which the local check would have rejected — so a policy could not scope to a dynamically registered op. Three tests added, one per failure mode. The empty-scope test is a round trip through createOperationToken rather than a verifyPerms unit check: the existing suite passed `[]` straight to verifyPerms and denied correctly, which is exactly why it missed a mint path that never emitted the claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): apply the token operation scope to the SQL path too Caught in review by claude[bot] on #2174, and it falsified the PR's central claim. chooseOperation dispatches `operation === 'sql'` to verifyPermsAST, in a branch mutually exclusive with the verifyPerms call — and the narrowing gate lived only in verifyPerms. So a token scoped to, say, `operations: ['get_status']` could send {"operation":"sql","sql":"DELETE FROM ..."} and run arbitrary SQL against whatever its role could reach. verifyPermsAST also returns null unconditionally for a super_user, so the identity most needing the constraint was the least constrained. The gate is now a shared tokenScopeDenial() called first by BOTH entry points, rather than a second copy in verifyPermsAST. The lesson of the bug is that a check living inside one of two mutually exclusive branches is one refactor away from being skipped, so the comment enumerates all three early-return paths that bypass it if it ever moves. On the AST path the scope is checked against `sql` — the operation the caller actually invoked — because verifyPermsAST's `operation` parameter is the statement variant (select/insert/...), not the API name. It runs ahead of AST parsing as well as the super_user bypass: an out-of-scope request should not get its SQL parsed at all. Five tests on the SQL path. Verified they fail without the fix (2 failing) and pass with it, so they pin the hole rather than describing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(security): carry the token operation scope across credential minting A cross-model review (codex + gemini) found a fourth bypass of the token operation scope, the same authz-escape class as the earlier SQL-path hole: the scope is only enforced inside verifyPerms/verifyPermsAST, but three operations PRODUCE a new credential or principal and dropped it. - create_authentication_tokens (the headline path): it is in NO_AUTH_OPERATIONS, so verifyPerms — and the scope gate inside it — never runs. A token scoped to e.g. deploy_component could call it with no username/password and receive fresh, UNSCOPED operation + refresh tokens for its own user: full-role escalation. - refresh_operation_token: dropped the operations claim when re-signing. - impersonation: enforceDowngrade bounds the impersonated role's perms but shed the token scope, so a scoped super_user token could drop the scope by impersonating. Fix: the scope carries forward on all three surfaces, so a scoped credential can only ever mint/become an equally-scoped one — the same "can only subtract" invariant, extended to the paths that leave verifyPerms. createTokens and refreshOperationToken copy the caller's scope into the minted payload; applyImpersonation copies it onto the new principal. Also moved the api_name resolution into tokenScopeDenial so the unscoped default path (every non-scoped request) does no registry lookup before its `== null` return, and updated the helper's comment to enumerate this fourth bypass class alongside the three in-function ones. Tests: createTokens carries the scope into both minted tokens and stays unscoped for an unscoped caller (reusing the existing mocked suite); refresh_operation_token preserves the scope through a real validate->decode->sign round trip; impersonation carries it onto the impersonated user. 253 passing across the affected suites. Adjudication of the rest of that review is in the PR description. Two flagged "blockers" were false positives (an undefined `op` — `op` is declared and the scope tests exercise that exact line; and a non-existent alter_oidc_trust operation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): deny a scoped token from minting a login token Fifth bypass of the token operation scope, found by claude[bot] on the (now-folded-in) #2174 review — same class as the create_authentication_tokens escalation: a path that produces a new credential and drops the scope. create_authentication_tokens with purpose:'login' is NO_AUTH, so the scope gate in verifyPerms never runs. Its login branch signs a username-only token, which the `login` operation trades for a cookie session — and a session is username-only by construction: session-restore reloads the FULL user via getUser (tokenOperations is only ever set from a JWT operations claim, never on a session-restored user). So a credential scoped to e.g. deploy_component could self-escalate to a fully unscoped session with two NO_AUTH calls, no password required. A session cannot carry an operation scope, so carrying it forward is not possible without reworking the session model; a scoped CI/OIDC credential has no use for a browser session anyway. Fix: deny purpose:'login' when the authenticating caller is scoped (reuses the inheritedScope already computed for the operation/refresh path just above). Tests: a scoped caller is refused (403); an unscoped caller still mints a login token (and no refresh token, as before). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(security): extract the token-scope carry-forward into one helper The operation scope had to be threaded through every path that produces a credential or principal — six sites, each re-inlining `Array.isArray(...)`, and one using `!= null` instead. That scatter is exactly why the five bypasses this feature closed turned up one at a time. `security/operationScope.ts` is now the single home for the guard: `hasOperationScope` (the predicate), `attachScopeToToken` (the `operations` claim on a payload), and `attachScopeToUser` (`tokenOperations` on a user principal). The six call sites — createTokens, its login-deny, refresh, createOperationToken, validateToken, and impersonation — each collapse to one self-documenting call, and the `!= null` outlier is normalized to the same array guard (behavior-identical: an empty deny-all scope is still carried, anything non-array still skipped). No behavior change — the module carries the same array-including-empty guard every site already used, verified by the full scope-path suite (createTokens scoped/unscoped/login-deny, refresh, impersonation, createOperationToken empty/absent, validateToken) plus 8 unit tests for the helper itself. The naming asymmetry (`operations` on tokens vs `tokenOperations` on users) is now documented once, in the module. Beyond making the diff tighter, this is the thing that makes the invariant maintainable: a future credential-producing path is one `attachScope*` call, and greppable rather than a pattern to remember. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): gate the token scope on the API operation, not the handler name Two findings from the combined cross-model review, one root cause. The scope gate resolved the operation name from the handler function (`requiredPermissions.get(op)?.api_name ?? op`) instead of using the API operation the caller actually sent — the namespace the policy scope is written in. - deploy_component (#481, the headline use case): its handler is registered with no api_name, so the gate resolved `deployComponent` and a policy scoped exactly to `deploy_component` was DENIED. The feature did not work for the operation it exists to scope. Fail-closed, so not an escalation — but functionally dead. Shared handlers (search_by_id/search_by_hash) were also conflated. - nested-SQL export jobs (#506): verifyPermsAST hardcoded `sql` as the scoped operation, but export_local/export_to_s3 carry their query as SQL through the same branch. A token scoped only to `sql` could start an export it was never granted, because the gate never saw `export_local`. Fix: verifyPerms passes `requestJson.operation`; verifyPermsAST takes the top-level API operation (threaded from checkASTPermissions' `jsonMessage.operation`, defaulting to `sql`); tokenScopeDenial compares that directly against the scope and no longer reconstructs a name from the handler. Using the real operation also distinguishes shared-handler aliases for free. Tests: deploy_component is allowed when scoped and denied when not; search_by_id vs search_by_value are distinguished though they share a handler; a `sql`-only scope cannot start an export_local job while an export_local-scoped one can. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): gate the token scope on the job op for non-SQL export jobs too Follow-up to the previous commit, found by claude[bot]: I fixed the export-job scope bypass on the SQL path (verifyPermsAST) but left the identical hole on the NoSQL path. The dispatcher hands verifyPerms the nested search_operation as requestJson for a job, so requestJson.operation is the inner op (search_by_conditions, search_by_value, ...), not export_local. The scope gate therefore checked the read op: a token scoped to ['search_by_conditions'] passed, then the super_user bypass returned allowed, and the export ran — writing exported data to local disk or S3 from a credential meant to be read-only. Same "can only subtract" violation as #506, via the NoSQL search operations. Fix mirrors the verifyPermsAST one: serverUtilities threads the top-level json.operation into verifyPerms via options.apiOperation, and the scope gate uses `options?.apiOperation ?? requestJson.operation`, so the job op is checked while direct callers keep using requestJson.operation. Test: a search-scoped token cannot run an export_local job whose nested op is search_by_conditions; an export_local-scoped one can. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): drop an unused path import left by the rebase conflict resolution The rebase onto main's deploy-setup change collided in the cliOperations import block; resolving it kept `import * as path from 'path'`, but the merged file no longer uses path. Removes the unused import (oxlint no-unused-vars). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): harden token scoping per deep-review (4 findings) A single-pass Harper-domain deep-review of the combined PR (the lens the cross-model run's domain leg failed to execute) surfaced four issues. 1. Scoped token could mint a long-lived credential (significant). create_authentication_tokens is NO_AUTH, so the scope gate never runs. The login path already denied scoped callers, but the standing operation+refresh path carried the scope forward yet honored expires_in verbatim and wrote a refresh_token — turning a minutes-long leak into a decade-long one (scoped, so not privilege escalation, but it defeats the exchange's ephemerality guarantee), reachable even by a deny-all scope. Now a scoped caller is denied outright (covers login + standing paths); a CI token holds the operation token the exchange already gave it. 2. Scope guarantee overstated in types.ts (doc). The scope is enforced on the operations-API and SQL paths only (verifyPerms/verifyPermsAST); the REST/GraphQL resource path authorizes via table-level checkPermission and doesn't consult it. Narrowed the doc and pointed resource-path enforcement at the CORE-3061 follow-up surface. NOT enforcing it there in this PR. 3. Replay table not audited (significant). hdb_oidc_token_use was created without an explicit audit flag, so with logging.auditLog:false its rows never replicate — silently dropping cross-node replay protection while the trust policies that gate it still propagate. Now audit:true, matching its sibling hdb_oidc_trust. (Kept lazy table() rather than the systemSchema bootstrap because the expiresAt TTL is not expressible via CreateTableObject; this matches hdb_certificate_cache.) 4. Exchange trusted stored rows to be write-validated (suggestion). add_oidc_trust enforces assertPolicyIsSpecific, but the exchange only backstopped the empty-claims and pull_request_target cases. A row that arrived via replication from an older node or a restored backup — e.g. repository pinned, no workflow/ref gate — was honored. findMatchingPolicy now re-runs assertPolicyIsSpecific/assertAudienceIsSpecific and skips (logs) any row that fails. Fail closed. Tests: scoped caller denied on standing/deny-all/login paths with no user-record write; an under-specified stored row is ignored at exchange. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): close credential-minting, scope, and lifetime gaps from review Two independent model reviews (Codex and Claude) converged on four of these. 1. create_authentication_tokens could still mint from an exchanged token. The previous guard fired only on a SCOPED caller, but a trust policy carries `operations` only when the operator opts in — so the ordinary exchanged token is unscoped and sailed through, taking a caller-controlled expires_in and a 30-day refresh token with it. Mint provenance is now a signed claim on every token createOperationToken produces, lifted onto the principal at validateToken and refused ahead of the user lookup, so a refused request reads nothing and writes nothing. Impersonation carries it forward too: it returns a new principal, which would otherwise launder the marker. 2. A `read_only` scope could run write SQL. The group expands to include `sql`, and verifyPermsAST returns null for a super_user before any table check, so DELETE/UPDATE/INSERT passed. A write statement now additionally requires its matching data operation in scope — which is exactly what separates read_only from standard_user, with no need to track which group admitted `sql`. 3. job_workflow_ref no longer satisfies the caller-ref gate. It names the reusable workflow that ran, not the caller that invoked it, so its @ref is constant however it is called and admitted any branch of any caller repo referencing that workflow. It remains valid as a workflow pin. 4. Identity tokens now require `iat` and bound `exp` against the verification clock. The ceiling was skipped entirely when `iat` was absent, and a pair shifted equally far into the future kept a small delta while staying valid for as long as it liked. Also corrects DESIGN.md, which asserted the opposite of the implemented per-policy allowlist, and records the REST/GraphQL enforcement boundary there rather than only on the types.ts field. The new createTokens cases drop the rewire mutations AGENTS.md prohibits — reachable now that the guard runs before any I/O — and the two positive-path cases they came with were already covered. Documents, rather than works around, a pre-existing gap in the shared grantable- operation registry: it is process-local and the OPERATION_REGISTERED bridge carries only name/thread routing, so add_role, alter_role, and impersonation validation reject worker-registered component operations on main identically. It fails closed, and the fix belongs to that bridge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): key replay on the signed input, and fix issuer/audience matching Third cross-model review (Barber AI). The replay finding is a real bypass and the premise it rests on was mine. 1. HIGH — replay protection was bypassable. The fingerprint hashed the whole token on the stated grounds that "a replayed token is byte-identical by definition". That is false: the signature segment is covered by nothing, and base64url decoding ignores the surplus low bits of its final character, so an RS256 signature has 16 distinct spellings that decode to identical bytes. Verified against this branch's jsonwebtoken — all 16 verify, each hashing differently, so one leaked identity token bought 16 operation tokens. ES* malleability (s -> n-s) is a second such vector. Now keyed on the signed input (header.payload), which is exactly what the issuer asserted, so every re-spelling collapses to one fingerprint. The regression test was confirmed to fail against the old fingerprint before being kept. 2. An issuer whose `iss` ends in `/` could never authenticate: the expectation passed to jwt.verify is normalized, the comparison is byte-for-byte. Azure AD v1 emits exactly that, and the generic profile exists to serve such issuers with no provider code. Both spellings are accepted now, which cannot widen trust — normalizeIssuer already collapses them for the cache key, the discovery check, and the policy lookup. 3. `audience` was stored raw while `issuer` was normalized, but the CLI requests its token for normalizeTarget(target) — port and trailing slash included. So the natural `audience=https://host` stored a policy that could never match, failing opaquely in CI. Rejected at write time now, where the administrator can see it. Rejected rather than canonicalized: silently rewriting a value whose job is byte-for-byte comparison is worse, and canonicalizing ahead of assertAudienceIsSpecific would disarm the shared-audience guard, since https://github.com/<owner> normalizes out of that regex. A test pins the check to normalizeTarget's real output so the two cannot drift. Also: audit records the x-forwarded-for client rather than the proxy, matching auth.ts; auditing can no longer change the outcome it records (a throwing success emit was caught and re-reported as a failure); JWKS/network errors are logged instead of vanishing into a misleading "no policy matched"; a missing trust table no longer answers an anonymous caller with a descriptive 400 that breaks the uniform-rejection property; the issuer filter moved into the scan so an unauthenticated request no longer allocates a record per stored policy; the exchange uses the standard CLI timeout rather than inheriting deploy_component's 10-minute SSE timeout; a blank-but-set token namespace no longer falls through to workload identity, which would deploy a failed CI secret as a different identity; clearJwksCache can no longer be undone by an in-flight fetch; and two JSDoc references to symbols that never existed now name the real ones. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): bootstrap the replay table and carry the job's real operation Closes the two remaining Barber AI findings. The replay table is now a properly bootstrapped system table: a systemSchema.json stub, a SYSTEM_TABLE_NAMES entry, and a 5.3.0 directive branch, matching the three touchpoints DESIGN.md requires. The previous comment cited hdb_certificate_cache as precedent for going lazy-only, which was wrong — that table is systemSchema-declared AND lazily extended, and the lazy half exists only because an expiresAt TTL is not expressible through CreateTableObject (confirmed: no systemSchema entry declares one and CreateTableObject has no support for it). This matters beyond tidiness: a table auto-provisioned by replication is created without the `audit` flag its schema declares, and auditing IS the replication feed, so a node that first learned of this table from a peer could end up with a non-replicating copy — losing exactly the cross-node replay protection the table exists to provide. That also forced the `??` short-circuit out of getTokenUseTable. With a bootstrap stub always present, short-circuiting on existence would have meant the TTL was never applied on any node — records accumulating in a system table forever. table() now runs once per process regardless, layering the TTL on top, as hdb_certificate_cache does. The exchange tests move their seam to the table factory accordingly, since seeding databases.system no longer intercepts it. Separately, an export job re-entered the SQL permission check with its own `operation: 'sql'`, because the checked parse is stashed on the top-level request while export dispatches the nested search_operation. A token scoped to `export_local` was therefore denied by its own job — fail-closed, so a broken feature rather than a hole, but the natural scope for an export-only CI identity did not work. serverUtilities now stamps the real operation onto the nested request and checkASTPermissions prefers it. This is also the only authorization that runs in the job worker, which never invokes the outer gate. Also guards the expanded-scope memo with an instanceof Set check: it rides on hdb_user, a job persists that user into hdb_job.request, and msgpackr returns a Set as a plain Array — .has() would then throw out of the auth gate as a 500 rather than a clean denial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): reject a non-boolean `enabled`, and pin exact claim matching `add_oidc_trust` with `"enabled": "false"` stored an ENABLED policy. Joi's boolean converts by default, but validateBySchema keeps only `result.error` and discards the converted value, so the string survived to `req.enabled !== false` — true for `"false"` — and `readPolicies` filters on the same comparison, so nothing downstream caught it either. An operator disabling a policy this way got no error and a policy that kept minting tokens. `Joi.boolean().strict()` now rejects it outright: a revocation control has to fail closed. Claim matching was already exact, but nothing pinned it — replacing `accepted.includes(actual)` with a `startsWith` left every OIDC test green, which makes it the one escalation-critical invariant a future refactor could relax silently. Prefix matching on `repository`/`sub` is the classic trusted- publishing escalation, since `HarperFast/my-app-evil` is a name anyone can register. Added negative cases in both argument orders, and confirmed they fail against exactly that mutation before keeping them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): act on the SQL permission denial processAST was discarding processAST computed a permission denial and then threw it away. The guard read `permissionsCheck && permissionsCheck.length > 0`, but checkASTPermissions returns null or a PermissionResponseObject, which has no `length` — so the test evaluated `undefined > 0` and was always false, and the statement executed anyway. This only bites where processAST is the FIRST checker rather than the second. A direct SQL call arrives with permissions_checked already true, set by chooseOperation, whose own guard is a correct bare truthiness test. An export job is the case that does not: it re-parses from its nested search_operation, and in the job worker no outer gate runs at all — so the denial dropped here was the only one standing. Now a bare truthiness test, matching serverUtilities. The existing scope tests all asserted that a denial is COMPUTED; none asserted anyone acts on it, which is why a dead consumer went unnoticed. Added a case that drives processAST itself, confirmed to fail against the old guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: split the processAST guard fix out to its own PR (#2202) A reviewer pointed out the dead `permissionsCheck.length > 0` guard is pre-existing and affects all SQL authorization, not just this feature, and asked for it as its own change with its own coverage rather than bolted onto an auth PR. Agreed — it is now #2202, with tests that drive processAST directly and cover the allowed and already-checked paths too, so it cannot start denying statements that were always permitted. This PR does not depend on it. The outer gate in serverUtilities refuses an out-of-scope job operation and sqlWriteScopeDenial refuses write SQL, both through correct truthiness tests, so export_local + DELETE is already refused at the front door. Left a note at the call site pointing at #2202 so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): never read the SQL scope's operation from the request body `checkASTPermissions` resolved the token-scope operation as `jsonMessage.api_operation ?? jsonMessage.operation`. On the direct-SQL path `jsonMessage` IS the client's request body, and that check is the ONLY gate there — the `sql` branch of chooseOperation is mutually exclusive with its verifyPerms call. So a caller could send `{operation: 'sql', sql: '...', api_operation: '<whatever their scope allows>'}` and run arbitrary SQL under it. Reproduced against this branch; the regression test was confirmed to fail before the fix. I introduced this in 11f2280c6, carrying a job's real operation to the nested check on a request property. That is reverted. The operation now comes from an explicit argument or the dispatched `json.operation`, never from a field on the message — chooseOperation passes the operation it already resolved. Stripping `api_operation` at the ingress points was the first fix I tried, and it is the wrong shape: it leaves the check trusting a body property and makes safety depend on every current and future entry point remembering to strip. The property is gone instead. The trade is that a job's SQL is checked as `sql` rather than as `export_local`. That changes no outcome today, because the branch in processAST that would act on the denial is dead — PermissionResponseObject has no `length`, so its guard never fires (#2202). When #2202 makes that branch live it needs a carrier for the job's operation that a client cannot forge; a request property is not one, however carefully it is stripped. Recorded at both sites so the next reader does not re-derive it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(upgrade): patch is_hash_attribute on the replay table; pin the enabled strictness Two review follow-ups. hdb_oidc_token_use is created through the same CreateTableObject + bridge.createTable path as hdb_oidc_trust, hdb_deployment, and hdb_secret, but skipped the is_hash_attribute __dbis__ patch all three of those apply. If the reason they need it holds — harperdb@4.x derives the LMDB DBI open flags from that field, and its absence opens the DBI with DUPSORT and throws MDB_INCOMPATIBLE — then a 5.3.0 install that later downgrades hits it here too. The helper is now parameterized by table name and applied on both branches for both tables, so the asymmetry is gone rather than undocumented. The `.strict()` fix on `enabled` had no test: dropping it back to a plain Joi.boolean() left the whole OIDC suite green, which is a poor state for a revocation control whose failure direction is "stops revoking". Added cases for the coercible values and for a genuinely disabled policy, and confirmed they fail against the un-strict schema. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(upgrade): record the TTL-on-first-use limitation for the replay table The expiresAt TTL is installed by the table() call on the exchange path, so a node that never performs an exchange has the table from this directive but never registers the TTL locally — replicated replay rows land there and are never evicted. Documented rather than fixed: hdb_certificate_cache has the identical shape, so the real fix is installing the TTL at system-table setup for every lazily-extended system table, not special-casing this one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: assert the export-job scope gate enforces, not just computes Reverting the processAST guard to #2202 removed the one test that asserted this invariant is ENFORCED rather than merely computed, and the safety argument now rests entirely on chooseOperation's front-door gate — which had no enforcement test of its own. The rest of the scope suite only checks that verifyPermsAST returns a denial object, which is exactly how a dead consumer goes unnoticed. Three cases on the real dispatch path: an export job carrying nested write SQL outside the scope throws 403, an export whose own operation is outside the scope throws 403, and an in-scope export still runs — the last so this cannot pass by refusing everything. Confirmed they fail when the front-door gate is given the same dead-guard shape (`astPermCheck && astPermCheck.length > 0`) that made the inner branch a no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: make the #2202 ordering constraint fail loudly instead of living in a comment The interaction between this PR and #2202 was documented only in prose, and the two can merge in either order. Removing the forgeable operation carrier leaves checkASTPermissions falling back to jsonMessage.operation, which at the processAST call site is the nested search_operation's own `sql` — so once #2202 makes that branch live, an export_local-scoped token 403s on its own export job. Added a tripwire that drives evaluateSQL with the exact shape export.ts:363 dispatches and asserts an in-scope export is not refused by the permission gate. It passes today and was confirmed to fail with #2202's one-line change applied on top, so whichever PR lands second turns CI red rather than shipping a silently broken feature. The comment on it says what to do when it fires — supply the job's real operation through a carrier a client cannot set, rather than relaxing the scope check. Preferred this over making apiOperation a required parameter: that turns the missing carrier into a compile error the next author satisfies by passing jsonMessage.operation, which is the wrong value and compiles clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: decouple the #2202 tripwire from the status literal it watches The tripwire compared against 403, which is UNAUTHORIZED_RESPONSE in the very file it exists to watch — so changing that constant would leave it green while the refusal it guards against still happened. A tripwire must not depend on a constant its own target owns. Now asserted by shape: the permission path is the only one that calls back with a bare numeric status, while every other failure forwards an Error. evaluateSQL drops the second callback argument on error, so the denial object never reaches the test and the number is the whole signal — which also rules out asserting on the PermissionResponseObject shape directly. Verified across four states: passes today, fails with #2202's guard applied, still fails with #2202 applied AND the status changed to 401 (the case the old assertion missed), and passes again restored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): refuse malformed stored policies; let exchange_oidc_token keep its token Two findings from kriszyp's review. The exchange-time recheck stopped at audience/claim specificity, so a row that reached the table another way — replication from an older node, a restored backup, a direct system-table write — could still fail OPEN in two shapes: operations: 'deploy_component' a scalar, not an array. hasOperationScope tests Array.isArray, so the scope was silently dropped and the token minted UNSCOPED, carrying the policy user's entire role. A malformed narrowing must never widen. enabled: 'false' a string. `row.enabled !== false` is true for it, so a policy an operator disabled kept minting tokens. Both are now refused rather than normalized, by running the SAME validators the add path uses (validateOperations, validateClaimConstraintShape) against the raw row before toRecord touches it — normalizing first is exactly what hid them. Two implementations of "is this row valid" is how a write path and a read path drift apart, so they share one. Regression cases write each shape straight to the store, with a control proving a well-formed direct write still authenticates. Separately, `token` is stripped from every CLI request body as transport-only, on the stated grounds that no operation takes a top-level `token`. This feature broke that premise: exchange_oidc_token's identity token IS its request, so the generic CLI path sent it without the field it requires and the issuer-agnostic operation was unusable there even though direct HTTP worked. The strip is now keyed on the operation rather than dropped — the mistyped-`setup` case it guards is real — with tests for both directions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): back off after a failed JWKS refresh; stop overclaiming registry support Once a cached key set passed its TTL, a failed refresh returned a stale key but advanced no clock — `fetchedAt` is only set on success — so every subsequent request wave started discovery again and rode the same timeout before falling back to the same stale key. The exchange is unauthenticated and picks its issuer from an unverified JWT, and key ids are public, so an anonymous caller could keep that cycle running for the length of an issuer outage: exactly when the stale-key grace is meant to absorb load rather than generate it. A failed refresh is now recorded on its own clock, and while a usable stale key is on hand the fetch is skipped entirely for the backoff interval — skipping the fetch is the point, since that is the expensive half. Recorded even when no stale key rescues the request, so the backoff also covers an issuer whose keys we have never held, and cleared on success. Also corrected the dynamic-operation test's claim. It said component-registered operations are supported; the registry is process-local, add_oidc_trust runs on main, and server.registerOperation runs in a worker whose announcement carries only name→thread routing. The test asserts the delegation to validateOperations, not the topology, and now says so — the production behavior is that such a policy is rejected, which fails closed and is shared with add_role/alter_role. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): apply the JWKS backoff to an issuer with no cached keys The skip-fetch gate required a stale key, so it never fired for an issuer whose keys had never been cached — every request still rode the full discovery and fetch timeout, and `failedRefreshAt` was written but never read on that path. The comment above it claimed the opposite. That is the worse half of the case: with no cached key there is nothing to fall back to, and the exchange is unauthenticated with the issuer chosen from an unverified JWT. The backoff now applies regardless: a stale key is served when there is one, and otherwise the request is refused for the interval instead of re-driving the fetch. Fails closed. The cost is that a legitimate first exchange waits out the interval after a blip, which is bounded and the right side to err on for an unauthenticated endpoint. This half is testable without a time seam, unlike the expired-cache half — so there are now two cases: repeated failures for a never-cached issuer stop producing fetches, and a recovered issuer is picked up again once cleared. The first was confirmed to fail against the stale-key-gated version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): tell the truth in list_oidc_trust; pin the guards the tests missed Two review follow-ups on the stored-policy validation. A row the exchange refuses still listed as healthy. `storedPolicyProblem` ran only on the exchange path, and `toRecord` normalized exactly the shapes it exists to refuse — `enabled: 'false'` rendered as `enabled: true`. So a row arriving by the routes this validation defends against would fail every exchange with the deliberately opaque 401 while `list_oidc_trust`, the one command an operator runs to check, confirmed the trust was fine. Validation now runs on both paths; the exchange refuses, and a listing reports `invalid_reason` and the stored `enabled` as-is rather than coerced. A listing's job is to describe what is stored. The two tests for the fail-open shapes did not actually pin their guards, which a mutation check demonstrated: operations — the case seeded a STRING scalar, so validateOperations iterated it character by character and refused the row by reporting 'd' as an unknown operation: right outcome, wrong check, guard deletable with tests green. Now seeded with a number, where `for (const op of 42)` throws TypeError and turns one malformed row into a 500 for every exchange against that issuer. claims — the case used a wholly-bad shape that matchTrustPolicyClaims refuses downstream anyway. Now a constraint list that MATCHES on its string entry and carries a non-string alongside it, which only the shape validator refuses. Both confirmed to fail with their guard deleted, and the string-scalar case is kept as well since it is the shape most likely to arrive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(security): report a deleted or deactivated policy user in list_oidc_trust `invalid_reason` covered row shape only, but the exchange also refuses a well-formed row whose user has since been deleted or deactivated — with the same opaque 401. That is the same availability trap the previous commit closed, reached by its most mundane cause: someone removes the CI user, every deploy starts failing, and the one command an operator would run to check reports the trust as enabled and healthy. Annotated in listOidcTrust rather than in readPolicies, deliberately. It is one users-cache read for the whole listing on an SU-only path; doing it per row in readPolicies would put a user lookup on the unauthenticated exchange path, which already resolves the user itself at the point it matters. A shape problem still wins, being the more fundamental complaint. Both cases confirmed to fail with the annotation removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: pin the invalid_reason precedence a row with both problems relies on The rule that a shape problem outranks a missing user was stated in the comment and the commit message but pinned by nothing: every shape case named a valid user and both user cases were well-formed, so no test had a row with both. The `continue` implementing it could be mutated to a no-op with all tests green. Added a row that is both malformed and names a deleted user, asserting the shape problem is what surfaces — it is the more fundamental complaint, since the row stays refused even if the user is restored. Confirmed it kills exactly the mutation that survived before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: pin the stale-key grace ceiling with an injected clock STALE_KEY_GRACE_MS was the one JWKS guard nothing pinned: replacing it with an unbounded stale fallback left the whole suite green. That bound is the security half of the blip-tolerance tradeoff — `fetchedAt` advances only on a SUCCESSFUL fetch, so without the ceiling a key the issuer has pulled stays honored for the entire length of an outage instead of 24 hours. getSigningKey now takes an optional `now`, mirroring the clockTimestamp seam verifyIdentityToken already exposes rather than inventing a second convention — production callers pass nothing. Reaching this branch otherwise needs a cache aged past an hour, and this repo bars new fake timers, which is why the gap was previously documented rather than closed. Both sides asserted: a cached key is still served at grace−1 (a blip must not break deploys) and refused at grace+1. Confirmed the reviewer's exact mutation now dies, and that it reached dist/ before running — a .ts-only edit would have been a silent no-op since .mocharc sets no --conditions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

github-actions Bot requested review from Ethan-Arrowood, cb1kenobi, heskew and kriszyp August 18, 2026 16:56

gemini-code-assist Bot reviewed Aug 18, 2026

View reviewed changes

dawsontoth marked this pull request as draft August 18, 2026 17:07

cb1kenobi mentioned this pull request Aug 18, 2026

feat: OIDC trusted publishing — deploy from CI with no stored credential #2173

Merged

Navigation Menu

Uh oh!

fix(security): honor the SQL permission denial processAST computes - #2202

fix(security): honor the SQL permission denial processAST computes#2202
dawsontoth wants to merge 1 commit into
mainfrom
fix/process-ast-permission-guard

dawsontoth commented Aug 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

claude Bot commented Aug 18, 2026

Uh oh!

dawsontoth commented Aug 18, 2026

Uh oh!

dawsontoth commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

dawsontoth commented Aug 18, 2026

The bug

Why nobody noticed

The fix

Tests

Scope

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

claude Bot commented Aug 18, 2026

Uh oh!

dawsontoth commented Aug 18, 2026

Uh oh!

dawsontoth commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant