Skip to content

Commit 37b82ed

Browse files
huangyiireneclaude
andauthored
fix(service-datasource): stop serving stored credentials on the datasource read path, and fix the false "credential-stripped" claim (#8081) (#8126)
`getDatasource()` returned the driver `config` verbatim while its own doc comment promised "with the credential stripped" and described `config` as "non-sensitive — credentials live in `sys_secret`, never in config". Nothing stripped anything. The comment was load-bearing: it is why the gap survived a 26-surface credential survey. #8078 closed the WRITE door (`config.password` / `config.authToken` are `z.never()` on every driver that has them) and deliberately left stored rows alone. Those rows still hold cleartext, and `GET /api/v1/datasources/:name` handed it to every caller. The refused-key set is DERIVED from each driver's contract — #8078 spells a refused inline credential as `z.never()`, so the schema is the list — plus the pre-#8078 alias spellings (a stored row never met the parse that renamed them) and turso's still-writable `encryptionKey`. A driver with no shipped contract keeps the canonical spellings hidden by name. A credential embedded in `config.url` is redacted too, at the password component of the userinfo only. Refusing such a URL at the write door stays UNRULED (#7990) and untouched: redacting on the way out is not refusing on the way in, and a scrub that dropped `config.password` while serving the same secret one key over would be a scrub in name only. `updateDatasource` carries the hidden material forward across a same-driver round-trip, after the validation gate rather than before it — the gate judges what the author wrote, and this is material the author never saw. Without it the scrub would have turned every untouched "Save" into silent credential deletion. It also repairs a regression #8078 introduced and this card measured: the form was served `config.password` verbatim, posted it back, and the gate refused it — so editing any legacy datasource answered 400 for a value the server itself had just supplied. The stored record is never mutated; the connect path is unaffected. Reverse verification: 9 of 21 pins fail on `origin/main` (each naming the defect it carries), 21/21 pass here. The two guard pins — #8078's refusal with its guidance intact, and URL credentials still ACCEPTED at the write door — pass on both sides by design. Claude-Session: https://claude.ai/code/session_01WCoM9vVXw6yAVgEc4oio4m Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent d99ff1a commit 37b82ed

5 files changed

Lines changed: 737 additions & 10 deletions

File tree

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
---
2+
"@objectstack/service-datasource": patch
3+
---
4+
5+
fix(service-datasource): the datasource read path stops serving stored credentials in cleartext, and the "credential-stripped" comment stops lying (#8081)
6+
7+
`GET /api/v1/datasources/:name` returned the driver `config` **verbatim**, while
8+
the method producing it carried a doc comment promising the opposite —
9+
"with the credential stripped", `config` described as "non-sensitive —
10+
credentials live in `sys_secret`, never in config". Nothing stripped anything.
11+
12+
The comment was not merely stale, it was **load-bearing**: it is the reason the
13+
gap survived a 26-surface credential survey. A safety claim that no code performs
14+
is worse than no claim, because it stops the next reader from looking.
15+
16+
#8078 closed the WRITE door — `config.password` / `config.authToken` are
17+
declared-unwritable on every driver that has them, so no new row can carry an
18+
inline credential. It deliberately did not touch rows already stored. Those rows
19+
still hold cleartext, and until now the admin read path handed it to every caller
20+
of that route.
21+
22+
**What is redacted.** The refused-key set is DERIVED from each driver's own
23+
contract rather than retyped here: #8078 spells a refused inline credential as
24+
`z.never()`, so the schema *is* the list, and a credential key refused tomorrow
25+
is covered the day it lands. Three sources feed the scrub — the derived keys, the
26+
pre-#8078 alias spellings (`passwd`/`pwd`/`token`/`jwt`/`auth_token`/`authtoken`,
27+
which a stored row can still hold verbatim because the wizard persists through
28+
`metadata.register` and never met the parse that would have renamed them), and
29+
turso's `encryptionKey`, an AES-256 key that remains writable because the secret
30+
binder has no slot for it. A driver the platform ships no contract for still has
31+
the canonical spellings hidden by name: declining to *refuse* an unrecognised key
32+
is a boundary choice about authoring, while serving a key literally named
33+
`password` back in cleartext is a leak under any boundary.
34+
35+
**URL-embedded credentials.** A `postgresql://user:pass@host/db` in `config.url`
36+
carries the same secret as `config.password`, and a scrub that dropped one while
37+
serving the other one key over would be a scrub in name only. The read path now
38+
redacts the **password component of a URL's userinfo**, preserving the scheme,
39+
the username and everything from the host onward. Refusing such a URL at the
40+
write door remains deliberately **unruled** (#7990) and is untouched: redacting a
41+
value on the way out is not the same act as refusing it on the way in.
42+
43+
**The response says what it withheld.** `getDatasource()` gains
44+
`redactedConfigKeys`, so a caller knows a credential is being held back rather
45+
than inferring it from an absence — the same courtesy the existing `hasSecret`
46+
flag pays for the bound `sys_secret` handle.
47+
48+
**A round-trip no longer destroys the credential — and no longer 400s.** The
49+
edit form reads this config and patches it straight back, so a scrub without an
50+
inverse would have turned every untouched "Save" into silent credential deletion.
51+
`updateDatasource` therefore carries the hidden material forward when a patch is
52+
round-tripping the same driver's config, after the validation gate rather than
53+
before it: the gate judges what the *author* wrote, and this material is
54+
something the author never saw and is not asking to change. Restoring it is the
55+
same rule the `credentialsRef` beside it has always followed.
56+
57+
This also repairs a regression that arrived with #8078 and is measured here for
58+
the first time: on `main` the form was served `config.password` verbatim, posted
59+
it back unchanged, and the write gate refused it — so **editing any legacy
60+
datasource through the wizard answered 400 for a value the server itself had
61+
just supplied**, including the `active: false` that takes a misconfigured
62+
datasource out of service.
63+
64+
**Not changed.** The stored record is never mutated: redaction is a read-path act
65+
only, the connect path reads the raw record, and a legacy datasource keeps
66+
authenticating exactly as before. Getting cleartext *out* of the store is a
67+
migration with its own decision to make and is not attempted here.
Lines changed: 320 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,320 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #8081 — the services half of #7990: the datasource credential READ path.
5+
*
6+
* ## What each pin reads on `origin/main` (reverse verification)
7+
*
8+
* RED on main (carries the defect this card fixes):
9+
* - "serves a stored row's cleartext password" — `getDatasource()` returned
10+
* `config` verbatim, so `config.password: 'hunter2'` came back on
11+
* `GET /api/v1/datasources/:name`. Measured on `origin/main` before the fix.
12+
* - "serves the credential embedded in a connection URL" — the same read
13+
* returned `postgresql://admin:hunter2@db.internal:5432/app`. This is the
14+
* half a `config.password`-only scrub would have left open.
15+
* - "an untouched Save does not destroy the stored credential" — red on main
16+
* for a REASON WORTH RECORDING, and not the one it was written for. On main
17+
* the edit form is served `config.password` verbatim, posts it straight
18+
* back, and #8078's write gate refuses it — so editing any legacy row
19+
* through the wizard answers 400 for a value the server itself supplied.
20+
* Redacting the read is what makes that round-trip legal again; the restore
21+
* is what stops it deleting the credential instead. Measured, not inferred.
22+
* - "the SERVED config is redacted while the STORED record keeps its
23+
* credential" — pins both halves at once, because the store-side half alone
24+
* is green on main for the worst reason (nothing redacts there).
25+
*
26+
* GREEN on main (guards behaviour that must NOT change):
27+
* - "#8078's parse refusal still fires, with its guidance intact" — guards the
28+
* spec half. The refusal message names the secret binder and
29+
* `external.credentialsRef`; this card must not weaken it while adding a
30+
* read-path scrub next to it. Passes on main and must keep passing.
31+
* - "a URL-embedded credential is still ACCEPTED at the write door" — guards
32+
* the deliberately UNRULED boundary (#7990): #8078 pinned URL credentials as
33+
* a fact rather than refusing them. Redacting on read must not drift into
34+
* refusing on write. Passes on main and must keep passing.
35+
* - "the connect path still sees the stored credential" — guards the fact that
36+
* redaction is a read-path act only, so a legacy datasource keeps working.
37+
* Passes on main trivially and would go red if the scrub ever mutated the
38+
* stored record.
39+
*/
40+
41+
import { describe, it, expect } from 'vitest';
42+
import { validateDriverConfig, getDriverConfigSchema, BUILTIN_DRIVER_IDS } from '@objectstack/spec/data';
43+
import {
44+
redactDatasourceConfig,
45+
restoreRedactedConfig,
46+
redactUrlPassword,
47+
refusedCredentialKeys,
48+
redactableConfigKeys,
49+
} from '../datasource-config-redaction.js';
50+
import {
51+
DatasourceAdminService,
52+
type DatasourceAdminServiceConfig,
53+
type StoredDatasource,
54+
} from '../datasource-admin-service.js';
55+
56+
/** A legacy row: written before #8078, so it carries inline cleartext. */
57+
const LEGACY_PG: StoredDatasource = {
58+
name: 'legacy_pg',
59+
driver: 'postgres',
60+
origin: 'runtime',
61+
config: {
62+
host: 'db.internal',
63+
database: 'app',
64+
username: 'admin',
65+
password: 'hunter2',
66+
url: 'postgresql://admin:hunter2@db.internal:5432/app',
67+
},
68+
external: { credentialsRef: 'sys_secret:abc' },
69+
};
70+
71+
function makeService(seed: StoredDatasource[]) {
72+
const records: StoredDatasource[] = seed.map((r) => ({ ...r, config: { ...r.config } }));
73+
const cfg: DatasourceAdminServiceConfig = {
74+
probe: async () => ({ ok: true }),
75+
listDatasourceRecords: async () => records,
76+
getDatasourceRecord: async (n) => records.find((r) => r.name === n),
77+
putDatasourceRecord: async (rec) => {
78+
const i = records.findIndex((r) => r.name === rec.name);
79+
if (i >= 0) records[i] = rec;
80+
else records.push(rec);
81+
},
82+
deleteDatasourceRecord: async () => {},
83+
writeSecret: async () => 'sys_secret:new',
84+
countBoundObjects: async () => 0,
85+
};
86+
return { records, service: new DatasourceAdminService(cfg) };
87+
}
88+
89+
describe('the refusal set is DERIVED from the driver contracts, not retyped', () => {
90+
it('every `z.never()` config key is found by the derivation', () => {
91+
// The property that makes this module self-extending: #8078 spelled a
92+
// refused inline credential as `z.never()`, so the schema IS the list.
93+
expect(refusedCredentialKeys('postgres')).toEqual(['password']);
94+
expect(refusedCredentialKeys('mysql')).toEqual(['password']);
95+
expect(refusedCredentialKeys('mongodb')).toEqual(['password']);
96+
expect(refusedCredentialKeys('turso')).toEqual(['authToken']);
97+
// Credential-less drivers declare none, and must not acquire one by accident.
98+
expect(refusedCredentialKeys('sqlite')).toEqual([]);
99+
expect(refusedCredentialKeys('memory')).toEqual([]);
100+
});
101+
102+
it('the unknown-driver fallback covers every spelling the contracts refuse', () => {
103+
// Guards the one hand-written list in the module: if a driver refuses a NEW
104+
// credential key, the fallback used for contract-less drivers must learn it
105+
// too, or an unknown driver's config would serve that spelling in cleartext.
106+
const declared = new Set<string>();
107+
for (const id of BUILTIN_DRIVER_IDS as readonly string[]) {
108+
for (const key of refusedCredentialKeys(id)) declared.add(key);
109+
}
110+
const fallback = new Set(redactableConfigKeys('a-driver-with-no-contract'));
111+
for (const key of declared) expect(fallback.has(key)).toBe(true);
112+
});
113+
114+
it('a driver with no shipped contract still has its canonical credentials hidden', () => {
115+
expect(getDriverConfigSchema('not-a-real-driver' as never)).toBeUndefined();
116+
const { config, redactedKeys } = redactDatasourceConfig('not-a-real-driver', {
117+
host: 'h',
118+
password: 'hunter2',
119+
authToken: 'jwt',
120+
});
121+
expect(config).toEqual({ host: 'h' });
122+
expect(redactedKeys).toEqual(['authToken', 'password']);
123+
});
124+
});
125+
126+
describe('read path: getDatasource() no longer serves a stored credential', () => {
127+
it('RED ON MAIN — a stored row\'s cleartext password does not reach the caller', async () => {
128+
const { service } = makeService([LEGACY_PG]);
129+
const ds = await service.getDatasource('legacy_pg');
130+
expect(ds).toBeDefined();
131+
expect(ds!.config).not.toHaveProperty('password');
132+
expect(JSON.stringify(ds!.config)).not.toContain('hunter2');
133+
// The non-credential half is untouched — this is a redaction, not a purge.
134+
expect(ds!.config).toMatchObject({ host: 'db.internal', database: 'app', username: 'admin' });
135+
});
136+
137+
it('RED ON MAIN — the credential embedded in `config.url` does not reach the caller either', async () => {
138+
const { service } = makeService([LEGACY_PG]);
139+
const ds = await service.getDatasource('legacy_pg');
140+
// The scrub that stops at `config.password` is the one that only looks
141+
// finished: on main this same read served the password twice over.
142+
expect(ds!.config!.url).toBe('postgresql://admin@db.internal:5432/app');
143+
expect(ds!.redactedConfigKeys).toEqual(['password', 'url']);
144+
});
145+
146+
it('names what it withheld, so a caller is not left inferring it from an absence', async () => {
147+
const { service } = makeService([
148+
{ name: 'clean', driver: 'postgres', origin: 'runtime', config: { host: 'h', database: 'd' } },
149+
]);
150+
const ds = await service.getDatasource('clean');
151+
expect(ds!.redactedConfigKeys).toEqual([]);
152+
expect(ds!.config).toEqual({ host: 'h', database: 'd' });
153+
});
154+
155+
it('turso: the still-writable `encryptionKey` is redacted on read as well', async () => {
156+
const { service } = makeService([{
157+
name: 'turso_ds', driver: 'turso', origin: 'runtime',
158+
config: { url: 'libsql://db.turso.io', authToken: 'jwt-token', encryptionKey: 'aes-256-key' },
159+
}]);
160+
const ds = await service.getDatasource('turso_ds');
161+
expect(ds!.config).toEqual({ url: 'libsql://db.turso.io' });
162+
expect(ds!.redactedConfigKeys).toEqual(['authToken', 'encryptionKey']);
163+
});
164+
165+
it('pre-#8078 ALIAS spellings are redacted — a stored row never met the parse that renamed them', async () => {
166+
const { service } = makeService([{
167+
name: 'ancient', driver: 'postgres', origin: 'runtime',
168+
config: { host: 'h', database: 'd', passwd: 'hunter2', pwd: 'hunter2' },
169+
}]);
170+
const ds = await service.getDatasource('ancient');
171+
expect(ds!.config).toEqual({ host: 'h', database: 'd' });
172+
expect(ds!.redactedConfigKeys).toEqual(['passwd', 'pwd']);
173+
});
174+
175+
it('RED ON MAIN — the SERVED config is redacted while the STORED record keeps its credential', async () => {
176+
const { service, records } = makeService([LEGACY_PG]);
177+
const ds = await service.getDatasource('legacy_pg');
178+
179+
// Both halves in one pin, deliberately. Asserting only that the store is
180+
// untouched passes on `origin/main` for the worst possible reason — nothing
181+
// redacts there, so served and stored are the same object's contents and
182+
// the pin is green while the defect is live. Pairing it with the served
183+
// side makes the pin fail on main and pass here, which is the only version
184+
// of it that pins anything (#7801).
185+
expect(ds!.config).not.toHaveProperty('password');
186+
// The connect path reads the raw record (`getDatasourceRecord`), not this
187+
// projection. If redaction ever mutated in place, a legacy datasource would
188+
// stop authenticating the moment someone opened its edit form.
189+
expect(records[0].config).toMatchObject({
190+
password: 'hunter2',
191+
url: 'postgresql://admin:hunter2@db.internal:5432/app',
192+
});
193+
});
194+
});
195+
196+
describe('URL redaction is surgical', () => {
197+
it('removes only the password component of userinfo', () => {
198+
expect(redactUrlPassword('postgresql://admin:hunter2@db:5432/app'))
199+
.toBe('postgresql://admin@db:5432/app');
200+
expect(redactUrlPassword('mongodb://u:p@a.example.com:27017/db?replicaSet=rs0'))
201+
.toBe('mongodb://u@a.example.com:27017/db?replicaSet=rs0');
202+
});
203+
204+
it('leaves a URL with no embedded password exactly as it was', () => {
205+
for (const url of [
206+
'postgresql://admin@db:5432/app',
207+
'postgresql://db:5432/app',
208+
'libsql://my-db.turso.io',
209+
'file:./data/objectstack.db',
210+
':memory:',
211+
]) {
212+
expect(redactUrlPassword(url)).toBe(url);
213+
}
214+
});
215+
216+
it('a malformed password containing `@` is redacted WHOLE, not split at the first one', () => {
217+
// The userinfo boundary is the LAST `@` before the path. Matching the first
218+
// would leave `ss@host` behind — a redaction that publishes part of the
219+
// password while looking like it worked.
220+
expect(redactUrlPassword('postgres://u:p@ss@host/db')).toBe('postgres://u@host/db');
221+
expect(redactUrlPassword('postgres://u:p@ss@host/db')).not.toContain('ss');
222+
});
223+
224+
it('does not mistake a colon in a path or query for userinfo', () => {
225+
// `@` and `:` after the first `/` are not userinfo, and a value that is not
226+
// a URL at all must survive byte-for-byte.
227+
expect(redactUrlPassword('https://host/a:b@c')).toBe('https://host/a:b@c');
228+
expect(redactUrlPassword('https://host/p?to=a:b@c')).toBe('https://host/p?to=a:b@c');
229+
expect(redactUrlPassword('public')).toBe('public');
230+
expect(redactUrlPassword('a:b@c')).toBe('a:b@c');
231+
});
232+
});
233+
234+
describe('write path: the scrub must not turn "Save" into credential deletion', () => {
235+
it('RED WITHOUT THE RESTORE — an untouched round-trip keeps the stored credential', async () => {
236+
const { service, records } = makeService([LEGACY_PG]);
237+
// Exactly what the edit form does: GET, then PATCH the config it was given.
238+
const read = await service.getDatasource('legacy_pg');
239+
await service.updateDatasource('legacy_pg', { config: read!.config, label: 'Renamed' });
240+
241+
expect(records[0].label).toBe('Renamed');
242+
expect(records[0].config).toMatchObject({
243+
password: 'hunter2',
244+
url: 'postgresql://admin:hunter2@db.internal:5432/app',
245+
});
246+
});
247+
248+
it('an author who edits a NON-credential field still gets that edit', async () => {
249+
const { service, records } = makeService([LEGACY_PG]);
250+
const read = await service.getDatasource('legacy_pg');
251+
await service.updateDatasource('legacy_pg', {
252+
config: { ...read!.config, database: 'app_v2' },
253+
});
254+
expect(records[0].config).toMatchObject({ database: 'app_v2', password: 'hunter2' });
255+
});
256+
257+
it('an author who rewrites the URL by hand WINS — the restore never overrides an edit', async () => {
258+
const { service, records } = makeService([LEGACY_PG]);
259+
await service.updateDatasource('legacy_pg', {
260+
config: { host: 'db.internal', database: 'app', username: 'admin', url: 'postgresql://admin@elsewhere:5432/app' },
261+
});
262+
// Not the stored URL: the patch differs from the redaction of it, so it is
263+
// an edit, not a round-trip.
264+
expect(records[0].config!.url).toBe('postgresql://admin@elsewhere:5432/app');
265+
});
266+
267+
it('changing the DRIVER does not carry the old driver\'s credential across', async () => {
268+
const { service, records } = makeService([LEGACY_PG]);
269+
const read = await service.getDatasource('legacy_pg');
270+
await service.updateDatasource('legacy_pg', {
271+
driver: 'mysql',
272+
config: { host: 'db.internal', database: 'app', username: 'admin' },
273+
});
274+
expect(read!.config).not.toHaveProperty('password');
275+
expect(records[0].driver).toBe('mysql');
276+
expect(records[0].config).not.toHaveProperty('password');
277+
});
278+
279+
it('restoreRedactedConfig is a no-op when there is nothing stored to restore', () => {
280+
expect(restoreRedactedConfig('postgres', { host: 'h' }, undefined)).toEqual({ host: 'h' });
281+
expect(restoreRedactedConfig('postgres', { host: 'h' }, { host: 'h' })).toEqual({ host: 'h' });
282+
});
283+
});
284+
285+
describe('GREEN ON MAIN — #8078 is not weakened by anything above', () => {
286+
it('the parse refusal still fires, and its guidance still reaches the caller', async () => {
287+
const { service } = makeService([]);
288+
await expect(
289+
service.createDatasource({
290+
name: 'nope', driver: 'postgres',
291+
config: { host: 'h', database: 'd', password: 'hunter2' },
292+
} as never),
293+
).rejects.toThrow(/is a credential and is not accepted inline/);
294+
295+
// The guidance names BOTH mechanisms the refusal diverts to. A refusal that
296+
// said only "not allowed" would leave the author with no next move.
297+
const issues = validateDriverConfig('postgres', { host: 'h', password: 'x' });
298+
expect(issues).toMatchObject({ known: true });
299+
const message = (issues as { issues: Array<{ message: string }> }).issues[0].message;
300+
expect(message).toContain('external.credentialsRef');
301+
expect(message).toContain('secret binder');
302+
});
303+
304+
it('an author who types a refused key into a PATCH is still refused', async () => {
305+
const { service } = makeService([LEGACY_PG]);
306+
await expect(
307+
service.updateDatasource('legacy_pg', {
308+
config: { host: 'h', database: 'd', password: 'newpassword' },
309+
} as never),
310+
).rejects.toThrow(/is a credential and is not accepted inline/);
311+
});
312+
313+
it('GREEN ON MAIN — a URL-embedded credential is still ACCEPTED at the write door', () => {
314+
// #7990 left refusing these UNRULED and #8078 pinned the acceptance as a
315+
// FACT. Redacting on the way out must not become refusing on the way in;
316+
// this pin fails the moment that boundary moves without a ruling.
317+
expect(validateDriverConfig('postgres', { url: 'postgresql://u:pass@h:5432/d' }))
318+
.toEqual({ known: true, issues: [] });
319+
});
320+
});

0 commit comments

Comments
 (0)