diff --git a/.changeset/28928-empty-datasource-secret-refused.md b/.changeset/28928-empty-datasource-secret-refused.md new file mode 100644 index 00000000000..cb97303399c --- /dev/null +++ b/.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 + + + +**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. diff --git a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts b/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts index 0f62a71db26..6a9d8545ddb 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts +++ b/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({ diff --git a/packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts b/packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts new file mode 100644 index 00000000000..262ccac0433 --- /dev/null +++ b/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(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 { + 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(); + }); +}); diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index ef8fb4d4c1a..1933a09ef47 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/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 { 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 diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 2753417323b..8057daae9ef 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/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',