feat/one-api

verifyBatuJwt: empty-string org_id bypasses the service-token forged-org guard

packages/api/src/auth/public-jwt.ts

The guard `if (orgId || orgPublicId)` uses JavaScript truthiness. If a forged service token carries `org_id: ''` (empty string), orgId is '' (falsy), so the check does NOT reject. The token then returns ok({..., orgId: null}) without triggering the documented invariant. In practice no downstream path grants access on an empty orgId, so it is not an active exploit, but the invariant ('a service token carrying an org is malformed — reject, never coerce') is silently violated. Fix: use `if (orgId !== null && orgId !== undefined) return err(...)` or check `typeof orgId === 'string'`.

mintServiceToken swallows DB errors as 401 instead of 500

apps/platform/src/api/handlers/public-v1/auth-token.handler.ts:78

When findActiveByPrefix throws (DB unreachable), the catch returns UNAUTHORIZED_RESPONSE (401). The customer bk_ path uses INTERNAL_ERROR_RESPONSE (500) for the same failure. The asymmetry is misleading: callers receiving 'Invalid API key' may react by rolling their credential when the DB is simply down. Fix: use INTERNAL_ERROR_RESPONSE for DB exceptions in mintServiceToken, matching the customer path.

revokeByPublicId conflates 'not found' with 'already revoked' — idempotency hazard

domains/core/src/service-key/service-key.queries.ts:71

The UPDATE WHERE status='active' returns no rows for both 'key never existed' and 'key already revoked'. Both map to ServiceKeyErrors.notFound (404). A caller that retries a revoke after a network timeout gets a 404 and may incorrectly conclude the key was never revoked. No HTTP handler for revoke exists yet, so this is not a live bug, but the contract is wrong before it ships. Fix: distinguish the two cases in the shell — a two-step check or a distinct 'AlreadyRevoked' error (409 or idempotent 200).

Service key prefix index is non-unique — defense-in-depth gap vs customer keys

packages/database/src/schema/service-keys.ts:62

Customer api_keys uses a partial UNIQUE index on prefix WHERE status='active'. service_keys uses a plain non-unique index (idx_service_keys_active_prefix). While the code correctly returns an array and iterates verifySecret to handle collisions, the DB offers no structural prevention. With 48 bits of prefix entropy this is astronomically unlikely, but it's a defense-in-depth regression. Should be uniqueIndex().where(sql`status = 'active'`) for parity with the customer-key design.

Back-compat actor fallback is broadly permissive: any non-'service' actor reads as 'machine'

packages/api/src/auth/public-jwt.ts

Line 331: `payload.actor === 'service' ? 'service' : 'machine'`. Any unknown actor value (e.g. 'admin') defaults to 'machine' instead of being rejected. Not currently exploitable (signing key required; machine path still needs valid org claims). But if a third actor type is added later, this permissive default could mask a type confusion. Consider narrowing to `payload.actor === 'machine' || payload.actor === undefined ? 'machine' : reject`.

createServiceKeyShell: findActiveByName is outside try/catch — can throw instead of returning err()

domains/core/src/service-key/service-key.shells.ts:58

The try/catch wraps only serviceKeyQueries.insert() (line 65+). The preceding `await serviceKeyQueries.findActiveByName(db, name)` (line 58) and `await hashSecret(secret)` (line 63) are outside any error boundary. If findActiveByName throws (DB connection lost, pool timeout), the shell violates its no-throw contract — the caller gets an unhandled rejection instead of a Result<_, _>. Fix: wrap the entire shell body in try/catch, or use db.transaction() which provides a consistent error boundary.

createServiceKeyShell missing db.transaction() — two separate DB calls without atomic boundary

domains/core/src/service-key/service-key.shells.ts:45

canonical-form.md: 'All writes (entity + outbox) inside the same db.transaction() — non-negotiable.' createServiceKeyShell does findActiveByName + insert as two separate calls. The api-key shell wraps its entire check+insert in db.transaction() with an advisory lock. The DB unique index does catch the race (and the 23505 is handled), but the shell's correctness relies on the DB guard rather than the FCIS pattern. Wrapping in db.transaction() would also provide the error boundary that resolves the no-throw issue above.

Shells omit ActorContext and structured logging

