fix(service-datasource)!: the datasource admin doors refuse an empty secret instead of sealing it over the working credential

.changeset/28928-empty-datasource-secret-refused.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/service-datasource': patch
+---
+
+fix(service-datasource)!: the datasource admin doors refuse an empty `secret` instead of sealing it over the working credential
+
+Clause-②: no (narrowing) — `POST /api/v1/datasources` and `PATCH /api/v1/datasources/:name` refuse one body that used to answer 201 / 200: a `secret` whose value is the empty string (`"secret": ""` or `"secret": { "value": "" }`), now `400 DATASOURCE_ADMIN_ERROR` naming `secret`. No new error code, no new answer member, no exported type changes.
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped: secret is the doors' own credential channel, split off the body before the datasource draft is judged. The refused value stored nothing usable, and a row sealed with it before is reported at connect with a reason that says the credential is empty, so there is nothing at rest for a migration to rewrite. -->
+
+**BREAKING** accept-set narrowing at the datasource admin doors (`@objectstack/service-datasource`): an empty `secret` is refused.
+
+- **Before.** An empty `secret` was taken as a value. A create sealed it as the datasource's credential. An edit took it as a rotation: a new `sys_secret` row holding an empty value, `external.credentialsRef` repointed at it, and the working row removed. The live pool kept serving, so the datasource broke only at its next connect, with the old credential already gone.
+- **Now.** Both doors refuse it before anything is written: no `sys_secret` row is written, the stored record and its `credentialsRef` are unchanged, and no row is removed. A whitespace-only value is still a value, sealed and used as sent. `null` and an absent `secret` keep the stored credential, as before.
+- **Rows sealed before.** A datasource whose stored credential decrypts to an empty value still fails closed at connect (`failed-credentials`), and its status reason now says the credential is empty instead of blaming a missing `sys_secret` row or a changed encryption key.
+- **Who moves.** The console sends `secret` only when the field holds a value, so it never sent an empty one. A script or SDK call that sends `"secret": ""` to mean "no change" gets the 400.
+
+**The remedy.** To keep the stored credential, leave `secret` out of the request (or send `null`). To replace it, send the new credential.

packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts

@@ -987,6 +987,19 @@ describe('D3 credential resolution — fail-closed', () => {
     expect(factory.create).not.toHaveBeenCalled();
   });
 
