From 863caed6d080021c79fe0ce5163258d6a92f5371 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Tue, 22 Sep 2026 11:06:19 +0000 Subject: [PATCH] =?UTF-8?q?fix(auth):=20read:secrets=20=E2=80=94=20withhol?= =?UTF-8?q?d=20app=20secret=20values=20from=20read-only=20principals=20(F0?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every unmatched GET names no capability, which is the documented operator model: if you hold a key to this host, you may read it (`authorizeRequest`). But the env-bearing reads carried each app's database password, API token and encryption secret in plaintext, so the read-only capability set an authenticated non-admin OIDC user receives was in practice full credential access to every installed app — and `HOLA_OIDC_ADMIN_GROUP` is fail-closed, so on a host with no admin group configured that is EVERY dashboard user. Reading an app's configuration and reading its credentials are now separate grants. `read:secrets` is held by `*`/admin and is deliberately absent from `READONLY_CAPABILITIES`; the secret VALUES are withheld from any principal lacking it. Enforced where the response is shaped, not as a route capability: making `/config` demand `read:secrets` would take the whole configuration view away from read-only users, who have a legitimate reason to see which variables an app is configured with. What they lose is only the values that were never theirs. Three surfaces go through `canReadSecrets(req)`: - GET /api/deployments/:id/config - GET /api/drafts/:id (same rows, including generated secrets) - GET /api/settings (host-wide systemEnv: SMTP_PASSWORD et al) `redactSecretEnvValues` blanks the value and sets `valueRedacted: true`. The marker is not decoration: a withheld secret and a genuinely empty one both carry `value: ''`, and the honest rendering of the two is not the same. `valueRedacted` is also a WRITE-side instruction, and that is what makes redacting an editable surface safe. The config read and the config write are the same rows, so a client saving the form it was given sends `value: ''` back for every secret it was not shown; without the rule the confidentiality fix would have introduced a secret-wiping write. The merge reads the flag as "no new value supplied for this key" and keeps the stored value — `hardenAppEnv`/`mergeAppEnv` for deployment/draft env, and the new `restoreWithheldEnvValues` for the full-replace `systemEnv` PATCH. An explicit `removeEnvKeys` entry still wins over it: withholding a value on read must not make a secret undeletable. The flag is stripped on every write path, so a forged one cannot make a stored row read as withheld. The check is `principalHasCapability`, decided from the principal alone and deliberately NOT routed through `AuthService.hasCapability`: `MockAuthService` returns true unconditionally and `RealAuthService` short-circuits to true whenever auth is disabled. Both are right for a route gate and both are wrong for shaping a response — they would make the rule a no-op in exactly the configurations where a test can observe it. When auth is disabled the middleware substitutes a wildcard system principal, so a single-operator host still sees everything, by holding `*` rather than by the check being skipped. No principal resolved fails closed. Web: a withheld value renders as `•••••••• hidden` with no reveal control (there is nothing behind that eye), is relaxed to optional for client-side validation so a `required` secret nobody may read cannot block every save, and drops its marker the moment the operator types a replacement. Tests +33 (server 1426 → 1459, web 379 → 383). The route-level half is driven through `route()` rather than `fetch`, because the auth middleware substitutes a wildcard principal whenever auth is disabled — which the test environment always is — so an HTTP-level request could never present a read-only principal, and a test that cannot present one cannot observe this rule at all. Revert-proof measured: with the two redaction calls removed, 6 of 12 route tests fail (the 6 that pass are the admin/non-secret cases, which must pass on both sides); with the three web changes reverted, all 4 new web tests fail. Residual, not in scope here: the tracker's own suggestion of restricting the complete configuration to administrators outright would also close this, and would additionally stop a stolen admin session from being an inventory of every app credential. That is a product decision about the dashboard's purpose, not a fix for this finding, and it is not made here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vck5KSX2CLxhohx14nb5Sh --- CLAUDE.md | 22 ++ docs/OPERATIONS.md | 26 ++ .../auth/secret-read-authorization.test.ts | 285 ++++++++++++++++++ .../deployments/merge-app-env.test.ts | 125 +++++++- packages/server/src/middleware/auth.ts | 24 ++ packages/server/src/server.ts | 52 +++- .../server/src/services/auth/auth-service.ts | 11 + .../server/src/services/auth/oidc-provider.ts | 20 +- packages/server/src/services/core/config.ts | 17 +- packages/server/src/services/core/draft.ts | 23 +- packages/shared/src/index.ts | 77 +++++ .../__tests__/pages/DeploymentDetail.test.tsx | 94 ++++++ packages/web/src/pages/DeploymentDetail.tsx | 42 ++- packages/web/src/pages/Settings.tsx | 11 +- 14 files changed, 808 insertions(+), 21 deletions(-) create mode 100644 packages/server/src/__tests__/auth/secret-read-authorization.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 0a43cef3..d9a7eb3d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -120,6 +120,28 @@ install as **Docker Compose** stacks, orchestrated by a server and routed by token. Authentik is the **default** — `hola init` always sets `HOLA_AUTH_MODE=authentik` (a compose profile); `none` remains an internal dev/test mode, not an install-time choice. +- **Secret-read authorization (F03).** Every unmatched GET names no capability — + the operator model is "if you hold a key to this host, you may read it" + (`authorizeRequest`) — but the env-bearing reads used to carry each app's + database password in plaintext, so the read-only set an authenticated + non-admin OIDC user gets was in practice full credential access. Reading + configuration and reading credentials are now separate grants: `read:secrets` + (held by `*`/admin, absent from `READONLY_CAPABILITIES`) gates the secret + VALUES, enforced where the response is shaped rather than as a route + capability, so the configuration view stays readable while the credentials do + not. Three surfaces go through it — `GET /api/deployments/:id/config`, + `GET /api/drafts/:id`, `GET /api/settings`'s `systemEnv` — via + `canReadSecrets(req)` + `redactSecretEnvValues`, which blanks the value and + sets `valueRedacted: true` (a genuinely empty secret is otherwise + indistinguishable). The check is `principalHasCapability`, decided from the + principal alone and deliberately NOT `AuthService.hasCapability`: both + implementations blanket-allow when auth is off, which would make the rule + unobservable in exactly the configurations a test can construct. `valueRedacted` + is also a WRITE-side instruction — the merge reads it as "no new value + supplied" and keeps the stored secret (`hardenAppEnv`/`mergeAppEnv`; + `restoreWithheldEnvValues` for the full-replace `systemEnv` PATCH) — because + the read and the write are the same rows, so redacting an editable surface + would otherwise turn the next save into a secret-wiping write. - **Release channels (ADR 0005).** A catalog `versions[]` entry may carry a `channel` (default `stable`) — a catalog-index attribute, not a manifest one. A version is eligible on channel `c` iff its own channel is `c` or `stable` diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 8c038ead..a4746969 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -72,6 +72,32 @@ login flow: When `HOLA_USE_AUTH=false` (dev/test) the dashboard loads with no login. +### What a non-admin dashboard user can see + +A user who authenticates through SSO but is **not** in `HOLA_OIDC_ADMIN_GROUP` +gets a read-only capability set (`read:system`, `read:deployments`, `read:logs`, +`read:backups`, `read:catalog`). They can browse every installed app, its +configuration and its logs, but they cannot install, change or remove anything. + +**Secret values are withheld from them.** Reading an app's configuration and +reading its credentials are separate grants: the second is `read:secrets`, which +only an admin (`*`) holds. So a read-only user sees *which* variables an app is +configured with — key, label, type, and that the variable is a secret — while +each secret's value comes back empty and flagged as hidden. The dashboard +renders those as `•••••••• hidden` with no reveal control, and `hola config` +shows `***`. The same policy covers draft reads and the host-wide `systemEnv` in +Settings. + +An admin's own reads are unchanged, and nothing about editing changes: a form +saved with a hidden value still in place keeps the stored secret rather than +blanking it, so an operator never has to re-enter a password to change something +next to it. + +Note that `HOLA_OIDC_ADMIN_GROUP` is fail-closed — with no admin group +configured, **every** authenticated user is read-only. Set it to `*` to make +every authenticated user an admin, which makes your IdP's application-access +policy the only gate. + ## Deploy an app The web dashboard browses a remote **catalog** of installable apps, set via diff --git a/packages/server/src/__tests__/auth/secret-read-authorization.test.ts b/packages/server/src/__tests__/auth/secret-read-authorization.test.ts new file mode 100644 index 00000000..6fc0f1a6 --- /dev/null +++ b/packages/server/src/__tests__/auth/secret-read-authorization.test.ts @@ -0,0 +1,285 @@ +/** + * A principal without `read:secrets` is never handed an app's secret values + * (F03). + * + * The exposure this covers: `getRequiredCapability` returns `null` for every + * unmatched GET, which is the documented operator model ("if you hold a key to + * this host, you may read it") — but the deployment-config read and the draft + * read carried each app's database password, API token and encryption secret in + * their `appEnv` rows. An authenticated non-admin OIDC user holds only the + * read-only set (`oidc-provider.ts`), so "read-only dashboard access" was in + * practice full credential access to every installed app. + * + * Driven through `route()` rather than `fetch`, for the same reason as + * `contract-broker-routes.test.ts`: the auth middleware substitutes a WILDCARD + * system principal whenever auth is disabled, which the test environment always + * is, so an HTTP-level request could never present a read-only principal — and + * a test that cannot present one cannot observe this rule at all. + * + * The services are doubled so the assertions are about the RESPONSE POLICY and + * nothing else: both routes are handed the same row set, and the only variable + * is who is asking. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; + +import type { AppEnvVar } from '@hola/shared'; +import { CAPABILITIES } from '@hola/shared'; +import { setupTestServer, teardownTestServer, TEST_BASE_URL } from '../utils/server'; +import { principalHasCapability } from '../../middleware/auth'; +import { READONLY_CAPABILITIES } from '../../services/auth/oidc-provider'; +import { route } from '../../server'; +import { getServices } from '../../services/simple-factory'; +import type { Principal } from '../../services/auth/auth-service'; + +/** The value that must never reach a principal lacking `read:secrets`. */ +const SECRET = 'db-password-must-not-leak'; + +const ROWS: AppEnvVar[] = [ + { key: 'MAX_CONNECTIONS', value: '10', isSecret: false, type: 'integer', min: 1, max: 100 }, + { key: 'DB_PASSWORD', value: SECRET, isSecret: true, required: true }, + // An intentionally EMPTY secret: it must stay distinguishable from a withheld + // one, which is the whole reason the marker exists rather than just `''`. + { key: 'OPTIONAL_TOKEN', value: '', isSecret: true, required: false }, +]; + +/** + * The capability set `oidc-provider.ts` mints for an authenticated non-admin — + * imported rather than restated, so the assertions below are about the set this + * host actually issues and cannot drift away from it. + */ +const READONLY = READONLY_CAPABILITIES; + +function principalRequest(path: string, capabilities: string[] | null): Request { + const req = new Request(`${TEST_BASE_URL}${path}`, { method: 'GET' }); + if (capabilities !== null) { + const principal: Principal = { + id: 'user-1', + type: 'user', + name: 'Dashboard user', + roles: capabilities.includes('*') ? ['admin'] : ['user'], + capabilities, + }; + (req as Request & { authContext?: { isAuthenticated: boolean; principal: Principal } }).authContext = { + isAuthenticated: true, + principal, + }; + } + return req; +} + +async function readEnv(path: string, capabilities: string[] | null): Promise { + const req = principalRequest(path, capabilities); + const res = await route(new URL(req.url), req); + expect(res.status).toBe(200); + return ((await res.json()) as { appEnv: AppEnvVar[] }).appEnv; +} + +/** Both env-bearing reads, so neither can be fixed while the other leaks. */ +const SURFACES: Array<{ name: string; path: string }> = [ + { name: 'GET /api/deployments/:id/config', path: '/api/deployments/dep-1/config' }, + { name: 'GET /api/drafts/:id', path: '/api/drafts/draft-1' }, +]; + +describe('secret-read authorization (F03)', () => { + const originals = new Map(); + + beforeAll(async () => { + await setupTestServer(); + const deployments = getServices().deployments as unknown as Record; + const drafts = getServices().drafts as unknown as Record; + originals.set('getConfig', deployments.getConfig); + originals.set('getDraft', drafts.getDraft); + // Same rows from both surfaces: the response policy is the only variable. + deployments.getConfig = async () => ({ appEnv: ROWS.map((r) => ({ ...r })), systemOverrides: {} }); + drafts.getDraft = async () => ({ + draftId: 'draft-1', + appId: 'app', + version: '1.0.0', + source: 'hola', + systemOverrides: {}, + appEnv: ROWS.map((r) => ({ ...r })), + ports: [], + composeOverride: '', + files: [], + }); + }); + + afterAll(async () => { + (getServices().deployments as unknown as Record).getConfig = originals.get('getConfig'); + (getServices().drafts as unknown as Record).getDraft = originals.get('getDraft'); + await teardownTestServer(); + }); + + for (const surface of SURFACES) { + test(`${surface.name} withholds secret values from a read-only principal`, async () => { + const env = await readEnv(surface.path, READONLY); + + const secret = env.find((e) => e.key === 'DB_PASSWORD')!; + expect(secret.value).toBe(''); + expect(secret.valueRedacted).toBe(true); + // The whole serialized response, not just the row we looked at — a leak + // through any other field (a duplicate, a nested copy) fails here too. + expect(JSON.stringify(env)).not.toContain(SECRET); + }); + + test(`${surface.name} keeps non-secret values readable for a read-only principal`, async () => { + // Redaction that took the configuration view away from read-only users + // would be a different, worse answer: they have a legitimate reason to see + // which variables an app is configured with. + const env = await readEnv(surface.path, READONLY); + const row = env.find((e) => e.key === 'MAX_CONNECTIONS')!; + expect(row.value).toBe('10'); + expect(row.valueRedacted).toBeUndefined(); + // The typed spec survives redaction, so the UI still renders the row. + expect(row.type).toBe('integer'); + expect(row.max).toBe(100); + expect(env.map((e) => e.key)).toEqual(['MAX_CONNECTIONS', 'DB_PASSWORD', 'OPTIONAL_TOKEN']); + }); + + test(`${surface.name} marks a withheld secret distinguishably from an empty one`, async () => { + const env = await readEnv(surface.path, READONLY); + // Both rows now carry `value: ''`. Without the marker a client cannot tell + // "this app has no token" from "you may not see this app's token", and the + // honest rendering of the two is not the same. + expect(env.find((e) => e.key === 'OPTIONAL_TOKEN')?.value).toBe(''); + expect(env.find((e) => e.key === 'OPTIONAL_TOKEN')?.valueRedacted).toBe(true); + expect(env.find((e) => e.key === 'MAX_CONNECTIONS')?.valueRedacted).toBeUndefined(); + }); + + test(`${surface.name} returns real secret values to a principal holding read:secrets`, async () => { + const env = await readEnv(surface.path, [...READONLY, 'read:secrets']); + expect(env.find((e) => e.key === 'DB_PASSWORD')?.value).toBe(SECRET); + expect(env.find((e) => e.key === 'DB_PASSWORD')?.valueRedacted).toBeUndefined(); + }); + + test(`${surface.name} returns real secret values to an admin wildcard`, async () => { + // An operator's own reads are unchanged by this fix — `*` matches + // `read:secrets`, so the Configuration tab and `hola config` keep working + // exactly as before for the principal that was always entitled to them. + const env = await readEnv(surface.path, ['*']); + expect(env.find((e) => e.key === 'DB_PASSWORD')?.value).toBe(SECRET); + }); + + test(`${surface.name} fails closed when no principal was resolved`, async () => { + // A handler reached outside the auth middleware has no principal. That is + // the case where guessing wrong publishes credentials, so it withholds. + const env = await readEnv(surface.path, null); + expect(env.find((e) => e.key === 'DB_PASSWORD')?.value).toBe(''); + expect(JSON.stringify(env)).not.toContain(SECRET); + }); + } +}); + + +/** + * Host-wide `systemEnv` is the third env-bearing read, and the same policy + * applies: `GET /api/settings` is an unmatched GET, and those rows hold the + * operator's SMTP password and whatever else they put there. Its PATCH is a + * FULL REPLACE, so the write-side rule (`restoreWithheldEnvValues`) is what + * keeps a replayed settings form from blanking every host secret at once. + */ +describe('GET /api/settings (F03)', () => { + const original = new Map(); + const SMTP = 'smtp-password-must-not-leak'; + + beforeAll(async () => { + await setupTestServer(); + const config = getServices().config as unknown as Record; + original.set('getSystemSettings', config.getSystemSettings); + config.getSystemSettings = async () => ({ + systemEnv: [ + { key: 'DOMAIN', value: 'hola.example.com', isSecret: false }, + { key: 'SMTP_PASSWORD', value: SMTP, isSecret: true }, + ], + docker: { host: '/var/run/docker.sock' }, + tls: { email: '' }, + notifications: { smtpHost: '', smtpUser: '', smtpPassword: SMTP }, + channels: { showPrerelease: false }, + }); + }); + + afterAll(async () => { + (getServices().config as unknown as Record).getSystemSettings = original.get('getSystemSettings'); + await teardownTestServer(); + }); + + async function readSettings(capabilities: string[] | null): Promise { + const req = principalRequest('/api/settings', capabilities); + const res = await route(new URL(req.url), req); + expect(res.status).toBe(200); + const body = (await res.json()) as { systemEnv: AppEnvVar[] }; + // The pre-existing `smtpPassword` redaction must still hold either way. + expect(JSON.stringify(body)).not.toContain('"smtpPassword"'); + return body.systemEnv; + } + + test('withholds a host secret from a read-only principal', async () => { + const env = await readSettings(READONLY); + expect(env.find((e) => e.key === 'SMTP_PASSWORD')?.value).toBe(''); + expect(env.find((e) => e.key === 'SMTP_PASSWORD')?.valueRedacted).toBe(true); + expect(env.find((e) => e.key === 'DOMAIN')?.value).toBe('hola.example.com'); + expect(JSON.stringify(env)).not.toContain(SMTP); + }); + + test('returns it to an operator holding read:secrets', async () => { + expect((await readSettings(['*'])).find((e) => e.key === 'SMTP_PASSWORD')?.value).toBe(SMTP); + }); +}); + +/** + * The boundary itself, asserted directly rather than by inference from a + * response: `read:secrets` must not be in the read-only set. Adding it there + * would restore the exposure in full while every redaction test above still + * passed, since each one would then be exercising a principal that legitimately + * holds the capability. + */ +describe('the read-only capability set (F03)', () => { + test('does not include read:secrets', () => { + expect(READONLY_CAPABILITIES).not.toContain(CAPABILITIES.READ_SECRETS); + }); + + test('still includes the reads a read-only dashboard user needs', () => { + // The fix withholds credential VALUES; it does not narrow what a read-only + // user may see. A regression in the other direction is also a bug. + expect(READONLY_CAPABILITIES).toEqual([ + 'read:system', + 'read:deployments', + 'read:logs', + 'read:backups', + 'read:catalog', + ]); + }); +}); + +/** + * `principalHasCapability` is the response-policy check, deliberately decided + * from the principal alone — both `hasCapability` implementations blanket-allow + * when auth is disabled, which would make every rule above unobservable. + */ +describe('principalHasCapability', () => { + const withCaps = (capabilities: string[]): Principal => ({ + id: 'p', type: 'user', name: 'p', roles: [], capabilities, + }); + + test('an exact capability holds', () => { + expect(principalHasCapability(withCaps(['read:secrets']), CAPABILITIES.READ_SECRETS)).toBe(true); + }); + + test('a wildcard holds every capability — an operator key is unaffected', () => { + expect(principalHasCapability(withCaps(['*']), CAPABILITIES.READ_SECRETS)).toBe(true); + }); + + test('a principal without it does not hold it, however many other reads it has', () => { + expect(principalHasCapability(withCaps([...READONLY_CAPABILITIES]), CAPABILITIES.READ_SECRETS)).toBe(false); + }); + + test('a contract-scoped token does not hold it', () => { + // Closed by default at the route (`authorizeRequest`), and closed here too — + // a provider app's credential is not an operator's. + expect(principalHasCapability(withCaps(['contract:backup']), CAPABILITIES.READ_SECRETS)).toBe(false); + }); + + test('no capabilities at all holds nothing', () => { + expect(principalHasCapability(withCaps([]), CAPABILITIES.READ_SECRETS)).toBe(false); + }); +}); diff --git a/packages/server/src/__tests__/deployments/merge-app-env.test.ts b/packages/server/src/__tests__/deployments/merge-app-env.test.ts index 313db74b..35c14d87 100644 --- a/packages/server/src/__tests__/deployments/merge-app-env.test.ts +++ b/packages/server/src/__tests__/deployments/merge-app-env.test.ts @@ -8,7 +8,8 @@ import { describe, test, expect } from 'bun:test'; import type { AppEnvVar } from '@hola/shared'; -import { mergeAppEnv } from '../../services/core/draft'; +import { mergeAppEnv, hardenAppEnv } from '../../services/core/draft'; +import { redactSecretEnvValues, restoreWithheldEnvValues } from '@hola/shared'; const stored: AppEnvVar[] = [ { key: 'MAX_CONNECTIONS', value: '10', isSecret: false, type: 'integer', min: 1, max: 100 }, @@ -60,3 +61,125 @@ describe('mergeAppEnv', () => { expect(mergeAppEnv(stored, [])).toEqual(stored); }); }); + +/** + * A withheld secret survives a round trip (F03). + * + * This is the half of the redaction that makes it safe to apply to an EDITABLE + * surface. The config read and the config write are the same rows: once the read + * withholds a secret's value, a client that saves the form it was given is + * sending `value: ''` back for that key. Without the rule below, redacting the + * read would silently blank every app secret on the operator's next save — a + * data-loss bug introduced by a confidentiality fix. + * + * `valueRedacted` is therefore read as "no new value supplied for this key", and + * stripped either way so it never reaches a persisted manifest. + */ +describe('redacted rows on the write path (F03)', () => { + /** Exactly what a client receives from a read it was not trusted with. */ + const asRead = redactSecretEnvValues(stored); + + test('mergeAppEnv: echoing the redacted row back keeps the stored secret', () => { + const merged = mergeAppEnv(stored, asRead); + expect(merged.find((e) => e.key === 'API_TOKEN')?.value).toBe('seed'); + // …and the marker is not persisted, or the row would read as withheld + // forever, including for the operator who can see it. + expect(merged.find((e) => e.key === 'API_TOKEN')?.valueRedacted).toBeUndefined(); + // Non-secret rows in the same request are applied normally. + expect(merged.find((e) => e.key === 'ADMIN_USER')?.value).toBe('admin'); + }); + + test('mergeAppEnv: a replacement value supplied for a secret still applies', () => { + // The flag means "I am not supplying a value". A client that IS supplying + // one drops it — otherwise a read-only-shaped response could make a secret + // permanently unchangeable. + const merged = mergeAppEnv(stored, [{ key: 'API_TOKEN', value: 'rotated', isSecret: true }]); + expect(merged.find((e) => e.key === 'API_TOKEN')?.value).toBe('rotated'); + }); + + test('mergeAppEnv: an explicit removal still wins over a redacted upsert', () => { + // Withholding a value on read must not make the secret undeletable. + const merged = mergeAppEnv(stored, asRead, ['API_TOKEN']); + expect(merged.some((e) => e.key === 'API_TOKEN')).toBe(false); + }); + + test('mergeAppEnv: a forged marker on a brand-new key is stripped, not honoured', () => { + const merged = mergeAppEnv(stored, [ + { key: 'NEW_SECRET', value: 'fresh', isSecret: true, valueRedacted: true }, + ]); + const row = merged.find((e) => e.key === 'NEW_SECRET')!; + expect(row.value).toBe('fresh'); + expect(row.valueRedacted).toBeUndefined(); + }); + + test('hardenAppEnv: full replace keeps a withheld secret rather than blanking it', () => { + // The draft wizard PATCHes its whole env set, so this is the path where a + // redacted read would do the most damage: every secret at once. + const hardened = hardenAppEnv(stored, asRead); + expect(hardened.find((e) => e.key === 'API_TOKEN')?.value).toBe('seed'); + expect(hardened.find((e) => e.key === 'API_TOKEN')?.valueRedacted).toBeUndefined(); + expect(hardened.map((e) => e.key)).toEqual(['MAX_CONNECTIONS', 'ADMIN_USER', 'API_TOKEN']); + }); + + test('hardenAppEnv: clearing a secret stays possible, and is explicit', () => { + // `value: ''` with NO marker is a deliberate clear, and must not be + // confused with a value that was never shown. + const hardened = hardenAppEnv(stored, [ + { key: 'API_TOKEN', value: '', isSecret: true }, + ]); + expect(hardened.find((e) => e.key === 'API_TOKEN')?.value).toBe(''); + }); +}); + +describe('redactSecretEnvValues', () => { + test('withholds every secret value and leaves the rest of the row intact', () => { + const redacted = redactSecretEnvValues(stored); + const secret = redacted.find((e) => e.key === 'API_TOKEN')!; + expect(secret).toEqual({ key: 'API_TOKEN', value: '', isSecret: true, valueRedacted: true }); + // The typed spec of a non-secret row is untouched — same object contents. + expect(redacted.find((e) => e.key === 'MAX_CONNECTIONS')).toEqual(stored[0]!); + }); + + test('does not mutate the rows it was given', () => { + // The same arrays feed the upgrade carry-forward and the restore-on-install + // env merge, which run as the platform and need the real values. + const input = stored.map((e) => ({ ...e })); + redactSecretEnvValues(input); + expect(input.find((e) => e.key === 'API_TOKEN')?.value).toBe('seed'); + }); +}); + +/** + * `restoreWithheldEnvValues` — the write-side rule for a row set whose PATCH + * replaces everything, which is how host `systemEnv` works. + */ +describe('restoreWithheldEnvValues (F03)', () => { + const host = [ + { key: 'DOMAIN', value: 'hola.example.com', isSecret: false }, + { key: 'SMTP_PASSWORD', value: 'stored-smtp', isSecret: true }, + ]; + + test('a replayed form keeps every withheld value instead of blanking it', () => { + const saved = restoreWithheldEnvValues(host, redactSecretEnvValues(host)); + expect(saved).toEqual(host); + }); + + test('a typed replacement wins — the marker is what defers, not the emptiness', () => { + const edited = redactSecretEnvValues(host).map((e) => + e.key === 'SMTP_PASSWORD' ? { key: e.key, value: 'rotated', isSecret: true } : e, + ); + expect(restoreWithheldEnvValues(host, edited).find((e) => e.key === 'SMTP_PASSWORD')?.value).toBe('rotated'); + }); + + test('a marked row whose key is not stored resolves to empty, not undefined', () => { + // Nothing to restore from. Writing `undefined` into a `value: string` would + // put a malformed row into the persisted settings file. + const [row] = restoreWithheldEnvValues(host, [{ key: 'GONE', value: '', isSecret: true, valueRedacted: true }]); + expect(row).toEqual({ key: 'GONE', value: '', isSecret: true }); + }); + + test('a deleted row stays deleted — restoring values is not restoring rows', () => { + const saved = restoreWithheldEnvValues(host, redactSecretEnvValues(host).filter((e) => e.key === 'DOMAIN')); + expect(saved.map((e) => e.key)).toEqual(['DOMAIN']); + }); +}); diff --git a/packages/server/src/middleware/auth.ts b/packages/server/src/middleware/auth.ts index c8fbecba..4f94c6e5 100644 --- a/packages/server/src/middleware/auth.ts +++ b/packages/server/src/middleware/auth.ts @@ -241,6 +241,30 @@ export function authorizeRequest( return 'allow'; } +/** + * Whether a principal holds one capability, decided from the principal alone. + * + * Identical in effect to `AuthService.hasCapability` for every real provider — + * each one is `capabilities.includes('*') || capabilities.includes(cap)` — but + * deliberately NOT routed through the service, for two reasons: + * + * - `MockAuthService.hasCapability` returns `true` unconditionally, and + * `RealAuthService` short-circuits to `true` whenever auth is disabled. Both + * are right for a ROUTE gate (no auth configured ⇒ no authorization to + * perform), and both are wrong for shaping a response around a capability: + * they would make the shaping a no-op in exactly the configurations where a + * test could observe it, leaving the real rule unverifiable. + * - The principal is the whole input. When auth is disabled the middleware + * substitutes a wildcard system principal, so a single-operator host still + * sees everything — by holding `*`, not by the check being skipped. + * + * Use it for response policy (see `canReadSecrets` in server.ts). Route + * authorization stays with `authorizeRequest` + the service. + */ +export function principalHasCapability(principal: Principal, capability: Capability): boolean { + return principal.capabilities.includes('*') || principal.capabilities.includes(capability); +} + /** * Create authentication middleware */ diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index d93bba90..ad169dc3 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -62,6 +62,8 @@ import { type AppEnvVar, type PublishRestoreIndexRequest, type CompleteRestoreRequestRequest, + CAPABILITIES, + redactSecretEnvValues, } from '@hola/shared'; import { BACKUP_CONTRACT_REF } from '@hola/shared/contracts'; import { resolveListedCandidate, groupIntoLineages, suppressInferredDefault } from './services/core/restore-candidates'; @@ -87,7 +89,7 @@ import { createSSEStream, createSSEHeaders } from './utils/sse'; import { mapErrorToResponse, asPromoteValidationError, ValidationError, ForbiddenError } from './middleware/error-mapping'; // Phase 3: Authentication imports -import { createAuthMiddleware, getPrincipal, SESSION_COOKIE } from './middleware/auth'; +import { createAuthMiddleware, getPrincipal, principalHasCapability, SESSION_COOKIE } from './middleware/auth'; import { resolveOidcConfig, setProvisionedOidc } from './config/oidc'; import { ldapOutpostTokenPath, persistLdapOutpostToken } from './services/auth/ldap-outpost-config'; import { authConfig } from './config/auth'; @@ -230,6 +232,37 @@ function redactNotifications(n: return rest as T; } +/** + * Whether this caller may be handed an app's secret env VALUES (F03). + * + * The sibling of `redactNotifications` above, and the same kind of rule: a + * response is shaped for the principal that asked for it. Reading an app's + * configuration and reading its credentials are separate grants — + * `read:deployments` answers the first, `read:secrets` the second — and an + * authenticated non-admin dashboard user holds only the first + * (`oidc-provider.ts`). Before this, a routine Configuration-tab read handed + * that user every app's database password. + * + * Fails closed: no resolved principal (a handler reached outside the auth + * middleware) withholds secrets rather than publishing them. When auth is + * DISABLED the middleware substitutes a wildcard system principal, so + * single-operator dev/`mode=none` hosts are unaffected — but by that principal + * holding `*`, not by the check being skipped (see `principalHasCapability`, + * which is why this does not go through the auth SERVICE: both implementations + * of `hasCapability` blanket-allow when auth is off). + * + * Deliberately NOT a route capability in `getRequiredCapability`: making + * `/config` demand `read:secrets` would take the whole configuration view away + * from read-only users, who have a legitimate reason to see which variables an + * app is configured with. What they lose is only the values that were never + * theirs. + */ +function canReadSecrets(req: Request): boolean { + const principal = getPrincipal(req); + if (!principal) return false; + return principalHasCapability(principal, CAPABILITIES.READ_SECRETS); +} + /** * The deployment a brokered-contract call is being made BY (#501): the id the * presented `hct_*` token was minted for, carried on the authenticated @@ -830,6 +863,10 @@ export async function route(url: URL, req: Request): Promise { const services = getServices(); const payload = await services.drafts.getDraft(draftId); + // F03: the same response policy as the deployment config read — a draft's + // env rows hold the same secrets (including ones the wizard generated), + // and this is an unmatched GET, so any authenticated principal reaches it. + if (!canReadSecrets(req)) payload.appEnv = redactSecretEnvValues(payload.appEnv ?? []); logger.info('Draft retrieved successfully', { requestId: context?.requestId, @@ -1452,6 +1489,10 @@ export async function route(url: URL, req: Request): Promise { try { const services = getServices(); const payload: GetDeploymentConfigResponse = await services.deployments.getConfig(deploymentId); + // F03: secret values are withheld unless the caller holds `read:secrets`. + // The rows themselves (key, label, type, `isSecret`) are not secret and + // stay, so the Configuration tab still renders for a read-only user. + if (!canReadSecrets(req)) payload.appEnv = redactSecretEnvValues(payload.appEnv); return json(payload); } catch (err) { return errorResponse(req, err); @@ -1871,8 +1912,13 @@ export async function route(url: URL, req: Request): Promise { const services = getServices(); const systemSettings = await services.config.getSystemSettings(); + // F03: host-wide `systemEnv` carries secrets too (SMTP_PASSWORD and + // whatever else the operator put there), and this is an unmatched GET. + // Same policy as the deployment-config and draft reads; the PATCH echo + // below gets it as well, and `updateSystemSettings` restores a withheld + // value from the stored row so a replayed form cannot blank it. const payload: GetSettingsResponse = { - systemEnv: systemSettings.systemEnv, + systemEnv: canReadSecrets(req) ? systemSettings.systemEnv : redactSecretEnvValues(systemSettings.systemEnv ?? []), docker: systemSettings.docker, tls: systemSettings.tls, notifications: redactNotifications(systemSettings.notifications), @@ -1902,7 +1948,7 @@ export async function route(url: URL, req: Request): Promise { const updatedSettings = await services.config.updateSystemSettings(body); const payload: PatchSettingsResponse = { - systemEnv: updatedSettings.systemEnv, + systemEnv: canReadSecrets(req) ? updatedSettings.systemEnv : redactSecretEnvValues(updatedSettings.systemEnv ?? []), docker: updatedSettings.docker, tls: updatedSettings.tls, notifications: redactNotifications(updatedSettings.notifications), diff --git a/packages/server/src/services/auth/auth-service.ts b/packages/server/src/services/auth/auth-service.ts index 44d876ac..34608c4e 100644 --- a/packages/server/src/services/auth/auth-service.ts +++ b/packages/server/src/services/auth/auth-service.ts @@ -346,6 +346,17 @@ export const CAPABILITIES = { READ_LOGS: 'read:logs', READ_BACKUPS: 'read:backups', READ_CATALOG: 'read:catalog', + // Reading an app's *secret* env values, separately from reading its + // configuration (F03). `read:deployments` answers "what is installed and how + // is it configured"; this answers "what are its credentials", and the two are + // not the same question. Held by `*`/admin and by nothing else — notably NOT + // by the read-only set `oidc-provider.ts` mints for an authenticated + // non-admin. Checked where a response carrying env rows is shaped + // (`server.ts`), not in the route capability table: every read route stays + // reachable by any operator who holds a key to this host, which is the + // documented model in `authorizeRequest` — what changes is that the secret + // VALUES are withheld from a principal that was never granted them. + READ_SECRETS: 'read:secrets', // Write operations WRITE_DEPLOYMENTS: 'write:deployments', diff --git a/packages/server/src/services/auth/oidc-provider.ts b/packages/server/src/services/auth/oidc-provider.ts index 2e0c5f7b..fb68e449 100644 --- a/packages/server/src/services/auth/oidc-provider.ts +++ b/packages/server/src/services/auth/oidc-provider.ts @@ -21,8 +21,24 @@ import { getLogger } from '../../lib/logger'; import { resolveOidcConfig, type OidcConfig } from '../../config/oidc'; import type { AuthProvider, AuthResult, Principal } from './auth-service'; -/** Read-only capability set for authenticated-but-non-admin OIDC users. */ -const READONLY_CAPABILITIES = [ +/** + * Read-only capability set for authenticated-but-non-admin OIDC users. + * + * `read:secrets` is deliberately NOT in this list (F03). These users can read + * every deployment's configuration, which is the point of a read-only + * dashboard — but a configuration read used to carry each app's database + * password, API token and encryption secret in plaintext, so "read-only" was + * in practice full credential access to every installed app. The values are + * now withheld at the response boundary from any principal lacking this + * capability; `admin`'s `*` matches it, so an operator's own reads are + * unchanged. + * + * Adding a capability here widens what every non-admin dashboard user may see + * on a host whose IdP has no admin group configured (the fail-closed default), + * so treat this list as a security boundary rather than a convenience. Exported + * so a test can assert that boundary directly rather than by inference. + */ +export const READONLY_CAPABILITIES = [ 'read:system', 'read:deployments', 'read:logs', diff --git a/packages/server/src/services/core/config.ts b/packages/server/src/services/core/config.ts index a5fd44e1..485e46d1 100644 --- a/packages/server/src/services/core/config.ts +++ b/packages/server/src/services/core/config.ts @@ -5,7 +5,7 @@ * Built on top of StorageService for file operations. */ -import { type AppEnvVar } from '@hola/shared'; +import { type AppEnvVar, restoreWithheldEnvValues } from '@hola/shared'; import { getLogger } from '../../lib/logger'; import { ValidationError } from '../../middleware/error-mapping'; import type { HealthCheckable, ServiceHealth } from './types'; @@ -179,8 +179,14 @@ export class RealConfigService implements ConfigService { ...(updates.channels && { channels: { ...current.channels, ...updates.channels } }), - // System env requires special handling to maintain array structure - ...(updates.systemEnv && { systemEnv: updates.systemEnv }), + // System env requires special handling to maintain array structure. + // `restoreWithheldEnvValues` is what makes redacting this surface on + // read safe (F03): this is a FULL REPLACE, so a client saving the form + // it was shown sends `value: ''` back for every secret it was not + // allowed to see, and without this the save would blank them all. + ...(updates.systemEnv && { + systemEnv: restoreWithheldEnvValues(current.systemEnv ?? [], updates.systemEnv), + }), }; // Validate settings before saving @@ -378,7 +384,10 @@ export class MockConfigService implements ConfigService { ...(updates.channels && { channels: { ...this.systemSettings.channels, ...updates.channels } }), - ...(updates.systemEnv && { systemEnv: updates.systemEnv }), + // Same rule as the real service — see the comment there (F03). + ...(updates.systemEnv && { + systemEnv: restoreWithheldEnvValues(this.systemSettings.systemEnv ?? [], updates.systemEnv), + }), }; this.logger.debug('Mock system settings updated'); diff --git a/packages/server/src/services/core/draft.ts b/packages/server/src/services/core/draft.ts index 0d85470d..c96dd5b9 100644 --- a/packages/server/src/services/core/draft.ts +++ b/packages/server/src/services/core/draft.ts @@ -32,7 +32,7 @@ import type { import { createHash } from 'crypto'; -import { STABLE_CHANNEL } from '@hola/shared'; +import { STABLE_CHANNEL, withoutRedactionMarker } from '@hola/shared'; import { getLogger } from '../../lib/logger'; import { NotFoundError, ConflictError, ValidationError, DraftValidationError, BundleUnavailableError, assertValidChannelName } from '../../middleware/error-mapping'; import { validateComposeDocument, APP_HOST_TOKEN, BASE_DOMAIN_TOKEN } from '@hola/shared/compose-validate'; @@ -240,6 +240,13 @@ function assertComposeParses(content: string, source: string): void { * (a custom/user-added var, e.g. the wizard's "Add variable" or an unknown CLI * `--set` key) passes through unmodified — it has no spec to protect. * + * `valueRedacted` (F03) is the one field a client may send that changes the + * OUTCOME rather than being re-imposed: it means "this row came back from a + * read with its secret withheld, I am not supplying a value", so the stored row + * is kept whole. Without that rule, redacting an editable surface would make + * every save silently blank the app's secrets. The flag is stripped either way, + * so it never reaches a persisted record. + * * Exported: `RealDeploymentService.updateDeployment` (deployment.ts) reuses the * exact same re-imposition semantics for a live deployment's config PATCH — a * client only ever owns `value` there either, never the manifest-declared spec. @@ -248,7 +255,9 @@ export function hardenAppEnv(storedEnv: AppEnvVar[], incomingEnv: AppEnvVar[]): const byKey = new Map(storedEnv.map((e) => [e.key, e])); return incomingEnv.map((incoming) => { const stored = byKey.get(incoming.key); - return stored ? { ...stored, value: incoming.value } : incoming; + if (!stored) return withoutRedactionMarker(incoming); + if (incoming.valueRedacted) return stored; + return { ...stored, value: incoming.value }; }); } @@ -262,6 +271,11 @@ export function hardenAppEnv(storedEnv: AppEnvVar[], incomingEnv: AppEnvVar[]): * * Stored order is preserved (with brand-new keys appended in `upserts` order). * A key that appears in both `upserts` and `removeKeys` is removed (delete wins). + * + * An upsert flagged `valueRedacted` supplies no value (F03) and is treated as + * though the key had been omitted — see `hardenAppEnv` above for why. An + * explicit `removeKeys` entry still wins over it: withholding a value on read + * must not make a secret undeletable. */ export function mergeAppEnv( storedEnv: AppEnvVar[], @@ -274,15 +288,16 @@ export function mergeAppEnv( const result: AppEnvVar[] = []; // Existing rows: drop if removed, else apply an upsert's value (spec preserved). + // A redacted upsert carries no value, so the stored row passes through whole. for (const stored of storedEnv) { if (remove.has(stored.key)) continue; const up = upsertByKey.get(stored.key); - result.push(up ? { ...stored, value: up.value } : stored); + result.push(up && !up.valueRedacted ? { ...stored, value: up.value } : stored); } // Brand-new keys (not already stored), minus any also flagged for removal. for (const up of upserts) { if (storedKeys.has(up.key) || remove.has(up.key)) continue; - result.push(up); + result.push(withoutRedactionMarker(up)); } return result; } diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index df9a3e7f..68f8dd9d 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -789,6 +789,13 @@ export const CAPABILITIES = { READ_LOGS: 'read:logs', READ_BACKUPS: 'read:backups', READ_CATALOG: 'read:catalog', + // Reading an app's *secret* env values, separately from reading its + // configuration (F03). Held by `*`/admin; deliberately absent from the + // read-only set an authenticated non-admin dashboard user receives, who + // would otherwise retrieve every app's database password from a routine + // configuration read. Enforced where the response is shaped rather than as a + // route capability: the route stays readable, the credentials do not. + READ_SECRETS: 'read:secrets', // Write operations WRITE_DEPLOYMENTS: 'write:deployments', @@ -1220,8 +1227,78 @@ export type AppEnvVar = { // --- secret generation --- /** Generation recipe for the wand/CLI auto-fill. Only meaningful when `isSecret` is true. */ generate?: ParamGenerate; + /** + * RESPONSE-ONLY marker (F03): this row's `value` was withheld, because the + * reading principal does not hold `read:secrets`. `value` is `''` — which is + * indistinguishable from a genuinely empty secret without this flag, so a + * client MUST NOT render `(empty)` for a row carrying it. + * + * It is also a WRITE-SIDE instruction, and the reason redaction is safe to + * apply to an editable surface: a client may echo a redacted row back + * verbatim, and the server reads the flag as "no new value supplied for this + * key" and keeps the stored secret (`hardenAppEnv`/`mergeAppEnv` in + * `services/core/draft.ts`). Clearing a secret is therefore an explicit act — + * send the row with `value: ''` and no flag. + * + * Never persisted: the write path strips it before anything reaches a + * manifest, so a forged flag cannot make a stored row read as redacted. + */ + valueRedacted?: true; }; +/** + * Withhold every secret value in a set of env rows, for a principal that may + * read an app's configuration but not its credentials (F03). + * + * Applied at the response boundary, not in the service layer: the services that + * assemble these rows have no request context, and the same rows are the + * carry-forward source for upgrades and the restore-on-install env merge, which + * run as the platform and must keep real values. Redaction is therefore a + * property of one *answer to one caller*, never of the stored record. + * + * `isSecret` is the whole test. Nothing is inferred from a key's name: a row's + * secrecy is declared by the packager in the bundle manifest and re-imposed by + * the server on every write (`hardenAppEnv`), so it is already the platform's + * own fact rather than the client's. + */ +export function redactSecretEnvValues(rows: AppEnvVar[]): AppEnvVar[] { + return rows.map((row) => (row.isSecret ? { ...row, value: '', valueRedacted: true as const } : row)); +} + +/** + * Drop the response-only redaction marker from a row a client sent back (F03). + * Used on the write path after the flag has been consumed as "keep the stored + * value" — see {@link AppEnvVar.valueRedacted}. + */ +export function withoutRedactionMarker(row: AppEnvVar): AppEnvVar { + if (!row.valueRedacted) return row; + const { valueRedacted: _redacted, ...rest } = row; + void _redacted; + return rest; +} + +/** + * The write-side counterpart of {@link redactSecretEnvValues}, for a surface + * whose PATCH REPLACES the whole row set rather than merging by key (F03). + * + * Every incoming row flagged `valueRedacted` has its value taken from the + * stored row of the same key — the client was never shown that value, so it + * cannot be supplying one, and a full replace would otherwise blank it. The + * marker is stripped from every row either way, so it never persists. + * + * Deployment/draft env has its own merge (`hardenAppEnv`/`mergeAppEnv` in the + * server's `services/core/draft.ts`) which also re-imposes the typed spec; this + * is for row sets with no spec to protect, where the operator owns the whole + * row — host `systemEnv`, notably. + */ +export function restoreWithheldEnvValues(storedEnv: AppEnvVar[], incomingEnv: AppEnvVar[]): AppEnvVar[] { + const stored = new Map(storedEnv.map((e) => [e.key, e])); + return incomingEnv.map((incoming) => { + if (!incoming.valueRedacted) return incoming; + return { ...withoutRedactionMarker(incoming), value: stored.get(incoming.key)?.value ?? '' }; + }); +} + export type DraftDefaults = { ports: Array<{ host?: number; container: number; protocol: 'tcp' | 'udp' }>; volumes: Array<{ hostPath?: string; containerPath: string; readOnly?: boolean }>; diff --git a/packages/web/src/__tests__/pages/DeploymentDetail.test.tsx b/packages/web/src/__tests__/pages/DeploymentDetail.test.tsx index 04d42035..60f27c16 100644 --- a/packages/web/src/__tests__/pages/DeploymentDetail.test.tsx +++ b/packages/web/src/__tests__/pages/DeploymentDetail.test.tsx @@ -249,6 +249,100 @@ describe('DeploymentDetail Configuration tab', () => { }); }); +/** + * A secret whose value the server withheld (F03: this user does not hold + * `read:secrets`) arrives as `value: '', valueRedacted: true`. + * + * The page has to get two things right about it: say that the value is hidden + * rather than that the app has none, and — because the same rows are the save + * payload — not turn the operator's next save into a secret-wiping write. + */ +describe('DeploymentDetail withheld secrets (F03)', () => { + const withRedacted = () => + deploymentsApi.config.mockResolvedValueOnce({ + appEnv: [ + ...config.appEnv, + { key: 'DB_PASSWORD', value: '', isSecret: true, required: true, valueRedacted: true as const }, + // A genuinely empty optional secret, for contrast: it has no marker, so + // "(empty)" is the truthful rendering for this one. + { key: 'OPTIONAL_TOKEN', value: '', isSecret: true, required: false }, + ], + systemOverrides: config.systemOverrides, + }); + + it('renders a withheld secret as hidden, with no reveal control', async () => { + withRedacted(); + renderDetail(); + + await waitFor(() => expect(screen.getByText('DB_PASSWORD')).toBeInTheDocument()); + // Said explicitly, and said once — for the withheld row only. + expect(screen.getAllByText('hidden')).toHaveLength(1); + // Both secret rows mask their value, so the mask alone says nothing; the + // difference between them is the label above and the control below. + expect(screen.getAllByText('••••••••')).toHaveLength(2); + // Exactly one reveal control — the genuinely-empty secret's. Offering one + // for the withheld row would promise something it cannot deliver: there is + // nothing behind that eye, because the value never reached the browser. + expect(document.querySelectorAll('.lucide-eye')).toHaveLength(1); + }); + + it('keeps the stored secret on save by echoing the marker back, not a blank value', async () => { + withRedacted(); + renderDetail(); + await waitFor(() => expect(screen.getByText('DB_PASSWORD')).toBeInTheDocument()); + + fireEvent.click(screen.getByRole('button', { name: /edit configuration/i })); + // Edit something else entirely; the withheld secret is left alone. + const adminInput = await screen.findByDisplayValue('admin'); + fireEvent.change(adminInput, { target: { value: 'root' } }); + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + + await waitFor(() => expect(deploymentsApi.update).toHaveBeenCalledTimes(1)); + const [, payload] = deploymentsApi.update.mock.calls[0]; + const row = payload.env.find((e: { key: string }) => e.key === 'DB_PASSWORD'); + // The marker survives the round trip, which is what tells the server to + // keep the stored value instead of writing this empty string over it. + expect(row.valueRedacted).toBe(true); + expect(row.value).toBe(''); + expect(payload.env.find((e: { key: string }) => e.key === 'ADMIN_USER').value).toBe('root'); + // Not deleted — a withheld value is not an absent variable. + expect(payload.removeEnvKeys).toBeUndefined(); + }); + + it('drops the marker once the operator types a replacement, so the new value applies', async () => { + withRedacted(); + renderDetail(); + await waitFor(() => expect(screen.getByText('DB_PASSWORD')).toBeInTheDocument()); + + fireEvent.click(screen.getByRole('button', { name: /edit configuration/i })); + const secretInput = await screen.findByLabelText(/DB_PASSWORD/i); + fireEvent.change(secretInput, { target: { value: 'rotated' } }); + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + + await waitFor(() => expect(deploymentsApi.update).toHaveBeenCalledTimes(1)); + const [, payload] = deploymentsApi.update.mock.calls[0]; + const row = payload.env.find((e: { key: string }) => e.key === 'DB_PASSWORD'); + expect(row.value).toBe('rotated'); + // Keeping the marker here would silently discard the rotation. + expect(row.valueRedacted).toBeUndefined(); + }); + + it('does not treat a withheld required secret as a missing required value', async () => { + // The row is `required` and reads as empty. Blocking the save on it would + // make every configuration change impossible for this user without ever + // saying why. + withRedacted(); + renderDetail(); + await waitFor(() => expect(screen.getByText('DB_PASSWORD')).toBeInTheDocument()); + + fireEvent.click(screen.getByRole('button', { name: /edit configuration/i })); + fireEvent.click(screen.getByRole('button', { name: /save changes/i })); + + await waitFor(() => expect(deploymentsApi.update).toHaveBeenCalledTimes(1)); + expect(screen.queryByText(/fix the highlighted fields/i)).not.toBeInTheDocument(); + }); +}); + describe('DeploymentDetail live updates (T009)', () => { it('re-renders the new status from a deployment_update event patched onto the same QueryClient the page reads from, with no page remount', async () => { const { queryClient } = renderDetail(); diff --git a/packages/web/src/pages/DeploymentDetail.tsx b/packages/web/src/pages/DeploymentDetail.tsx index 5246b75f..0e687fed 100644 --- a/packages/web/src/pages/DeploymentDetail.tsx +++ b/packages/web/src/pages/DeploymentDetail.tsx @@ -218,7 +218,16 @@ export const DeploymentDetail: React.FC = () => { // Every row's issues against its own spec (legacy/custom rows with no spec // reduce to just the required-tri-state check — see param-validate.ts). - const paramIssues = useMemo(() => validateParams(envVars), [envVars]); + // + // A row the server withheld the value of (F03: this user lacks + // `read:secrets`) arrives with `value: ''`, which the required check would + // read as "the operator cleared a required secret". It is stored and unchanged, + // so it is relaxed to optional for validation only — leaving it blank means + // "keep what is stored", which is exactly what the server does with it. + const paramIssues = useMemo( + () => validateParams(envVars.map((e) => (e.valueRedacted && !e.value ? { ...e, required: false } : e))), + [envVars], + ); const issuesForKey = (key: string): ValidationIssue[] => touchedKeys.has(key) ? paramIssues.filter((i) => i.path === `env.${key}`) : []; @@ -423,8 +432,16 @@ export const DeploymentDetail: React.FC = () => { .map((e) => e.key) .filter((k) => k && !currentKeys.has(k)); + // F03: a withheld secret left blank keeps its `valueRedacted` flag, which + // the server reads as "no new value supplied" and preserves the stored + // secret. Once the operator types over it the flag must go, or the + // replacement would be silently discarded. + const envForPatch = envVars + .filter((e) => e.key) + .map((e) => (e.valueRedacted && e.value ? { ...e, valueRedacted: undefined } : e)); + await updateConfiguration({ - env: envVars.filter((e) => e.key), + env: envForPatch, ...(removeEnvKeys.length ? { removeEnvKeys } : {}), systemOverrides, }); @@ -766,7 +783,13 @@ export const DeploymentDetail: React.FC = () => {
{envVar.key ? ( handleParamChange(index, v)} issues={issuesForKey(envVar.key)} @@ -801,6 +824,10 @@ export const DeploymentDetail: React.FC = () => { ); const renderReadOnlyRow = ({ env: envVar }: { env: AppEnvVar; index: number }) => { + // A withheld value (F03) is masked with no reveal control: there is + // nothing behind the eye, and rendering it as `(empty)` would claim + // the app has no password rather than that this user may not see it. + const withheld = envVar.valueRedacted === true; const showValue = envVar.isSecret && !showSecrets[envVar.key]; return (
{ {envVar.label ?? envVar.key} - {showValue ? '••••••••' : (envVar.value || '(empty)')} + {withheld ? ( + <> + •••••••• + hidden + + ) : showValue ? '••••••••' : (envVar.value || '(empty)')} - {envVar.isSecret && ( + {envVar.isSecret && !withheld && (