Skip to content

Commit 179598e

Browse files
fix(service-settings): a secret overwrite's audit row records the previous value's keyed digest as old_hash (#26600)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XFUNxcpaxEv3ncMRqqPo9pj)_ Fixes #26534 Clause-②: no Metadata-protocol: none ## 维护者速读 - **改了什么**:密钥类设置(`encrypted: true`)被覆盖时,审计行 `old_hash` 不再恒为 null,而是记上一次值的带密钥摘要(`hmac-sha256:…`),与 `new_hash` 同一算法;所以覆盖时 `old_hash` 等于上一行的 `new_hash`,同值重写时 `old_hash == new_hash`。 - **为什么改**:列声明“仅在之前未设置时为 null”,但密钥行一直写 null,运维无法从审计判断一次轮换是真换了密钥还是写回了同一个值,而这恰是最需要判断的行。 - **风险与回滚**:低。只有“密钥覆盖”这一种写多一次 `sys_secret` 读和一次解密,且在旧密文被回收之前;读不到或解不开时记 null,写入照常成功;明文只在函数内用于算摘要,不记日志、不返回、不存储;从不写无密钥哈希。回滚即 revert 本 PR。 - **先看**:`packages/services/service-settings/src/settings-service.ts`(`setMany` 中 `upsertRow` 与 `reapRotatedSecret` 之间新增的 `oldHash`,以及 `previousSecretAuditDigest`);`src/settings-audit-old-hash.test.ts`(`#26534` 一组用例)。 - **两分钟验证**:`pnpm --filter @objectstack/service-settings exec vitest run src/settings-audit-old-hash.test.ts`,预期 14/14 通过,其中 `#26534` 八条。 - **需要你做的**:无,席位落地。 ## What changes `SettingsService.setMany` now computes the audit's `oldHash` after `upsertRow` (whose existence probe names the previous row) and before `reapRotatedSecret` (which destroys the previous secret's `sys_secret` row): - **Secret write** (`encrypted: true` key): `old_hash` is `previousSecretAuditDigest(...)`, the same keyed digest `new_hash` records (`secretAuditDigest`, the provider's `keyedDigest`), taken over the previous value. The previous plaintext is read through a new private `decryptStored(row)`, factored out of `materialiseRow` with no behaviour change there, and is digested and dropped inside that frame. `null` when the previous row held no secret (no row, a reset row, or a row written while the key was not secret-valued), when no keyed digest is available (checked before the read, so that write pays nothing), or when the previous value cannot be read back (handle gone, decrypt refused; the latter warns once on the logger with the key and the decrypt error, never the value). It never fails the write. - **Non-secret write**: unchanged, `previousAuditDigest(previous)` (#26465). - With no `auditWriter` wired, nothing is computed. Never a digest of ciphertext (it changes on every re-encryption), never an unkeyed hash of the plaintext. The secret reaper (#8030 `previousEnc`) is unchanged. Changeset: `@objectstack/service-settings` patch. ## Why `sys_setting_audit.old_hash` and `sys_platform_setting_audit.old_hash` declare "Null when previously unset", but the secret arm passed `oldHash: isEncrypted ? null : ...`, so every secret overwrite recorded null. The `encrypted` column exists to flag rotation events, and without `old_hash` an operator cannot tell a rotation that changed the secret from one that rewrote the same value. Rulings: triage `6436617436` and the server seat 2 ruling, arm (a). ## Tests All through `PUT /api/settings/:namespace` on the #26465 harness (real `ObjectQL`, real platform objects, the plugin's own `sys_secret` store and audit writer). The old secret CONTROL that pinned `old_hash: null` is replaced by a `#26534` block: | Case | Asserted | |---|---| | first secret write | `old_hash` null, keyed `new_hash` | | secret overwrite | `old_hash` keyed and equal to the previous row's `new_hash`; differs from the new `new_hash` | | same-value secret rewrite | `old_hash == new_hash == ` previous row's `new_hash` | | secret → reset → set | reset: `old_hash` = previous keyed digest, `new_hash` null; next set: `old_hash` null | | cost | a non-secret write reads no `sys_secret`; a first write reads only its own handle; an overwrite reads the previous handle exactly one more time than a boot with no keyed digest, and that handle is still reaped afterwards | | no keyed digest | both columns null, write lands | | previous handle gone | `old_hash` null, keyed `new_hash`, write lands and reads back | | manifest flips the key non-secret → secret → non-secret | both crossings record `old_hash` null; no unkeyed digest of the secret | Every secret case keeps the "no plaintext and no unkeyed SHA-256 of the secret anywhere in the row" assertion. **Ablation** (committed fix at `d66cfb39`, `scripts/ablation-replace.mjs`): `await this.previousSecretAuditDigest(namespace, key, previous)` replaced by `null`, anchor 1 → 0, blob `148247002eaf` → `0994b6cb2558`. Result: 4 failed, 10 passed (overwrite, same-value, reset, cost). Restored: blob == HEAD `148247002eaf`, `git diff HEAD` empty. Package at `d66cfb39`: `pnpm --filter @objectstack/service-settings test` Test Files 61 passed (61), Tests 1014 passed (1014); `typecheck` (`tsc --noEmit`, `include: ["src"]`, tests included) exit 0. `eslint --no-inline-config` on `packages/services/service-settings`: exit 0. ## Acceptance notes - **Cost measured.** The write path already decrypts the key's current value once in `validatePatch` (it resolves `get()` for every touched key), so a secret overwrite now makes two reads of the previous handle where it made one. That earlier read is the cascade's effective value, which can come from another rung or the environment, so it is not the row's previous value and is not reused. - **Crossing between non-secret and secret is not reachable through PUT.** `encrypted` comes from the manifest, and only a re-registration (`registerManifest` replaces the namespace's entry; in practice a deploy that edits the manifest while stored rows persist) changes it. Both crossings record `old_hash` null: non-secret → secret has no previous secret to key-digest, and secret → non-secret must not write an unkeyed digest of the previous secret. Pinned by the last case. - **QA checklist.** `platform-core.settings-audit-secret-fingerprint` says nothing about a secret row's `old_hash`, and no digest algorithm changed, so its revision is not bumped. Its "no unkeyed digest in any row" clause still holds: the new `old_hash` is keyed. ## Verification <details><summary>Derived gate families (`dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, run at d66cfb3)</summary> 60 derived, 59 run with exit 0, 1 NOT MEASURED: `pnpm check:dual-build-cjs-loads` exit 3 (prerequisite not met: needs a full build; CI runs it). `node scripts/check-plugin-teardown-shape.mjs --self-test` first exited 3 on the shallow clone; after `git fetch origin 621a487` (its pinned fixture) it ran: exit 0, 48 cases. `dispatch-gates.mjs --ran`: "60 derived famil(ies) accounted for — 59 run, 1 NOT-MEASURED (1 DERIVED from a recorded exit 3)". `test:repo` was not run locally (the diff touches no repo-project test); CI Test Core runs it. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (No `packages/spec` change.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. (Scoped to the touched package.) - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (Not touched.) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FEkQ5nYPKQq7oxF4u1xFKB Co-authored-by: Claude <noreply@anthropic.com>
1 parent c8ed4c9 commit 179598e

3 files changed

Lines changed: 295 additions & 47 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/service-settings': patch
3+
---
4+
5+
fix(service-settings): a secret overwrite's audit row records the previous value's keyed digest as `old_hash` (#26534)
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
Ruling-ref: #26534 triage `6436617436`; the server seat 2 ruling, arm (a)
10+
11+
A secret-valued setting's overwrite used to record `old_hash` null on `sys_setting_audit` / `sys_platform_setting_audit`, though the column declares null only when the key was previously unset. It now records the same keyed digest (`hmac-sha256:…`) that `new_hash` records, taken over the previous value, so an overwrite's `old_hash` equals the previous row's `new_hash` and a rewrite of the same secret reads `old_hash == new_hash`. It stays null when the key was unset or reset, when no keyed digest is available, or when the previous value can no longer be read, and it is never an unkeyed hash of the secret. Only a secret overwrite pays for it: one `sys_secret` read and one decrypt of the previous value, before the rotated-away secret is reaped.

‎packages/services/service-settings/src/settings-audit-old-hash.test.ts‎

Lines changed: 186 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
* expected digests are computed here with `node:crypto`, independently of the
1717
* implementation.
1818
*
19-
* A secret-valued key is the CONTROL: its keyed fingerprint path
20-
* (`hmac-sha256:…`, `secretAuditDigest`) is untouched, so its rows keep a null
21-
* `old_hash` and a keyed `new_hash`.
19+
* [#26534] A secret-valued key records the SAME keyed digest on both columns
20+
* (`hmac-sha256:…`, `secretAuditDigest`): `old_hash` is the keyed digest of
21+
* the previous value, read back and decrypted inside the write, so it equals
22+
* the previous row's `new_hash`. No row ever carries an unkeyed digest of a
23+
* secret.
2224
*/
2325

2426
import { createHash } from 'node:crypto';
@@ -182,19 +184,27 @@ class MockHttp implements IHttpServer {
182184
}
183185

184186
/** The pieces the running server binds: real engine, real objects, the plugin's audit writer, real routes. */
185-
async function boot() {
187+
async function boot(opts: { cryptoProvider?: LocalCryptoProvider } = {}) {
186188
const engine = new ObjectQL();
187189
const { driver, rowsOf } = makeMemoryDriver();
188190
engine.registerDriver(driver, true);
189191
await engine.init();
190192
for (const o of [SysSetting, SysPlatformSetting, SysSecret, SysSettingAudit, SysPlatformSettingAudit]) {
191193
engine.registry.registerObject(o as any, OWNER_PACKAGE);
192194
}
195+
// The plugin's own `sys_secret` store, with its reads recorded: which
196+
// handles a write dereferences is what the cost cases measure.
197+
const store = (new SettingsServicePlugin() as any).buildSecretStore(engine);
198+
const secretReads: string[] = [];
199+
const secretStore = {
200+
...store,
201+
get: async (id: string) => { secretReads.push(id); return store.get(id); },
202+
};
193203
const svc = new SettingsService({
194204
env: {},
195205
engine: withEngineRegistry(wrapEngineAsSettingsEngine(engine as any), engine),
196-
cryptoProvider: new LocalCryptoProvider(),
197-
secretStore: (new SettingsServicePlugin() as any).buildSecretStore(engine),
206+
cryptoProvider: opts.cryptoProvider ?? new LocalCryptoProvider(),
207+
secretStore,
198208
auditWriter: buildSettingAuditWriter(engine as any),
199209
});
200210
for (const m of [TENANT_MANIFEST, GLOBAL_MANIFEST, USER_MANIFEST]) svc.registerManifest(m);
@@ -238,7 +248,19 @@ async function boot() {
238248
const auditOf = (object: 'sys_setting_audit' | 'sys_platform_setting_audit', key: string) =>
239249
[...rowsOf(object).values()].filter((r) => r.key === key);
240250

241-
return { put, auditOf, asUser: (id: string) => { userId = id; } };
251+
/** The `sys_secret` handle the global row of `key` names now. */
252+
const handleOf = (key: string) =>
253+
[...rowsOf('sys_platform_setting').values()].find((r) => r.key === key)?.value_enc as string | undefined;
254+
255+
return {
256+
put,
257+
auditOf,
258+
asUser: (id: string) => { userId = id; },
259+
svc,
260+
rowsOf,
261+
secretReads,
262+
handleOf,
263+
};
242264
}
243265

244266
describe('#26465 — old_hash / new_hash on the tenant rung (sys_setting_audit)', () => {
@@ -330,8 +352,36 @@ describe('#26465 — the global rung (sys_platform_setting_audit) records the sa
330352
[sha256Of('smtp2.example.com'), sha256Of('smtp2.example.com')],
331353
]);
332354
});
355+
});
356+
357+
/** The unkeyed digests a leak of `value` would carry, for the "never in any row" check. */
358+
const unkeyedOf = (value: string) => [
359+
sha256Of(value).slice('sha256:'.length),
360+
createHash('sha256').update(value, 'utf8').digest('hex'),
361+
];
362+
const expectNoUnkeyedDigest = (rows: Record<string, unknown>[], values: string[]) => {
363+
for (const row of rows) {
364+
for (const v of values) {
365+
expect(JSON.stringify(row)).not.toContain(v);
366+
for (const d of unkeyedOf(v)) expect(JSON.stringify(row)).not.toContain(d);
367+
}
368+
}
369+
};
370+
371+
describe('#26534 — a secret value records the keyed digest of its previous value as old_hash', () => {
372+
it('first write: old_hash null, keyed new_hash', async () => {
373+
const { put, auditOf } = await boot();
374+
375+
await put('hash_global', { api_key: 'pw-alpha' });
376+
377+
const rows = auditOf('sys_platform_setting_audit', 'api_key');
378+
expect(rows).toHaveLength(1);
379+
expect(rows[0]).toMatchObject({ action: 'set', encrypted: true, old_hash: null });
380+
expect(rows[0].new_hash).toMatch(KEYED_SHAPE);
381+
expectNoUnkeyedDigest(rows, ['pw-alpha']);
382+
});
333383

334-
it('CONTROL — a secret value keeps its keyed path: keyed new_hash, null old_hash, no unkeyed digest anywhere', async () => {
384+
it('overwrite: old_hash is the previous row\'s keyed new_hash, and no unkeyed digest is anywhere in a row', async () => {
335385
const { put, auditOf } = await boot();
336386

337387
await put('hash_global', { api_key: 'pw-alpha' });
@@ -341,11 +391,135 @@ describe('#26465 — the global rung (sys_platform_setting_audit) records the sa
341391
expect(rows).toHaveLength(2);
342392
for (const row of rows) {
343393
expect(row.encrypted).toBe(true);
344-
expect(row.old_hash).toBeNull();
345394
expect(row.new_hash).toMatch(KEYED_SHAPE);
346395
}
347-
expect(rows[1].new_hash).not.toBe(rows[0].new_hash);
348-
const unkeyed = [sha256Of('pw-alpha'), sha256Of('pw-beta'), 'sha256:' + createHash('sha256').update('pw-alpha').digest('hex')];
349-
for (const row of rows) for (const d of unkeyed) expect(JSON.stringify(row)).not.toContain(d.slice('sha256:'.length));
396+
expect(rows[1].old_hash).toMatch(KEYED_SHAPE);
397+
expect(rows[1].old_hash).toBe(rows[0].new_hash);
398+
expect(rows[1].new_hash).not.toBe(rows[1].old_hash);
399+
expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']);
400+
});
401+
402+
it('same-value rewrite: old_hash equals new_hash, so a rotation that changed nothing is visible', async () => {
403+
const { put, auditOf } = await boot();
404+
405+
await put('hash_global', { api_key: 'pw-alpha' });
406+
await put('hash_global', { api_key: 'pw-alpha' });
407+
408+
const rows = auditOf('sys_platform_setting_audit', 'api_key');
409+
expect(rows).toHaveLength(2);
410+
expect(rows[1].old_hash).toMatch(KEYED_SHAPE);
411+
expect(rows[1].old_hash).toBe(rows[1].new_hash);
412+
expect(rows[1].old_hash).toBe(rows[0].new_hash);
413+
expectNoUnkeyedDigest(rows, ['pw-alpha']);
414+
});
415+
416+
it('set → reset → set: the reset carries the previous keyed digest and no new one; the next set starts from unset', async () => {
417+
const { put, auditOf } = await boot();
418+
419+
await put('hash_global', { api_key: 'pw-alpha' });
420+
await put('hash_global', { api_key: null });
421+
await put('hash_global', { api_key: 'pw-beta' });
422+
423+
const rows = auditOf('sys_platform_setting_audit', 'api_key');
424+
expect(rows.map((r) => r.action)).toEqual(['set', 'reset', 'set']);
425+
expect(rows[1]).toMatchObject({ old_hash: rows[0].new_hash, new_hash: null });
426+
expect(rows[1].old_hash).toMatch(KEYED_SHAPE);
427+
expect(rows[2].old_hash).toBeNull();
428+
expect(rows[2].new_hash).toMatch(KEYED_SHAPE);
429+
expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']);
430+
});
431+
432+
it('cost: only a secret overwrite pays one more read of the previous handle, and that secret is still reaped after it', async () => {
433+
/** The reads of the previous handle one overwrite makes, on a fresh boot. */
434+
const overwriteReads = async (provider?: LocalCryptoProvider) => {
435+
const { put, auditOf, rowsOf, secretReads, handleOf } = await boot({ cryptoProvider: provider });
436+
// A non-secret write never touches sys_secret.
437+
await put('hash_global', { host: 'smtp.example.com' });
438+
expect(secretReads).toEqual([]);
439+
// A first write has no previous handle to read: every read is of the
440+
// handle it just wrote (the response's re-resolve).
441+
await put('hash_global', { api_key: 'pw-alpha' });
442+
const first = handleOf('api_key')!;
443+
expect(first).toMatch(/^sec_/);
444+
expect(secretReads.every((id) => id === first)).toBe(true);
445+
secretReads.length = 0;
446+
await put('hash_global', { api_key: 'pw-beta' });
447+
// The rotated-away secret is reaped AFTER the old_hash read (#8030).
448+
expect(rowsOf('sys_secret').has(first)).toBe(false);
449+
expect(rowsOf('sys_secret').has(handleOf('api_key')!)).toBe(true);
450+
return {
451+
reads: secretReads.filter((id) => id === first).length,
452+
oldHash: auditOf('sys_platform_setting_audit', 'api_key')[1].old_hash,
453+
};
454+
};
455+
// The baseline: with no keyed digest the write skips the old_hash read,
456+
// and what is left is the read the write path already made before this
457+
// rule (`validatePatch` resolves the key's current value).
458+
const unkeyed = new LocalCryptoProvider();
459+
(unkeyed as any).keyedDigest = undefined;
460+
const baseline = await overwriteReads(unkeyed);
461+
const keyed = await overwriteReads();
462+
463+
expect(baseline.oldHash).toBeNull();
464+
expect(keyed.oldHash).toMatch(KEYED_SHAPE);
465+
expect(keyed.reads - baseline.reads).toBe(1);
466+
});
467+
468+
it('no keyed digest available: old_hash null like new_hash, and the write lands', async () => {
469+
const provider = new LocalCryptoProvider();
470+
(provider as any).keyedDigest = undefined;
471+
const { put, auditOf, svc } = await boot({ cryptoProvider: provider });
472+
473+
await put('hash_global', { api_key: 'pw-alpha' });
474+
await put('hash_global', { api_key: 'pw-beta' });
475+
476+
const rows = auditOf('sys_platform_setting_audit', 'api_key');
477+
expect(rows.map((r) => [r.old_hash, r.new_hash])).toEqual([[null, null], [null, null]]);
478+
expect((await svc.get('hash_global', 'api_key')).value).toBe('pw-beta');
479+
expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']);
480+
});
481+
482+
it('a previous handle that no longer resolves records old_hash null and the write still lands', async () => {
483+
const { put, auditOf, rowsOf, handleOf, svc } = await boot();
484+
485+
await put('hash_global', { api_key: 'pw-alpha' });
486+
rowsOf('sys_secret').delete(handleOf('api_key')!);
487+
await put('hash_global', { api_key: 'pw-beta' });
488+
489+
const rows = auditOf('sys_platform_setting_audit', 'api_key');
490+
expect(rows).toHaveLength(2);
491+
expect(rows[1].old_hash).toBeNull();
492+
expect(rows[1].new_hash).toMatch(KEYED_SHAPE);
493+
expect((await svc.get('hash_global', 'api_key')).value).toBe('pw-beta');
494+
});
495+
496+
it('a key whose manifest changes between non-secret and secret never records an unkeyed digest of a secret', async () => {
497+
// Not reachable through PUT: `encrypted` is the manifest's, and only a
498+
// re-registration (a deploy that edits the manifest) changes it.
499+
const { put, auditOf, svc } = await boot();
500+
const asSecret = (secret: boolean): SettingsManifest => ({
501+
...GLOBAL_MANIFEST,
502+
specifiers: [
503+
secret
504+
? { type: 'password', key: 'host', label: 'Host', required: false, encrypted: true }
505+
: { type: 'text', key: 'host', label: 'Host', required: false },
506+
],
507+
});
508+
509+
await put('hash_global', { host: 'plain-value' });
510+
svc.registerManifest(asSecret(true));
511+
await put('hash_global', { host: 'secret-value' });
512+
svc.registerManifest(asSecret(false));
513+
await put('hash_global', { host: 'plain-again' });
514+
515+
const rows = auditOf('sys_platform_setting_audit', 'host');
516+
expect(rows.map((r) => r.encrypted)).toEqual([false, true, false]);
517+
// non-secret → secret: the previous row held no secret, so no keyed digest of it.
518+
expect(rows[1].old_hash).toBeNull();
519+
expect(rows[1].new_hash).toMatch(KEYED_SHAPE);
520+
// secret → non-secret: the previous value was a secret, so no unkeyed digest of it.
521+
expect(rows[2].old_hash).toBeNull();
522+
expect(rows[2].new_hash).toBe(sha256Of('plain-again'));
523+
expectNoUnkeyedDigest(rows, ['secret-value']);
350524
});
351525
});

0 commit comments

Comments
 (0)