[security] GET /api/settings/:namespace returns encrypted setting values as plaintext — no redaction at the REST read boundary

[huangyiirene]

Extracted from the QA run #7514 (settings-hub-roundtrip, framework a86db175). The run held back the step-by-step recipe pending a maintainer disclosure decision; the defect and its root cause were already public in that report, and this card carries the same level of detail — no new disclosure. The reproduction recipe stays withheld here too.

Symptom

Storage of encrypted settings is correct: sys_setting.value is null, encrypted: true, value_enc: 'sec_…', and sys_secret holds aes-256-gcm ciphertext. The leak is on the way out:

GET /api/settings/:namespace returns the secret's plaintext in values.<key>.value, and repeats that same plaintext inside every cascadeChain entry.

Both specifier flavours are affected — type: 'password' and the explicit encrypted: true.

The endpoint requires setup.access (anonymous callers get 403), so this is defense-in-depth, not privilege escalation. It is still a real exposure: any operator or integration holding setup access — and any log, proxy, browser cache, or HAR capture on that response path — receives the cleartext of every secret in the namespace, when the whole point of value_enc + sys_secret is that those values never materialise outside the crypto boundary.

Root cause

Verified against origin/main at the time of filing (not merely restated from the run):

1. packages/services/service-settings/src/settings-service.ts — materialiseRow() (~:1870) sees row.encrypted, dereferences the sec_ handle through secretStore, and calls cryptoProvider.decrypt(...), returning plaintext.
2. The same file's get() builds cascadeChain by calling materialiseRow() per scope (~:1068 global, ~:1082 tenant, ~:1094 user), so the plaintext is copied into each chain entry as well as the resolved value.
3. getNamespace() (~:1118) loops this.get(namespace, key, ctx) over every key and returns { manifest, values } with no redaction step.
4. packages/services/service-settings/src/settings-routes.ts (~:75-87) — the GET ${base}/:namespace handler calls service.getNamespace(ns, ctx) and hands the payload straight to sendOk. There is no redaction anywhere on this path.

Where the fix belongs — and where it must NOT go

The missing boundary is the REST read, not the service.

The service-layer decryption is deliberate and load-bearing: snapshotOf() / createClient() (~:1190-1200) consume payload.values[k].value to hand plugins their real secret values, and settings-service.test.ts pins that round-trip on purpose. Redacting inside materialiseRow() or getNamespace() would break every legitimate in-process consumer.

⛔ So do not "fix" this by making the service stop decrypting. The REST response is the only surface that should never carry the cleartext.

Acceptance criteria

• GET /api/settings/:namespace never emits the plaintext of a key whose specifier is encrypted: true or type: 'password' — neither in values.<key>.value nor anywhere in cascadeChain.
• The redaction is presence-preserving: a caller must still be able to tell that a value is set vs unset (so the console can render "configured" state and the existing env-lock affordances keep working). Pick the marker shape that matches whatever this repo already uses for masked reads — check the encrypted-field read-mask convention that records-forms' encrypted-field-behavior item pins, and follow it rather than inventing a new sentinel.
• The existing write path keeps working, including the no-op echoed-mask case: PUTting back a redacted marker must not overwrite the stored secret with the marker's literal text. Assert this explicitly — it is the classic second bug introduced by a redaction fix.
• source, locked, and the 409 SETTINGS_LOCKED env-lock behaviour are unchanged (the run verified these are currently correct).
• In-process consumers (createClient / snapshotOf) still receive real plaintext — add or keep a test proving the service layer is untouched.
• A regression test at the route level asserting the response body contains no ciphertext-backed cleartext for both specifier flavours.

Source

Extracted from the QA run #7514 (framework a86db175, vendored console 09987b68). Root cause re-verified against origin/main before filing.

[huangyiirene]

