🗝️ fix: Enforce Skill Share Role Permission (#13062)

* fix: enforce skill share role permission

* fix: preserve share capability bypass

* refactor: move share policy middleware to api package

* style: order share middleware imports

* fix: satisfy share middleware type checks

* test: cover share policy resource types

api/server/middleware/checkSharePublicAccess.js

@@ -1,86 +1,8 @@
-const { logger } = require('@librechat/data-schemas');
-const { ResourceType, PermissionTypes, Permissions } = require('librechat-data-provider');
+const { createSharePolicyMiddleware } = require('@librechat/api');
+const { hasCapability } = require('~/server/middleware/roles/capabilities');
 const { getRoleByName } = require('~/models');
 
-/**
- * Maps resource types to their corresponding permission types
- */
-const resourceToPermissionType = {
-  [ResourceType.AGENT]: PermissionTypes.AGENTS,
-  [ResourceType.PROMPTGROUP]: PermissionTypes.PROMPTS,
-  [ResourceType.MCPSERVER]: PermissionTypes.MCP_SERVERS,
-  [ResourceType.REMOTE_AGENT]: PermissionTypes.REMOTE_AGENTS,
-  [ResourceType.SKILL]: PermissionTypes.SKILLS,
-};
-
-/**
- * Middleware to check if user has SHARE_PUBLIC permission for a resource type
- * Only enforced when request body contains `public: true`
- * @param {import('express').Request} req - Express request
- * @param {import('express').Response} res - Express response
- * @param {import('express').NextFunction} next - Express next function
- */
-const checkSharePublicAccess = async (req, res, next) => {
-  try {
-    const { public: isPublic } = req.body;
-
-    // Only check if trying to enable public sharing
-    if (!isPublic) {
-      return next();
-    }
-
-    const user = req.user;
-    if (!user || !user.role) {
-      return res.status(401).json({
-        error: 'Unauthorized',
-        message: 'Authentication required',
-      });
-    }
-
-    const { resourceType } = req.params;
-    const permissionType = resourceToPermissionType[resourceType];
-
-    if (!permissionType) {
-      return res.status(400).json({
-        error: 'Bad Request',
-        message: `Unsupported resource type for public sharing: ${resourceType}`,
-      });
-    }
-
-    const role = await getRoleByName(user.role);
-    if (!role || !role.permissions) {
-      return res.status(403).json({
-        error: 'Forbidden',
-        message: 'No permissions configured for user role',
-      });
-    }
-
-    const resourcePerms = role.permissions[permissionType] || {};
-    const canSharePublic = resourcePerms[Permissions.SHARE_PUBLIC] === true;
-
-    if (!canSharePublic) {
-      logger.warn(
-        `[checkSharePublicAccess][${user.id}] User denied SHARE_PUBLIC for ${resourceType}`,
-      );
-      return res.status(403).json({
-        error: 'Forbidden',
-        message: `You do not have permission to share ${resourceType} resources publicly`,
-      });
-    }
-
-    next();
-  } catch (error) {
-    logger.error(
-      `[checkSharePublicAccess][${req.user?.id}] Error checking SHARE_PUBLIC permission`,
-      error,
-    );
-    return res.status(500).json({
-      error: 'Internal Server Error',
-      message: 'Failed to check public sharing permissions',
-    });
-  }
-};
-
-module.exports = {
-  checkSharePublicAccess,
-};
+module.exports = createSharePolicyMiddleware({
+  getRoleByName,
+  hasCapability,
+});

api/server/middleware/checkSharePublicAccess.spec.js

@@ -1,8 +1,15 @@
-const { ResourceType, PermissionTypes, Permissions } = require('librechat-data-provider');
-const { checkSharePublicAccess } = require('./checkSharePublicAccess');
-const { getRoleByName } = require('~/models');
+jest.mock('~/models', () => ({
+  getRoleByName: jest.fn(),
+}));
 
-jest.mock('~/models');
+jest.mock('~/server/middleware/roles/capabilities', () => ({
+  hasCapability: jest.fn(),
+}));
+
+const { ResourceType, PermissionTypes, Permissions } = require('librechat-data-provider');
+const { hasCapability } = require('~/server/middleware/roles/capabilities');
+const { checkShareAccess, checkSharePublicAccess } = require('./checkSharePublicAccess');
+const { getRoleByName } = require('~/models');
 
 describe('checkSharePublicAccess middleware', () => {
   let mockReq;
@@ -11,6 +18,7 @@ describe('checkSharePublicAccess middleware', () => {
 
   beforeEach(() => {
     jest.clearAllMocks();
+    hasCapability.mockResolvedValue(false);
     mockReq = {
       user: { id: 'user123', role: 'USER' },
       params: { resourceType: ResourceType.AGENT },
@@ -162,3 +170,137 @@ describe('checkSharePublicAccess middleware', () => {
     });
   });
 });
+
+describe('checkShareAccess middleware', () => {
+  let mockReq;
+  let mockRes;
+  let mockNext;
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    hasCapability.mockResolvedValue(false);
+    mockReq = {
+      user: { id: 'user123', role: 'USER' },
+      params: { resourceType: ResourceType.SKILL },
+      body: { updated: [] },
+    };
+    mockRes = {
+      status: jest.fn().mockReturnThis(),
+      json: jest.fn(),
+    };
+    mockNext = jest.fn();
+  });
+
+  it('should return 403 when user role has no SHARE permission for skills', async () => {
+    getRoleByName.mockResolvedValue({
+      permissions: {
+        [PermissionTypes.SKILLS]: {
+          [Permissions.SHARE]: false,
+          [Permissions.SHARE_PUBLIC]: false,
+        },
+      },
+    });
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(mockRes.status).toHaveBeenCalledWith(403);
+    expect(mockRes.json).toHaveBeenCalledWith({
+      error: 'Forbidden',
+      message: `You do not have permission to share ${ResourceType.SKILL} resources`,
+    });
+    expect(mockNext).not.toHaveBeenCalled();
+  });
+
+  it('should call next() when user has resource management capability for skills', async () => {
+    hasCapability.mockResolvedValue(true);
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(hasCapability).toHaveBeenCalledWith(mockReq.user, 'manage:skills');
+    expect(getRoleByName).not.toHaveBeenCalled();
+    expect(mockNext).toHaveBeenCalled();
+    expect(mockRes.status).not.toHaveBeenCalled();
+  });
+
+  it('should call next() when user role has SHARE permission for skills', async () => {
+    getRoleByName.mockResolvedValue({
+      permissions: {
+        [PermissionTypes.SKILLS]: {
+          [Permissions.SHARE]: true,
+          [Permissions.SHARE_PUBLIC]: false,
+        },
+      },
+    });
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(hasCapability).toHaveBeenCalledWith(mockReq.user, 'manage:skills');
+    expect(mockNext).toHaveBeenCalled();
+    expect(mockRes.status).not.toHaveBeenCalled();
+  });
+
+  it('should return 401 when user is not authenticated', async () => {
+    mockReq.user = null;
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(mockRes.status).toHaveBeenCalledWith(401);
+    expect(mockRes.json).toHaveBeenCalledWith({
+      error: 'Unauthorized',
+      message: 'Authentication required',
+    });
+    expect(mockNext).not.toHaveBeenCalled();
+  });
+
+  it('should return 400 for unsupported resource type', async () => {
+    mockReq.params = { resourceType: 'unsupported' };
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(mockRes.status).toHaveBeenCalledWith(400);
+    expect(mockRes.json).toHaveBeenCalledWith({
+      error: 'Bad Request',
+      message: 'Unsupported resource type for sharing: unsupported',
+    });
+  });
+
+  it('should return 403 when role has no permissions object', async () => {
+    getRoleByName.mockResolvedValue({ permissions: null });
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(mockRes.status).toHaveBeenCalledWith(403);
+    expect(mockNext).not.toHaveBeenCalled();
+  });
+
+  it('should return 500 on error', async () => {
+    getRoleByName.mockRejectedValue(new Error('Database error'));
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+
+    expect(mockRes.status).toHaveBeenCalledWith(500);
+    expect(mockRes.json).toHaveBeenCalledWith({
+      error: 'Internal Server Error',
+      message: 'Failed to check sharing permissions',
+    });
+  });
+
+  it('should reuse the role permission lookup for public sharing checks', async () => {
+    mockReq.body = { updated: [], public: true };
+    getRoleByName.mockResolvedValue({
+      permissions: {
+        [PermissionTypes.SKILLS]: {
+          [Permissions.SHARE]: true,
+          [Permissions.SHARE_PUBLIC]: true,
+        },
+      },
+    });
+
+    await checkShareAccess(mockReq, mockRes, mockNext);
+    await checkSharePublicAccess(mockReq, mockRes, mockNext);
+
+    expect(getRoleByName).toHaveBeenCalledTimes(1);
+    expect(mockNext).toHaveBeenCalledTimes(2);
+    expect(mockRes.status).not.toHaveBeenCalled();
+  });
+});

api/server/routes/accessPermissions.js

@@ -8,9 +8,12 @@ const {
   getResourceRoles,
   searchPrincipals,
 } = require('~/server/controllers/PermissionsController');
+const {
+  checkShareAccess,
+  checkSharePublicAccess,
+} = require('~/server/middleware/checkSharePublicAccess');
 const { requireJwtAuth, checkBan, uaParser, canAccessResource } = require('~/server/middleware');
 const { checkPeoplePickerAccess } = require('~/server/middleware/checkPeoplePickerAccess');
-const { checkSharePublicAccess } = require('~/server/middleware/checkSharePublicAccess');
 const { findMCPServerByObjectId, getSkillById } = require('~/models');
 
 const router = express.Router();
@@ -104,12 +107,13 @@ router.get(
 /**
  * PUT /api/permissions/{resourceType}/{resourceId}
  * Bulk update permissions for a specific resource
- * SECURITY: Requires SHARE permission to modify resource permissions
+ * SECURITY: Requires resource ACL SHARE and role SHARE to modify resource permissions
  * SECURITY: Requires SHARE_PUBLIC permission to enable public sharing
  */
 router.put(
   '/:resourceType/:resourceId',
   checkResourcePermissionAccess(PermissionBits.SHARE),
+  checkShareAccess,
   checkSharePublicAccess,
   updateResourcePermissions,
 );

api/server/routes/accessPermissions.sharePolicy.test.js

@@ -0,0 +1,211 @@
+jest.mock('~/models', () => ({
+  getRoleByName: jest.fn(),
+  findMCPServerByObjectId: jest.fn(),
+  getSkillById: jest.fn(),
+}));
+
+jest.mock('~/server/middleware', () => ({
+  requireJwtAuth: (req, _res, next) => next(),
+  checkBan: (_req, _res, next) => next(),
+  uaParser: (_req, _res, next) => next(),
+  canAccessResource: jest.fn(() => (_req, _res, next) => next()),
+}));
+
+jest.mock('~/server/middleware/checkPeoplePickerAccess', () => ({
+  checkPeoplePickerAccess: jest.fn((_req, _res, next) => next()),
+}));
+
+jest.mock('~/server/middleware/roles/capabilities', () => ({
+  hasCapability: jest.fn(),
+}));
+
+jest.mock('~/server/controllers/PermissionsController', () => ({
+  getUserEffectivePermissions: jest.fn((_req, res) => res.json({ permissions: [] })),
+  getAllEffectivePermissions: jest.fn((_req, res) => res.json({ permissions: [] })),
+  updateResourcePermissions: jest.fn((_req, res) => res.json({ success: true })),
+  getResourcePermissions: jest.fn((_req, res) => res.json({ permissions: [] })),
+  getResourceRoles: jest.fn((_req, res) => res.json({ roles: [] })),
+  searchPrincipals: jest.fn((_req, res) => res.json({ principals: [] })),
+}));
+
+const express = require('express');
+const request = require('supertest');
+const {
+  SystemRoles,
+  ResourceType,
+  AccessRoleIds,
+  PrincipalType,
+  PermissionBits,
+  PermissionTypes,
+  Permissions,
+} = require('librechat-data-provider');
+
+const { updateResourcePermissions } = require('~/server/controllers/PermissionsController');
+const { hasCapability } = require('~/server/middleware/roles/capabilities');
+const { canAccessResource } = require('~/server/middleware');
+const accessPermissionsRouter = require('./accessPermissions');
+const { getRoleByName } = require('~/models');
+
+describe('Access permissions share policy', () => {
+  let app;
+
+  const resourceId = '507f1f77bcf86cd799439011';
+  const sharePolicyCases = [
+    {
+      label: 'agent',
+      resourceType: ResourceType.AGENT,
+      permissionType: PermissionTypes.AGENTS,
+      accessRoleId: AccessRoleIds.AGENT_VIEWER,
+      middlewareOptions: {
+        resourceType: ResourceType.AGENT,
+        requiredPermission: PermissionBits.SHARE,
+        resourceIdParam: 'resourceId',
+      },
+    },
+    {
+      label: 'prompt group',
+      resourceType: ResourceType.PROMPTGROUP,
+      permissionType: PermissionTypes.PROMPTS,
+      accessRoleId: AccessRoleIds.PROMPTGROUP_VIEWER,
+      middlewareOptions: {
+        resourceType: ResourceType.PROMPTGROUP,
+        requiredPermission: PermissionBits.SHARE,
+        resourceIdParam: 'resourceId',
+      },
+    },
+    {
+      label: 'MCP server',
+      resourceType: ResourceType.MCPSERVER,
+      permissionType: PermissionTypes.MCP_SERVERS,
+      accessRoleId: AccessRoleIds.MCPSERVER_VIEWER,
+      middlewareOptions: {
+        resourceType: ResourceType.MCPSERVER,
+        requiredPermission: PermissionBits.SHARE,
+        resourceIdParam: 'resourceId',
+        idResolver: expect.any(Function),
+      },
+    },
+    {
+      label: 'remote agent',
+      resourceType: ResourceType.REMOTE_AGENT,
+      permissionType: PermissionTypes.REMOTE_AGENTS,
+      accessRoleId: AccessRoleIds.REMOTE_AGENT_VIEWER,
+      middlewareOptions: {
+        resourceType: ResourceType.REMOTE_AGENT,
+        requiredPermission: PermissionBits.SHARE,
+        resourceIdParam: 'resourceId',
+      },
+    },
+    {
+      label: 'skill',
+      resourceType: ResourceType.SKILL,
+      permissionType: PermissionTypes.SKILLS,
+      accessRoleId: AccessRoleIds.SKILL_VIEWER,
+      middlewareOptions: {
+        resourceType: ResourceType.SKILL,
+        requiredPermission: PermissionBits.SHARE,
+        resourceIdParam: 'resourceId',
+        idResolver: expect.any(Function),
+      },
+    },
+  ];
+
+  const createUpdatedPrincipal = (accessRoleId) => ({
+    type: PrincipalType.USER,
+    id: 'target-user',
+    accessRoleId,
+  });
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    hasCapability.mockResolvedValue(false);
+
+    app = express();
+    app.use(express.json());
+    app.use((req, _res, next) => {
+      req.user = { id: 'skill-owner', role: SystemRoles.USER };
+      next();
+    });
+    app.use('/api/permissions', accessPermissionsRouter);
+  });
+
+  it.each(sharePolicyCases)(
+    'blocks non-public $label sharing when ACL SHARE passes but role SHARE is disabled',
+    async ({ resourceType, permissionType, accessRoleId, middlewareOptions }) => {
+      getRoleByName.mockResolvedValue({
+        permissions: {
+          [permissionType]: {
+            [Permissions.SHARE]: false,
+            [Permissions.SHARE_PUBLIC]: false,
+          },
+        },
+      });
+
+      const response = await request(app)
+        .put(`/api/permissions/${resourceType}/${resourceId}`)
+        .send({ updated: [createUpdatedPrincipal(accessRoleId)], public: false });
+
+      expect(response.status).toBe(403);
+      expect(response.body).toEqual({
+        error: 'Forbidden',
+        message: `You do not have permission to share ${resourceType} resources`,
+      });
+      expect(canAccessResource).toHaveBeenCalledWith(middlewareOptions);
+      expect(updateResourcePermissions).not.toHaveBeenCalled();
+    },
+  );
+
+  it('allows non-public skill sharing when both ACL SHARE and role SKILLS.SHARE pass', async () => {
+    getRoleByName.mockResolvedValue({
+      permissions: {
+        [PermissionTypes.SKILLS]: {
+          [Permissions.SHARE]: true,
+          [Permissions.SHARE_PUBLIC]: false,
+        },
+      },
+    });
+
+    const response = await request(app)
+      .put(`/api/permissions/${ResourceType.SKILL}/${resourceId}`)
+      .send({ updated: [createUpdatedPrincipal(AccessRoleIds.SKILL_VIEWER)], public: false });
+
+    expect(response.status).toBe(200);
+    expect(response.body).toEqual({ success: true });
+    expect(updateResourcePermissions).toHaveBeenCalledTimes(1);
+  });
+
+  it('preserves resource management capability bypass for non-public skill sharing', async () => {
+    hasCapability.mockResolvedValue(true);
+
+    const response = await request(app)
+      .put(`/api/permissions/${ResourceType.SKILL}/${resourceId}`)
+      .send({ updated: [createUpdatedPrincipal(AccessRoleIds.SKILL_VIEWER)], public: false });
+
+    expect(response.status).toBe(200);
+    expect(response.body).toEqual({ success: true });
+    expect(getRoleByName).not.toHaveBeenCalled();
+    expect(updateResourcePermissions).toHaveBeenCalledTimes(1);
+  });
+
+  it('still requires SHARE_PUBLIC when enabling public skill sharing', async () => {
+    getRoleByName.mockResolvedValue({
+      permissions: {
+        [PermissionTypes.SKILLS]: {
+          [Permissions.SHARE]: true,
+          [Permissions.SHARE_PUBLIC]: false,
+        },
+      },
+    });
+
+    const response = await request(app)
+      .put(`/api/permissions/${ResourceType.SKILL}/${resourceId}`)
+      .send({ public: true, publicAccessRoleId: AccessRoleIds.SKILL_VIEWER });
+
+    expect(response.status).toBe(403);
+    expect(response.body).toEqual({
+      error: 'Forbidden',
+      message: `You do not have permission to share ${ResourceType.SKILL} resources publicly`,
+    });
+    expect(updateResourcePermissions).not.toHaveBeenCalled();
+  });
+});