🔒 fix: Prevent Role Grants on Shared Links (#14137)

* 🔒 fix: Prevent Cross-Tenant Role Grants on Shared Links

The shared-link access middleware falls back to a system-wide lookup so a
share owned by another tenant still resolves, then evaluates the ACL under
the share owner's tenant. It was passing the viewer's `role` from their own
tenant into that check. Role ACL principals are unqualified name strings, so
a tenant-B user with role USER could satisfy a tenant-A private share granted
to role USER (default role names collide across every tenant).

Pass the viewer's role only for same-tenant views; use `null` for cross-tenant
views so getUserPrincipals never builds a ROLE principal. `null` (not
`undefined`) is required: `undefined` re-derives the role via an unscoped
User lookup under runAsSystem for tenantId-less shares, reopening the bypass.

Widen checkPermission's `role` param to `string | null`. Adds regression
coverage for both the cross-tenant denial and same-tenant allow paths.

* 🔒 fix: Use authenticated user tenant for same-tenant role trust

Codex P2: share file routes (<img>/download) authenticate via
optionalShareFileAuth from the refresh cookie and never establish tenant ALS
context, so getTenantId() is undefined there. Comparing the share tenant to the
ALS context wrongly denied a legitimate same-tenant viewer their ROLE grant on
file/preview/download requests, even though the main share route authorized them.

Compare the share tenant against the authenticated user's own tenantId, which is
populated in both the JWT and cookie-auth paths (tenantContextMiddleware itself
derives the ALS tenant from user.tenantId, and req.tenantId is never set, so
user.tenantId is the authoritative, non-spoofable source). Adds a regression test
for the no-ALS-context cookie-auth file request.

packages/api/src/acl/accessControlService.ts

@@ -351,7 +351,7 @@ export class AccessControlService {
     requiredPermission,
   }: {
     userId: string;
-    role?: string;
+    role?: string | null;
     resourceType: ResourceType;
     resourceId: string | Types.ObjectId;
     requiredPermission: number;

packages/api/src/shared-links/access.test.ts

@@ -6,7 +6,13 @@ jest.mock('@librechat/data-schemas', () => ({
 import mongoose, { Types, Model } from 'mongoose';
 import { MongoMemoryServer } from 'mongodb-memory-server';
 import { createModels, createMethods, tenantStorage } from '@librechat/data-schemas';
-import { ResourceType, PrincipalType, AccessRoleIds } from 'librechat-data-provider';
+import {
+  ResourceType,
+  PrincipalType,
+  PrincipalModel,
+  AccessRoleIds,
+  PermissionBits,
+} from 'librechat-data-provider';
 import type { Request, Response, NextFunction } from 'express';
 import type { IAclEntry, ISharedLink } from '@librechat/data-schemas';
 import { AccessControlService } from '~/acl/accessControlService';
@@ -105,6 +111,33 @@ async function grantUserViewer(resourceId: Types.ObjectId, uid: Types.ObjectId)
   });
 }
 
+/**
+ * Inserts a ROLE VIEW grant directly (bypassing the role-name lookup, which is
+ * tenant-scoped and would miss the globally seeded roles under a tenant context).
+ * Inherits the caller's tenant context so the entry is tenant-scoped on save.
+ */
+async function grantRoleViewer(resourceId: Types.ObjectId, role: string) {
+  await new AclEntry({
+    principalType: PrincipalType.ROLE,
+    principalModel: PrincipalModel.ROLE,
+    principalId: role,
+    resourceType: ResourceType.SHARED_LINK,
+    resourceId,
+    permBits: PermissionBits.VIEW,
+    grantedBy: userId,
+  }).save();
+}
+
+/** Mirrors getUserPrincipals: a ROLE principal is only added when a role is supplied. */
+function mockPrincipalsWithRole(viewer: Types.ObjectId) {
+  mockGetUserPrincipals.mockImplementation(({ role }: { role?: string | null }) =>
+    Promise.resolve([
+      { principalType: PrincipalType.USER, principalId: viewer },
+      ...(role ? [{ principalType: PrincipalType.ROLE, principalId: role }] : []),
+    ]),
+  );
+}
+
 describe('canAccessSharedLink', () => {
   describe('input validation', () => {
     test('returns 400 when shareId is missing', async () => {
@@ -241,6 +274,98 @@ describe('canAccessSharedLink', () => {
   });
 
   describe('cross-tenant lookup', () => {
+    test('does not let a cross-tenant viewer role satisfy a ROLE ACL from another tenant', async () => {
+      // Share owned by tenant-a, granted VIEW to role USER. A tenant-b viewer whose
+      // own role is also USER must not inherit that grant: role principals are just
+      // name strings, so the middleware suppresses the viewer's role for cross-tenant
+      // views. getUserPrincipals is called with role: null, so no ROLE principal is
+      // built and the tenant-a ROLE:USER grant never matches → 403.
+      const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => {
+        const tenantLink = await createTestLink();
+        await grantRoleViewer(tenantLink._id, 'USER');
+        return tenantLink;
+      });
+      const viewer = new Types.ObjectId();
+      mockPrincipalsWithRole(viewer);
+
+      const req = createReq({
+        params: { shareId: link.shareId },
+        user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-b' },
+      });
+      const res = createRes();
+      const next = jest.fn();
+      await tenantStorage.run({ tenantId: 'tenant-b' }, () =>
+        canAccessSharedLink(req, res, next as unknown as NextFunction),
+      );
+
+      expect(next).not.toHaveBeenCalled();
+      expect(res._status).toBe(403);
+      expect(mockGetUserPrincipals).toHaveBeenCalledWith({
+        userId: viewer.toString(),
+        role: null,
+      });
+    });
+
+    test('still honors a ROLE ACL for a same-tenant viewer with that role', async () => {
+      // Same-tenant view: the viewer's role is trusted, so the tenant-a ROLE:USER
+      // grant matches and access is allowed. Confirms the cross-tenant fix does not
+      // break legitimate role-based sharing within a tenant.
+      const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => {
+        const tenantLink = await createTestLink();
+        await grantRoleViewer(tenantLink._id, 'USER');
+        return tenantLink;
+      });
+      const viewer = new Types.ObjectId();
+      mockPrincipalsWithRole(viewer);
+
+      const req = createReq({
+        params: { shareId: link.shareId },
+        user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-a' },
+      });
+      const res = createRes();
+      const next = jest.fn();
+      await tenantStorage.run({ tenantId: 'tenant-a' }, () =>
+        canAccessSharedLink(req, res, next as unknown as NextFunction),
+      );
+
+      expect(next).toHaveBeenCalled();
+      expect((req as unknown as Record<string, unknown>).shareResourceId).toBe(link._id.toString());
+      expect(mockGetUserPrincipals).toHaveBeenCalledWith({
+        userId: viewer.toString(),
+        role: 'USER',
+      });
+    });
+
+    test('honors a same-tenant ROLE grant when no ALS tenant context is set (cookie-auth file request)', async () => {
+      // Share file routes (<img>/download) authenticate via optionalShareFileAuth,
+      // which sets req.user from the refresh cookie but establishes no tenant ALS
+      // context, so getTenantId() is undefined here. The comparison must use the
+      // authenticated user's own tenantId, else a same-tenant viewer is wrongly denied
+      // their ROLE grant. No tenantStorage.run wrapper mirrors the file route.
+      const link = await tenantStorage.run({ tenantId: 'tenant-a' }, async () => {
+        const tenantLink = await createTestLink();
+        await grantRoleViewer(tenantLink._id, 'USER');
+        return tenantLink;
+      });
+      const viewer = new Types.ObjectId();
+      mockPrincipalsWithRole(viewer);
+
+      const req = createReq({
+        params: { shareId: link.shareId },
+        user: { id: viewer.toString(), _id: viewer, role: 'USER', tenantId: 'tenant-a' },
+      });
+      const res = createRes();
+      const next = jest.fn();
+      await canAccessSharedLink(req, res, next as unknown as NextFunction);
+
+      expect(next).toHaveBeenCalled();
+      expect((req as unknown as Record<string, unknown>).shareResourceId).toBe(link._id.toString());
+      expect(mockGetUserPrincipals).toHaveBeenCalledWith({
+        userId: viewer.toString(),
+        role: 'USER',
+      });
+    });
+
     test('resolves a share owned by another tenant via system fallback, then enforces the ACL', async () => {
       // Share created under tenant-a; an authenticated viewer from tenant-b would
       // miss the tenant-scoped lookup and previously get a 404 before access could

packages/api/src/shared-links/access.ts

@@ -56,6 +56,7 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) {
       return;
     }
 
+    const viewerTenantId = getTenantId();
     const SharedLink = mg.models.SharedLink as Model<RawSharedLink>;
     const findShare = async () =>
       (await SharedLink.findOne({
@@ -68,8 +69,8 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) {
     // different tenant — still resolves. Access remains gated by the ACL check
     // below, which runs under the share's own tenant, so this only broadens the
     // lookup, never the authorization.
-    let rawShare = getTenantId() ? await findShare() : await runAsSystem(findShare);
-    if (!rawShare && getTenantId()) {
+    let rawShare = viewerTenantId ? await findShare() : await runAsSystem(findShare);
+    if (!rawShare && viewerTenantId) {
       rawShare = await runAsSystem(findShare);
     }
 
@@ -136,7 +137,10 @@ export function createSharedLinkAccessMiddleware(deps: SharedLinkAccessDeps) {
 
       const hasAccess = await aclService.checkPermission({
         userId,
-        role: user.role,
+        // Trust the viewer's role only for a same-tenant view, comparing the share
+        // tenant to the user's own tenantId (the ALS context is absent on cookie-auth
+        // file requests). null suppresses the ROLE principal for cross-tenant views.
+        role: rawShare.tenantId === user.tenantId ? user.role : null,
         resourceType: ResourceType.SHARED_LINK,
         resourceId,
         requiredPermission: PermissionBits.VIEW,