From ea7f927ebd4d3674d1a8850c5a1c940c64b0ef4e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 05:20:41 +0000 Subject: [PATCH] fix(service-settings): redact encrypted setting values at the REST read boundary (#7522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/settings/:namespace returned the plaintext of every encrypted setting in the namespace — in values..value and repeated once more inside each cascadeChain entry — for both specifier flavours (type: 'password' and an explicit encrypted: true). Storage was always correct: sys_setting.value is null, value_enc holds a sec_ handle, and sys_secret holds aes-256-gcm ciphertext. The leak was entirely on the way out, where there was no redaction step at all. The endpoint requires setup.access, so this is defense-in-depth rather than privilege escalation — but every operator, integration, proxy, browser cache and HAR capture on that response path received the cleartext of every secret in the namespace, defeating the point of the value_enc + sys_secret split. The fix lives at the REST boundary and nowhere else, reusing the mask convention ADR-0100 pins for encrypted FIELDS on the generic CRUD path rather than inventing a sentinel: - read — a set secret is served as SETTINGS_SECRET_MASK (the same eight bullets as objectql's SECRET_MASK); an unset one stays null, so the response is presence-preserving. cascadeChain is masked entry by entry. source, locked, lockedReason and the 409 SETTINGS_LOCKED env-lock behaviour are untouched. - write — a submitted value equal to the mask means "unchanged" and the key is dropped from the patch, so a form echoing what it read cannot overwrite the stored secret with the mask's literal text. Scoped to secret keys, so a plain setting whose value genuinely is eight bullets still writes verbatim. PUT's own response is redacted the same way — it carries resolved values too, including cascade entries the caller never submitted. SettingsService still decrypts. materialiseRow(), get(), getNamespace(), snapshotOf() and createClient() keep handing real plaintext to in-process consumers, because the mail/sms/storage/auth plugins read their credentials through exactly that path; a test pins that round-trip so this cannot be "fixed" one layer down. New public API: SETTINGS_SECRET_MASK, redactSecretValues, dropEchoedSecretMasks and SettingsService.secretKeysOf(namespace) — the last one so the boundary reads the SAME encrypted-key set setMany consults, which is what stops the two sides drifting into "encrypted on write, cleartext on read". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CMcwKmYDRFjtfT8jCRRXwB --- .../settings-redact-encrypted-rest-read.md | 48 +++ .../services/service-settings/src/index.ts | 11 + .../src/settings-routes.test.ts | 312 ++++++++++++++++++ .../service-settings/src/settings-routes.ts | 28 +- .../src/settings-secret-redaction.ts | 132 ++++++++ .../service-settings/src/settings-service.ts | 26 ++ 6 files changed, 554 insertions(+), 3 deletions(-) create mode 100644 .changeset/settings-redact-encrypted-rest-read.md create mode 100644 packages/services/service-settings/src/settings-secret-redaction.ts diff --git a/.changeset/settings-redact-encrypted-rest-read.md b/.changeset/settings-redact-encrypted-rest-read.md new file mode 100644 index 0000000000..72957106d6 --- /dev/null +++ b/.changeset/settings-redact-encrypted-rest-read.md @@ -0,0 +1,48 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): redact encrypted setting values at the REST read boundary (#7522) + +`GET /api/settings/:namespace` returned the **plaintext** of every encrypted +setting in the namespace — in `values..value` and repeated once more in each +`cascadeChain` entry. Both specifier flavours were affected: `type: 'password'` +and an explicit `encrypted: true`. Storage was never the problem +(`sys_setting.value` is null, `value_enc` holds a `sec_` handle, and `sys_secret` +holds aes-256-gcm ciphertext); the leak was entirely on the way out. + +The endpoint requires `setup.access`, so this is defense-in-depth rather than +privilege escalation — but every operator, integration, proxy, browser cache and +HAR capture on that response path received the cleartext of every secret in the +namespace, which is precisely what the `value_enc` + `sys_secret` split exists to +prevent. + +**What changed.** The REST handlers now redact before the payload leaves the +process, reusing the mask convention ADR-0100 already pins for encrypted +*fields* on the generic CRUD path rather than inventing a sentinel: + +- **Read** — a set secret is served as `SETTINGS_SECRET_MASK` (`••••••••`, the + same eight bullets as objectql's `SECRET_MASK`); an unset one stays `null`. The + redaction is presence-preserving, so "configured vs not configured" is still + readable, and it covers `cascadeChain` entry by entry as well as the effective + value. `source`, `locked`, `lockedReason` and the `409 SETTINGS_LOCKED` + env-lock behaviour are untouched. +- **Write** — a submitted value equal to the mask means "unchanged" and the key + is dropped from the patch, so a form round-trip that echoes what it read does + not overwrite the stored secret with the mask's literal text. The drop is + scoped to secret keys: a plain setting whose value genuinely is eight bullets + still writes verbatim. `PUT`'s own response is redacted the same way — it + carries resolved values too, including cascade entries the caller never + submitted. + +**What deliberately did not change.** `SettingsService` still decrypts. +`materialiseRow()`, `get()`, `getNamespace()`, `snapshotOf()` and `createClient()` +keep returning real plaintext, because the mail / sms / storage / auth plugins +read their credentials through exactly that path. Redaction belongs to the REST +boundary and nowhere else; a test pins the in-process round-trip so this cannot +be "fixed" one layer down. + +New public API on `@objectstack/service-settings`: `SETTINGS_SECRET_MASK`, +`redactSecretValues`, `dropEchoedSecretMasks`, and +`SettingsService.secretKeysOf(namespace)` — published so a client can recognise a +masked read instead of comparing against a hard-coded string. diff --git a/packages/services/service-settings/src/index.ts b/packages/services/service-settings/src/index.ts index bb6ad09504..09038292a8 100644 --- a/packages/services/service-settings/src/index.ts +++ b/packages/services/service-settings/src/index.ts @@ -55,6 +55,17 @@ export { registerSettingsRoutes, type SettingsRoutesOptions, } from './settings-routes.js'; +// #7522 — the REST read mask for encrypted settings, plus the two halves of the +// boundary it defines. Published because a client has to be able to RECOGNISE a +// masked read: the console renders "configured" from it and echoes it back +// unchanged on save, and comparing against a string hard-coded in the console is +// exactly the drift this export prevents. The SERVICE layer is unaffected — it +// still hands real plaintext to in-process consumers; see the module header. +export { + SETTINGS_SECRET_MASK, + redactSecretValues, + dropEchoedSecretMasks, +} from './settings-secret-redaction.js'; export { settingsObjects, settingsPluginManifestHeader, diff --git a/packages/services/service-settings/src/settings-routes.test.ts b/packages/services/service-settings/src/settings-routes.test.ts index e2747629bf..f599c8d20a 100644 --- a/packages/services/service-settings/src/settings-routes.test.ts +++ b/packages/services/service-settings/src/settings-routes.test.ts @@ -6,6 +6,11 @@ import { SettingsService } from './settings-service.js'; import { registerSettingsRoutes } from './settings-routes.js'; import { brandingSettingsManifest } from './manifests/branding.manifest.js'; import { localizationSettingsManifest } from './manifests/localization.manifest.js'; +import { SETTINGS_SECRET_MASK } from './settings-secret-redaction.js'; +// `InMemoryCryptoProvider` is a value-only alias (`export const … = LocalCryptoProvider`), +// so the class itself is the one that can also be spelled as a type. +import { LocalCryptoProvider } from './local-crypto-provider.js'; +import type { SettingsManifest } from '@objectstack/spec/system'; class MockHttp implements IHttpServer { routes = new Map(); @@ -304,3 +309,310 @@ describe('settings-routes', () => { ]); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// #7522 — encrypted settings are REDACTED at the REST read boundary. +// +// The service decrypts on purpose and keeps doing so; these cases pin the +// boundary in both directions: nothing ciphertext-backed leaves over HTTP, and +// an in-process consumer still receives the real plaintext. Both specifier +// flavours are covered — `type: 'password'` (implicitly encrypted) and an +// explicit `encrypted: true` on a non-password type — because `registerManifest` +// folds them into one set and a fix that only saw one of them would still leak. +// ───────────────────────────────────────────────────────────────────────────── + +const SMTP_PLAINTEXT = 'smtp-pa55word-plaintext'; +const TOKEN_PLAINTEXT = 'webhook-token-plaintext'; +const GLOBAL_PLAINTEXT = 'global-scope-plaintext'; + +/** Two secret flavours + one plain control key, resolved down the full cascade. */ +const secretsManifest: SettingsManifest = { + namespace: 'secretsns', + version: 1, + label: 'Secrets', + // `user` so the cascade walks global → tenant → user and `cascadeChain` + // carries more than one entry to leak through. + scope: 'user', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + // Flavour A — implicitly encrypted because the TYPE is `password`. + { type: 'password', key: 'smtp_password', label: 'SMTP password', required: false }, + // Flavour B — an ordinary type carrying an explicit `encrypted: true`. + { type: 'text', key: 'webhook_token', label: 'Webhook token', required: false, encrypted: true }, + // Control — never encrypted, must survive the redaction untouched. + { type: 'text', key: 'smtp_host', label: 'Host', required: false, default: 'smtp.example.com' }, + ], +}; + +const secretAdmin = () => ({ + enforced: true, + permissions: ['setup.access', 'setup.write'], + userId: 'usr_1', +}); + +function makeSecretStack() { + const secretRows = new Map(); + const cryptoProvider = new LocalCryptoProvider(); + const svc = new SettingsService({ + env: {}, + cryptoProvider, + secretStore: { + async insert(row) { secretRows.set(row.id, row); return { id: row.id }; }, + async get(id) { return secretRows.get(id) ?? null; }, + async update(id, patch) { secretRows.set(id, { ...secretRows.get(id), ...patch }); }, + }, + }); + svc.registerManifest(secretsManifest); + const http = new MockHttp(); + registerSettingsRoutes(http, svc, { contextFromRequest: secretAdmin }); + return { svc, http, secretRows, cryptoProvider }; +} + +/** + * Seed an upper-scope (`global`) encrypted row directly, exactly as `setMany` + * would write it — ciphertext in the secret store, only the `sec_` handle on the + * setting row. The public write path resolves one scope per key, so this is the + * only way to give `cascadeChain` a second ciphertext-backed entry. + */ +async function seedGlobalSecret( + svc: SettingsService, + secretRows: Map, + cryptoProvider: LocalCryptoProvider, + key: string, + plaintext: string, +) { + const handle = await cryptoProvider.encrypt(plaintext, { namespace: 'secretsns', key }); + secretRows.set(handle.id, { + id: handle.id, + namespace: 'secretsns', + key, + kms_key_id: handle.kmsKeyId, + alg: handle.alg, + version: handle.version, + ciphertext: handle.ciphertext, + }); + await (svc as any).upsertRow({ + namespace: 'secretsns', + key, + scope: 'global', + user_id: null, + value: null, + value_enc: handle.id, + encrypted: true, + }); + return handle.id; +} + +describe('settings-routes — #7522 encrypted values are redacted at the REST boundary', () => { + it('GET /:ns leaks no ciphertext-backed cleartext — both flavours, value AND cascadeChain', async () => { + const { svc, http, secretRows, cryptoProvider } = makeSecretStack(); + + // Written through the trusted in-process path, the way a seed/bootstrap or + // a plugin would. + await svc.set('secretsns', 'smtp_password', SMTP_PLAINTEXT, { userId: 'usr_1' }); + await svc.set('secretsns', 'webhook_token', TOKEN_PLAINTEXT, { userId: 'usr_1' }); + await seedGlobalSecret(svc, secretRows, cryptoProvider, 'smtp_password', GLOBAL_PLAINTEXT); + + const h = http.routes.get('GET /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ params: { namespace: 'secretsns' } }); + await h(req, res); + + expect(state.status).toBe(200); + + // The whole-body assertion is the one that cannot be satisfied by masking + // only the places we happened to think of. + const wire = JSON.stringify(state.body); + expect(wire).not.toContain(SMTP_PLAINTEXT); + expect(wire).not.toContain(TOKEN_PLAINTEXT); + expect(wire).not.toContain(GLOBAL_PLAINTEXT); + + // …and the specific places the issue names, so a future regression says + // WHICH surface broke rather than just "a string appeared". + const values = state.body.data.values; + expect(values.smtp_password.value).toBe(SETTINGS_SECRET_MASK); + expect(values.webhook_token.value).toBe(SETTINGS_SECRET_MASK); + for (const key of ['smtp_password', 'webhook_token']) { + const chain = values[key].cascadeChain as Array<{ scope: string; value: unknown }>; + expect(chain.length).toBeGreaterThan(0); + for (const entry of chain) { + expect([SETTINGS_SECRET_MASK, null]).toContain(entry.value); + } + } + // The global entry is present and masked — a second ciphertext-backed + // scope, not an artefact of the user row being the only one. + expect(values.smtp_password.cascadeChain).toEqual( + expect.arrayContaining([expect.objectContaining({ scope: 'global', value: SETTINGS_SECRET_MASK })]), + ); + + // The non-encrypted control key is untouched. + expect(values.smtp_host.value).toBe('smtp.example.com'); + }); + + it('redaction is presence-preserving: an UNSET secret stays null, not a mask', async () => { + const { svc, http } = makeSecretStack(); + await svc.set('secretsns', 'smtp_password', SMTP_PLAINTEXT, { userId: 'usr_1' }); + + const h = http.routes.get('GET /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ params: { namespace: 'secretsns' } }); + await h(req, res); + + // Set vs unset stays observable — the console renders "configured" from it. + expect(state.body.data.values.smtp_password.value).toBe(SETTINGS_SECRET_MASK); + expect(state.body.data.values.webhook_token.value).toBeNull(); + }); + + it('`source` and `locked` survive the redaction unchanged', async () => { + const { svc, http } = makeSecretStack(); + await svc.set('secretsns', 'webhook_token', TOKEN_PLAINTEXT, { userId: 'usr_1' }); + + const h = http.routes.get('GET /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ params: { namespace: 'secretsns' } }); + await h(req, res); + + expect(state.body.data.values.webhook_token.source).toBe('user'); + expect(state.body.data.values.webhook_token.locked).toBe(false); + expect(state.body.data.values.smtp_host.source).toBe('default'); + }); + + it('an env-locked secret is masked while `source: env` / `locked` / 409 SETTINGS_LOCKED are unchanged', async () => { + // The env override is itself a secret value; it must not ride out on the + // read path either, and the lock affordances must keep working. + const svc = new SettingsService({ env: { OS_SECRETSNS_SMTP_PASSWORD: 'env-supplied-secret' } }); + svc.registerManifest(secretsManifest); + const http = new MockHttp(); + registerSettingsRoutes(http, svc, { contextFromRequest: secretAdmin }); + + const read = http.routes.get('GET /api/settings/:namespace')!; + const r1 = makeReqRes({ params: { namespace: 'secretsns' } }); + await read(r1.req, r1.res); + expect(JSON.stringify(r1.state.body)).not.toContain('env-supplied-secret'); + expect(r1.state.body.data.values.smtp_password.value).toBe(SETTINGS_SECRET_MASK); + expect(r1.state.body.data.values.smtp_password.source).toBe('env'); + expect(r1.state.body.data.values.smtp_password.locked).toBe(true); + expect(r1.state.body.data.values.smtp_password.lockedReason).toContain('OS_SECRETSNS_SMTP_PASSWORD'); + expect(r1.state.body.data.values.smtp_password.cascadeChain).toEqual([ + expect.objectContaining({ scope: 'env', value: SETTINGS_SECRET_MASK, locked: true }), + ]); + + const write = http.routes.get('PUT /api/settings/:namespace')!; + const r2 = makeReqRes({ params: { namespace: 'secretsns' }, body: { smtp_password: 'new' } }); + await write(r2.req, r2.res); + expect(r2.state.status).toBe(409); + expect(r2.state.body.error.code).toBe('SETTINGS_LOCKED'); + }); + + // ── the echoed-mask write, i.e. the second bug a redaction fix introduces ── + + it('PUTting the echoed mask back is a NO-OP — the stored secret is not overwritten', async () => { + const { svc, http, secretRows } = makeSecretStack(); + await svc.set('secretsns', 'smtp_password', SMTP_PLAINTEXT, { userId: 'usr_1' }); + const handlesBefore = [...secretRows.keys()]; + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ + params: { namespace: 'secretsns' }, + body: { smtp_password: SETTINGS_SECRET_MASK }, + }); + await h(req, res); + + expect(state.status).toBe(200); + expect(state.body.error).toBeUndefined(); + // No new ciphertext row: the mask was never encrypted and stored. + expect([...secretRows.keys()]).toEqual(handlesBefore); + // And the in-process read still yields the ORIGINAL plaintext — not the + // mask's literal text, which is what an unguarded write would have stored + // (and which would decrypt back to itself, so nothing would look wrong + // until the SMTP login failed). + const resolved = await svc.get('secretsns', 'smtp_password', { userId: 'usr_1' }); + expect(resolved.value).toBe(SMTP_PLAINTEXT); + expect(resolved.value).not.toBe(SETTINGS_SECRET_MASK); + }); + + it('the echoed mask inside the read-shape {values:{k:{value}}} envelope is a no-op too', async () => { + const { svc, http } = makeSecretStack(); + await svc.set('secretsns', 'webhook_token', TOKEN_PLAINTEXT, { userId: 'usr_1' }); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + // Exactly what GET now returns, echoed back wholesale by a form save. + const { req, res, state } = makeReqRes({ + params: { namespace: 'secretsns' }, + body: { + values: { + webhook_token: { value: SETTINGS_SECRET_MASK, source: 'user', locked: false }, + }, + }, + }); + await h(req, res); + + expect(state.status).toBe(200); + expect((await svc.get('secretsns', 'webhook_token', { userId: 'usr_1' })).value) + .toBe(TOKEN_PLAINTEXT); + }); + + it('a REAL new secret still writes, and the write RESPONSE is redacted too', async () => { + const { svc, http } = makeSecretStack(); + await svc.set('secretsns', 'smtp_password', SMTP_PLAINTEXT, { userId: 'usr_1' }); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ + params: { namespace: 'secretsns' }, + body: { smtp_password: 'a-genuinely-new-secret' }, + }); + await h(req, res); + + expect(state.status).toBe(200); + // The write took effect in the store… + expect((await svc.get('secretsns', 'smtp_password', { userId: 'usr_1' })).value) + .toBe('a-genuinely-new-secret'); + // …but the response body does not echo it back over the wire. + expect(JSON.stringify(state.body)).not.toContain('a-genuinely-new-secret'); + expect(state.body.data.values.smtp_password.value).toBe(SETTINGS_SECRET_MASK); + }); + + it('a non-encrypted key whose value genuinely IS the mask is written verbatim', async () => { + // The drop is scoped to secret keys — it must not swallow a legal write. + const { svc, http } = makeSecretStack(); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ + params: { namespace: 'secretsns' }, + body: { smtp_host: SETTINGS_SECRET_MASK }, + }); + await h(req, res); + + expect(state.status).toBe(200); + expect((await svc.get('secretsns', 'smtp_host', { userId: 'usr_1' })).value) + .toBe(SETTINGS_SECRET_MASK); + }); + + // ── the other half of the boundary: the service layer is NOT redacted ────── + + it('in-process consumers still receive REAL plaintext (createClient / snapshotOf)', async () => { + // This is the guard against someone later "fixing" #7522 in the service + // layer: the mail/sms/storage/auth plugins read their credentials through + // exactly this path, and a mask here would break every one of them. + const { svc } = makeSecretStack(); + await svc.set('secretsns', 'smtp_password', SMTP_PLAINTEXT, { userId: 'usr_1' }); + await svc.set('secretsns', 'webhook_token', TOKEN_PLAINTEXT, { userId: 'usr_1' }); + + const client = await svc.createClient('secretsns', { ctx: { userId: 'usr_1' } }); + expect(client.current.smtp_password).toBe(SMTP_PLAINTEXT); + expect(client.get('webhook_token')).toBe(TOKEN_PLAINTEXT); + + // …and so does the raw service read the routes wrap. + const payload = await svc.getNamespace('secretsns', { userId: 'usr_1' }); + expect(payload.values.smtp_password.value).toBe(SMTP_PLAINTEXT); + expect(payload.values.webhook_token.value).toBe(TOKEN_PLAINTEXT); + client.dispose(); + }); + + it('secretKeysOf reports both flavours and refuses an unknown namespace', async () => { + const { svc } = makeSecretStack(); + expect([...svc.secretKeysOf('secretsns')].sort()).toEqual(['smtp_password', 'webhook_token']); + // Fail-closed: "unknown namespace" must never answer "nothing is secret". + expect(() => svc.secretKeysOf('nope')).toThrow( + expect.objectContaining({ code: 'SETTINGS_UNKNOWN_NAMESPACE' }), + ); + }); +}); diff --git a/packages/services/service-settings/src/settings-routes.ts b/packages/services/service-settings/src/settings-routes.ts index a1657c6d7a..bc0b20d2a8 100644 --- a/packages/services/service-settings/src/settings-routes.ts +++ b/packages/services/service-settings/src/settings-routes.ts @@ -19,6 +19,11 @@ import type { IHttpServer, IHttpRequest, RouteHandler } from '@objectstack/spec/ // #4224 retired from this module cannot come back through it either. import { sendOk, sendError } from '@objectstack/types'; import { SettingsService } from './settings-service.js'; +// #7522 — the REST boundary is where an encrypted setting stops being cleartext. +// The service decrypts on purpose (in-process plugins need the real secret); +// nothing that leaves over HTTP may carry it. See the module header for the +// mask shape and why it mirrors ADR-0100's encrypted-field convention. +import { dropEchoedSecretMasks, redactSecretValues } from './settings-secret-redaction.js'; import { SettingsForbiddenError, SettingsLockedError, @@ -77,7 +82,11 @@ export function registerSettingsRoutes( try { const ctx = await ctxOf(req); const payload = await service.getNamespace(ns, ctx); - sendOk(res, payload); + // #7522 — redact BEFORE the payload leaves the process. `values..value` + // and every `cascadeChain` entry of a secret-backed key are masked; + // `source`, `locked` and `lockedReason` are untouched, so the console's + // "configured" state and the env-lock affordances read the same as before. + sendOk(res, { ...payload, values: redactSecretValues(payload.values, service.secretKeysOf(ns)) }); } catch (err: any) { if (err instanceof SettingsForbiddenError) { sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } }); @@ -114,8 +123,21 @@ export function registerSettingsRoutes( } try { const ctx = await ctxOf(req); - const result = await service.setMany(ns, body, ctx); - sendOk(res, { values: result }); + // #7522 — the echoed-mask no-op. GET now answers a secret with the mask; + // a form that submits back what it read means "unchanged", so the key is + // dropped rather than persisted as the mask's literal text. Resolved here + // (not in the service) for the same reason the redaction is: in-process + // callers write real values and must keep doing so. + // + // `secretKeysOf` throws `UnknownNamespaceError` for an unregistered + // namespace — the same 404 `setMany` would have raised one line later, in + // the same order relative to the 403 authz check. + const secretKeys = service.secretKeysOf(ns); + const result = await service.setMany(ns, dropEchoedSecretMasks(body, secretKeys), ctx); + // The write response carries resolved values too — including cascade + // entries the caller never submitted (an upper-scope secret it may not + // have set). Same boundary, same redaction. + sendOk(res, { values: redactSecretValues(result, secretKeys) }); } catch (err: any) { if (err instanceof SettingsForbiddenError) { sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } }); diff --git a/packages/services/service-settings/src/settings-secret-redaction.ts b/packages/services/service-settings/src/settings-secret-redaction.ts new file mode 100644 index 0000000000..262ba08bf7 --- /dev/null +++ b/packages/services/service-settings/src/settings-secret-redaction.ts @@ -0,0 +1,132 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Secret redaction for the settings **REST boundary** (#7522). + * + * `SettingsService` decrypts on purpose: `materialiseRow()` dereferences the + * `sec_` handle through `sys_secret` and hands back cleartext, because the + * in-process consumers of this service — `createClient()` / `snapshotOf()`, and + * through them the mail / sms / storage / auth plugins — need the real secret to + * open an SMTP session or sign an API call. That decryption is load-bearing and + * stays exactly where it is. + * + * What was missing is the boundary on the way OUT. `getNamespace()` copies that + * plaintext into `values..value` **and** into every `cascadeChain` entry, + * and the REST handler used to serve the payload verbatim — so every operator, + * integration, proxy, browser cache and HAR capture on that response path + * received the cleartext of every secret in the namespace, defeating the whole + * point of the `value_enc` + `sys_secret` split. The endpoint requires + * `setup.access`, so this is defense-in-depth rather than privilege escalation; + * it is still an exposure, and the REST response is the one surface that should + * never carry the cleartext. + * + * The mask shape is NOT invented here — it mirrors the encrypted-**field** + * convention ADR-0100 pins for `secret` / `password` columns on the generic CRUD + * path (`SECRET_MASK` in `@objectstack/objectql`, exercised by the + * `records-forms.encrypted-field-behavior` checklist item): + * + * - **read**: a set value becomes the mask; an unset one stays `null`, so the + * response is presence-preserving and the console can still render + * "configured" vs "not configured" (and the env-lock affordances keep + * working — `source`, `locked` and `lockedReason` are untouched). + * - **write**: a submitted value equal to the mask means "unchanged" and the + * key is DROPPED from the patch, so a form round-trip that echoes the mask + * does not overwrite the stored secret with the mask's literal text. + * + * The constant is redeclared rather than imported because this service is + * deliberately framework-agnostic (see the `settings-service.ts` header): it + * defines its own minimal `SettingsEngine` instead of importing `IDataEngine`, + * and does not depend on `@objectstack/objectql` at all. Taking a runtime + * dependency on the whole data engine to reach one string would undo that. The + * long-term fix is to hoist the mask into a package both sides already depend on + * (`@objectstack/spec`) and have objectql re-export it — recorded on #7522 as + * follow-up rather than done here, since it is a cross-package move on a + * security card. + */ + +import type { ResolvedSettingValue } from '@objectstack/spec/system'; + +/** + * Value served in place of a set secret on the REST read path. Says "a secret + * is set" without leaking its cleartext; an unset secret resolves to `null` + * instead, so set-vs-unset stays observable. + * + * Byte-identical to `SECRET_MASK` in `@objectstack/objectql` (ADR-0100) — eight + * U+2022 BULLET characters — so one client-side comparison recognises a masked + * read from either surface. Spelled as the literal, not an escape, so a grep for + * the mask finds both declarations. + */ +export const SETTINGS_SECRET_MASK = '••••••••'; + +/** Mask one resolved value: the effective value AND every cascade entry. */ +function maskResolved(resolved: ResolvedSettingValue): ResolvedSettingValue { + return { + ...resolved, + // `== null` covers both null and undefined: an unset secret must stay + // distinguishable from a set one. + value: resolved.value == null ? null : SETTINGS_SECRET_MASK, + // The chain is where the same plaintext was repeated once per scope. Masked + // entry-by-entry rather than dropped, so "global sets this, your user + // overrides it" still renders — only the values go. + ...(resolved.cascadeChain + ? { + cascadeChain: resolved.cascadeChain.map((entry) => ({ + ...entry, + value: entry.value == null ? null : SETTINGS_SECRET_MASK, + })), + } + : {}), + }; +} + +/** + * Redact every secret-backed value in a resolved map, returning a NEW map. + * + * Non-mutating on purpose: the same helper serves the GET payload and the PUT + * response, and neither may hand a mutated object back to an in-process caller + * that is entitled to the plaintext. + * + * `secretKeys` is the service's own encrypted-key set (`secretKeysOf()`), i.e. + * exactly the set the write path uses to decide what gets encrypted — so the + * two sides cannot drift into "encrypted on write, cleartext on read". + */ +export function redactSecretValues( + values: Record, + secretKeys: ReadonlySet, +): Record { + if (secretKeys.size === 0) return values; + const out: Record = {}; + for (const [key, resolved] of Object.entries(values)) { + out[key] = secretKeys.has(key) ? maskResolved(resolved) : resolved; + } + return out; +} + +/** + * Drop every secret key whose submitted value is the echoed read mask. + * + * The classic second bug of a redaction fix: the console reads `••••••••`, + * submits the form unchanged, and the write path stores the mask's literal text + * over the real secret — silently destroying it, with the mask then decrypting + * back to itself so nothing looks wrong until the SMTP login fails. Dropping the + * key makes the echo a no-op, which is what "unchanged" means. + * + * Scoped to `secretKeys`: a plain text setting whose value genuinely IS eight + * bullets is a legal write and is left alone. + */ +export function dropEchoedSecretMasks( + patch: Record, + secretKeys: ReadonlySet, +): Record { + if (secretKeys.size === 0) return patch; + let dropped = false; + const out: Record = {}; + for (const [key, value] of Object.entries(patch)) { + if (secretKeys.has(key) && value === SETTINGS_SECRET_MASK) { + dropped = true; + continue; + } + out[key] = value; + } + return dropped ? out : patch; +} diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index ca040d62ce..eb29079332 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -1132,6 +1132,32 @@ export class SettingsService { return { manifest: reg.manifest, values }; } + /** + * The namespace's secret-backed keys — every specifier declared + * `encrypted: true` or `type: 'password'` (#7522). + * + * Published so the REST boundary can redact exactly the values this service + * encrypts, reading the SAME set `setMany` consults to decide what gets + * encrypted at all. Re-deriving the predicate at the boundary is how the two + * sides drift into "encrypted on write, cleartext on read", which is the + * defect this accessor exists to close. + * + * Note what it deliberately does NOT do: the values themselves keep coming + * back as plaintext from `get()` / `getNamespace()` / `snapshotOf()`, because + * in-process consumers (the mail, sms, storage and auth plugins, via + * `createClient()`) need the real secret. Redaction is the caller's step, and + * belongs to the REST read boundary only. + * + * Throws `UnknownNamespaceError` for an unregistered namespace rather than + * answering an empty set: "I don't know this namespace" must never read as + * "nothing here is secret". + */ + secretKeysOf(namespace: string): ReadonlySet { + const reg = this.registry.get(namespace); + if (!reg) throw new UnknownNamespaceError(namespace); + return reg.encryptedKeys; + } + // --------------------------------------------------------------------- // Reactive client (Phase 1) // ---------------------------------------------------------------------