4 commits

Author	SHA1	Message	Date
Atef Bellaaj	86fe79c37d	🔗 feat: Add Granular Access Control to Shared Links via ACL System (#13051) ... * feat: Add granular access control to shared links via ACL system * fix(shared-links): preserve isPublic on failed migration grants Transient ACL failures during auto-migration permanently stranded links — $unset ran unconditionally, removing the legacy flag that triggers retry. Now only $unset isPublic after all grants succeed. * fix(config): skip isPublic unset for failed ACL grants Bulk migration unconditionally removed isPublic from all links, even those whose ACL writes failed. Failed links then lost the legacy marker needed for auto-migration retry. Now tracks failed link IDs per-batch and excludes them from the $unset step. Also adds sharedLink to AccessRole resourceType schema enum — was missing, only worked because seedDefaultRoles uses findOneAndUpdate which bypasses validation. * ci(config): add jest config and PR workflow for migration tests config/__tests__/ specs depend on api/jest.config.js module mappings but had no dedicated runner. Adds config/jest.config.js extending api config with absolutized paths, npm test:config script, and a GitHub Actions workflow triggered by changes to config/, api/models/, api/db/, or packages/ ACL code. * fix(permissions): honor boolean sharedLinks config SHARED_LINKS has no USE permission, so boolean config produced an empty update payload — gate conditions only matched object form, making `sharedLinks: false` a no-op on existing perms. * fix(share): resolve role before creating shared link Role lookup between create and grant left an orphaned link without ACL entries if getRoleByName threw — retry then hit "Share already exists" with no recovery path. * fix: Restore Public ACL Access Checks * fix: Type Public ACL Lookup * fix: Preserve Private Legacy Shared Links * chore: Promote Shared Link Permission Migration * fix: Address Shared Link Review Findings * fix: Repair Shared Link CI Follow-Up * fix: Narrow Shared Link Mongoose Test Mock * fix: Address Shared Link Review Follow-Ups * fix: Close Shared Link Review Gaps * fix: Guard Missing Shared Link Permission Backfill * test: Add Shared Link Mock E2E * test: Stabilize Shared Link Mock E2E --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-06-03 14:17:17 -04:00
Danny Avila	749eb06e67	🧭 fix: Reduce MCP Registry ACL Lookups (#13195)	2026-05-19 17:16:37 -04:00
Danny Avila	3bd2681272	🪐 fix: Replace $bitsAllSet ACL Queries for Azure Cosmos DB Compatibility (#12736) ... * 🐛 fix: replace `$bitsAllSet` ACL queries for Cosmos DB compatibility Azure Cosmos DB for MongoDB API does not implement the `$bitsAllSet` query operator, so every permission check against Cosmos DB threw. The five read paths in `aclEntry.ts` (`hasPermission`, `findAccessibleResources`, `findPublicResourceIds`, and two sites in `getSoleOwnedResourceIds`) now fetch candidate entries and apply the bitwise mask in application code. This matches the existing FerretDB-compatible pattern. Fixes #12729. * 🐛 fix: delegate `findPubliclyAccessibleResources` to fixed DB method `AccessControlService.findPubliclyAccessibleResources` inlined the same `$bitsAllSet` query as the data-schemas layer, which fails on Azure Cosmos DB for MongoDB. Delegate to `_dbMethods.findPublicResourceIds` so a single implementation carries the Cosmos-compatible bitwise logic. Refs #12729. * 🐛 fix: move `$bitsAllSet` filter out of remote-agent aggregation `enrichRemoteAgentPrincipals` used `$bitsAllSet` inside an aggregation `$match`, which Azure Cosmos DB for MongoDB does not implement. Project `permBits` through the pipeline and filter for `PermissionBits.SHARE` in application code. The extra documents fetched are bounded by ACL entries on a single agent resource, so the cost is negligible. Refs #12729. * 🧪 test: rename misleading public-dedup test and add real dedup coverage The test previously named "returns deduplicated IDs even if the public principal has multiple entries" only set up a single ACL entry, so it did not actually exercise deduplication. Split into two tests: one for the happy path (single entry with required bits), and one that bypasses `grantPermission`'s upsert via `AclEntry.create` to confirm the application-layer dedup Map handles genuine duplicates. Refs #12729. * 🧪 test: cover SHARE-bit filter in `enrichRemoteAgentPrincipals` The `$bitsAllSet` match stage in `enrichRemoteAgentPrincipals` previously guaranteed every aggregation row had SHARE; the Cosmos DB fix moved that check into a JS `continue` branch with no direct coverage. Add a dependency-injected unit test that stubs the aggregation with mixed SHARE / non-SHARE / zero-bit rows and asserts only SHARE holders are enriched and queued for backfill. Also includes a regression guard that the `$match` pipeline stage no longer contains a `permBits` filter. Refs #12729. * ♻️ refactor: extract `filterByBitsAndDedup` helper for ACL reads `findAccessibleResources` and `findPublicResourceIds` each inlined the same bitmask-filter + `Map`-based dedup loop. Lift it into a private `filterByBitsAndDedup(entries, requiredBits)` helper so the Cosmos-DB compatible pattern lives in one place. Pure rename/extract — no behavior change. Refs #12729. * 📝 docs: fix stale `\$bitsAllSet` references in FerretDB spec The describe block and header comment in the FerretDB parity spec still referenced `\$bitsAllSet queries` after the Cosmos DB compatibility fix moved the bit mask into application code. Update the title to \"Bitwise permission queries\" and rewrite the header comment to describe the application-layer behavior being validated. Refs #12729. * ⚡ perf: push permission-mask filter back to the database via `$in` The original fix for #12729 moved `$bitsAllSet` filtering into application code, which meant every ACL read fetched the full set of rows for a principal/resource and filtered in JS. For tenants with large ACL collections this inflates wire transfer and heap. Replace the JS filter with `permBits: { $in: permissionBitSupersets(X) }`. For the 4-bit `PermissionBits` enum the `$in` list is at most 16 values (8 for a single-bit mask like SHARE). `$in` is indexable and supported by Azure Cosmos DB for MongoDB, so the filter runs on the server again — restoring `.distinct('resourceId')` and `findOne()` semantics. `permissionBitSupersets(requiredBits)` is memoized and exported from `@librechat/data-schemas`. Callers restored: - `hasPermission`: back to `findOne` short-circuit - `findAccessibleResources` / `findPublicResourceIds`: back to `.distinct()` - `getSoleOwnedResourceIds`: back to the `$match` + `$group` aggregation - `enrichRemoteAgentPrincipals`: bit filter back in `$match`, JS `continue` removed Refs #12729. * 🧪 test: add `\$bitsAllSet` vs `\$in` parity + perf spec Introduces `aclEntry.parity.spec.ts` — a side-by-side spec that runs the legacy `\$bitsAllSet` query and the current `\$in`-based query against the same `mongodb-memory-server` fixture and asserts identical output sets for every affected method (`hasPermission`, `findAccessibleResources`, `findPublicResourceIds`, `getSoleOwnedResourceIds`) across all 7 meaningful permBits combinations. Also logs median wall-clock time for the two query paths over 20 runs on an 800-entry fixture, with a loose 3x guard against catastrophic regressions. Initial local numbers: 1.05 ms vs 1.07 ms (findAccessibleResources), 1.10 ms vs 1.05 ms (findPublicResourceIds). Refs #12729. * 🔒 hardening: freeze `permissionBitSupersets` cache + enum-shape guard Two defensive changes from the comprehensive audit: * Cached superset arrays are now `Object.freeze`d and the return type is `readonly number[]`. Previously the cached arrays were returned by mutable reference, so a caller that mutated the result would silently corrupt the process-wide cache for every subsequent permission check. `Object.freeze` turns that into a loud `TypeError` at the mutation site. All existing call sites pass the result directly to Mongoose's `$in`, which does not mutate. * Added a module-load guard `if (MAX_PERM_BITS === 0) throw`. If `PermissionBits` is ever refactored to a `const` object or string enum, `Object.values(...).filter(isNumber)` would return `[]` and `MAX_PERM_BITS` would silently become 0, making every query match no rows and breaking every permission check. The guard fails loudly instead. Also collapsed four identical JSDoc lines across `hasPermission`, `findAccessibleResources`, `findPublicResourceIds`, and `getSoleOwnedResourceIds` into a single `{@link permissionBitSupersets}` reference. Refs #12729. * 🧪 test: add focused unit tests for `permissionBitSupersets` The helper is the single point of correctness for every ACL read path (every query uses `permBits: { $in: permissionBitSupersets(X) }`), so it warrants direct coverage independent of the higher-level parity and behavior specs. Six cases added: * `requiredBits=0` returns all 16 values * `requiredBits=15` returns `[15]` only * every returned value is a bitwise superset of `requiredBits` * full parity against the `$bitsAllSet` definition for every `required` in 0..15 * memoization: repeat calls return the same frozen reference * frozen result throws `TypeError` on mutation attempts Refs #12729. * 🧪 test: tighten parity perf guard and document fixture constants The `expect(currentMs).toBeLessThan(legacyMs * 3 + 50)` form was dominated by the `+ 50` additive term at typical sub-ms query latencies — at legacy=1ms, a 50x regression would still pass. Replace with `Math.max(legacyMs * 5, 50)` so the multiplicative ceiling is intact once the new path climbs out of the fixed noise floor. Also added inline rationale for the `FIXTURE_SIZE = 800` and `PERF_ITERATIONS = 20` constants. Refs #12729. * 🧹 chore: remove stale perf-guard comment, hoist rationale to describe Commit a75f122 left a "Sanity check: A 3x multiplier" comment block above the first perf guard, and 21852ac layered a second "Multiplicative-dominant guard" block directly below it. The stale block described the `legacyMs * 3 + 50` formula that no longer exists, so both blocks coexisted and contradicted each other. Delete the stale block from the first test, remove the redundant copy from the second test, and lift the (now-single) rationale to the enclosing `describe('performance')` block. Each test is now one `expect` line — the rationale lives once, at the scope it applies to. Refs #12729. * 📝 docs: sharpen MAX_PERM_BITS guard message + test-sort consistency Two NITs from the follow-up audit: * The `MAX_PERM_BITS === 0` guard now names the specific refactors that could cause it (const object / string enum / all-string shape) and gives two concrete remediation paths (rewrite the reducer, or give `permissionBitSupersets` an explicit bit-width parameter). Previously the message just said "update permissionBitSupersets before continuing", which was vague. * The `requiredBits=15` unit test now applies the same `[...result].sort((a, b) => a - b)` normalization as the other tests so the set-equality assertions are uniform. The function happens to return values in ascending order, but the helper's JSDoc does not promise ordering, so sorting before comparing is the correct defensive pattern. Refs #12729. * 🔒 fix: enforce `permBits <= MAX_PERM_BITS` at the schema level Codex flagged a real silent regression: `permissionBitSupersets` enumerates `$in` candidates only in `[0, MAX_PERM_BITS]` (currently 15), so any ACL row with bits above the enum — e.g. `permBits = 31` from a future role or a manual DB write — would be excluded from reads that `$bitsAllSet` would have matched. Current write paths only produce values in `[0, 15]` via the `PermissionBits` / `RoleBits` enums, so no live data is affected, but the schema did not prevent out-of-range writes, so the divergence was reachable. Fix: add `min: 0`, `max: MAX_PERM_BITS`, and an `isInteger` validator to the `permBits` field in the AclEntry schema. `MAX_PERM_BITS` is derived from `PermissionBits` the same way `permissionBitSupersets` computes it, so when a new bit is added to the enum both the read-side enumeration and the write-side bound expand together. Tests: four new cases cover the upper bound, over-limit, negative, and non-integer inputs, each asserting `Mongoose.Error.ValidationError`. Plus updated JSDoc on `permissionBitSupersets` to document the invariant that the schema now enforces. Refs #12729. * ♻️ refactor: hoist `MAX_PERM_BITS` + enum-shape/ceiling guards to shared util Addresses one MINOR DRY finding and one NIT defensive-guard finding from the latest audit. `MAX_PERM_BITS` was duplicated in `methods/aclEntry.ts` and `schema/aclEntry.ts` with identical computations; they could silently diverge. Move the constant plus both module-load guards to `src/common/permissions.ts`: * `MAX_PERM_BITS === 0` guard — fail loudly if `PermissionBits` is ever refactored to a `const` object or string enum (applied in both sites now, not just methods). * `MAX_PERM_BITS > 255` guard — circuit-breaker for the `$in` enumeration strategy. At 4 bits the list tops out at 16; at 8 bits 256. Beyond that the approach degrades, so fail at module load rather than emit an unusably-large `$in` list. Both the schema and the methods file now import from the single `src/common/permissions.ts`. `readonly number[]` return type and `Object.freeze` cache-value protection from the prior commits are preserved. Refs #12729. * 🔒 fix: reject out-of-range `requiredBits` without caching (DoS fix) Codex flagged a real unbounded-cache DoS vector: `api/server/controllers/agents/v1.js` parses `req.query.requiredPermission` via `parseInt` and forwards it unvalidated to `findAccessibleResources`, which eventually calls `permissionBitSupersets(requiredBits)`. Because the old code memoized *every* distinct input in a process-global Map, an attacker could flood the cache with unique integers and grow memory without bound. Fix: when `requiredBits` is not an integer, is negative, or has any bits set above `MAX_PERM_BITS`, return a single shared frozen empty array and do NOT touch the cache. An empty `$in` list correctly matches zero rows (rows cannot satisfy a bit we do not support), so the response is also semantically correct — previously it "worked" only because invalid inputs coincidentally expanded to `[]` too, but at the cost of a cache entry each time. Five new tests exercise the rejection path: upper-bound overflow, mixed in-range+out-of-range bits, shared-instance identity across rejected inputs, a 2000-unique-value cache-growth probe, and a regression check that legitimate in-range inputs still get memoized normally. Refs #12729. * 🧪 test: probe `Map.prototype.set` to prove cache doesn't grow on rejection Strengthens the DoS cache-growth test the review pass flagged: reference identity alone allows a hypothetical regression like `supersetCache.set(requiredBits, EMPTY_SUPERSETS); return EMPTY_SUPERSETS;` to pass while still leaking one entry per attacker request. Add a second test that spies on `Map.prototype.set`, records the global call count before and after a burst of 1500 rejected inputs (500 each of over-MAX integers, negatives, and non-integers), and asserts the delta is zero. Manually verified the test has teeth: injecting the hypothetical regression in the implementation produced exactly 1500 extra `Map.set` calls and the new test failed as expected; reverting restored the clean state. Renames the existing identity-based test to `(reference identity)` so its scope is clear alongside the new `(Map-write probe)` companion. Refs #12729.	2026-04-19 22:28:48 -04:00
Atef Bellaaj	99f8bd2ce6	🏗️ feat: Dynamic MCP Server Infrastructure with Access Control (#10787) ... * Feature: Dynamic MCP Server with Full UI Management * 🚦 feat: Add MCP Connection Status icons to MCPBuilder panel (#10805) * feature: Add MCP server connection status icons to MCPBuilder panel * refactor: Simplify MCPConfigDialog rendering in MCPBuilderPanel --------- Co-authored-by: Atef Bellaaj <slalom.bellaaj@external.daimlertruck.com> Co-authored-by: Danny Avila <danny@librechat.ai> * fix: address code review feedback for MCP server management - Fix OAuth secret preservation to avoid mutating input parameter by creating a merged config copy in ServerConfigsDB.update() - Improve error handling in getResourcePermissionsMap to propagate critical errors instead of silently returning empty Map - Extract duplicated MCP server filter logic by exposing selectableServers from useMCPServerManager hook and using it in MCPSelect component * test: Update PermissionService tests to throw errors on invalid resource types - Changed the test for handling invalid resource types to ensure it throws an error instead of returning an empty permissions map. - Updated the expectation to check for the specific error message when an invalid resource type is provided. * feat: Implement retry logic for MCP server creation to handle race conditions - Enhanced the createMCPServer method to include retry logic with exponential backoff for handling duplicate key errors during concurrent server creation. - Updated tests to verify that all concurrent requests succeed and that unique server names are generated. - Added a helper function to identify MongoDB duplicate key errors, improving error handling during server creation. * refactor: StatusIcon to use CircleCheck for connected status - Replaced the PlugZap icon with CircleCheck in the ConnectedStatusIcon component to better represent the connected state. - Ensured consistent icon usage across the component for improved visual clarity. * test: Update AccessControlService tests to throw errors on invalid resource types - Modified the test for invalid resource types to ensure it throws an error with a specific message instead of returning an empty permissions map. - This change enhances error handling and improves test coverage for the AccessControlService. * fix: Update error message for missing server name in MCP server retrieval - Changed the error message returned when the server name is not provided from 'MCP ID is required' to 'Server name is required' for better clarity and accuracy in the API response. --------- Co-authored-by: Atef Bellaaj <slalom.bellaaj@external.daimlertruck.com> Co-authored-by: Danny Avila <danny@librechat.ai>	2025-12-11 16:38:37 -05:00