fix(auth): read:secrets — withhold app secret values from read-only principals (F03)

[pofallon]

What

Closes F03 on the Security & Code Quality Review Remediation Tracker — "Read-only dashboard users can retrieve application secrets."

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; secret values are withheld from any principal lacking it.

Why

Every unmatched GET names no capability — that is the documented operator model (authorizeRequest: "if you hold a key to this host, you may read it"). But the env-bearing reads carried each app's database password, API token and encryption secret in plaintext, so the read-only set an authenticated non-admin OIDC user receives was in practice full credential access to every installed app.

Worse than the reviewer's framing: HOLA_OIDC_ADMIN_GROUP is fail-closed, so on a host where no admin group is configured, every authenticated dashboard user is read-only — and every one of them could read every app secret.

Design

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):

Surface	Carries
GET /api/deployments/:id/config	the active release's appEnv
GET /api/drafts/:id	the same rows, including wizard-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

This 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, a confidentiality fix would have shipped 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, new restoreWithheldEnvValues for the full-replace systemEnv PATCH. An explicit removeEnvKeys entry still wins: 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.

Why not AuthService.hasCapability

principalHasCapability decides from the principal alone. MockAuthService.hasCapability returns true unconditionally and RealAuthService short-circuits to true whenever auth is disabled — both right for a route gate, both wrong for shaping a response: they would make this 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 •••••••• hidden with no reveal control (there is nothing behind that eye — the value never reached the browser), is relaxed to optional for client-side validation so a required secret nobody may read cannot silently block every save, and drops its marker the moment the operator types a replacement.

An admin's experience is unchanged. * matches read:secrets, so the Configuration tab, Settings and hola config behave exactly as before for the principal that was always entitled to them.

Test evidence

bun run typecheck && bun run lint && bun run typecheck && bun run test && bun run build — green.

• server 1426 → 1459, web 379 → 383, CLI 289.
• The route-level half is driven through route() rather than fetch, for the same reason as contract-broker-routes.test.ts: 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.
• Both env-bearing deployment surfaces are driven from one table, so neither can be fixed while the other leaks.
• The boundary itself is asserted directly (READONLY_CAPABILITIES must not contain read:secrets) — adding it there would restore the exposure in full while every redaction test still passed.

Revert-proof, measured:

Reverted	Result
the two route redaction calls	6 of 12 route tests fail (the 6 that pass are the admin/non-secret cases, which must pass on both sides)
the three web changes	all 4 new web tests fail

Residual, deliberately not done here

The tracker's own alternative — restricting the complete configuration to administrators outright — would also close this, and would additionally stop a stolen admin session or dashboard XSS from being an inventory of every app credential. That is a product decision about what the dashboard is for, not a fix for this finding, and it is not made here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Vck5KSX2CLxhohx14nb5Sh