domains/core/src/service-key/service-key.shells.ts:45

domain-patterns.md: 'Accept ActorContext for audit trail' and 'Use structured logging: createShellLogger(domain, operation, actor)'. Both createServiceKeyShell and revokeServiceKeyShell omit ActorContext and do no structured logging. The comment justifies skipping the outbox (operator action, no domain subscribers) — acceptable per the escape hatch — but that rationale does NOT cover omitting ActorContext and createShellLogger, which are for audit attribution. 'Who minted this service key' is exactly the kind of operator-audit signal ActorContext was built for.

service-key barrel omits individual error leaf type exports

domains/core/src/service-key/index.ts:31

The api-key barrel exports individual error types (ApiKeyNotFoundError, etc.) alongside the union types. The service-key barrel exports only union types and the ServiceKeyErrors object. Consumers who need to name a specific error variant in a type annotation cannot import them from the FCIS namespace. Minor ergonomics gap but deviates from the established pattern.

service-key entity absent from domains/core/CLAUDE.md entity table

domains/core/CLAUDE.md

The core CLAUDE.md Entities table lists api-key (apk_) and webhook-endpoint (whk_) but omits the new service-key (svk_). The prefix mapping and the platform-vs-org distinction are non-obvious and belong in the table. Adding new entities should update CLAUDE.md as part of the PR.

service-key.lib.ts imports from sibling entity lib (should use barrel)

domains/core/src/service-key/service-key.lib.ts:13

`export { extractPrefix, hashSecret, verifySecret } from '../api-key/api-key.lib'` bypasses the api-key barrel. canonical-form.md: the barrel is the sole import surface. The intent is sound (avoid duplicating scrypt), but the right import is from '../api-key' (the barrel). Minor but deviates from the FCIS boundary convention.

No shell tests for createServiceKeyShell or revokeServiceKeyShell

domains/core/src/service-key/__tests__/service-key.shells.test.ts

The api-key entity has api-key.shells.test.ts covering happy path, null-insert error, 23505 race condition, and each error path. createServiceKeyShell has real branching: name validation, pre-check findActiveByName, insert, 23505 catch; revokeServiceKeyShell has revokeByPublicId null-return → notFound. None of this is tested. The 23505 race-condition path is especially important — a bare DB error the shell manually catches and maps to ServiceKeyNameTaken is invisible without a test that simulates it.

DUMMY_HASH timing-equalization path not pinned by a test assertion

apps/platform/src/api/handlers/public-v1/__tests__/auth-token.service.test.ts:103

mintServiceToken runs `await verifySecret(secret, await DUMMY_HASH)` when candidates.length === 0 to equalize timing. The 'unknown sk_ key gets 401' test sets svcFindActiveByPrefix to [] and verifySecret to false, but does not assert that verifySecret was called with the dummy hash — if a refactor accidentally removes the timing equalizer, the test still passes. Add `expect(h.verifySecret).toHaveBeenCalledOnce()` to pin the invariant.

Forged service token with org_id: null (not absent) not tested

packages/api/src/auth/__tests__/public-jwt.test.ts:449

The forged-token test covers org_id: 'org-victim' (truthy string). The verifier's rejection check is `if (orgId || orgPublicId)` — if org_id is the literal null JSON value (not absent), `null || null = false` and the check does NOT reject. A test with `org_id: null` would confirm whether the guard is strict enough. The service-token invariant states 'no org claim at all — not null, not absent'; the test should verify both axes.

No test for signBatuJwt failure → 500 in the service-key token path

apps/platform/src/api/handlers/public-v1/__tests__/auth-token.service.test.ts

mintServiceToken returns INTERNAL_ERROR_RESPONSE (500) when signBatuJwt returns ok:false. The test suite covers DB failure → 401, unknown prefix → 401, wrong secret → 401, but not JWT signing failure → 500. The 500 body is a different shape from 401 — a test would confirm the curated error response is correct and that a signBatuJwt failure does not accidentally surface a plain JS Error to the client.

internal-auth.test.ts: no test for valid static key AND valid service JWT both present

packages/api/src/middleware/__tests__/internal-auth.test.ts:176

