Repository navigation
fix(security): require an operator credential to export stored credentials - #16680
Merged
diegosouzapw merged 3 commits intoOct 15, 2026
Merged
Conversation
…tials GET /api/settings/export-json returns every provider connection decrypted plus the raw API keys, and GET /api/db-backups/export and /api/db-backups/exportAll ship the whole SQLite database. All three accepted any credential that passed isAuthenticated(), so an API key with the `manage` (or `admin`) scope could download every stored secret. They now go through requireOperatorCredential() and accept only the instance operator: a signed-in dashboard session, the local CLI subject (pipeline- stamped or validated on loopback), or an instance with login disabled. API keys of any scope and `oma_` access tokens get 403; no credential is still 401. The operator check already existed twice (sync token issuance and admin CLI token minting). requireOperatorCredential() in src/lib/api/operatorCredential.ts now wraps the management-auth step plus resolveOperatorCredential(), and the sync token issuer is rebuilt on it (with the internal-service token opt-in it already accepted), so issuance keeps the same issuer kinds and messages. Imports (import-json, db-backups import/restore) are unchanged.
Contributor
CI Coverage Report
Coverage artifact was not available for this run. |
This was referenced Oct 15, 2026
diegosouzapw
added a commit
that referenced
this pull request
Oct 17, 2026
…nge retention (#16958)⚠️ base-red inherited: #15306 ## Summary Authorization boundary fix on the destructive side of the backup surface. Restoring a backup became operator-only in #16899; destroying restore points still accepted any management credential: `DELETE /api/db-backups` (cleanup — keep the latest N / drop older than D days) gated on `isAuthenticated()` only, so a `manage`-scoped API key or an `oma_` access token could delete every stored restore point but one. Restore points are the recovery path after exactly that kind of leaked machine credential. **`PATCH /api/db-backups` is included on purpose and flagged for the owner's review.** The owner approved gating `DELETE`; `PATCH` is the closure of the same hole: it persists the same `keepLatest` / `retentionDays`, and `backupDbFile()` calls `cleanupDbBackups()` with no arguments after every backup (`src/lib/db/backup.ts:414`), which falls back to those persisted values (`backup.ts:84-91`). Gating `DELETE` alone is bypassable — a machine credential saves `keepLatest: 1`, then triggers a manual backup (`PUT`, still management-reachable) or simply waits for the next automatic one, and the pruning happens anyway. Both handlers now go through the existing `requireOperatorCredential()` (`src/lib/api/operatorCredential.ts`, #16680) with route-local 403 messages — no new export was added to the shared helper and `backup.ts` semantics are untouched. ## Behavior before / after | Handler | Credential | Before | After | | --- | --- | --- | --- | | `DELETE /api/db-backups` | none | 401 | 401 (unchanged) | | `DELETE /api/db-backups` | `manage` API key / `oma_` admin token | **200 — backups pruned** | **403**, settings and files untouched | | `DELETE /api/db-backups` | inference-only API key | 403 (`manage` scope) | 403 (unchanged) | | `DELETE /api/db-backups` | dashboard session / login disabled | 200 | 200 (unchanged) | | `DELETE /api/db-backups` | pipeline-stamped local CLI subject | 401 (`isAuthenticated()` never honored the stamp) | 200 | | `PATCH /api/db-backups` | none | 401 | 401 (unchanged) | | `PATCH /api/db-backups` | `manage` API key / `oma_` admin token | **200 — retention persisted (drives automatic pruning)** | **403**, settings untouched | | `PATCH /api/db-backups` | dashboard session / login disabled | 200 | 200 (unchanged) | | `PATCH /api/db-backups` | pipeline-stamped local CLI subject | 401 | 200 | | `GET` / `PUT /api/db-backups` | `manage` API key | 200 | 200 (unchanged — not destructive, covered by positive tests) | The CLI has no generated command for `PATCH`/`DELETE /api/db-backups` (`bin/cli/api-commands/system.mjs` only exposes `get-`/`post-api-db-backups`; `bin/cli/commands/backup.mjs` prunes its own local export directory), so nothing in `bin/` changes. ## Tests New sibling file `tests/unit/backup-cleanup-require-operator.test.ts` (same style as `tests/unit/backup-restore-require-operator.test.ts`): handlers invoked directly with a crafted `Request`, isolated `DATA_DIR`, four seeded restore points, retention settings read back before/after. Matrix per handler: no credential → 401; `manage` key, inference-only key, admin `oma_` token → 403 with the retention settings unchanged and all four files still on disk; dashboard session / local CLI subject / login disabled → 200 and the handler really does its job (PATCH persists and never prunes; DELETE prunes down to the requested count). Positive `GET` and `PUT` with a `manage` key → 200. - **RED** (commit `3429f715c0`, on the unfixed route): `ℹ tests 17 / pass 5 / fail 12` — root failure `actual: 200, expected: 403` on the manage-key `DELETE`; the unguarded handler then persisted `1/1` and pruned all four files, cascading into every later "nothing changed" assertion; plus `401 vs 200` for the local CLI subject on both handlers. - **GREEN** (fix commit `b9aba368db`): `17/17`; on the merged tree together with `backup-restore-require-operator` + `db-backup-extended`: `56/56`. - **Related suites** (`grep -rlE "db-backup|backup-restore|backup-export|operatorCredential|credential-export|cli-backup" tests/` → 30 files): 29 node:test files in 3 batches → 75/75, 117/117, 127/127; 1 vitest file (`tests/unit/ui/system-storage-backup-auth-envelope-14846.test.tsx`) → 2/2. No assertion removed or weakened anywhere. ## Validation | Check | Result | | --- | --- | | `npx prettier --check` (7 touched files) | OK | | `npx eslint --suppressions-location config/quality/eslint-suppressions.json` (route + test) | 0 errors | | `check-complexity-ratchets.mjs --base-ref <merge-base>` | OK (complexity 0 / cognitive 0 on new code) | | `check:file-size --base-ref <merge-base>` | OK (no frozen file grown) | | `scripts/dev/typecheck-touched.sh --base origin/release/v3.8.52` | 0 errors in touched files; 1 error in `tests/helpers/managementSession.ts(62,5)` TS2322 — file byte-identical to the base (inherited, same helper #16899 imports) | | `check:openapi-security-tiers` / `check:api-docs-refs` / `check:openapi-coverage` / `check:changelog-integrity` | PASS | | `check:docs-all` | docs-sync, frontmatter, doc-links PASS; `check:fabricated-docs` fails on `/api/balance` in `docs/reference/ENVIRONMENT.md:368` — file identical to the base (inherited) | | i18n doc mirrors (`npm run i18n:run -- --files=<3 docs>`) | translation backend answered **404** (`DEPLOYMENT_NOT_FOUND`), nothing written. The drift gate was already red on the base with 34 drifted sources, and all three docs I edited were already among them (state hash ≠ base content), so this PR adds no new drift entry; the 65 mirrors still need a real `i18n:run` once the backend is back. No blanket `--adopt` (it would mask the 34 genuine stale sources). | ## Removed / rewritten assertions, allowlist and suppression deltas None. `config/quality/eslint-suppressions.json` and `config/quality/test-masking-allowlist.json` untouched. ## References Refs #16899 (precedent: restoring a backup requires an operator credential). No issue to close. ## Out of scope - `docs/openapi.yaml` has no `delete` operation for `/api/db-backups` at all (only `get`/`post`/`patch`); adding one would change what the next `npm run build:cli-api` generates (a new `delete-api-db-backups` CLI command + locale keys), so the 403 is documented on `patch` only. - `bin/cli/api-commands/system.mjs` still describes `post-api-db-backups` as "Create database backup" (the pre-#16899 summary; `POST` is the restore-by-id). - Inherited base reds named above (#15306, `/api/balance` doc claim, `managementSession.ts` TS2322, i18n drift of 34 sources).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Exporting stored credentials now requires the instance operator. A dashboard session, the local CLI, or login disabled is accepted. API keys, including
manage/admin-scoped ones, are not.GET /api/settings/export-json(decrypted provider connections + raw API keys)isAuthenticated()accepts, so a manage key got 200GET /api/db-backups/export(the SQLite file)GET /api/db-backups/exportAll(full archive)oma_access tokens. The response is built withcreateErrorResponseand does not includeerr.message./api/settings/databaseis unchanged:GETreturns the database settings with cache secrets redacted, andPATCH/PUTupdate them. No method there downloads data.How
The operator check already existed in two places: sync token issuance (
src/lib/sync/issuer.ts, #16478) and admin CLI token minting (resolveOperatorCredential()insrc/lib/api/operatorCredential.ts, #16489).operatorCredential.tsnow exportsrequireOperatorCredential(request, { forbiddenMessage, allowInternalService? }). It runsrequireManagementAuth()first, so 401 and invalid-key statuses do not change. Then it callsresolveOperatorCredential()and returns 403 for machine credentials, or 503 if the auth backend throws.resolveOperatorCredential()takes an opt-inallowInternalServiceoption.authorizeSyncTokenIssuer()is now a thin wrapper over the shared helper withallowInternalService: true. It keeps the same order (open → session → internal → cli), the same issuer kinds and the same 403 message.CREDENTIAL_EXPORT_FORBIDDEN_MESSAGE.exportAllmoved intobuildFullExportResponse()so the new guard does not trip the complexity ratchet.Local CLI is allowed. The CLI token only works on loopback and is tied to the machine. Whoever holds it already has the data directory, which contains the database and the encryption key. The CLI can also mint a sync token, and a sync token opens
/api/sync/bundle, which returns the same decrypted credentials. Refusing the CLI here would not protect anything and would contradict the sync-token rule. Before this change the export handlers returned 401 to the pipeline-stamped CLI subject, becauseisAuthenticated()ignores that stamp. So the CLI gains export access with this PR.The internal-service token is not allowed for exports. No internal service calls these routes, so it stays opt-in for sync issuance only.
Out of scope / follow-ups
POST /api/settings/import-json,POST /api/db-backups/import, and the restore viaPOST /api/db-backups./api/providers/[id]/{claude,codex}-auth/export(raw OAuth tokens),/api/providers/client(connection secrets for the dashboard), and/api/logs/export(prompts and responses).omniroute sync pull(bin/cli/commands/sync.mjs) sendsPOST /api/db-backups/exportAll, but the route only hasGET. That call was already broken before this PR.Tests
tests/unit/credential-export-require-session.test.tshas 24 cases: 8 per endpoint, covering manage key, admin key,oma_admin token, session, session plus manage key, local CLI, unauthenticated, and login disabled.oma_token got 401 instead of 403, and the local CLI got 401 instead of 200.exportAllrefactor, 78/78 on the affected subset.Validation
eslint --suppressions-location config/quality/eslint-suppressions.jsonon the touched files: 0check-complexity-ratchets --base-ref <merge-base>: OK (complexity 0, cognitive 0)check:file-size: only the known base overages (responseSanitizer.ts,error.ts,translator-openai-responses-req.test.ts,vscode-token-routes.test.ts)typecheck:core: cleantscon the touched files: no errors in them. The one error reported is in the existingtests/helpers/managementSession.ts, which this PR does not change.check:docs-all: pass (drift warnings are advisory and were already there)check:changelog-integrity: OKDocs:
AUTHZ_GUIDE.md,ROUTE_GUARD_TIERS.md,API_REFERENCE.md, andopenapi.yaml(403 responses).