Repository navigation
Fix/16836 encrypt provider specific secrets - #16863
diegosouzapw merged 40 commits into
Conversation
…ses and legacy migration
… update responses
…aks nested credentials
…orage and audit logs
… from compliance audits
…rom dashboard rules
…yption in SQLite CRUD
…r metadata encryption
…ata during session dedup
…out breaking connection CRUD
…nstead of base URL
…rging across accounts
…, not shared base URL
diegosouzapw
left a comment
There was a problem hiding this comment.
Thanks for this — the approach (declared allowlist in providerSpecificDataSecrets.ts, lazy decrypt in both views, cookie dedup on plaintext) reads well and the two new test files are thorough. One regression blocks the merge:
tests/unit/db-encryption.test.ts → "connection field helpers encrypt and decrypt all supported credential fields" fails on b024237f32 (passes on the merge-base 906fb59a05):
+ actual - expected
{
accessToken: 'access-123',
apiKey: 'sk-123',
idToken: 'id-123',
+ providerSpecificData: undefined,
refreshToken: 'refresh-123',
untouched: 'keep-me'
}
Cause: decryptConnectionFields() now always spreads providerSpecificData into the result, so rows without that column gain an explicit providerSpecificData: undefined key (deepStrictEqual sees it; JSON.stringify would not, which is why the new tests do not catch it).
Minimal fix in src/lib/db/encryption.ts, in the returned object:
...(row.providerSpecificData !== undefined ? { providerSpecificData } : {}),(or only call mapProviderSpecificDataSecrets when row.providerSpecificData is set). Please add the "row without providerSpecificData keeps its exact shape" case to provider-specific-secrets-16836.test.ts so it stays covered.
Everything else I ran on your head is clean: both new test files (20/20), encryption-production-warn-13330, bulk-web-session-import, claude-web-session, complexity ratchets (new-code mode, 0 regressions). The 3 failures in deepseek-web-session-persistence-2942.test.ts are timeouts that fail identically on the base — not yours. CI on this repo is queued behind ~350 runs right now, so the local result is what we have for the moment.
Once the fix lands I will re-run and merge. One request: please stop force-pushing once it is green — the queue restarts on every push and I merge on a stable head.
|
Two more items from the targeted typecheck on
File-size and complexity ratchets are clean on your head; the only ESLint hit ( |
diegosouzapw
left a comment
There was a problem hiding this comment.
Re-checked on 60d979fd61: db-encryption.test.ts 7/7 plus both new suites (25/25 together), targeted typecheck 0 errors, complexity ratchets and file-size clean. Thanks for the quick turnaround.
3c3edb9
into
cheaper-inference:release/v3.8.52
|
É o mínimo que poderia fazer para ajudar num trabalho excelente como esse que desenvolveu |
…the shared allowlist The at-rest encryption of nested connection credentials now lives in PROVIDER_SPECIFIC_SECRET_FIELDS + mapProviderSpecificDataSecrets (#16863). Drop the branch's own allowlist and helper module and rewrite the three test files around the shared one: list-valued secrets (extraApiKeys), refresh-flow sub-tokens, web-session cookies, the first-use sweep of plaintext rows, mergeConnectionProviderSpecificData patches, the export/import round-trip, and the masked/stripped provider API responses. The production files go back to the release tip in this commit so the suite is red until the follow-up fix commits land.
…data secrets, mask them in provider responses (#16966)⚠️ base-red inherited: #15306 ## Summary Builds on #16863 (`3c3edb9ef3`), which declared the credential-bearing `providerSpecificData` fields in `PROVIDER_SPECIFIC_SECRET_FIELDS` and runs them through the connection AES-GCM envelope. This PR keeps that single allowlist and single encryption path and closes the gaps #16863 left open — a data-integrity / authorization hardening of the provider-connection store and its management API: - **Allowlist (same module, same helper)** — adds the `extraApiKeys` key-rotation pool (a *list* of full upstream API keys; `mapProviderSpecificDataSecrets` now maps string lists entry by entry and drops an entry that no longer decrypts instead of forwarding it upstream as a key), the sub-tokens refresh/mint flows write back into the metadata (`copilotToken`, `dcaToken`, `clientSecret`, `recaptchaV3Token`) and the web-session storage keys that hold a browser session (`cookie`, `token`, `sso`, `__Secure-*`, …). Identity/display fields (`workspaceId`, `chatgptUserId`, `username`, `uuid`, `email`, …) stay plaintext and SQLite-queryable. The response-only Codex handles get their own inventory (`PROVIDER_SPECIFIC_RESPONSE_ONLY_FIELDS`) in the same zero-import module. - **Provider API responses** — `sanitizeProviderSpecificDataForResponse` reads the shared allowlist instead of its own hand-written delete list; every declared field is stripped and `extraApiKeys` comes back masked with the same mask as `apiKey` (revealed in full only when key reveal is on). `PUT /api/providers/[id]` maps echoed masked placeholders back to the stored keys and drops orphans, so a placeholder can never overwrite a stored key. The compliance-audit redaction derives from the same inventories. - **Legacy rows** — rows written before the nested fields were encrypted are swept once per database instance on the providers module's first read (`ensureProviderSpecificDataSecretsEncrypted`, same lazy in-module style as the `api_keys` backfill in #16898): idempotent, transactional, `looksEncrypted` guard, no-op without `STORAGE_ENCRYPTION_KEY`, rows without plaintext secrets untouched byte-for-byte. - **Merge path** — `mergeConnectionProviderSpecificData` patches (quota fetchers refreshing a console cookie, executors minting a bearer) now go through `encryptConnectionFields` like a full-row write. - **Import path** — verified covered by the tip: `importProviderConnectionRecord` already calls `encryptConnectionFields`, which with the extended allowlist envelopes the pool too (round-trip test). No code change. Removed from this branch as duplicates of #16863: the branch's own allowlist (`src/shared/constants/providerSpecificDataSecrets.ts`) and encrypt/decrypt helper (`src/lib/db/providers/secretFields.ts`), and its edits to `src/lib/db/providers.ts` (lazy decrypt / dedup / serialization). The only `providers.ts` change left is the two sweep trigger calls. ## Behavior before / after | Surface | Before (tip) | After | | --- | --- | --- | | `GET /api/providers`, `GET /api/providers/[id]` | `extraApiKeys` returned in full; `copilotToken`, `cookie`, `clientSecret`, … returned in full | `extraApiKeys` masked like `apiKey`; every declared secret field stripped; non-secret metadata untouched | | `PUT /api/providers/[id]` echoing the response back (what the edit modal does) | masked placeholders would be stored as keys | placeholders resolve to the stored keys, orphans dropped, added/removed keys honored | | `provider_specific_data` on disk — `extraApiKeys`, refresh sub-tokens, web-session cookies | plaintext (list values skipped by the helper) | `enc:v1:` envelopes, every list entry | | Rows already on disk in plaintext | re-encrypted only on their next write | swept once on first read, idempotent | | `mergeConnectionProviderSpecificData` patch with a secret field | written in plaintext | enveloped; existing ciphertext not re-wrapped | | Compliance audit snapshot | hand-written redaction list | derived from the shared allowlist + response-only fields | | Undecryptable `extraApiKeys` entry (rotated key) | n/a | dropped from the list, `credentialDecryptFailed` raised | ## Tests RED (tree = release tip + the pure `apiKeyMask.ts` helper, branch's duplicate modules deleted; commit `f84021ac75`): the five files together — `ℹ tests 42 / pass 24 / fail 18`; all 18 failures in the three branch files (e.g. `GET /api/providers masks every extraApiKeys entry … list response must not carry the first extra key`, `sweep left sk-extra-one… in plaintext`). #16863's two files passed throughout. Log: `_artifacts/security-wave/red-provider-secrets.log`. GREEN (final head): same five files in one run — `ℹ tests 42 / pass 42 / fail 0`. - `tests/unit/provider-specific-data-secrets-response.test.ts` — route-level masking/stripping, PUT reconciliation (echo, add+remove, orphan), PUT response, reveal switch. - `tests/unit/provider-specific-data-secrets-at-rest.test.ts` — list envelopes on create/update, first-use sweep (idempotent, untouched rows byte-identical, second call returns 0), merge-patch envelopes, export→import round-trip, grok-web cookie dedup, passthrough without key. - `tests/unit/provider-specific-data-secrets-drift.test.ts` — iterates the live `PROVIDER_SPECIFIC_SECRET_FIELDS`: sanitizer, `encryptConnectionFields`/`decryptConnectionFields` round-trip + idempotence, audit redaction, every web-session requirement has a declared storage key, list handling in `mapProviderSpecificDataSecrets`, undecryptable list entry dropped, mask identity, `reconcileMaskedExtraApiKeys`. Related suites: the brief's grep matches **750** test files (`providerSpecificData` appears nearly everywhere), i.e. the full unit suite the same brief forbids. Ran the narrowed set of files referencing the symbols this change alters (sanitizer, provider routes, mask/exposure, merge path, import, encryption + lazy views, audit, cookie dedup, #16836): **67 files in 7 batches of ≤10 at concurrency 2 — 547 tests, 0 failures** (6 skipped in batch 0). Lists: `_artifacts/related-provider-secrets.txt` (750) and `_artifacts/related-provider-secrets-narrowed.txt` (67). ## Validation | Check | Result | | --- | --- | | `prettier --check` (14 touched files) | OK (only `src/lib/db/providers.ts` was flagged — the base file itself fails Prettier on #16863's `if (byName && …)` block; lint-staged collapsed it, 4-line inherited reformat) | | `eslint --suppressions-location …` (13 touched TS files) | 0 errors | | `check-complexity-ratchets --base-ref 3c3edb9` | complexity OK (10 → 10), cognitive OK (7 → 7) | | `check:file-size --base-ref 3c3edb9` | OK | | `check:migration-numbering` | OK (202 migrations) — no numbered migration added | | `check:cycles:ratchet` | OK (14 cycles, ceiling 14) | | `scripts/dev/typecheck-touched.sh --base origin/release/v3.8.52` | 0 errors in my changes; 4 inherited errors in `src/app/api/providers/route.ts` lines 84/394/434/519 — outside my single-line hunk (line 151), byte-identical on the base | | `check:docs-all` | not run — no `docs/**`, `.env.example`, `openapi.yaml`, `bin/cli/**` touched | ## Removed / rewritten assertions Relative to the branch's pre-reconciliation tests (`4f33e39941`): - at-rest: dropped "a legacy plaintext row is readable and gets re-encrypted on its next write" — duplicate of #16863's `provider-specific-secrets-db-16836.test.ts` ("Existing databases may contain plaintext … migrate them on the next write"). Added the merge-patch test and the `ensureProviderSpecificDataSecretsEncrypted(...) === 0` idempotence assertion. - drift: dropped the `PREVIOUSLY_STRIPPED` 29-key check and the "constant is well-formed: masked ∪ stripped" test — the former duplicates #16863's "audit redaction cannot drift from dashboard redaction" / "dashboard responses never expose …"; the latter asserted the deleted two-list constant. Dropped the assertions that `openrouterAccountKey`, `authorization`, `browserSessionKey` are secret keys — no code path writes them into `providerSpecificData` as credentials (`openrouterAccountKey` is a free-window bucket label), so they are not on the shared list. Re-pointed every remaining assertion at `PROVIDER_SPECIFIC_SECRET_FIELDS` / `encryptConnectionFields` / `mapProviderSpecificDataSecrets`. - response: unchanged. - #16863's two test files: untouched. Allowlist / suppression deltas: none (`config/quality/*` untouched). ## Reference (no public reference) ## Out of scope - Object-valued web-session blobs (`storageState`, `cookies`, `origins`) are stripped from responses but, like every non-string value, not enveloped at rest. - `runtimeKey` (Codex web) remains response-only, as in #16863; it was not promoted to an at-rest secret. - The four pre-existing type errors in `src/app/api/providers/route.ts` and the Prettier drift of `src/lib/db/providers.ts` on the base. Co-authored-by: jackyfan01 <227755718+jackyfan01@users.noreply.github.com>
Summary
Related Issues
Target Branch
See the Release Strategy, the
Contribution Golden Path and the
ROADMAP:
release/v3.8.xbranch (the highest openrelease/v*), notmainv4-featurelabel for the v4 channel?template=feature.mdor?template=bugfix.mdto the compare URLValidation
Choose the change type and focused loop from the
Contribution Golden Path. The full unit suite,
Vitest, the 60% coverage gate, and the production build all run in CI on this PR (#8329):
npm run lintTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes