🧾 fix: Validate Principal Config Overrides Against configSchema - #16433
danny-avila merged 20 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical issues remain in refinement preservation and dotted record-key path handling.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds schema-aware validation for principal configuration overrides on write and merge, preventing invalid values from replacing base configuration.
Changes:
- Validates partial
PUT/PATCHoverrides and rejects invalid paths. - Filters invalid persisted fields during config resolution.
- Adds unit, handler, resolution, and E2E coverage.
- Two critical validation issues remain regarding
ZodEffectsrefinements and dotted record keys.
| File | Description |
|---|---|
packages/data-schemas/src/app/resolution.ts |
Filters invalid stored overrides |
packages/data-schemas/src/app/resolution.spec.ts |
Tests merge behavior |
packages/data-provider/src/overrides.ts |
Implements override validation |
packages/data-provider/src/overrides.spec.ts |
Tests validation behavior |
packages/data-provider/src/index.ts |
Exports the validator |
packages/api/src/app/service.spec.ts |
Updates override fixtures |
packages/api/src/admin/config.ts |
Validates admin writes |
packages/api/src/admin/config.handler.spec.ts |
Tests handler validation |
e2e/specs/mock/scenarios/config-override-validation.spec.ts |
Covers end-to-end validation scenarios |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
47145e8 to
e1c569c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: dc7a0f74fd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b93f0eec78
ℹ️ 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".
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f3a73db95
ℹ️ 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".
| const principal = `${config.principalType}/${config.principalId}`; | ||
| let stripped: unknown = config.overrides; | ||
| /** Removing a field can leave its parent incomplete, so check again until nothing fails. */ | ||
| for (let pass = 0; pass < MAX_STRIP_PASSES; pass++) { |
There was a problem hiding this comment.
The four-pass cap can discard an entire stored override document even when only one nested legacy value is invalid. For example, an Azure group whose models value has the wrong type is repaired successively by removing models, the now-incomplete group item, the now-empty groups array, and the now-incomplete azureOpenAI object; after that fourth removal endpoints merely needs one more pass, but the function instead returns {} and loses unrelated valid sections such as interface.customWelcome. Continue while removals make progress, or at least retain the independently valid sections rather than discarding the whole document.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c343969: stripping continues while removals make progress, and if it still fails only the sections that remain invalid are dropped, so an unrelated valid section such as interface.customWelcome survives. Covered in resolution.spec.
5f3a73d to
c343969
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c34396913d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fb04765cc
ℹ️ 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".
Reject admin config writes whose fields fail configSchema, and drop invalid fields from stored overrides at merge time so the base value from librechat.yaml survives.
…reject paths past scalars
…ons after repairs
5fb0476 to
fcfeaa7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcfeaa722c
ℹ️ 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".
| const remapped: AnyObject = {}; | ||
| for (const [key, value] of Object.entries(config.overrides)) { | ||
| const applied: AnyObject = {}; | ||
| for (const [key, value] of Object.entries(stripInvalidOverrides(config, raw))) { |
There was a problem hiding this comment.
When required fields are intentionally split across priorities, this strips the lower layer before a higher layer can complete it. For example, a lower-priority CloudFront override containing imageSigning: "cookies", cookieDomain, and requireSignedAccess: true but relying on a higher-priority override for domain is accepted at write time, yet is removed here as incomplete; the higher layer then leaves signing at its defaults, silently weakening the effective configuration. Validate the final accumulated configuration, or defer absence-only repairs until all applicable layers have been merged.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2410929: a priority layer now loses only the nodes whose supplied values are invalid, never what it leaves out for a higher layer to supply, so the cloudfront split (signing at priority 10, domain at 20) keeps all four fields. Completeness is still judged inside arrays the merge replaces, where no layer can complete an item. Verified on 4ef74cc by resolution.spec and scenario config-override-layers-complete-each-other.
| } | ||
| } | ||
| merged = deepMerge(merged, remapped); | ||
| raw = deepMerge(raw as AnyObject, applied) as Partial<TCustomConfig>; |
There was a problem hiding this comment.
In mixed-version data, applied can contain a legacy database-supplied stdio MCP server that filterMCPServerOverrides just removed from merged, but this line still adds it to raw. A later override for the same server can therefore validate using the hidden stdio command/args; for example, a title-only partial passes validation, while the actual merge has no server beneath it and installs an incomplete { title } MCP entry. Update the validation state with the filtered MCP result rather than the unfiltered override.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
| if (Array.isArray(target) && /^\d+$/.test(segment)) { | ||
| const next = [...target]; | ||
| next[Number(segment)] = setPath(next[Number(segment)], rest, value); | ||
| return next; |
There was a problem hiding this comment.
When target already contains endpoints.custom, this branch reconstructs indexed field patches as an apparently valid full array, so the intended rejection of index-addressed writes is bypassed. For example, if a stored partial custom endpoint at index 0 inherits required fields from YAML endpoint a, patching endpoints.custom.0.name to b passes validation; Mongo renames the stored item, and the keyed runtime merge then appends an incomplete endpoint b instead of updating a. Reject descendants of merged-by-key arrays regardless of whether the stored array currently exists.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ef74cc939
ℹ️ 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".
| const result = shape[section as keyof typeof shape].safeParse(merged[section]); | ||
| if (result.success) { | ||
| continue; |
There was a problem hiding this comment.
When any union branch parses successfully, this exits without validating supplied keys belonging to the other branches. Fresh evidence at this head is { memory: { agent: { enabled: true, id: 5, provider: "openAI", model: "gpt-4o" } } }: the provider/model branch succeeds because Zod strips its unknown id, but the raw override retains the invalid number; api/server/controllers/agents/client.js then prioritizes any non-null agent.id and passes 5 as agent_id instead of using the provider/model configuration. Require a successful branch to account for every schema-recognized supplied key, or persist the parsed output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9f73ce4: after a section parses, a supplied key the accepted union option dropped is reported (union_dropped_key) when another option defines it, so memory.agent { enabled, id: 5, provider, model } is rejected on write and, if already stored, merges as the provider/model form without id. Keys no option defines stay accepted. Covered by resolution.spec; scenarios pass on 9f73ce4.
| return node.findIndex( | ||
| (item) => | ||
| isPlainObject(item) && isPlainObject(mergedItem) && item[keyField] === mergedItem[keyField], | ||
| ); |
There was a problem hiding this comment.
For a merged-by-key array, this always maps an issue to the first override item with the matching key, even when Zod reported a later duplicate. For example, endpoints.custom containing { name: "x", models: { default: ["m"] } } followed by { name: "x", baseURL: 5 } produces an issue for the second item, but this selects the first; because that item has no baseURL, attribution stops early and the issue is discarded as an incomplete path. The write is therefore accepted with the invalid value, and keyed runtime merging may make that last duplicate the effective endpoint. Reject duplicate merge keys or map the issue to the actual winning source item.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request
Summary
Role, group and user config overrides are written through the admin config API without passing
configSchema, then deep-merged overlibrechat.yaml, so a malformed override field replaced a valid base value before any consumer read it.Overrides are now validated on the config they produce: merged over the base with the runtime merge, each touched section parsed with
configSchema, and each failure attributed to the override node that caused it.PUTandPATCH /fieldsreturn 400 naming each invalid supplied path and store nothing; partial sections are still accepted. At merge time, nodes that leave the accumulated config invalid are dropped with a warning, so the value beneath survives for documents stored before this change.Follow-up to #16259, tracked at berry-13#128.
Type of change
Testing
Tested environments/configuration:
e2e/config/librechat.e2e.yaml(interface.contextCost: true).Automated tests:
getConfigOverrideIssuesandgetConfigFieldIssuescases plus stored-override merge cases topackages/data-schemas/src/app/resolution.spec.ts(partial sections, unions, refinements, required fields, merged-by-name arrays, dotted record keys, unknown keys, stored secret shapes,null), and validation cases topackages/api/src/admin/config.handler.spec.ts. Three existing fixtures that used schema-invalid override shapes now use valid ones.e2e/specs/mock/scenarios/config-override-validation.spec.ts: invalidPUTandPATCHrejected with nothing stored, a valid partial override merged, and an invalid stored override leaving the YAML value in place.npx jestforsrc/adminandsrc/appinpackages/api,src/appandsrc/methods/config.spec.tsinpackages/data-schemas,src/configinpackages/data-provider;npx tsc --noEmitin all three workspaces.Screenshots / recordings
No user-facing change.
Risk / compatibility
An admin write that previously stored a schema-invalid value now gets a 400 with the offending paths, and a PATCH reads the principal's stored override to validate against it. Stored invalid nodes stop reaching consumers, which fall back to the value beneath. Completeness is judged per priority layer, so a lower layer that only a higher layer completes is dropped (berry-13#170).