🔗 feat: Add Granular Access Control to Shared Links via ACL System#13051
Conversation
|
@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: d785e7ebfc
ℹ️ 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".
|
Note, this is relevant: #13062 There should also be a permission gate |
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
from
d785e7e to
bd494fc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
LibreChat/api/server/routes/convos.js
Line 149 in 880d63d
Clean up shared-link ACLs on delete-all conversations
The /api/convos/all path still calls db.deleteAllSharedLinks(req.user.id) instead of the new cleanup helper that also removes ACL entries. After deleting all conversations, shared-link documents are removed but their ACL rows remain orphaned, causing permission data drift and accumulating stale ACL records over time.
ℹ️ 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 please review all changes again, because of force push. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 880d63d9ee
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e3e1996e1
ℹ️ 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 share = (await SharedLink.findOne({ conversationId, user }) | ||
| .select('shareId _id') | ||
| .lean()) as { shareId?: string; _id?: import('mongoose').Types.ObjectId } | null; |
There was a problem hiding this comment.
getSharedLink() now returns the shared link _id, which the UI immediately uses to compute ACL bits before showing “Manage Access”, but this path never runs the legacy ACL migration. For pre-migration links (no AclEntry rows yet), the owner receives zero effective permissions and the manage-access control stays hidden until someone visits the public share URL first. Triggering the same legacy-migration logic during owner lookup avoids this regression for existing links.
Useful? React with 👍 / 👎.
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
2 times, most recently
from
e00dd80 to
07bcffd
Compare
|
@codex review |
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
from
07bcffd to
53a53c0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07bcffd3e6
ℹ️ 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".
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
3 times, most recently
from
d1513e8 to
7878f50
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7878f502fb
ℹ️ 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".
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
3 times, most recently
from
ac0cac1 to
278cec0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278cec0fc7
ℹ️ 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".
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
from
278cec0 to
f2275a8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2275a8d6d
ℹ️ 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".
AtefBellaaj
force-pushed
the
feat/shared-links-access-control
branch
from
f2275a8 to
462b651
Compare
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4968fdf91c
ℹ️ 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 SharedLink = mg.models.SharedLink as Model<RawSharedLink>; | ||
| const rawShare = (await SharedLink.findOne({ shareId }).lean()) as RawSharedLink | null; |
There was a problem hiding this comment.
In deployments with TENANT_ISOLATION_STRICT=true, anonymous public-link requests reach this middleware without a tenant in ALS, but SharedLink has the tenant-isolation plugin applied (packages/data-schemas/src/models/sharedLink.ts) and its pre('findOne') hook throws when no tenant context exists. Because the first lookup runs before runWithTenant, public shared links return a 500 instead of loading; perform this shareId lookup in an explicit system/cross-tenant context or via a raw collection query before restoring rawShare.tenantId for ACL checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c1ec4b5: the initial shared-link lookup now runs under the request tenant when present, or under runAsSystem when no tenant context exists, and no-tenant legacy rows continue under system context for ACL/message checks.
| } else if (resourceType === ResourceType.SHARED_LINK) { | ||
| middleware = canAccessResource({ | ||
| resourceType: ResourceType.SHARED_LINK, | ||
| requiredPermission, | ||
| resourceIdParam: 'resourceId', | ||
| }); |
There was a problem hiding this comment.
Adding sharedLink to the generic permissions route exposes the existing PUT /api/permissions/:resourceType/:resourceId updater for shared links, but the route only verifies the caller currently has SHARE and then passes arbitrary updated/removed principals to bulkUpdateResourcePermissions. For shared links, a caller with owner permissions can still demote/remove the owner or assign sharedLink_owner to another principal by calling the API directly, despite the new shared-link flow relying on OWNER being protected; add a server-side guard for ResourceType.SHARED_LINK before delegating to the generic updater.
Useful? React with 👍 / 👎.
| const checkSharedLinksAccess = generateCheckAccess({ | ||
| permissionType: PermissionTypes.SHARED_LINKS, | ||
| permissions: [Permissions.CREATE], | ||
| getRoleByName, |
There was a problem hiding this comment.
This middleware is reused for PATCH and DELETE below, so when an admin disables sharedLinks.create to stop new link creation, users are also blocked from refreshing or deleting shared links they already own even though the route methods still enforce ownership by shareId and user. That leaves migrated or previously created links stuck until the role is allowed to create new links again; gate only POST on CREATE, and use the appropriate ownership/share check for modifying existing links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c1ec4b5: CREATE role gating remains on POST only; PATCH and DELETE now rely on authenticated ownership/resource checks so existing links can still be refreshed or deleted after new creation is disabled.
| } | ||
| } | ||
| } | ||
| const needsPublicGrant = share.isPublic !== false && !!share.user; |
There was a problem hiding this comment.
For legacy SharedLink documents that have isPublic: true but no user field (the schema allows this and the bulk migration explicitly counts them as missingUserWarnings), this condition skips the PUBLIC grant and the function later unsets isPublic, so the first anonymous visit permanently converts a formerly public link into one that returns 401/403. The public viewer grant should depend on the legacy public flag, not on whether an owner can also be granted.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // --- Count totals for progress reporting --- | ||
| const totalLinks = await SharedLink.countDocuments({}); |
There was a problem hiding this comment.
With TENANT_ISOLATION_STRICT=true, SharedLink has tenant-isolation query middleware, so this cross-tenant migration’s first model call without ALS tenant context throws before the dry run or live migration can inspect any rows. Since the script is intended to scan all tenants, wrap the Mongoose model operations in runAsSystem or use the raw collection consistently.
Useful? React with 👍 / 👎.
| if (share._id && share.success) { | ||
| ensureLinkPermissions(share._id, req.user.id).catch(() => {}); |
There was a problem hiding this comment.
This fire-and-forget backfill races the UI path that immediately calls /api/permissions/sharedLink/:id/effective to decide whether to show Manage Access. For legacy links without an OWNER ACL yet, the response can reach the client before ensureLinkPermissions has inserted the ACL entry, so the effective-permissions query returns 0 and the Manage Access button stays hidden until a later refetch/reopen; await the backfill (or create it inside getSharedLink) before sending the _id response.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa9742de6c
ℹ️ 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 grantsOwner = updated.some( | ||
| (principal) => principal?.accessRoleId === AccessRoleIds.SHARED_LINK_OWNER, | ||
| ); |
There was a problem hiding this comment.
This guard only checks updated, but the controller later turns public: true plus publicAccessRoleId into a PUBLIC ACL entry. If a caller sends publicAccessRoleId: sharedLink_owner, the public principal gets owner bits, and because ACL checks include the PUBLIC principal for authenticated users, other users can satisfy SHARE and manage this shared link's permissions. Please also reject req.body.publicAccessRoleId === AccessRoleIds.SHARED_LINK_OWNER for shared links.
Useful? React with 👍 / 👎.
| {canCreateSharedLinks && ( | ||
| <div className="pb-3"> | ||
| <SharedLinks /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Gating the entire Shared Links settings panel on CREATE means that when an admin revokes shared-link creation from a role, users who already have shared links lose the UI to view or delete those existing links. The server-side delete route was explicitly left outside the CREATE gate, so this leaves stale links active unless the user calls the API directly; the list/delete panel should remain accessible when shared links exist even if creating new ones is disabled.
Useful? React with 👍 / 👎.
| const isPublicFalseCount = await rawCollection.countDocuments({ isPublic: false }); | ||
| if (isPublicFalseCount > 0 && !force) { |
There was a problem hiding this comment.
Because this abort check runs before the dryRun branch and ignores dryRun, npm run migrate:shared-link-permissions:dry-run exits as aborted whenever any legacy isPublic: false link exists. That prevents operators from using the dry-run report below to assess how many private links they have before deciding whether to run with --force; the abort should only block the live migration path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b99a27a70
ℹ️ 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 publicIds: Types.ObjectId[] = await this._dbMethods.findPublicResourceIds( | ||
| resourceType, | ||
| PermissionBits.VIEW, | ||
| ); |
There was a problem hiding this comment.
When /api/share/:shareId checks a public shared link, this call materializes every public resource ID for the resource type before the caller scans for the one requested link. On installations with many public shared links, each share view becomes O(all public links) and allocates a large array on the hot path; query for the single (resourceType, resourceId, PUBLIC, VIEW) ACL entry instead.
Useful? React with 👍 / 👎.
| [ResourceType.MCPSERVER]: PermissionTypes.MCP_SERVERS, | ||
| [ResourceType.REMOTE_AGENT]: PermissionTypes.REMOTE_AGENTS, | ||
| [ResourceType.SKILL]: PermissionTypes.SKILLS, | ||
| [ResourceType.SHARED_LINK]: PermissionTypes.SHARED_LINKS, |
There was a problem hiding this comment.
With shared links added to the generic share policy, a user who has SHARE but not SHARE_PUBLIC can still add a PUBLIC viewer by sending it in updated and omitting public; checkSharePublicAccess only gates req.body.public, while updateResourcePermissions grants PUBLIC principals from updated. This bypasses the new shared-link public-sharing restriction whenever admins disable SHARE_PUBLIC, so PUBLIC entries in updated need the same gate or should be rejected.
Useful? React with 👍 / 👎.
| expiredAt, | ||
| ); | ||
| if (created) { | ||
| await grantCreationPermissions(created._id, req.user.id, grantPublic); |
There was a problem hiding this comment.
When this route creates a retained share with expiredAt, the SharedLink document is later removed by the schema TTL index, but the ACL entries granted here have no matching TTL or delete hook. Those orphan USER/PUBLIC ACLs accumulate and continue to appear in permission scans after the link is gone; either expire the ACL entries with the link or run cleanup when TTL-based deletion occurs.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ 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". |
danny-avila
changed the title
…anny-avila#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>
2 participants
Summary
Adds granular access control to shared links by integrating them into the existing ACL permission system. Previously, shared links were globally visible with no per-link access control (a boolean
isPublicfield, always true). This feature lets users share conversation links with specific users, groups, roles, or everyone — using the same principal system as agents, prompts, and skills.Key behaviors:
CREATE: true, SHARE: true, SHARE_PUBLIC: true— all sharing features are enabled out of the box. Admins can restrict permissions per-role vialibrechat.yamlor the admin panel:CREATEgates link creation,SHAREgates the "Manage Access" UI, andSHARE_PUBLICcontrols whether links are created public by default and whether users can toggle the "shared with everyone" switch.SHARE_PUBLIC: true(preserving current default-public behavior). WhenSHARE_PUBLIC: false, links are created private — the creator must explicitly share via "Manage Access." Both grants roll back on failure.GET /link/:conversationId) also ensures OWNER permission exists, so the "Manage Access" button appears immediately without requiring a public visit first. A bulk migration script is also available for pre-migration.isPublicboolean field is removed from SharedLink documents — "shared with everyone" is now derived from the presence of a PUBLIC AclEntry.Important notes:
Tenant Context in Shared Link Access
Shared links require special tenant handling because they support anonymous/public access.
Normal routes (prompts, agents): requireJwtAuth → tenantContextMiddleware sets tenant from user's JWT. All downstream queries are automatically tenant-scoped.
Shared link viewing: Anonymous users have no JWT, so no tenant context exists. But:
Solution: After fetching the SharedLink (cross-tenant lookup by shareId), we restore tenant context from the document via tenantStorage.run({ tenantId: rawShare.tenantId }). This ensures AclEntry permission checks are correctly tenant-scoped.
For authenticated users, optionalJwtAuth already sets tenant context from their JWT — but since cross-tenant sharing is prevented upstream (people picker is tenant-filtered), using the document's tenantId is equivalent and maintains consistency.
Change Type
Testing
Automated tests
All test suites pass with zero regressions:
New test files:
packages/api/src/share/service.test.ts: 10 tests — grantCreationPermissions (happy path, rollback), ensureOwnerPermission (creation, idempotency, no-rollback), delete-with-cleanup variantspackages/api/src/share/access.test.ts: 11 tests — resolveShareAccess input validation, public links (anonymous + authenticated), private links (401/403/200), legacy auto-migration (public/private/disabled)Test areas covered
_idreturn assertionsdeleteAllSharedLinksWithCleanupanddeleteConvoSharedLinksWithCleanupManual testing checklist
SHARE_PUBLIC: true)SHARE_PUBLIC: false→ verify only OWNER AclEntry created (private link)SHARE: true, verify "Manage Access" button appears on existing linkSHARED_LINKS_AUTO_MIGRATE=false→ verify legacy links return 403 until bulk migration runsnode config/migrate-shared-link-permissions.js --dry-run→ verify report without writesisPublicfield$unsetfrom all SharedLink documentsCREATE: false, verify share button is hidden in conversation menu and chat headerTest Configuration
ALLOW_SHARED_LINKS_PUBLIC: controls whether "shared with everyone" links allow anonymous accessSHARED_LINKS_AUTO_MIGRATE: controls auto-migration of legacy links (default: true)Checklist