Conversation
|
@danny-avila, I need your review on this. Thanks |
|
@codex review |
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: 92c585cd46
ℹ️ 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".
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1ca2b29d
ℹ️ 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".
| if (hasLangfuseHeadersOverride(rawOverrides)) { | ||
| return res.status(400).json({ error: LANGFUSE_HEADERS_CONFIG_ERROR }); | ||
| } | ||
| const sanitizedOverrides = sanitizeConfigOverrides(rawOverrides) as Record<string, unknown>; |
There was a problem hiding this comment.
In atomic overrides mode, only the outer payload shape is validated; sanitizeConfigOverrides does not recursively reject BSON-invalid property names. A request such as {"expectedVersion":null,"overrides":{"foo":{"\u0000bar":1}}} therefore reaches Config.create/findOneAndUpdate, where BSON serialization rejects the NUL-containing key and the handler returns HTTP 500 rather than a validation error. Recursively validate override keys before beginning the mutation; the same validation should cover object values supplied through atomic field entries.
Useful? React with 👍 / 👎.
| axios.defaults.headers.common['Accept-Encoding'] = 'gzip'; | ||
| } | ||
| await connectDb(); | ||
| await ensureConfigIndexes(mongoose); |
There was a problem hiding this comment.
The supported backend:experimental entry point connects in api/server/experimental.js without calling this new initializer. When that deployment runs with MONGO_AUTO_INDEX=false, the config, revision, and epoch uniqueness indexes and the deduplication migration are never installed before the atomic route accepts traffic; two concurrent first saves of an absent base config can therefore both commit separate config and epoch documents instead of one losing with a version conflict. Mirror this blocking initialization in the experimental startup path as well.
Useful? React with 👍 / 👎.
| tenantId: params.actor.tenantId, | ||
| principalType: params.principalType, | ||
| principalId: params.principalId, | ||
| overrides: cloneOverrides(current?.overrides), |
There was a problem hiding this comment.
When an existing base config contains a legacy plaintext credential, snapshotFromConfig copies that credential verbatim into admin_config_revisions. An atomic replace can encrypt or rotate the live config in the same transaction, but the predecessor plaintext then remains recoverable from the revision collection for up to 50 saves, and restoring it writes the plaintext back to the config document. Normalize registered secret fields to ciphertext when constructing and restoring revision snapshots so rollback history does not bypass the at-rest encryption applied by the admin handlers.
Useful? React with 👍 / 👎.
| const existing = await findConfigByPrincipal(principalType, principalId, { | ||
| includeInactive: true, | ||
| ...(user.tenantId !== undefined ? { tenantId: user.tenantId } : {}), | ||
| }); |
There was a problem hiding this comment.
In a non-strict deployment where a no-tenant/platform admin coexists with tenant-scoped configs, user.tenantId is undefined and this omits the explicit default-tenant predicate, so the tenant plugin leaves the query unscoped. findOne can then return another tenant's base config, while the subsequent atomic mutation deliberately targets the default tenant via actor.tenantId === ''; preserveConfigSecrets may consequently copy that tenant's omitted credentials into the default config. Pass tenantId: user.tenantId ?? '' for this read and the identical fields-mode preflight read so they use the same scope as the transactional mutation.
Useful? React with 👍 / 👎.
|
Resolved the conflicts with current Validation on the merged tree:
|
|
Hi @danny-avila, could you take a look at this PR |
2 participants
Summary
Adds atomic, versioned mutation and revision rollback for database-backed admin configuration.
expectedVersionandexpectedTenantId; return409for stale configuration or tenant context,400for invalid mutations, and404for missing revisions.Scope
This is a large PR. It changes how configuration is saved, who can access it, how secrets are handled, and how MongoDB is deployed. These changes support safe configuration updates, but they also make the PR harder to review and require a careful rollout.
Paired admin-panel PR: LibreChat-AI/admin-panel#143
The panel now uses authenticated backend APIs for snapshots and revision history; it no longer connects directly to MongoDB. No new backend dependencies are introduced.
Deployment and compatibility
Recreaterollout; old and new writers must not coexist during this transition.mongodb.architectureandlibrechat.mongoArchitectureAck. Existing standalone-to-replica-set upgrades require a backup and validated PVC/data migration; changing the chart setting does not migrate data automatically.helm/librechat/readme.mdfor the deployment procedure.Change Type
Latest validation
The latest focused review passed:
packages/data-schemas: 6 suites, 195 tests covering override policy, indexed paths, resolution, atomic transactions, config methods, and tenant isolation.packages/api: 9 suites, 765 tests covering config handlers, real atomic transactions, secrets, Langfuse, grants, app configuration, and tenant middleware.tsc --noEmitinpackages/apiandpackages/data-schemas.git diff --check.Checklist