fix(service-datasource)!: refuse every top-level key DatasourceSchema refuses at the datasource test, create and update doors

.changeset/28612-datasource-undeclared-top-level-key.md

@@ -0,0 +1,18 @@
+---
+'@objectstack/service-datasource': patch
+---
+
+fix(service-datasource)!: `POST /api/v1/datasources/test`, `POST /api/v1/datasources` and `PATCH /api/v1/datasources/:name` refuse every top-level key `DatasourceSchema` refuses, with that schema's own message, instead of dropping it
+
+Clause-②: no (narrowing) — a test, create or edit body carrying a top-level key the datasource schema does not declare (a `password` on the test door; on all three doors a `url` or `host` one level too high, an alias such as `type`, a retired block such as `readReplicas`, or any unknown key), which the test door probed without and the write doors answered 201 / 200 with the key silently dropped, is refused with `400 DATASOURCE_ADMIN_ERROR`. Nothing is widened.
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped, and no export or type changes: the datasource admin doors now refuse the top-level keys that `DatasourceSchema` already refuses at the build door, with that schema's own message. A refused body was never stored (the key was dropped), so there is nothing at rest for a migration to rewrite and no tombstone. -->
+
+**BREAKING** accept-set narrowing at the datasource admin doors (`@objectstack/service-datasource`).
+
+- **Before.** The unsaved-draft probe and the two write doors built their record from its known keys only. A top-level `password` on the probe vanished, so the probe tested a different connection than the one described. On all three doors any other key the strict schema refuses vanished the same way, and the write answered success.
+- **Now.** A top-level key `DatasourceSchema` does not declare is refused before anything is read, written or probed, naming the key and never its value. The message is the schema's own refusal, the one `os compile` prints: for example `url` belongs inside `config`, `type` is `driver`, `readReplicas` is retired. The test door answers under the heading `This connection draft is not a valid datasource.`; create and edit under `Invalid datasource 'NAME'.`. This supersedes the "other unknown top-level keys are still dropped as before" line of the #28549 entry.
+- **Unchanged.** Keys the schema declares pass the check, including the ones the doors do not store (`origin`, `description`, `ssl`, `autoConnect`). A credential sent as `secret` is still split off by the route and bound at `external.credentialsRef`. The probe's other answers (`ok: false` with a reason) are unchanged.
+
+**The remedy.** Remove the key, or move it where the message says: connection keys into `config`, the credential into `secret`. Keys copied from a `GET /api/v1/datasources` or `GET /api/v1/datasources/NAME` answer that the schema does not declare (`hasSecret`, `redactedConfigKeys`, `definedIn`, `status`, `statusReason`) are not sent back on an edit.

packages/services/service-datasource/src/__tests__/datasource-admin-top-level-credential.test.ts