+  // [#28928] A row sealed before the doors refused an empty `secret` decrypts
+  // to ''. Still fail-closed, but the reason names an empty credential, not a
+  // missing row or a changed key.
+  it('fails closed on a credential that decrypts to an empty value, and says it is empty', async () => {
+    const factory = fakeFactory();
+    const { service } = svc({ factory, secrets: { resolve: async () => '' } });
+    const result = await service.connect(credExternal, { context: { trigger: 'runtime-admin' } });
+    expect(result.status).toBe('failed-credentials');
+    expect(result.reason).toMatch(/credential 'sys_secret:abc' is empty/);
+    expect(result.reason).not.toMatch(/could not be resolved|missing sys_secret row|encryption key/);
+    expect(factory.create).not.toHaveBeenCalled();
+  });
+
   it('fails closed when the resolver throws', async () => {
     const factory = fakeFactory();
     const { service } = svc({

packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts

@@ -0,0 +1,193 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect, vi } from 'vitest';
+import { HonoHttpServer } from '@objectstack/plugin-hono-server';
+import { registerDatasourceAdminRoutes } from '../admin-routes.js';
+import {
+  DatasourceAdminService,
+  type DatasourceAdminServiceConfig,
+  type ProbeInput,
+  type StoredDatasource,
+} from '../datasource-admin-service.js';
+import {
+  ENTITLED_CREDENTIAL,
+  createSessionAuthService,
+  createGrantsEngine,
+} from './entitled-caller.fixture.js';
+
+/**
+ * #28928 — an empty `secret` is refused at the datasource admin doors, never
+ * sealed as the credential.
+ *
+ * `POST /api/v1/datasources` and `PATCH /api/v1/datasources/:name` take a
+ * door-only `secret` and seal it into the secret store. `"secret": ""` was
+ * read as a value: the create sealed an empty credential, and an update
+ * minted a new `sys_secret` holding an empty value, repointed
+ * `external.credentialsRef` at it and removed the working one. The pool in
+ * service kept running, so the datasource broke only at the next connect.
+ *
+ * Now both doors refuse it `400` naming `secret`, before any write: no secret
+ * is written, the stored record and its `credentialsRef` are unchanged, and no
+ * secret is removed. `null`, an absent `secret` and a real one are unchanged.
+ *
+ * Driven through the REAL routes over the REAL service on in-memory stores
+ * (the harness of `datasource-admin-declared-keys-kept.test.ts`). Every
+ * secret below is an obvious placeholder.
+ */
+
+const authService = createSessionAuthService();
+const grantsEngine = createGrantsEngine();
+
+const BASE = { driver: 'postgres', config: { host: 'db.example.invalid', database: 'analytics', username: 'qa' } };
+const WORKING_REF = 'sys_secret:sec_working';
+const PLACEHOLDER_SECRET = 'dummy-rotated-password';
+
+/** In-memory stores behind a real {@link DatasourceAdminService}, recording every seam call. */
+function makeDoor(seed: StoredDatasource[] = []) {
+  const records = new Map<string, StoredDatasource>(seed.map((r) => [r.name, structuredClone(r)]));
+  const ops: string[] = [];
+  const pooled: StoredDatasource[] = [];
+  const probes: ProbeInput[] = [];
+  const written: string[] = [];
+
+  const config: DatasourceAdminServiceConfig = {
+    probe: async (input) => {
+      ops.push('probe');
+      probes.push(structuredClone(input));
+      return { ok: true, latencyMs: 1 };
+    },
+    listDatasourceRecords: async () => [...records.values()].map((r) => structuredClone(r)),
+    getDatasourceRecord: async (name) => {
+      const r = records.get(name);
+      return r ? structuredClone(r) : undefined;
+    },
+    putDatasourceRecord: async (record) => {
+      ops.push(`put:${record.name}`);
+      records.set(record.name, structuredClone(record));
+    },
+    deleteDatasourceRecord: async (name) => {
+      records.delete(name);
+    },
+    writeSecret: async (input, hint) => {
+      ops.push('writeSecret');
+      written.push(input.value);
+      return `sys_secret:sec_${hint.name}_${written.length}`;
+    },
+    removeSecret: async (ref) => {
+      ops.push(`removeSecret:${ref}`);
+    },
+    countBoundObjects: async () => 0,
+    listObjectsBoundTo: async () => [],
+    registerPool: (record) => {
+      ops.push('registerPool');
+      pooled.push(structuredClone(record));
+    },
+    unregisterPool: () => {
+      ops.push('unregisterPool');
+    },
+    reregisterPool: (_previous, next) => {
+      ops.push('reregisterPool');
+      pooled.push(structuredClone(next));
+    },
+  };
+
+  const service = new DatasourceAdminService(config);
+  const server = new HonoHttpServer(0);
+  const ctx = {
+    getService: vi.fn((name: string) =>
+      name === 'auth'
+        ? authService
+        : name === 'objectql' || name === 'data'
+          ? grantsEngine
+          : name === 'datasource-admin'
+            ? service
+            : undefined,
+    ),
+  } as any;
+  registerDatasourceAdminRoutes(server, ctx, '/api/v1');
+  const app = server.getRawApp();
+
+  const send = (method: string, path: string, body?: unknown) =>
+    app.fetch(
+      new Request(`http://local/api/v1${path}`, {
+        method,
+        headers: { 'content-type': 'application/json', authorization: ENTITLED_CREDENTIAL },
+        ...(body === undefined ? {} : { body: JSON.stringify(body) }),
+      }),
+    );
+
+  return { send, records, ops, pooled, probes, written };
+}
+
+/** A runtime datasource bound to a working credential. */
+const STORED: StoredDatasource = {
+  name: 'wh',
+  ...BASE,
+  origin: 'runtime',
+  external: { credentialsRef: WORKING_REF },
+} as StoredDatasource;
+
+async function expectSecretRefusal(res: Response): Promise<string> {
+  const body = (await res.json()) as { success: boolean; error?: { code: string; message: string } };
+  expect({ status: res.status, success: body.success, code: body.error?.code }).toEqual({
+    status: 400,
+    success: false,
+    code: 'DATASOURCE_ADMIN_ERROR',
+  });
+  expect(body.error!.message).toContain('`secret`');
+  return body.error!.message;
+}
+
+describe('PATCH /api/v1/datasources/:name — an empty secret (#28928)', () => {
+  for (const [label, secret] of [['an empty string', ''], ['an empty `value` object', { value: '' }]] as const) {
+    it(`${label} is refused 400 naming \`secret\`; the credential, its ref and the record are unchanged`, async () => {
+      const { send, records, ops, written } = makeDoor([STORED]);
+      await expectSecretRefusal(await send('PATCH', '/datasources/wh', { secret }));
+      expect(records.get('wh')).toEqual(STORED);
+      expect(written).toEqual([]);
+      expect(ops).toEqual([]);
+    });
+  }
+
+  it('control: a real secret still rotates (a new ref, the old one removed)', async () => {
+    const { send, records, ops, written } = makeDoor([STORED]);
+    expect((await send('PATCH', '/datasources/wh', { secret: PLACEHOLDER_SECRET })).status).toBe(200);
+    expect(written).toEqual([PLACEHOLDER_SECRET]);
+    expect(records.get('wh')?.external?.credentialsRef).toBe('sys_secret:sec_wh_1');
+    expect(ops).toContain(`removeSecret:${WORKING_REF}`);
+  });
+
+  for (const [label, body] of [['`secret: null`', { secret: null, label: 'Warehouse' }], ['no `secret`', { label: 'Warehouse' }]] as const) {
+    it(`control: ${label} changes no credential`, async () => {
+      const { send, records, ops, written } = makeDoor([STORED]);
+      expect((await send('PATCH', '/datasources/wh', body)).status).toBe(200);
+      expect(written).toEqual([]);
+      expect(records.get('wh')?.external?.credentialsRef).toBe(WORKING_REF);
+      expect(ops.some((op) => op.startsWith('removeSecret'))).toBe(false);
+    });
+  }
+});
+
+describe('POST /api/v1/datasources — an empty secret (#28928)', () => {
+  it('is refused 400 naming `secret`; nothing is stored and no secret is written', async () => {
+    const { send, records, ops, written } = makeDoor();
+    await expectSecretRefusal(await send('POST', '/datasources', { name: 'wh', ...BASE, secret: '' }));
+    expect(records.has('wh')).toBe(false);
+    expect(written).toEqual([]);
+    expect(ops).toEqual([]);
+  });
+
+  it('control: a real secret is sealed and bound', async () => {
+    const { send, records, written } = makeDoor();
+    expect((await send('POST', '/datasources', { name: 'wh', ...BASE, secret: PLACEHOLDER_SECRET })).status).toBe(201);
+    expect(written).toEqual([PLACEHOLDER_SECRET]);
+    expect(records.get('wh')?.external?.credentialsRef).toBe('sys_secret:sec_wh_1');
+  });
+
+  it('control: `secret: null` creates the datasource with no credential', async () => {
+    const { send, records, written } = makeDoor();
+    expect((await send('POST', '/datasources', { name: 'wh', ...BASE, secret: null })).status).toBe(201);
+    expect(written).toEqual([]);
+    expect(records.get('wh')?.external?.credentialsRef).toBeUndefined();
+  });
+});

packages/services/service-datasource/src/datasource-admin-service.ts

@@ -473,6 +473,29 @@ export function datasourceNotFoundError(name: string): Error {
   return err;
 }
 
+/**
+ * [#28928] Refuse a `secret` whose value is the empty string, on create and on
+ * edit, before anything is read for the secret or written.
+ *
+ * A create sealed it as the datasource's credential, and an edit took it as a
+ * rotation: a new `sys_secret` row holding nothing, `external.credentialsRef`
+ * repointed at it, and the working row removed. The live pool kept serving, so
+ * the datasource broke only at its next connect, with nothing left to restore.
+ * The console never sends one (it leaves `secret` out of an untouched field);
+ * a script or an SDK call that clears the field is what reached it.
+ *
+ * `null` and an absent `secret` are unchanged: they keep the stored
+ * credential. A whitespace-only value is a value, sealed and used as sent.
+ * Only the empty string is judged, so the answer cannot echo a credential.
+ */
+function assertSecretNotEmpty(heading: string, secret: SecretInput | undefined): void {
+  if (secret == null || (secret as { value?: unknown }).value !== '') return;
+  throw new Error(
+    `${heading}\nThe \`secret\` is empty. Send the credential to store, or leave \`secret\` out (or null) `
+      + 'to keep the stored one. Nothing was written.',
+  );
+}
+
 function metadataIncompleteError(message: string, errors: string[]): Error {
   const err = new Error(message) as Error & { code?: string; status?: number; cause?: unknown };
   err.code = 'SERVICE_UNAVAILABLE';
@@ -793,6 +816,7 @@ export class DatasourceAdminService implements IDatasourceAdminService {
 
   async createDatasource(input: DatasourceDraft, secret?: SecretInput): Promise<DatasourceSummary> {
     this.assertValidName(input?.name);
+    assertSecretNotEmpty(`Invalid datasource '${input.name}'.`, secret);
     this.assertNoDraftCredentialsRef(input);
     this.assertNoUndeclaredTopLevelKey(`Invalid datasource '${input.name}'.`, input);
     assertNoInlineSslKey(`Invalid datasource '${input.name}'.`, input);
@@ -862,6 +886,7 @@ export class DatasourceAdminService implements IDatasourceAdminService {
     // judges a key the author is writing now.
     this.assertNoUndeclaredTopLevelKey(`Invalid datasource '${existing.name}'.`, patch);
     assertNoInlineSslKey(`Invalid datasource '${existing.name}'.`, patch);
+    assertSecretNotEmpty(`Invalid datasource '${existing.name}'.`, secret);
 
     // Merge patch over the existing record; `name`/`origin` are never patched.
     // [#28633] Every key the create door stores is patchable the same way

packages/services/service-datasource/src/datasource-connection-service.ts

@@ -727,7 +727,21 @@ export class DatasourceConnectionService {
       } catch (err) {
         return this.handleFailure(record, 'failed-credentials', `resolving credential '${credentialsRef}' threw: ${errMsg(err)}`, opts.context, opts.objects, opts.mappedObjects, err);
       }
-      if (secret == null || secret === '') {
+      // [#28928] An empty credential is told apart from one that is gone: the
+      // admin doors refuse an empty `secret` now, but a row sealed before that
+      // decrypts to '' and its datasource still reaches this branch. Blaming a
+      // missing row or a changed key would send the operator after the wrong
+      // fault; the fix is to set the credential again.
+      if (secret === '') {
+        return this.handleFailure(
+          record,
+          'failed-credentials',
+          `credential '${credentialsRef}' is empty (its sys_secret row holds an empty value); set the datasource's secret again`,
+          opts.context,
+          opts.objects,
+        );
+      }
+      if (secret == null) {
         return this.handleFailure(
           record,
           'failed-credentials',