The dual-accept middleware evaluates the static key first — if both headers are present, the static key short-circuits and validateAuth is never called. No test sends BOTH a correct x-api-key and a valid Bearer simultaneously to verify the priority ordering. A code reordering that accidentally evaluates Bearer first when both are present would not be caught by the current suite.

meta-gate: no test for service token on a mixed auth:['machine','service'] route

apps/platform/src/api/utils/__tests__/public-v1-meta-gate.test.ts

Tests cover: service accepted on auth:['service'] (internalCatalog), service rejected on auth:['machine','session'] (billsList). No test covers auth:['machine','service'] — the mixed case where both credential classes should be admitted. The gate's .includes() logic should handle it, but the test coverage is asymmetric to the declared contract.

No end-to-end integration test for the service-key lifecycle

No test covers: (1) createServiceKeyShell with real DB, (2) presenting the plaintext secret to the token exchange, (3) using the minted JWT against withInternalAuth. The api-key entity has api-key.integration.test.ts. A wiring bug (wrong field name in the JWT payload) would only surface in production. At minimum an integration test covering step (1) with real DB is needed, matching the api-key pattern.

createServiceKeyShell name validation invariants not unit-tested

domains/core/src/service-key/service-key.shells.ts:50

The shell enforces: must start with a letter, 1-64 chars, lowercase/digits/hyphens, whitespace trimmed. None are tested. Gaps: (a) name starting with a digit → ServiceKeyInvalidName, (b) uppercase letters, (c) exactly 64 chars accepted, (d) 65 chars rejected, (e) leading spaces trimmed to valid name. For api-key this logic lives in a pure decisions module (unit-tested separately); for service-key it's inline in the shell, making shell tests the only coverage point.

meta-gate beforeEach uses mockReset() to prevent queue leaks — pattern is fragile

apps/platform/src/api/utils/__tests__/public-v1-meta-gate.test.ts:100

The comment correctly documents that clearAllMocks does not flush the mockResolvedValueOnce queue. Using mockReset() is the right fix, but if a future describe block adds its own beforeEach without mockReset(), the queue leak will resurface silently. Prefer mockReturnValue (overwrites rather than queues) or document the pattern with a shared fixture function.

Dead ternary branch in withInternalAuth headers extraction

packages/api/src/middleware/internal-auth.ts:59

Both branches of the ternary evaluate identically: `req instanceof Request ? req.headers : req.headers`. The ternary resolves to a no-op. Should be `const headers = req.headers;`.

Inconsistent console.warn vs console.error for equivalent fire-and-forget failure

apps/platform/src/api/handlers/public-v1/auth-token.handler.ts:121

mintServiceToken's updateLastUsedAt failure uses console.warn (line 121); the customer path's equivalent uses console.error. Choose one level — error is more defensible for an unexpected write failure — and apply it in both places.

revokeServiceKeyShell catch block swallows errors with no logging

domains/core/src/service-key/service-key.shells.ts:102

The catch block is bare — `catch { return err(ServiceKeyErrors.databaseError('revoke')); }` — with no logging of the original exception. A DB error during revoke silently disappears from logs. Log the error before returning, matching the spirit of the create path.

Missing void on fire-and-forget promise in the customer bk_ token path

apps/platform/src/api/handlers/public-v1/auth-token.handler.ts

The service-key updateLastUsedAt is prefixed with void (line 118), correctly marking fire-and-forget. The customer path omits void. Add void to the customer path for consistency.

mintServiceToken is a structural near-duplicate of the customer token path

apps/platform/src/api/handlers/public-v1/auth-token.handler.ts:71

Both paths share: prefix-based DB lookup, zero-candidate timing equalizer, iterate-to-verify loop, sign JWT, fire-and-forget updateLastUsedAt. The only genuine differences are which query table and the absence of org lookup. A shared findAndVerifyCandidate(candidates, secret, dummyHash) helper would eliminate the duplicated timing-equalization and verify loop. Not urgent, but the next credential class will copy-paste this again.

withMetaGate dual-stash pattern lacks a type guard — brittle to ts-rest internals rename

apps/platform/src/api/utils/public-v1-meta-gate.ts:122

Reading nextRequest via double cast to Record<string,unknown> is a side-effect-only write with no type safety around the property name 'nextRequest'. A named constant or type predicate would protect against a ts-rest internals rename going unnoticed. The current approach works but is fragile to change.