Claim: PM loop — security extraction from the QA run batch
Session: session_01T4VrzFdnQETy7CUUnfxcan
Branch: claude/issue-7522-settings-redact-encrypted-read
Worktree: objectstack-issue-7522
Domain: domain:services
File surface: packages/services/service-settings/src/settings-routes.ts (the fix), packages/services/service-settings/src/settings-routes.test.ts (route-level regression), and read-only reference to settings-service.ts / settings-service.test.ts — ⛔ the service layer is explicitly out of scope for behaviour change (see the card's "where it must NOT go").
Container & model: M, mode:cloud single container, model: opus (security fix with a real contract judgment — the redaction marker shape and the echoed-mask write path — so not an S-grade mechanical card).
Serial constraints cleared: no in-flight claim on packages/services/service-settings/**; the sibling QA extraction cards being filed in this same batch are all in other packages (packages/rest, packages/metadata*, objectui). No same-file predecessor.

Premise re-verified against origin/main before claiming, rather than taken from the run: materialiseRow() decrypts sec_ handles to plaintext (settings-service.ts :1870), get() copies that plaintext into every cascadeChain entry (:1068/:1082/:1094), getNamespace() returns it unredacted (:1118), and the GET ${base}/:namespace handler in settings-routes.ts (:75-87) hands the payload straight to sendOk with no redaction. The defect is live on main, not an expired report premise.

Maintainer instruction of record (2026-08-11, verbatim, untranslated): 「你认领其中的安全发现的私有问题，立刻就处理」 — this card is the actionable half of that instruction. The other half, the undisclosed cross-persona data-disclosure finding on #7463, cannot be filed or dispatched from here: its write-up lives only in the QA author's private channel and is not in this repository. It is reported back to the maintainer as blocked on delivery of that write-up.

---

Generated by Claude Code

[huangyiirene]

os-dev-report

{
  "issue": 7522,
  "status": "done",
  "branch": "claude/issue-7522-settings-redact-encrypted-read",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7554",
  "premise_still_valid": true,
  "summary": "Premise confirmed line-for-line against the checked-out tree: materialiseRow() decrypts at settings-service.ts:1870, get() copies that plaintext into every cascadeChain entry at :1068/:1082/:1094, getNamespace() returns it at :1118, and the GET handler in settings-routes.ts sendOk'd it with no redaction. Redaction now lives at the REST boundary only: a set secret is served as SETTINGS_SECRET_MASK (the eight-bullet string byte-identical to objectql's SECRET_MASK, ADR-0100's encrypted-FIELD convention reused rather than a new sentinel), an unset one stays null so the response is presence-preserving, and cascadeChain is masked entry by entry; source/locked/lockedReason and the 409 SETTINGS_LOCKED env-lock path are untouched. PUT drops a submitted value equal to the mask so an echoed form save is a no-op instead of overwriting the secret with the mask's literal text, and PUT's own response is redacted the same way because it carries resolved values (including cascade entries the caller never submitted). The service layer is deliberately unchanged: get()/getNamespace()/snapshotOf()/createClient() still return real plaintext to in-process consumers, pinned by a test. The encrypted-key set is read from the registry via a new SettingsService.secretKeysOf(namespace) rather than re-derived at the boundary, so the read side cannot drift from what setMany encrypts; it throws UnknownNamespaceError rather than answering an empty set.",
  "tests": "pnpm --filter @objectstack/service-settings test -- --maxWorkers=2 => 'Test Files 18 passed (18) / Tests 395 passed (395)', 10 of them new and all at the ROUTE level (verbose run confirms each): leak-free GET for both flavours (type:'password' and encrypted:true) asserted whole-body via JSON.stringify AND per-surface on values.<key>.value plus every cascadeChain entry, with a second ciphertext-backed entry seeded at global scope; presence-preserving null for unset; source/locked passthrough; env-locked secret masked while source:'env'/locked/lockedReason survive and PUT still answers 409 SETTINGS_LOCKED; echoed-mask PUT is a no-op in BOTH body shapes (flat and the read-shape {values:{k:{value}}} envelope) proven by re-reading the ORIGINAL plaintext in-process and by the sys_secret handle set being unchanged; a real new secret still writes but its write RESPONSE is redacted; a non-encrypted key whose value genuinely is eight bullets still writes verbatim; createClient/snapshotOf/getNamespace still return real plaintext; secretKeysOf reports both flavours and throws SETTINGS_UNKNOWN_NAMESPACE. Secrets are real ciphertext throughout (LocalCryptoProvider + a sys_secret store, sec_ handles, the production code path). REVERSE VERIFICATION, direction predicted before running: took the route fix out with `git checkout origin/main -- settings-routes.ts` (never git stash) => 'Tests 6 failed | 19 passed (25)' — exactly the 6 whose subject the route touches; the 4 new cases that stayed green are the source/locked passthrough, the non-secret verbatim write, the in-process plaintext guard and secretKeysOf (which lives in the service). Restored, re-ran green. BUILD CLOSURE ran first: pnpm --filter '@objectstack/service-settings^...' build. TYPECHECK: tsc --noEmit -p packages/services/service-settings => 13 errors, exactly the number ledgered in scripts/check-type-check-coverage.mjs, i.e. zero added (one WAS introduced en route — InMemoryCryptoProvider is a value-only alias `export const … = LocalCryptoProvider` and cannot be spelled as a type — and fixed by using LocalCryptoProvider in the test). DOWNSTREAM SWEEP used the PREFIX direction, --filter '...@objectstack/service-settings' = the 9 consuming packages (cli, plugin-email, service-sms, verify, driver-turso, dogfood, example-crm/showcase/todo), not upstream deps: turbo run typecheck => 'Tasks: 72 successful, 72 total'. node scripts/check-nul-bytes.mjs => OK. Changeset added (.changeset/settings-redact-encrypted-rest-read.md, @objectstack/service-settings patch) so no skip-changeset label applies. CI on PR #7554 is in_progress at report time — not waited on, per the report-at-draft-PR-time contract.",
  "open_questions": [
    {
      "question": "The eight-bullet read mask is now declared TWICE in the repo — objectql's SECRET_MASK and service-settings' SETTINGS_SECRET_MASK — with no gate binding them, so a future edit to one silently desynchronises the two masked-read surfaces a client sees.",
      "options": [
        "A — hoist the constant into @objectstack/spec (both packages already depend on it, ADR-0100 is already documented in spec/src/data/field.zod.ts) and have objectql re-export it for back-compat; both sides then import one definition",
        "B — leave the duplication and add a cross-package pin test in a package that depends on both (cli / plugin-email / verify)",
        "C — add @objectstack/objectql as a dependency of service-settings and import SECRET_MASK directly"
      ],
      "recommendation": "A, as a separate follow-up PR — not on this card. C is wrong outright: service-settings is deliberately framework-agnostic (it defines its own minimal SettingsEngine rather than importing IDataEngine, per its own module header), and pulling in the whole data engine to reach one string undoes that. B binds the constants but puts a contract pin in a package whose subject is something else. A is the only option that leaves ONE definition, which is the axis that matters here — a mask is a client-facing contract, and 'hard to get wrong' means the console cannot be shown two different masks. I did not do it in this PR because it is a cross-package move (spec + objectql, whose consumer sweep is effectively the whole repo) riding on a security fix, and the duplication is inert until someone edits one of the two literals. The comment above SETTINGS_SECRET_MASK states the mirroring and names this follow-up so the next reader does not re-derive it."
    },
    {
      "question": "The issue's acceptance criteria name GET only. I also redacted PUT's response body — beyond the letter of the card.",
      "options": [
        "A — redact PUT's response too (what I shipped)",
        "B — redact GET only, exactly as specified"
      ],
      "recommendation": "A. PUT's response is not merely an echo of what the caller submitted: setMany returns full ResolvedSettingValues, whose cascadeChain carries upper-scope entries the caller never sent — so a user-scope write can hand back a global secret the caller never had. Restricting the fix to GET would leave a strictly smaller version of the same leak on the same boundary, one HTTP verb away. Cost is one line and it is covered by a test ('a REAL new secret still writes, and the write RESPONSE is redacted too'); flag it if the PM wants the narrower reading."
    },
    {
      "question": "docs/qa/platform-checklist/areas/platform-core.json's clause reads 'GET /api/settings never returns the plaintext' — true again now, but it never says what the endpoint DOES return, so a tester cannot distinguish 'masked' from 'omitted' or from 'the key vanished'.",
      "options": [
        "A — leave it; the clause passes as written",
        "B — strengthen the clause to name SETTINGS_SECRET_MASK and the presence-preserving null"
      ],
      "recommendation": "A for this PR, B as a checklist-author follow-up. Editing the checklist is that skill's domain (and may touch coverage.json), and it is not needed for this card to be verifiable — but the clause is measurably weaker than the behaviour it now guards, and the next QA run will read it."
    }
  ],
  "out_of_scope_findings": []
}

---

Generated by Claude Code

[huangyiirene]

Review: ACCEPT (landing held until CI converges)

PR #7554 reviewed against GitHub, not against the report's own claims. The diff does what the card asked and stops where the card said to stop.

The load-bearing check — the service layer is untouched. settings-service.ts is +26 / −0, purely additive: a secretKeysOf() accessor that reads reg.encryptedKeys and throws UnknownNamespaceError. materialiseRow(), get(), getNamespace(), snapshotOf() and createClient() are unmodified, so the in-process plaintext path the mail / sms / storage / auth plugins depend on is intact. That was the one way this fix could have gone wrong, and it didn't.

Redaction is at the boundary and covers both surfaces the card named — values.<key>.value and every cascadeChain entry, for both specifier flavours. The whole-body JSON.stringify(...).not.toContain(plaintext) assertion is the right shape: it cannot be satisfied by masking only the places someone thought of.

The echoed-mask write is properly guarded, which is the second bug this class of fix usually introduces. The test proves it two ways — the sys_secret handle set is unchanged, and an in-process re-read still returns the original plaintext — and covers both body shapes (flat and the {values:{k:{value}}} read-shape envelope). There is also a negative proving a non-secret key whose value genuinely is eight bullets still writes verbatim, so the drop is scoped rather than blanket.

Reverse verification is real. The fix was removed with git checkout origin/main -- settings-routes.ts (explicitly not git stash, per the repo's shared-stack rule) and 6 tests went red — exactly the route-subject ones — with the 4 that stayed green individually explained. Downstream sweep used the prefix direction ('...@objectstack/service-settings', 9 consuming packages, 72 tasks) after a build closure, so it answers the consumer question rather than the upstream one.

Declared file surface was exceeded, and correctly so. The claim named settings-routes.ts + its test; the PR also adds settings-secret-redaction.ts and the secretKeysOf() accessor. Both are the anti-drift mechanism — reading the same encrypted-key set the write path uses, instead of re-deriving the predicate at the boundary — which is a better design than what the claim described. Reported rather than done silently; accepted.

Answers to the three open questions

1. The mask is now declared twice with no gate binding them — real, and filed. SETTINGS_SECRET_MASK and objectql's SECRET_MASK are byte-identical by convention only. Your recommendation A (hoist into @objectstack/spec, objectql re-exports) is right, and your reason for not doing it here is also right: a cross-package move whose consumer sweep is the whole repo has no business riding on a security fix. Filed as a follow-up.

2. Redacting PUT's response as well — accepted, keep it. Your argument is correct and decides it: setMany returns full ResolvedSettingValues whose cascadeChain carries upper-scope entries the caller never submitted, so a user-scope write can hand back a global secret the caller never set. Restricting to GET would have left a strictly smaller version of the same leak one HTTP verb away. The card's acceptance criteria named GET because that is where the run found it, not as a scope ceiling.

3. The checklist clause is weaker than the behaviour it now guards — filed separately. Agreed it is not this PR's job; docs/qa/platform-checklist/ is the checklist-author skill's territory and may touch coverage.json.

Landing

Not flipping to ready yet. ESLint is completed: success, but TypeScript Type Check and two Dogfood Regression Gate shards are still in_progress — the gate families ride in those jobs, and "no failure yet" is not "green". Holding for their real conclusions, then ready + merge queue.

Noted for the record: the PR touches no docs/adr/** path, so the ADR hold does not apply here.

---

Generated by Claude Code

[huangyiirene]

Landing: queued

All 25 checks on PR #7554 reached completed with zero failures — including the two jobs that carry the gate families: ESLint success (05:25:32Z) and TypeScript Type Check success (05:30:06Z), plus all three Dogfood Regression Gate shards and Temporal Conformance (live PG + MySQL). Flipped to ready and enqueued.

Queue position confirmed by the authoritative reading — the gh-readonly-queue/* branches, not the auto_merge field:

gh-readonly-queue/main/pr-7540-b03b0e1d…   → b78016f3
gh-readonly-queue/main/pr-7528-b78016f3…   → f7a60d96
gh-readonly-queue/main/pr-7554-f7a60d96…   → 23bc6e16

Chaining the base shas puts #7554 third, behind #7540 and #7528. Not yet on origin/main.

Operational note 21 fired here — a fourth recorded instance

enable_pr_auto_merge on this already-clean PR returned the empty-field signature (method: , enabled at , versus the normal method: MERGE, enabled at <timestamp>). Per the recorded prescription that means auto-merge was armed but the PR would not enqueue — it sits indefinitely in "green, waiting to be queued". The disable → enable flip was applied once and the queue branch appeared immediately.

Adding this to the ledger of that signature (previously #6034, #6092, #6197): the tell is the empty return, the fix is one flip, and the confirmation is the queue branch — never the auto_merge field, which carries no information about queue membership on this repo.

Follow-through: confirm MERGED against both the queue branches and origin/main, then archive the cloud dev session. Both are armed on a check-in.

---

Generated by Claude Code