fix(ipc): redact credential leaves before broadcasting config:changed#59
Open
armstrongsamr wants to merge 1 commit into
Open
fix(ipc): redact credential leaves before broadcasting config:changed#59armstrongsamr wants to merge 1 commit into
armstrongsamr wants to merge 1 commit into
Conversation
A recent security review flagged that `flushConfigBroadcast` in
`electron/ipc/config.ts` was passing the raw `AppConfig` to
`broadcastToAllWindows`. That helper fans out to two surfaces with
different trust profiles:
- Every `BrowserWindow.webContents.send`, which delivers the payload
to any in-process renderer — including third-party plugin code
loaded via the `plugin-renderer://` custom scheme. Plugin renderer
scripts can subscribe to `window.app.config.onChanged` directly
and were bypassing the manifest-permission gate that the sandboxed
plugin API enforces via `electron/plugins/safe-config.ts`.
- `broadcastToWebClients`, which JSON-encodes the payload onto every
connected WebSocket on the local web server (`electron/web-server`).
Once the web server is enabled, the unredacted config crosses a
network trust boundary on every config change.
The affected fields are the credential leaves in `electron/config/schema.ts`:
`models.providers.*.{apiKey, accessKeyId, secretAccessKey, sessionToken,
extraHeaders}`, `audio.azure.subscriptionKey`,
`{realtime, imageGeneration, videoGeneration}.{openai, azure, custom}.apiKey`,
`memory.semanticRecall.embeddingProvider.*.apiKey`, `mcpServers[i].env`,
`webServer.auth.password`, and `webServer.tls.keyPath`.
Fix
---
Add `toBroadcastSafeConfig` to `electron/plugins/safe-config.ts` as a thin
named wrapper around the existing `toPluginSafeConfig`. Both produce the
same redacted shape so there is a single denylist source of truth, but the
separate name documents the intent at the broadcast call site and provides
a seam if the broadcast redaction ever needs to diverge from the plugin
redaction.
`flushConfigBroadcast` now passes the redacted payload to
`broadcastToAllWindows` while continuing to feed the raw `currentConfig`
to `onChanged?.(...)`, which the agent runtime, MCP loader, and other
main-process subsystems depend on.
Renderer side
-------------
`src/providers/ConfigProvider.tsx` previously trusted the broadcast payload
to refresh local state, which would empty out the API-key inputs in
`src/components/settings/` on every external config change. It now treats
`config:changed` as a "something changed" signal and re-fetches via
`app.config.get()`. The dedicated IPC channel is only readable by
first-party renderer code, so secret values stay on the privileged path.
Tests
-----
`electron/ipc/__tests__/config-broadcast.test.ts` adds four assertions:
- Every credential sentinel planted in a fully-populated `AppConfig`
(covering every documented secret slot) is absent from the broadcast
payload, while the raw `currentConfig` still reaches the main-process
`onChanged` callback.
- A schema-walk regression guard rejects any surviving key name that
matches a credential-shape regex — a new credential field added to
`AppConfig` without an update to the redactor will surface here.
- A string-leaf sweep rejects any payload containing a known credential
prefix (sk-test, AKIA, ghp_, Bearer), guarding against fields whose
name does not match the schema-walk regex.
- Non-secret fields (provider type/enabled/endpoint, MCP server
name/command, webServer port/enabled, `envKeys` for env name
enumeration) are preserved so the broadcast remains useful for
public state.
Test count: 323 -> 327 (+4); lint and type-check clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Apply credential redaction to the
config:changedIPC broadcast. Beforethis fix,
flushConfigBroadcastwas passing the rawAppConfigtobroadcastToAllWindows(electron/ipc/config.ts:1110), which fans thepayload out to:
BrowserWindow.webContents.send— delivering to all in-processrenderers, including third-party plugin renderers, for which
PluginAPI.config.onChangedwould otherwise apply the redaction definedin
electron/plugins/safe-config.ts;broadcastToWebClients— every connected WebSocket on the local webserver, which crosses a network trust boundary when the web server is
enabled.
The affected fields are every credential-bearing leaf in
electron/config/schema.ts; the authoritative denylist lives inelectron/plugins/safe-config.ts.Why
The redaction shape already existed:
PluginAPI.config.getwas gated bythe
config:read-secretsmanifest permission, andtoPluginSafeConfigstripped credential leaves on that path. The
config:changedbroadcastwas the asymmetric gap — every renderer subscribed to the raw IPC channel
received cleartext secrets on every config change, regardless of any
permission declaration, with no audit trail. This PR closes that
asymmetry.
toBroadcastSafeConfigis a thin named alias fortoPluginSafeConfig(same denylist) — the separate symbol documents intent at the broadcast
call site and creates a seam for future divergence.
flushConfigBroadcastnow passes the redacted payload tobroadcastToAllWindowswhile continuing to pass rawcurrentConfigtoonChanged?.(...), which the agent runtime and MCP loader depend on.Renderer side: the only consumer of
config:changedisConfigProvider.tsx. It previously trusted the broadcast to refreshlocal state, which would have emptied the API-key inputs in the settings
UI. It now treats the broadcast as a "something changed" signal and
re-fetches via
app.config.get()— the pull channel, only reachable fromfirst-party renderer code.
Tests
Four regression tests in
electron/ipc/__tests__/config-broadcast.test.tsassert that: every credential sentinel planted in a fully-populated
AppConfigis absent from the broadcast while still reachingonChanged?.(...); a schema-walk guard rejects surviving key namesmatching a credential-shape regex (forcing function for future credential
fields added without redactor updates); a string-leaf sweep rejects
payloads containing known credential prefixes; non-secret fields are
preserved.
Test count: 323 → 327 (+4).
Known scope boundary
This PR narrows scope to the push path — IPC channels that fan out
unsolicited to many recipients (every
BrowserWindow, every WebSocketpeer). The pull path is unchanged:
config:getandconfig:setbothstill return the raw
AppConfigto their caller. TheipcRenderer.invokeresponse is scoped to the callingwebContents,which today is always first-party renderer code (plugin renderers reach
config via the permission-gated
PluginAPI.config.get, not the raw IPCchannel). Caller-identity-aware redaction on the pull path is a separate
decision worth its own PR if the maintainer wants tighter coverage.
Other channels on the same fan-out path (
plugin:event,plugin:ui-state-changed,conversations:changed, etc.) were audited —none currently embed credentials.
Checklist
pnpm lintpassespnpm type-checkpassespnpm testpasses (or note "no tests yet for this area" with rationale)pnpm buildsucceedselectron/preload.ts) is updated and the type contract still holdsdesktopConfigPayload()allowlist is updatedNotes on the checklist:
config:changedpayload type changed from rawAppConfigtoBroadcastSafeConfig(=PluginSafeConfig— same shape,credential leaves removed).
ConfigProvider.tsxupdated to re-fetchvia
config:get. Preload bridge itself is unchanged; this is apayload-shape refinement of an existing channel.
destructured credential fields off the raw
config:changedpayload —those fields are now
undefined. Plugin-main code viaPluginAPI.config.onChangedalready receivedPluginSafeConfigunlessthe plugin declared
config:read-secrets(
plugin-manager.ts:696-720), so it's a no-op for them.desktopConfigPayload()allowlist unchanged.redaction denylist is documented in
electron/plugins/safe-config.ts.