Uh oh!
There was an error while loading. Please reload this page.
fix(service-settings): repoint sys_setting.value_enc on a secret rotation, and reap the retired ciphertext (#8030) - #8063
Conversation
…tion, and reap the retired ciphertext (#8030) A second PUT of a new value for an encrypted setting key answered 200 with a correctly redacted body, advanced `updated_at`, wrote an audit row and inserted a genuinely new `sys_secret` row holding the new plaintext — and left `sys_setting.value_enc` pointing at the FIRST handle. The effective secret never changed, so a leaked credential survived the rotation meant to retire it, with every visible signal saying the rotation had landed. The first write of any secret was correct, so it was invisible until the second. Cause: `sys_setting.value_enc` (and `updated_by`) are declared `readonly: true`, and the engine strips author-declared read-only columns from a NON-system caller's UPDATE payload (`stripReadonlyFields`, gated on `!opCtx.context?.isSystem`). `SettingsService.upsertRow` persisted through a plain, un-elevated `engine.update`, so the handle could never be repointed. The INSERT path is deliberately exempt from that strip (#3413) — which is exactly why the first write landed and every later one did not. Fix, on the service side only: - `upsertRow` performs its UPDATE as a system write. `SettingsService` is a privileged writer — the manifest capability gate, the env/upper-scope lock pre-flight and `validatePatch` have all run by then, and these are columns it owns rather than ones a caller forged. Same posture, same reason, as `ObjectQL.recomputeSummaries`' elevation (#7673). - `SettingsEngine.update` gains a `context` member and the `IDataEngine` adapter forwards it on BOTH branches. The settings row write takes the `multi` one (its `where` is the composite key, never an `id`), so the by-id branch is the half that would rot unnoticed. - `value_enc` STAYS `readonly: true`. The elevation is scoped to this one write, so an external caller reaching `sys_setting` through the data layer still cannot repoint a secret handle. A test fails if that flag is removed. Orphans are reaped rather than accepted: the `sys_secret` row a rotated-away handle named is deleted once the repoint has committed. An orphan is a decryptable copy of the credential the admin just retired, one more per rotation. The store's `delete` is optional and the call is best-effort — the new secret is already in force by then, and a failed cleanup is reported, never raised. Tests boot a real ObjectQL over the real `SysSetting` / `SysSecret` schemas and drive the real service through the real adapter, so the verdict is about the engine's actual strip rule rather than about a fake written to match the fix. `vitest.config.ts` aliases `@objectstack/objectql` and `@objectstack/core` to source, per `check-test-source-alias`. Fixes#8030 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AFizXytCQYaGCNvewL7TaE
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 5 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also reference the affected code. These are read-only:
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#8030
The defect
A second
PUTof a new value for an encrypted setting key answered 200 with a correctly redacted body, advancedupdated_at, wrote an audit row and inserted a genuinely newsys_secretrow holding the new plaintext — and leftsys_setting.value_encpointing at the first handle. The effective secret never changed.Every signal an administrator can see said the rotation landed. Rotating a leaked SMTP password or provider API key looked exactly like a rotation that worked, while the leaked credential stayed the one in force. The first write of any secret was correct, so it was invisible until the second.
Root cause — the filer's analysis held, verified end-to-end
sys_setting.value_enc(andupdated_by) are declaredreadonly: trueinpackages/platform-objects/src/system/sys-setting.object.ts, and the engine strips author-declared read-only columns from a non-system caller's UPDATE payload (stripReadonlyFields, gated onif (!opCtx.context?.isSystem)inpackages/objectql/src/engine.ts).SettingsService.upsertRowpersisted through a plain, un-elevatedengine.update, so the handle could never be repointed. The INSERT path is deliberately exempt from that strip (#3413) — exactly why the first write landed and every later one did not.The engine's own warning names the fix:
The fix — service side only, no cross-lane change
upsertRowperforms its UPDATE as a system write.SettingsServiceis a privileged writer: the manifest capability gate (assertPermitted), the env-lock / upper-scope-lock pre-flight andvalidatePatchhave all run by then, and these are columns it owns rather than ones a caller forged. Same posture and same measured reason asObjectQL.recomputeSummaries' elevation (crud-permission-matrix: an allowed member create of showcase_task returns 500 while the row is written (summary recompute runs under caller context) #7673).SettingsEngine.updategains acontextmember, and theIDataEngineadapter forwards it on both branches. The settings row write takes themultione (itswhereis the composite(namespace, key, scope, user_id), never anid), so the by-id branch is the half that would rot unnoticed — it is pinned.value_encstaysreadonly: true. The elevation is scoped to this one write, so an external caller reachingsys_settingthrough the data layer still cannot repoint a secret handle. That flag is a security control; removing it would have been the wrong direction on this defect. There is a test that fails if someone removes it.Nothing in
packages/platform-objectsorpackages/objectqlis touched — the STOP fork on the card did not fire.Orphan
sys_secretrows — reaped, not acceptedThe row a rotated-away handle named is deleted once the repoint has committed. An orphan is a decryptable copy of exactly the credential the admin just retired, accumulating one per rotation (the filer measured 7 → 8 → 9 across three writes) — so the exposure grows with the hygiene we ask operators to practise. Nothing else can reference the handle: ids are minted per
encrypt()call,value_encis the only column that holds one, and the audit trail records digests rather than handles, so it stays readable after the ciphertext is gone.The store's
deleteis optional (pre-existing fakes and the legacy inline-crypto path simply have none) and the call is best-effort: the new secret is already in force by then, so a failed cleanup is reported loudly, never raised.Verification
Tests boot a real
ObjectQLover the realSysSetting/SysSecretschemas and drive the realSettingsServicethrough the real adapter — the four pieces the running server bolts together — so the verdict is about the engine's actual strip rule rather than about a fake written to match the fix.Reverse-verification, with only the fix's two behavioural lines removed and everything else identical:
expected 'sec_d261eae…' not to be 'sec_d261eae…'at the second write,sys_secret1 → 2The 6 that stayed green with the fix removed are the ones that should: the first-write path, the readonly-for-external-callers pin, and the mask-echo no-op.
service-settingssuite: 413 passed / 20 files (was 401 / 19)tsc --noEmitinservice-settings: clean (0 errors — the package graduated out of the DEBT ledger inservice-settingshas notypecheckscript — turbo silently no-ops it, and ~5 test files carry pre-existing type errors behind the unwired gate #7925 and stays out)check-test-source-alias: OK, registry unchanged —@objectstack/objectqland@objectstack/coreare aliased to source in a newvitest.config.tsrather than added toKNOWN_UNALIASED_TEST_IMPORTScheck:engine-double-contract: OK — 166 pinned, 133 ledgered, 2 exempt (unchanged)Not regressed
PUTcarrying••••••••still leaves the stored ciphertext byte-identical; the key is dropped from the patch before the service sees it, so no write happens at all. Pinned.409 SETTINGS_LOCKEDfor env-locked secrets — untouched; the write path's pre-flight runs before any of this.Generated by Claude Code