Skip to content

fix(service-settings): fail closed on a secret the settings path cannot encrypt (#8026) - #8274

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-8026-settings-crypto-fail-closed
Aug 13, 2026
Merged

fix(service-settings): fail closed on a secret the settings path cannot encrypt (#8026)#8274
os-zhuang merged 1 commit into
mainfrom
claude/issue-8026-settings-crypto-fail-closed

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#8026

The settings write path now fails closed on a declared-encrypted value it cannot actually encrypt, restoring parity with the engine's Field.secret() posture.

The divergence this closes

SettingsService constructed without a cryptoProvider + secretStore fell back to NoopCryptoAdapter, whose encrypt() is 'b64:' + base64(plaintext) — encoding, not encryption. Worse than plaintext in one specific way: sys_setting.value_enc comes back populated, so the row reads as protected to the next author and to the next audit.

pathno CryptoProvider ⇒
engine Field.secret()refuses the write (fail-closed)
SettingsService — beforewrites base64 (fail-open)
SettingsService — afterrefuses the write (fail-closed)

⚠ This is not a live-leak fix and is not written as one. The shipped plugin path wires a real LocalCryptoProvider at kernel:ready once an objectql engine resolves, so a default deployment never took the base64 branch. What lands here is the removal of the fail-open direction on a path an engine-less deployment can still reach. A security tightening restoring an already-declared platform posture — not a new capability, not a contract widening.

Premise re-measured on origin/main

The card's justification held, with one refinement:

  • 148 new SettingsService( sites; 147 are tests, the single production site is settings-service-plugin.ts:108 and it passes only opts.crypto (undefined by default ⇒ NoopCryptoAdapter). The real cryptoProvider + secretStore bind later, in kernel:ready, and only when an objectql engine resolves. So an engine-less deployment genuinely took the base64 path. Unchanged from triage.
  • Refinement: no caller outside this package writes settings values at all (repo-wide grep for .setMany( / settings .set( finds only prose references). The runtime blast radius outside the package is real deployments only.

What changed

  • Refusal at the write, not at construction.SettingsCryptoUnavailableError is thrown when the sys_secret path is unwired and the inline CryptoAdapter declares no confidentiality. Construction-time refusal was considered and rejected: the plugin builds the service in init() and binds the provider at kernel:ready, so a constructor throw would refuse every shipped deployment, and a namespace with no encrypted specifier needs no provider at all. See "boot vs write" below.
  • Two call sites, one rule. A pre-flight pass rejects the whole batch (so a plain sibling key is not half-written), and the persist site enforces the same rule so no future caller can reach that branch and fall open.
  • Ordered after validatePatch deliberately. Both refuse the whole batch, but a validation error names something the caller can fix in the form in front of them, while this one names a deployment they cannot reconfigure. Checked first, it masked every field-level diagnostic on a namespace that happens to carry a secret — measured: 6 existing tests changed their reported error before the reorder, and went back to their own diagnostics after.
  • Declared, not inferred. New optional CryptoAdapter.confidential; NoopCryptoAdapter declares false. Absent means yes — every adapter written before the flag is a deliberately-injected real one, so silence must not start refusing their writes; opting in to the refusal is one line. Exported predicate providesConfidentiality(adapter) is what the write path asks. The check is provenance-independent: explicitly passing new NoopCryptoAdapter() is refused just the same, so the fail-open path is not reachable by one line of caller code.
  • Loudness. One operator-actionable line through the deployment's own logger, deduped per key (same shape as the env 来源的 settings 值绕过 manifest 的 options 表校验 —— #5094 在写入 API 上堵住的洞,在 OS_* 覆盖这一侧原样敞开 #5204 env reporter), so a caller that swallows the thrown error still leaves a trace.

Boot vs write

The card asked for the refusal to be loud at boot; it lands at the write, and the two genuinely pull apart here. SettingsServicePlugin constructs the service in init() and binds the real provider later at kernel:ready, so at construction time "can this deployment encrypt?" is not yet knowable — a boot-time throw would refuse every shipped deployment, and a boot-time warning would fire on every shipped boot as a false alarm. The fact becomes knowable at the write, which is where the refusal sits; the loudness the card wanted is the logger line, which reaches the operator even when the caller swallows the error. Recording the tension rather than papering over it.

What deliberately did not change

  • NoopCryptoAdapter stays exported (public API) and decrypt() is untouched — existing b64: rows stay readable, reportable and migratable. The refusal is write-only; refusing reads too would strand exactly the data this card wants surfaced. (The engine fails closed on secret reads too; the settings side deliberately does not, and that residual asymmetry is stated here rather than silently taken.)
  • Injected adapters (SettingsServicePluginOptions.crypto) and the sys_secret + ICryptoProvider path are unaffected.
  • Clearing an encrypted key (writing null) is still allowed: no plaintext to protect, and an operator must always be able to REMOVE a value on a deployment that cannot store one.

Reverse verification

A temporary probe (main-compatible APIs only, not committed) drove a genuinely provider-less construction — real ObjectQL over the real SysSetting, no crypto, no cryptoProvider, no secretStore — and asserted the persisted row, not the thrown error. Direction predicted before running, observed as predicted:

  • on origin/main: GREEN — one sys_setting row persisted with value_enc equal to b64:cmUtc2VjcmV0LTEyMw==, decoded back to re-secret-123 from the stored column alone. This is the non-vacuity proof the pin needed: the fixture really reaches the Noop path.
  • on this branch: REDCannot persist encrypted setting 'crypto_ns.api_key' ... Refusing to store a reversible value (fail-closed)., thrown at the pre-flight, and no row written.

The committed pin file itself was also run against origin/main's sources: 9 of its 12 cases go red. Reported honestly — 3 of those reds are import-shaped (the error class does not exist on main), so the probe above is the load-bearing measurement, not the pin file's red count.

Tests

packages/services/service-settings: 426 passed / 426 (baseline on origin/main was 413). New file settings-crypto-fail-closed.test.ts (12 cases) covers the refusal and the persisted state, both encrypted flavours, the explicit-Noop case, batch atomicity, the logger line (and that it never echoes the secret), the non-encrypted control, clearing, the confidential-adapter path, legacy b64: readability, the providesConfidentiality arms, and the REST boundary (status 500 + code INTERNAL_ERROR + the actionable message, with nothing persisted and no secret echoed).

Six existing tests were re-pointed, and why that is not "editing tests to fit the refusal": every one writes a declared secret only as incidental setup for a complete provider config; their subjects are the audit digest, the option table, required/visible validation and temperature's window. None had the Noop persist path as its subject. Each fixture now declares an adapter that can hold a secret, so those assertions stay about what they claim. The one fixture that was pinned to the removed limb (persists encrypted=true values via crypto adapter, which injected new NoopCryptoAdapter()) was replaced rather than re-spelled, because its assertion would otherwise have kept passing while nothing was stored.

Wire spelling — a deliberate gap, filed separately

SETTINGS_CRYPTO_UNAVAILABLE is an in-process discriminator today. Giving it a dedicated HTTP status and code requires registering it in ERROR_CODE_LEDGER under packages/spec, which this card's scope excludes; emitting an unregistered code on the wire would be the silent fourth state ADR-0112 forbids. So the refusal takes the same 500 / INTERNAL_ERROR arm every unmapped service error takes, carrying the full actionable message — no envelope change. Follow-up filed as #8273.

Scope notes

Local gates

check:docs-audit-scope, check:route-envelope, check:test-source-alias, check:type-source-resolution, check-changeset-fixed, check:error-code-casing (a new code is introduced), check:nul-bytes — all green. Re-derived against the actual changed paths, which added two convention-triggered families the dispatch list did not name: check:query-options-erasure and check:type-check-coverage (both green). Downstream consumer sweep (prefix filter, ...@objectstack/service-settings — 10 projects): all typecheck clean.


Generated by Claude Code

…ot encrypt (#8026)
`SettingsService` without a `cryptoProvider` + `secretStore` fell back to
`NoopCryptoAdapter`, whose `encrypt()` is `'b64:' + base64(plaintext)` —
encoding, not encryption, and it leaves `sys_setting.value_enc` populated so the
row reads as protected. The engine's `Field.secret()` path has always refused
the write instead; this aligns the settings side onto that posture.
- refuse a declared-encrypted write (`encrypted: true` / manifest
`type: 'password'`) with `SettingsCryptoUnavailableError` when the `sys_secret`
path is unwired AND the inline adapter declares no confidentiality;
- whole batch rejected (pre-flight, after `validatePatch` so caller-fixable
diagnostics still win), plus the load-bearing guard at the persist site;
- one deduped, operator-actionable line through the deployment logger;
- new optional `CryptoAdapter.confidential` (absent means yes) + exported
`providesConfidentiality`; `NoopCryptoAdapter` stays exported and its
`decrypt` is untouched, so existing `b64:` rows stay readable.
Not a live leak: the shipped plugin wires `LocalCryptoProvider` at
`kernel:ready`. This closes the fail-open direction on the path an engine-less
deployment still reaches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk
@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 2:06am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-settings.

5 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/kernel/runtime-services/audit-service.mdx(via packages/services/service-settings)
  • content/docs/kernel/runtime-services/index.mdx(via packages/services/service-settings)
  • content/docs/kernel/runtime-services/settings-service.mdx(via packages/services/service-settings)
  • content/docs/plugins/packages.mdx(via @objectstack/service-settings)
  • content/docs/protocol/kernel/config-resolution.mdx(via @objectstack/service-settings)

2 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/service-settings)
  • content/docs/releases/v9.mdx(via @objectstack/service-settings)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SettingsService falls back to a NoopCryptoAdapter whose encrypt() is base64 — fail-open where the engine's Field.secret() path fails closed

2 participants

@os-zhuang@claude