🤫 feat: Better Secret Redaction - #99
Merged
Merged
Conversation
The backend redacts registered secret fields (ocr.apiKey, webSearch API keys, speech tts/stt apiKeys, endpoints.assistants.apiKey) from admin config reads and returns a display<Key> sibling (first6...last4) in their place. The Configuration UI previously rendered these as empty inputs indistinguishable from never-configured fields. String fields whose parent value carries a non-empty display companion now render the masked value read-only with an explicit Replace flow. Display companion paths map back to their real secret paths so the configured indicator and Reset affordance survive redaction. Save payloads exclude display companion leaf paths and strip display companion strings nested inside object values, so a masked display value can never be submitted as a real secret. Schema extraction also filters display companion fields so they never render as editable inputs once the shared config schema ships them.
Scope-mode configured paths drive the override indicator and reset affordance while editing a profile; without the mapping a redacted scope secret lost both, same as the base-config case.
dustinhealy
changed the title
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a656e24201
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
filterSecretDisplayFields was imported from the @/utils/secrets subpath instead of the @/utils barrel. SavePayload was declared locally in utils.ts instead of src/types, where all local interfaces belong per this repo's import conventions.
A masked secret field's Replace state was local useState, disconnected from the parent's editedValues/touchedPaths clears. Discarding, saving, confirming a scope change, or resetting base config left an in-progress (possibly untyped) replacement visible instead of reverting to the masked display. editSessionId is bumped on each of those clears and threaded down to key SecretField's remount, so a stale replacement session never survives past the edit session that started it.
…erers ProvidersRenderer and ProviderSection destructured and forwarded a fixed prop list that predated pendingResets and editSessionId, so fields rendered under a named endpoint provider (e.g. endpoints.assistants.apiKey, a registered secret) never received either. A pending reset on such a field kept showing the masked value instead of reverting to an editable input, and the SecretField remount fix from the previous commit didn't reach this subtree either.
…review previewChangedPaths was passed the raw scopeChangedPaths from the backend, which reports a changed secret at its display path (e.g. endpoints.openAI.displayApiKey), not the real field path FieldRenderer renders at. A secret changed at scope level never got a "changed" indicator in scope-preview mode and was dropped entirely by the changed-only filter. scopeConfiguredPaths already ran scopeChangedPaths through mapSecretDisplayPaths for the configured/db-override indicators; scopeChangedPathsMapped applies the same mapping for the preview-diff path, preserving null when there are no scope changes.
Contributor
Author
|
@codex re-review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
dustinhealy
marked this pull request as ready for review
July 29, 2026 04:29
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
7 tasks
…ention The backend (danny-avila/LibreChat#14509) renamed its masked-companion fields from a display<Field> prefix to a <field>Preview suffix, since the prefix form could collide with real display-label config fields like modelDisplayLabel and required a hand-declared displayPath per registry entry instead of a mechanical derivation. Renamed every consumer (secrets.ts, ConfigPage, FieldRenderer, the save-payload stripping in utils.ts, and extractSchemaTree's field filter) to match, plus all test fixtures. Two tests that exercised the old prefix-shaped false-positive case (a real field that merely looked display-shaped) were updated to exercise the equivalent suffix-shaped case instead, since the collision risk moved with the convention.
Replace and Cancel were bare colored text with no icon, no resting border, and no focus-visible indicator, and unlike every other per-field action in this UI (e.g. ConfigRow's Reset) they carried no accessible name distinguishing which field's secret they act on. Added a pencil/cross icon matching the Reset convention, a resting border plus hover fill so the control reads as a button rather than a link at every state (WCAG 1.4.11 non-text contrast for UI component boundaries), an explicit focus-visible outline, and a field-name-aware aria-label via two new locale keys so multiple secret fields on one page remain distinguishable to screen readers.
editSessionId only incremented inside the "discard unsaved edits" branch, so opening a SecretField's Replace input without typing anything and then switching scope left editedValues empty and skipped the bump entirely. React kept the same SecretField instance across the scope change, leaving the untyped Replace input open against the newly selected scope's data instead of reverting to the masked view.
Abandoning an in-progress Replace called onChange(path, undefined), reusing the same signal handleResetField uses for a real reset — but unlike handleResetField, this went through applyConfigEdit's baseline-match diffing instead of writing the reset sentinel directly. applyConfigEdit only treats undefined as a no-op when it matches the baseline exactly; the base config baseline for a redacted secret is always absent (undefined), but a scope-resolved baseline for a path with no override at that scope isn't guaranteed to be. If it ever reads back as '' instead, cancelling a replacement would register a real pending reset, and a later save could delete the stored credential without the admin choosing reset. onDiscardField removes the path from editedValues/touchedPaths directly, the same way the "Discard all" button already does, so Cancel can never be misread as an intentional reset regardless of what the baseline happens to contain. Threaded through ConfigTabContent, FieldRenderer, and the provider-section renderers alongside the existing onResetField prop.
handleImportAsProfile already strips preview values from imported config before submitting, since a re-imported YAML export can carry stale display*/*Preview companions from a previous read (or, from an untrusted YAML source, spoofed ones). The base-config import path skipped this and sent the parsed YAML straight through. The backend's own registry-driven encrypt/preserve/redact protection already covers this (upsertConfigOverrides runs unconditionally for every principal, base included, so a spoofed or stale preview value can't actually corrupt a real secret) — this closes the gap between the two import paths for consistency and defense-in-depth rather than a live vulnerability.
Contributor
Author
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e793f73a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SecretField decided whether to show its input using only its own local replacing state plus the current value. That state resets on remount (e.g. switching config tabs and back), so an admin who clears a replacement to an empty string and then remounts the field would see the stale masked secret again, even though the empty string is still queued and will clear the stored credential on save. Thread this through the existing touchedPaths tracking instead of relying on local component state alone, so the input stays visible for any path with a queued edit regardless of remounts.
Contributor
Author
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af051ab854
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
touchedPaths only ever grows within an edit session, but applyConfigEdit removes a path from editedValues whenever a typed value matches the active baseline. For scope-mode edits where a redacted secret's baseline reads as an empty string, clearing a replacement back to empty hits exactly that match and gets removed from editedValues while staying in touchedPaths, and it can outlive a clean scope change too since touchedPaths is only cleared when editedValues is nonempty at the time. Using touchedPaths for hasPendingEdit kept the field open and empty with nothing queued to save. Deriving it from editedValues membership instead reflects only what is actually pending.
…entries renderInlineField, used for fields nested inside custom endpoint and similar collection entries, always rendered a plain empty TextField regardless of whether the field had a preview companion sibling. An already-configured custom endpoint secret showed as unset with no Replace flow, since the top-level masked-secret handling in SingleFieldRenderer never applied to this rendering path. Mirrors the existing top-level pattern: check for a sibling <field>Preview value and render SecretField when the entry's real value is empty.
Editing an existing custom-endpoint entry saves a path like endpoints.custom.0, so secretPathForPreviewPath constructed endpoints.custom.0.apiKeyPreview against a schema path set that only knows the index-free endpoints.custom.apiKey, and never recognized it as a preview companion. The stripped save then reached mergeIndexedArrayEntriesIntoBase, which spliced that one clean entry into the full array fetched from /api/admin/config/base but sent the untouched sibling entries' preview companions right back with it, since that merge never re-stripped the array it built. Fixes both ends: secretPathForPreviewPath now strips numeric segments before matching against schema paths, and the indexed-array merge strips preview companions from the array it constructs before sending it as a save entry.
Contributor
Author
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cc8acba5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed value Cancel in renderInlineField called onChange(field.key, '') for a masked secret inside an array/record entry, which queues an explicit apiKey: '' onto the containing entry even if nothing was typed. That entry no longer matches its baseline, so save would submit it and the backend's empty-value handling clears the real stored secret. Pass undefined instead: spread-merging it into the entry keeps the key present with an undefined value, which JSON serialization drops entirely, so a save either resolves to a true no-op (matches baseline) or reaches the backend without an apiKey key at all, leaving the existing secret untouched either way.
ProviderSection and ProvidersRenderer forwarded onDiscardField/reset/ session props to SingleFieldRenderer but not editedValues, so hasPendingEdit always computed false for provider secret fields. A cleared-but-queued provider secret would revert to the stale masked display after remounting (e.g. a tab switch), the same gap already fixed for the top-level renderer.
Contributor
Author
|
@codex re-review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
renderInlineField's masked-secret branch treated any empty stringValue as untouched, matching on stringValue alone the same way SingleFieldRenderer did before its own hasPendingEdit fix. A queued empty replacement (typed then cleared, or an in-progress remount) could show the stale read-only mask again with the clear still queued for save. Derives hasPendingEdit from whether fieldValue is a defined string at all, including '', versus absent/undefined for an untouched baseline, and passes it into SecretField the same way the top-level renderer does.
renderInlineField's masked-secret SecretField instances omitted the editSessionId-based key SingleFieldRenderer uses, so local Replace mode could survive a global discard, a successful save, or a scope change even though editedValues had already reverted for that path. Threads editSessionId through the render-fields chain (ArrayObjectField, RecordObjectField, and ObjectEntryCard's CollectionRenderFields call, plus the custom-endpoint grouped-fields renderer) down to renderInlineField, and uses it in SecretField's key the same way SingleFieldRenderer already does.
serializedEditedValues and originalValuesForDialog serialized raw editedValues/baseline entries for the confirm-save diff view without the stripSecretPreviewValues pass buildSavePayload already applies. An edited array or nested-object entry could show apiKeyPreview (and similar companions) in the before/after YAML even though those keys never reach the actual PATCH payload.
Contributor
|
Verified this branch against the merged backend (danny-avila/LibreChat#14509, squashed as
Test run on |
Cancel in renderInlineField always called onChange(field.key, undefined), which updates the containing entry and gets recorded in editedValues under the entry's flat path regardless of content, since flattenObject never produces a flat key for a whole array entry to compare against. Opening Replace and cancelling without ever typing anything marked the page dirty even though the mask correctly reappeared, unlike the top-level onDiscardField path which stays clean in the same scenario. Only calls onChange when hasPendingEdit is already true, i.e. there is a real queued value to discard.
… merges mergeIndexedArrayEntriesForScope rebuilds a full array from base, scope-override, and pending sources when a scope-profile save touches an indexed array entry, but never re-stripped preview companions from the result the way mergeIndexedArrayEntriesIntoBase already does for base-config saves. Editing one scope-override array entry could PATCH sibling entries still carrying masked apiKeyPreview strings from base or scope overrides. Exports getSchemaPathSet from config.ts and reuses it here, matching the base-config fix.
…e entries originalValuesForDialog only stripped a reconstructed value when the edited path was absent from the flat baseline, but flattenObject never descends into arrays, so a whole-array edit path like endpoints.custom is itself a flat baseline key. The before side of the confirm-save diff could still show sibling entries' apiKeyPreview companions for that case, even though the after side was already stripped. Strips the baseline value too when the edited path already exists in the flat baseline.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9592f12. Configure here.
renderInlineField threads editSessionId to its own SecretField, but nested object fields render through NestedGroupWithAddField's inner InlineFieldRenderer, which never received it. A secret inside a nested inline group (a field with children, rendered inside a collection entry) could stay in an open Replace state across a discard, scope change, or save.
danny-avila
approved these changes
Jul 29, 2026
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
2 participants
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.
Summary
packages/api/src/admin/secrets.tswas introduced by the already-merged Encrypted Langfuse Fanout Config feature (github.com/danny-avila/pull/14107) and was hardcoded to exactly one field,langfuse.secretKey: encrypt-at-rest on write, redact on read, preserve-on-omit.Every other credential-shaped field in the admin-editable config schema had no such protection. Because
BASE_ONLY_CONFIG_SECTIONSis empty, the wholeTCustomConfigdocument is admin-writable and readable through the admin config API, so any operator who stored a literal (non-${ENV_VAR}) key on the base config document got that plaintext value back verbatim in the API response to any admin holdingread:configs:<section>.This PR replaces the single hardcoded field with a registry (
CONFIG_SECRET_FIELDS) and generalizes every function in the module (encryptConfigSecretFields,encryptConfigSecrets,getConfigSecretMutationPaths,getConfigSecretInputError,isConfigSecretAncestorPath,isConfigSecretDescendantPath,preserveConfigSecrets,redactConfigSecrets, plus a newgetConfigSecretSectionsand a runtimeresolveConfigSecret) to operate over arbitrary dot-paths at any depth.langfuse.secretKeysemantics are preserved byte-for-byte (display companion, always-encrypt, array-section stripping); its existing tests stay green.Two behaviors distinguish the new fields from langfuse:
${ENV_VAR}references keep those references stored and returned as plain, visible values (an env-var name is not a secret, and admins need to see which var is wired). A literal value in the same field is always encrypted.langfuse.secretKeykeeps its original no-exemption behavior.v3:ciphertext, so the consumers that read them from the merged app config must decrypt.resolveConfigSecret(decrypt, else resolve${ENV_VAR}, else pass through) is wired into the speech STT/TTS services; the Mistral OCR auth loader usesdecryptConfigSecret. webSearch and the assistants endpoints need no runtime change (see scout table).Change Type
Scout list — every credential-shaped field in the config schema
Paths are in
packages/data-provider/src/config.ts/mcp.ts. "Registered" = added toCONFIG_SECRET_FIELDS.Registered (encrypt-at-rest + redact-on-read)
langfuse.secretKeyz.string().optional()langfuse/utils.ts)ocr.apiKeyz.string().optional().default('${OCR_API_KEY}')files/mistral/crud.tsloadAuthConfigspeech.tts.openai.apiKeyz.string()STTService/TTSServiceviaresolveConfigSecretspeech.tts.azureOpenAI.apiKeyz.string()speech.tts.elevenlabs.apiKeyz.string()speech.tts.localai.apiKeyz.string().optional()speech.stt.openai.apiKeyz.string()speech.stt.azureOpenAI.apiKeyz.string()webSearch.serperApiKeyz.string().optional().default('${SERPER_API_KEY}')webSearch.searxngApiKey...default('${SEARXNG_API_KEY}')webSearch.firecrawlApiKey...default('${FIRECRAWL_API_KEY}')webSearch.tavilyApiKey...default('${TAVILY_API_KEY}')webSearch.jinaApiKey...default('${JINA_API_KEY}')webSearch.cohereApiKey...default('${COHERE_API_KEY}')endpoints.assistants.apiKeyz.string().optional()endpoints.azureAssistants.apiKeyz.string().optional()¹
webSearchcredentials are resolved bypackages/api/src/web/web.tsloadWebSearchAuth, which only ever collects${VAR}references (extractWebSearchEnvVarsskips any non-placeholder string). A literal webSearch key never reached the runtime auth path pre-PR, so encrypting the literal at rest changes nothing at runtime — the protection is purely the at-rest/redaction fix. Documented so a reviewer knows this is deliberate, not a forgotten decrypt.²
endpoints.assistants.apiKey/endpoints.azureAssistants.apiKeyare never read from the merged app config at runtime —api/server/services/Endpoints/{assistants,azureAssistants}/initialize.jssource the key fromprocess.env.ASSISTANTS_API_KEY/AZURE_ASSISTANTS_API_KEYviaEndpointService.js. The config-schema field is a config-document field only, so registering it gives at-rest + redaction protection with no runtime decrypt.Considered sensitive-looking but deliberately NOT registered
mcpServers.<name>.apiKey.key,.oauth.client_secretServerConfigsDB), which already encrypts these withencryptV2. Av3:value written through the generic config API could not be decrypted by MCP runtime, so registering here would break MCP. Residual: the generic config API still round-trips these two in plaintext (secondary write path) — a known scoped gap, not closed here to avoid the v2/v3 scheme conflict.mcpServers.<name>.headers,.oauth_headers,.envz.record(z.string(), z.string())maps of mixed-sensitivity values (some entries are auth tokens, some are not). Redacting the whole record would blank non-secret entries; per-entry classification is out of scope for a dot-path registry.endpoints.custom[].apiKey,endpoints.azureOpenAI.groups[].apiKeypackages/data-provider/src/azure.ts, shared with the frontend bundle, which cannot import the backend decrypt.endpoints.*.headers/additionalHeadersturnstile.siteKeyTURNSTILE_SECRET), not in this schema.langfuse.publicKey,langfuse.displaySecretKeyskillSync.github.sources[].token${VAR}reference and rejects literals, so a literal secret cannot be stored.agents.remoteApi.auth.apiKey{ enabled: boolean }toggle.anthropic.vertex.serviceKeyFileTesting
Automated
packages/api/src/admin/secrets.spec.ts— unit coverage for every registry function over the new fields; the original langfuse cases are unchanged and green.packages/api/src/admin/secrets.integration.spec.ts(new) — drives the realcreateAdminConfigHandlersagainst a realmongodb-memory-serverConfig collection with realcreateModels/createMethods(hydrated Mongoose documents, not mocks). For all 16 registered fields it asserts, per field: dotted-patch write is encrypted at rest and redacted from the response; object-valued upsert is encrypted at rest and redacted fromgetConfig/listConfigs; an unrelated write preserves the stored ciphertext; a redacted read round-tripped back through a full upsert does not clobber the secret; explicit-empty clears it;v3:submissions are rejected. Plus env-placeholder pass-through, legacy-plaintext redaction, andgetBaseConfigYAML-literal redaction.secrets.tsto the origin/main single-field version makes the new-field cases fail (e.g. theendpoints.assistants.apiKeycases: 5 fail), restoring makes them pass.packages/api:admin+langfuse+files/mistralsuites — 670/670 pass. Speech services (apiworkspace,STTService.spec.js) — 29/29 pass.tsc --noEmitclean;eslintclean;prettier --checkclean. Fullpackages/apisuite: 7929 pass; the 202 failures are entirely pre-existing Redis/stream*_integration.spec.tssuites (no local Redis), none in changed areas.Live verification (real backend + real MongoDB + real admin-panel frontend)
Isolated instance: ephemeral MongoDB on
:27057, backend on:3092, admin panel on:3006, so it doesn't collide with other local dev.ocr.apiKeyandwebSearch.serperApiKeythrough the Configuration UI, reloads, and the plaintext values come straight back into the form fields. Confirmed at rest in Mongo (overrides.ocr.apiKey: "sk-mistral-LEAKED-...") and in the reloaded UI. The vulnerability is real, not theoretical.v3:...ciphertext, and after reload the UI form fields are empty (redacted) — no plaintext, no ciphertext, zero leaks anywhere in the rendered form.resolveConfigSecret(the exact helper wired into speech;decryptConfigSecretfor OCR) was run against the actual ciphertext stored in Mongo and recovered the exact secret the admin typed (sk-mistral-PATCHED-...,sk-serper-PATCHED-...);${ENV_VAR}references still resolve. The encryption round-trip is genuinely usable by the backend, not just accepted by the UI.Before — plaintext leak on origin/main:
ocr.apiKeyAfter — redacted on the patched branch:
ocr.apiKeyTest Configuration
packages/api+packages/data-schemasrebuilt (npm run build) so theapi/workspace picks up the compileddist/.Checklist