diff --git a/.changeset/28612-datasource-undeclared-top-level-key.md b/.changeset/28612-datasource-undeclared-top-level-key.md new file mode 100644 index 00000000000..9287c9f062f --- /dev/null +++ b/.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 + + + +**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. diff --git a/packages/services/service-datasource/src/__tests__/datasource-admin-top-level-credential.test.ts b/packages/services/service-datasource/src/__tests__/datasource-admin-top-level-credential.test.ts index 196eb24d895..9b98dc141ef 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-admin-top-level-credential.test.ts +++ b/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'); + }); +}); diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index fae370d6fc0..30943306cc1 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/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 { + // [#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 { 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)[key] !== undefined, - ); + private assertNoUndeclaredTopLevelKey(heading: string, body: object): void { + const present = Object.keys(body).filter((key) => (body as Record)[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 {