@@ -50,7 +50,10 @@ function makeDoor(seed: StoredDatasource[] = []) {
   const ops: string[] = [];
 
   const config: DatasourceAdminServiceConfig = {
-    probe: async () => ({ ok: true, latencyMs: 1 }),
+    probe: async () => {
+      ops.push('probe');
+      return { ok: true, latencyMs: 1 };
+    },
     listDatasourceRecords: async () => [...records.values()].map((r) => structuredClone(r)),
     getDatasourceRecord: async (name) => {
       const r = records.get(name);
@@ -275,3 +278,123 @@ describe('#28549 — controls: the doors keep their other answers', () => {
     expect(ops).toEqual([]);
   });
 });
+
+/**
+ * #28612 — the rest of #28549: the unsaved-draft probe, and every top-level
+ * key `DatasourceSchema` refuses, not only the two credential keys.
+ *
+ * `POST /api/v1/datasources/test` built its probe through the same known-key
+ * projection, so a top-level `password` vanished and the probe tested a
+ * different connection than the one described. And the write doors dropped
+ * every other key the strict shape refuses (`foo`, a `url` one level too high,
+ * an alias such as `type`). All three doors now give the schema's own
+ * root refusal for whatever top-level keys it does not declare. Keys the
+ * schema declares stay accepted, and `secret` (split off by the route before
+ * the service sees the draft) stays the credential channel.
+ */
+describe('#28612 — POST /api/v1/datasources/test refuses a top-level key the schema refuses', () => {
+  const PROBE_HEADING = 'This connection draft is not a valid datasource.';
+
+  it.each([
+    ['password', { password: PLACEHOLDER_PASSWORD }],
+    ['connectionString', { connectionString: PLACEHOLDER_DSN }],
+    ['foo', { foo: 'bar' }],
+  ])('refuses a top-level `%s` before any probe runs', async (key, extra) => {
+    const { send, records, secrets, ops } = makeDoor();
+
+    const message = await expectRefusal(await send('POST', '/datasources/test', { ...BASE, ...extra }));
+
+    expect(message).toBe(`${PROBE_HEADING}\n${specRefusal([key])}`);
+    expect(message).not.toContain(PLACEHOLDER_PASSWORD);
+    expect(records.size).toBe(0);
+    expect(secrets.size).toBe(0);
+    expect(ops).toEqual([]);
+  });
+
+  it('refuses it beside a supplied `secret` too', async () => {
+    const { send, ops } = makeDoor();
+
+    const message = await expectRefusal(
+      await send('POST', '/datasources/test', { ...BASE, password: PLACEHOLDER_PASSWORD, secret: PLACEHOLDER_PASSWORD }),
+    );
+
+    expect(message).toBe(`${PROBE_HEADING}\n${specRefusal(['password'])}`);
+    expect(message).not.toContain(PLACEHOLDER_PASSWORD);
+    expect(ops).toEqual([]);
+  });
+});
+
+describe('#28612 — the write doors refuse every top-level key the schema refuses', () => {
+  it.each([
+    ['foo', { foo: 'bar' }],
+    ['url', { url: PLACEHOLDER_DSN }],
+    ['type', { type: 'postgres' }],
+    ['readReplicas', { readReplicas: [] }],
+  ])('POST refuses a top-level `%s`, storing nothing', async (key, extra) => {
+    const { send, records, secrets, ops } = makeDoor();
+
+    const message = await expectRefusal(await send('POST', '/datasources', { name: 'u_key', ...BASE, ...extra }));
+
+    expect(message).toBe(`Invalid datasource 'u_key'.\n${specRefusal([key])}`);
+    expect(message).not.toContain(PLACEHOLDER_PASSWORD);
+    expect(records.has('u_key')).toBe(false);
+    expect(secrets.size).toBe(0);
+    expect(ops).toEqual([]);
+  });
+
+  it('POST names every refused key in one answer', async () => {
+    const { send, records } = makeDoor();
+
+    const message = await expectRefusal(
+      await send('POST', '/datasources', { name: 'u_many', ...BASE, foo: 1, host: 'db.example.invalid', password: PLACEHOLDER_PASSWORD }),
+    );
+
+    expect(message).toBe(`Invalid datasource 'u_many'.\n${specRefusal(['foo', 'host', 'password'])}`);
+    expect(records.has('u_many')).toBe(false);
+  });
+
+  it.each([
+    ['foo', { config: BASE.config, foo: 'bar' }],
+    ['foo', { label: 'Warehouse', foo: 'bar' }],
+    ['hasSecret', { label: 'Warehouse', hasSecret: false }],
+  ])('PATCH refuses a top-level `%s`, leaving the stored record as it was', async (key, body) => {
+    const { send, records, ops } = makeDoor([STORED]);
+
+    const message = await expectRefusal(await send('PATCH', '/datasources/wh', body));
+
+    expect(message).toBe(`Invalid datasource 'wh'.\n${specRefusal([key])}`);
+    expect(records.get('wh')).toEqual(STORED);
+    expect(ops).toEqual([]);
+  });
+});
+
+describe('#28612 — controls: declared keys and the secret channel still pass', () => {
+  it('still probes a valid draft, its credential arriving as `secret`', async () => {
+    const { send, ops } = makeDoor();
+
+    const res = await send('POST', '/datasources/test', { ...BASE, secret: PLACEHOLDER_PASSWORD });
+
+    expect(res.status).toBe(200);
+    const body = (await res.json()) as { data?: { result?: { ok: boolean } } };
+    expect(body.data?.result?.ok).toBe(true);
+    expect(ops).toEqual(['probe']);
+  });
+
+  it('still creates with a key the schema declares and the projection leaves out (`description`, `origin`)', async () => {
+    const { send, records } = makeDoor();
+
+    const res = await send('POST', '/datasources', { name: 'wh_decl', ...BASE, description: 'Warehouse', origin: 'code' });
+
+    expect(res.status).toBe(201);
+    expect(records.get('wh_decl')?.origin).toBe('runtime');
+  });
+
+  it('still edits with a key the schema declares (`name`, `origin`)', async () => {
+    const { send, records } = makeDoor([STORED]);
+
+    const res = await send('PATCH', '/datasources/wh', { name: 'wh', origin: 'runtime', label: 'Warehouse' });
+
+    expect(res.status).toBe(200);
+    expect(records.get('wh')?.label).toBe('Warehouse');
+  });
+});

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

@@ -86,17 +86,6 @@ const PROBE_DRAFT_NAME = 'connection_test_draft';
  */
 const RECORD_JUDGED_PATCH_KEYS = ['driver', 'schemaMode', 'config', 'external', 'pool'] as const;
 
-/**
- * [#28549] The top-level body keys a credential arrives under that the record
- * projection ({@link DatasourceAdminService.toRecord}, the update merge) would
- * otherwise drop without a word: a `password`, and a `connectionString` that
- * can carry one as URL userinfo. `DatasourceSchema` refuses both at the build
- * door; these doors judge exactly these keys through it. Not every key the
- * projection drops: those carry no credential, and a closed body at these
- * doors is a change of its own.
- */
-const TOP_LEVEL_CREDENTIAL_KEYS = ['password', 'connectionString'] as const;
-
 /** A copy of `record` carrying the binding a supplied secret is about to create. */
 function withPendingSecretRef(record: StoredDatasource): StoredDatasource {
   return { ...record, external: { ...(record.external ?? {}), credentialsRef: PENDING_SECRET_REF } };
@@ -635,6 +624,14 @@ export class DatasourceAdminService implements IDatasourceAdminService {
   }
 
   async testConnection(input: DatasourceDraft, secret?: SecretInput): Promise<TestConnectionResult> {
+    // [#28612] First, and as a refusal (a `400` at the route) rather than a
+    // failed probe: the probe below is built from the known keys only, so a
+    // top-level `password` or any other key the schema refuses would vanish
+    // and the answer would be about a different connection than the one
+    // described.
+    if (input && typeof input === 'object') {
+      this.assertNoUndeclaredTopLevelKey('This connection draft is not a valid datasource.', input);
+    }
     if (!input?.driver) {
       return { ok: false, error: 'A driver is required to test a connection.' };
     }
@@ -678,7 +675,7 @@ export class DatasourceAdminService implements IDatasourceAdminService {
   async createDatasource(input: DatasourceDraft, secret?: SecretInput): Promise<DatasourceSummary> {
     this.assertValidName(input?.name);
     this.assertNoDraftCredentialsRef(input);
-    this.assertNoTopLevelCredential(input.name, input);
+    this.assertNoUndeclaredTopLevelKey(`Invalid datasource '${input.name}'.`, input);
     if (!input.driver) throw new Error('A driver is required to create a datasource.');
     this.assertValidConfig(input.driver, input.config);
     // The wizard is a publish door for `pool` too (#5714). Rejected BEFORE the
@@ -740,10 +737,10 @@ export class DatasourceAdminService implements IDatasourceAdminService {
     if (existing.origin !== 'runtime') {
       throw new Error(`Datasource '${name}' is code-defined and cannot be edited at runtime.`);
     }
-    // [#28549] Whatever else the patch touches, a `label`/`active`-only edit
-    // included: that exemption spares a row stored before a gate, and this
-    // judges a credential the author is writing now.
-    this.assertNoTopLevelCredential(existing.name, patch);
+    // [#28549 · #28612] Whatever else the patch touches, a `label`/`active`-only
+    // edit included: that exemption spares a row stored before a gate, and this
+    // judges a key the author is writing now.
+    this.assertNoUndeclaredTopLevelKey(`Invalid datasource '${existing.name}'.`, patch);
 
     // Merge patch over the existing record; `name`/`origin` are never patched.
     const merged: StoredDatasource = {
@@ -1261,23 +1258,29 @@ export class DatasourceAdminService implements IDatasourceAdminService {
   }
 
   /**
-   * [#28549] Refuse a body that writes a credential at the top level
-   * ({@link TOP_LEVEL_CREDENTIAL_KEYS}). The record is built from the known
-   * keys only, so such a key would vanish and the write answer success over a
-   * datasource that cannot connect, while the build door refuses the same
-   * datasource by name.
+   * [#28549 · #28612] Refuse a body that carries a top-level key
+   * `DatasourceSchema` does not declare: a credential (`password`, a
+   * `connectionString` with URL userinfo), a connection key one level too high
+   * (`url`, `host`, …), an alias (`type`), a retired block, or any unknown key.
+   * The record and the probe are built from the known keys only, so such a key
+   * would vanish and the door answer success (or probe) over a datasource
+   * other than the one described, while the build door refuses it by name.
+   *
+   * The key set and the prescription are `DatasourceSchema`'s own: the body's
+   * key names, each with `null` for its value, are parsed through the strict
+   * shape and only its root `unrecognized_keys` refusal is kept, the one
+   * `os compile` prints. A key the schema declares passes here even when the
+   * projection leaves it out (`origin`, `description`, …). Only key names are
+   * judged, never values, so the answer cannot echo a credential. ⛔ No key
+   * list or prescription is restated here; the spec is the one copy. Thrown
+   * before anything is read, written or probed (a `400` at the route).
    *
-   * The verdict and its prescription are `DatasourceSchema`'s own refusal of
-   * those keys, the one `os compile` prints, under the heading
-   * {@link assertValidRecord} gives. Only the key names are judged, never the
-   * values, so the answer names the key and cannot echo the credential. ⛔ No
-   * prescription is restated here; the spec is the one copy. Thrown before
-   * anything is read or written (a `400` at the route).
+   * `secret` never reaches this check: the route splits it off the body
+   * (`splitSecret`) before the service sees the draft, and it stays the one
+   * credential channel.
    */
-  private assertNoTopLevelCredential(name: string, body: object): void {
-    const present = TOP_LEVEL_CREDENTIAL_KEYS.filter(
-      (key) => (body as Record<string, unknown>)[key] !== undefined,
-    );
+  private assertNoUndeclaredTopLevelKey(heading: string, body: object): void {
+    const present = Object.keys(body).filter((key) => (body as Record<string, unknown>)[key] !== undefined);
     if (present.length === 0) return;
     const result = DatasourceSchema.safeParse(Object.fromEntries(present.map((key) => [key, null])));
     const detail = result.success
@@ -1286,7 +1289,7 @@ export class DatasourceAdminService implements IDatasourceAdminService {
           .filter((issue) => issue.code === 'unrecognized_keys' && issue.path.length === 0)
           .map((issue) => issue.message);
     if (detail.length === 0) return;
-    throw new Error(`Invalid datasource '${name}'.\n${detail.join('\n')}`);
+    throw new Error(`${heading}\n${detail.join('\n')}`);
   }
 
   private toRecord(input: DatasourceDraft): StoredDatasource {