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

.changeset/settings-crypto-fail-closed.md

@@ -0,0 +1,49 @@
+---
+"@objectstack/service-settings": patch
+---
+
+fix(service-settings): refuse to persist a secret through the base64 `NoopCryptoAdapter` — the settings write path now fails closed like the engine's (#8026)
+
+`SettingsService` constructed without a `cryptoProvider` + `secretStore` fell
+back to `NoopCryptoAdapter`, whose `encrypt()` is `'b64:' + base64(plaintext)`.
+That is **encoding, not encryption**: trivially reversible, and it leaves
+`sys_setting.value_enc` populated — so the row reads as protected to the next
+author and to the next audit while being plaintext with extra steps.
+
+The engine's `Field.secret()` path has always taken the opposite posture: with
+no `CryptoProvider` registered it throws rather than store cleartext. The
+platform therefore had two credential-encryption paths with **opposite failure
+modes**. This aligns the settings side onto the engine's.
+
+**Not a live leak, and 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 this closes is the
+fail-open *direction* on a path an engine-less deployment can still reach.
+
+**What changed.** A write of a declared-encrypted key (`encrypted: true` or the
+manifest's `type: 'password'`, which means "encrypt this") is now refused with
+`SettingsCryptoUnavailableError` when the `sys_secret` path is not wired *and*
+the inline `CryptoAdapter` declares no confidentiality. The whole batch is
+rejected — a plain sibling key in the same patch is not half-written — and one
+operator-actionable line is reported through the deployment logger, deduped per
+key, so a caller that swallows the error still leaves a trace. Over REST the
+refusal answers `500` on the declared envelope, carrying the fix in the message.
+
+**What did not change.**
+
+- `NoopCryptoAdapter` remains exported (public API) and its `decrypt()` is
+ untouched: existing `b64:` rows stay readable, reportable and migratable. The
+ refusal is write-only — refusing the reads too would strand exactly the data
+ worth surfacing.
+- Injected adapters (`SettingsServicePluginOptions.crypto`) are unaffected. An
+ adapter declares itself fit to hold a secret with the new optional
+ `CryptoAdapter.confidential`; **absent means yes**, so every adapter written
+ before this flag keeps working, and only the base64 default declares `false`.
+ The exported `providesConfidentiality(adapter)` is the predicate the write
+ path uses.
+- Clearing an encrypted key (writing `null`) is still allowed: there is no
+ plaintext to protect, and an operator must always be able to REMOVE a value on
+ a deployment that cannot store one.
+- Validation still runs first, so a caller submitting a bad value on a namespace
+ that happens to carry a secret still gets the field-level diagnostic they can
+ act on rather than a deployment fault they cannot.

packages/services/service-settings/README.md

@@ -71,8 +71,20 @@ behind the same `ICryptoProvider` seam via `cryptoProvider` plugin option.
 > `InMemoryCryptoProvider` is a deprecated alias for `LocalCryptoProvider`
 > (the old name wrongly implied an ephemeral key).
 
-The legacy `CryptoAdapter` / `NoopCryptoAdapter` (a base64 wrapper) remains
-only as a pre-Phase-3 backward-compat path when no `cryptoProvider` is wired.
+The legacy `CryptoAdapter` seam remains as a pre-Phase-3 backward-compat path
+when no `cryptoProvider` is wired — but only for an adapter that actually
+encrypts. `NoopCryptoAdapter` (a base64 wrapper) no longer takes part in a
+**write**: since #8026 the write path **fails closed** and refuses to persist a
+declared-encrypted value (`encrypted: true` / `type: 'password'`) when the only
+thing available is an adapter that provides no confidentiality, matching the
+engine's `Field.secret()` posture. Base64 is encoding, not encryption, and a
+populated `value_enc` reads as protected to the next author and the next audit.
+
+The class stays exported and its `decrypt()` is unchanged, so existing `b64:`
+rows remain readable and migratable. An adapter declares itself fit to hold a
+secret with `confidential: true` (absent means "yes" — only the base64 default
+declares `false`), and `providesConfidentiality(adapter)` is the exported
+predicate the write path uses.
 
 ## Audit
 

packages/services/service-settings/src/crypto-adapter.ts

@@ -18,6 +18,24 @@ export interface CryptoAdapter {
  * operators can correlate value changes without leaking secrets.
  */
  digest(plaintext: string): string;
+ /**
+ * Does `encrypt()` actually provide confidentiality (#8026)?
+ *
+ * DECLARED, never inferred: the write path cannot tell an AES envelope from
+ * a base64 wrapper by looking at the output, so an adapter that does not
+ * protect its input has to say so — and {@link NoopCryptoAdapter} does.
+ * `false` makes the settings write path refuse to persist a declared-secret
+ * value through this adapter, matching the engine's `Field.secret()`
+ * posture (fail-closed, never store something reversible under a name that
+ * reads as encryption).
+ *
+ * OPTIONAL and defaulting to "yes" on purpose: every adapter written before
+ * this flag existed is a deliberately-injected real one (the base64 default
+ * is the only implementation in this repo that is not), so absence must not
+ * start refusing their writes. Opting IN to the refusal is a one-line
+ * declaration; opting out of protection is not something silence should buy.
+ */
+ readonly confidential?: boolean;
 }
 
 /**
@@ -26,8 +44,30 @@ export interface CryptoAdapter {
  *
  * Operators are expected to override this via
  * `SettingsServicePluginOptions.crypto`.
+ *
+ * ## What this adapter can and cannot do since #8026
+ *
+ * It still DECODES: `decrypt()` is unchanged, so a deployment that already
+ * has `b64:` rows can still read them (and migrate them). What it can no
+ * longer do is take part in a WRITE: `SettingsService` refuses to persist a
+ * declared-encrypted value when this is the only path available, because
+ * `'b64:' + base64(x)` is encoding, not encryption — trivially reversible,
+ * while producing a `value_enc` that looks protected to the next author and
+ * to the next audit. The refusal restores parity with the engine's
+ * `Field.secret()` path, which has always thrown rather than persist a secret
+ * with no CryptoProvider registered.
+ *
+ * The class stays exported (public API) and the read half stays useful; a
+ * subclass that supplies REAL encryption declares `confidential = true` and
+ * is accepted by the write path like any other adapter.
  */
 export class NoopCryptoAdapter implements CryptoAdapter {
+ /**
+ * #8026 — base64 is encoding, not encryption. This is the declaration the
+ * write path reads before it agrees to store an `encryptedKeys` value.
+ */
+ readonly confidential: boolean = false;
+
  async encrypt(plaintext: string): Promise<string> {
  return 'b64:' + Buffer.from(plaintext, 'utf8').toString('base64');
  }
@@ -48,3 +88,22 @@ export class NoopCryptoAdapter implements CryptoAdapter {
  return 'fnv32:' + h.toString(16).padStart(8, '0');
  }
 }
+
+/**
+ * Is this adapter allowed to hold a declared-secret value at rest (#8026)?
+ *
+ * Two arms, in this order:
+ *
+ * 1. **The declaration wins.** `confidential === false` refuses, `true`
+ * accepts. That is how a subclass of {@link NoopCryptoAdapter} supplying
+ * real encryption opts back in, and how any future development-only
+ * adapter opts out without this function having to know its name.
+ * 2. **Undeclared falls back to the class identity**, which catches an
+ * instance of this module's `NoopCryptoAdapter` whose flag was removed or
+ * overwritten. Anything else undeclared is assumed real — see the
+ * `confidential` doc for why silence must not refuse.
+ */
+export function providesConfidentiality(adapter: CryptoAdapter): boolean {
+ if (typeof adapter.confidential === 'boolean') return adapter.confidential;
+ return !(adapter instanceof NoopCryptoAdapter);
+}

packages/services/service-settings/src/index.ts

@@ -9,6 +9,11 @@ export { SettingsService } from './settings-service.js';
 export {
  type CryptoAdapter,
  NoopCryptoAdapter,
+ // #8026 — the predicate the write path asks before it agrees to hold a
+ // declared-secret value. Published with the interface it reads: an adapter
+ // author needs to be able to check their own `confidential` declaration the
+ // same way the service does, rather than re-deriving the two-arm rule.
+ providesConfidentiality,
 } from './crypto-adapter.js';
 // Default, KMS-free ICryptoProvider. AES-256-GCM keyed off `OS_SECRET_KEY`
 // (production) or a persisted dev key; fails loud in production rather than
@@ -32,6 +37,10 @@ export {
  type SettingsRow,
  type SettingsServiceOptions,
  envKeyOf,
+ // #8026 — thrown when a declared-encrypted write has nothing able to encrypt
+ // it. Exported so an in-process caller can branch on the refusal (there is no
+ // dedicated wire code for it yet; see the class doc).
+ SettingsCryptoUnavailableError,
  SettingsLockedError,
  SettingsValidationError,
  UnknownKeyError,

packages/services/service-settings/src/manifests/ai.manifest.test.ts

@@ -3,8 +3,29 @@
 import { describe, it, expect } from 'vitest';
 import { SettingsManifestSchema } from '@objectstack/spec/system';
 import { SettingsService } from '../settings-service.js';
+import type { CryptoAdapter } from '../crypto-adapter.js';
 import { aiSettingsManifest, aiTestActionHandler, aiTestEmbedderActionHandler } from './ai.manifest.js';
 
+/**
+ * #8026 — the write-door cases below save a complete `openai` provider config,
+ * which includes `openai_api_key` (a declared secret). Since the settings write
+ * path fails CLOSED on a secret it cannot protect, the fixture supplies an
+ * adapter that DECLARES confidentiality; without it these cases would be
+ * measuring the crypto gate instead of `temperature`'s declared window.
+ */
+class ConfidentialTestAdapter implements CryptoAdapter {
+ readonly confidential = true;
+ async encrypt(plaintext: string): Promise<string> {
+ return 'kms:' + Buffer.from(plaintext, 'utf8').toString('base64');
+ }
+ async decrypt(ciphertext: string): Promise<string> {
+ return Buffer.from(ciphertext.replace(/^kms:/, ''), 'base64').toString('utf8');
+ }
+ digest(): string {
+ return 'fnv32:deadbeef';
+ }
+}
+
 describe('aiSettingsManifest', () => {
  it('parses against SettingsManifestSchema', () => {
  expect(() => SettingsManifestSchema.parse(aiSettingsManifest)).not.toThrow();
@@ -143,7 +164,7 @@ describe('aiSettingsManifest — temperature declares a window, not a grid (#655
  // default provider is `memory`, so the patch carries a real provider (and
  // its required key) — otherwise the TOUCH/visible contract skips the
  // specifier entirely and this test would be green for the wrong reason.
- const svc = new SettingsService({ env: {} });
+ const svc = new SettingsService({ env: {}, crypto: new ConfidentialTestAdapter() });
  svc.registerManifest(aiSettingsManifest);
  await expect(
  svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 0.15 }),
@@ -157,7 +178,7 @@ describe('aiSettingsManifest — temperature declares a window, not a grid (#655
  });
 
  it('write door: the window still binds — out-of-range values are refused in the min/max vocabulary', async () => {
- const svc = new SettingsService({ env: {} });
+ const svc = new SettingsService({ env: {}, crypto: new ConfidentialTestAdapter() });
  svc.registerManifest(aiSettingsManifest);
  await expect(
  svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 2.5 }),