diff --git a/api/server/routes/admin/skills.js b/api/server/routes/admin/skills.js index 5e27388e1c..00f50fc172 100644 --- a/api/server/routes/admin/skills.js +++ b/api/server/routes/admin/skills.js @@ -6,6 +6,7 @@ const { hasCapability, requireCapability } = require('~/server/middleware/roles/ const { requireJwtAuth } = require('~/server/middleware'); const { upsertSkillSyncCredential, deleteSkillSyncCredential } = require('~/models'); const { getGitHubSkillSyncRunnerForRequest } = require('~/server/services/Skills/sync'); +const { getAppConfig } = require('~/server/services/Config'); const configMiddleware = require('~/server/middleware/config/app'); const router = express.Router(); @@ -41,6 +42,26 @@ function hasResolvedSkillSyncOverride(req) { return Boolean(resolved?.github && !isSameSkillSyncConfig(resolved, base)); } +function hasServerCredentialReference(config) { + return Boolean(config?.github?.sources?.some((source) => source.credentialKey || source.token)); +} + +async function attachBaseSkillSyncConfig(req, res, next) { + try { + const baseConfig = await getAppConfig({ baseOnly: true }); + req.config = { + ...(req.config ?? {}), + config: { + ...(req.config?.config ?? {}), + skillSync: baseConfig?.skillSync, + }, + }; + return next(); + } catch { + return res.status(500).json({ message: 'Internal Server Error' }); + } +} + async function hasSkillCapability(req, capability, { platformOnly = false } = {}) { const user = getCapabilityUser(req, { platformOnly }); if (!user) { @@ -88,10 +109,16 @@ async function requireSyncRunCapability(req, res, next) { req.skillSyncAllowServerCredentials = true; return next(); } + const resolved = parseSkillSyncConfig(req.config?.skillSync); if ( hasResolvedSkillSyncOverride(req) && (await hasSkillCapability(req, SystemCapabilities.MANAGE_SKILLS)) ) { + if (hasServerCredentialReference(resolved)) { + return res.status(403).json({ + message: 'Tenant-scoped skill sync runs cannot use server credentials', + }); + } req.skillSyncAllowServerCredentials = false; return next(); } @@ -112,7 +139,7 @@ const handlers = createAdminSkillsSyncHandlers({ deleteCredential: deleteSkillSyncCredential, }); -router.use(requireJwtAuth, requireAdminAccess, configMiddleware); +router.use(requireJwtAuth, requireAdminAccess, configMiddleware, attachBaseSkillSyncConfig); router.get('/sync/status', requireReadSkills, attachCredentialReadAccess, handlers.getSyncStatus); router.post('/sync/run', requireSyncRunCapability, handlers.runSync); diff --git a/api/server/routes/admin/skills.test.js b/api/server/routes/admin/skills.test.js index 92a56fa289..b96649a5fc 100644 --- a/api/server/routes/admin/skills.test.js +++ b/api/server/routes/admin/skills.test.js @@ -13,6 +13,7 @@ const mockConfigMiddleware = jest.fn((req, res, next) => { req.config = mockResolvedConfig; next(); }); +const mockGetAppConfig = jest.fn(); const mockGetGitHubSkillSyncRunnerForRequest = jest.fn(); const mockHandlers = { getSyncStatus: jest.fn((req, res) => res.status(200).json({ ok: true })), @@ -44,6 +45,10 @@ jest.mock('~/server/middleware', () => ({ jest.mock('~/server/middleware/config/app', () => mockConfigMiddleware); +jest.mock('~/server/services/Config', () => ({ + getAppConfig: mockGetAppConfig, +})); + jest.mock('~/models', () => ({ upsertSkillSyncCredential: jest.fn(), deleteSkillSyncCredential: jest.fn(), @@ -58,6 +63,7 @@ describe('admin skills sync routes', () => { jest.clearAllMocks(); mockHasCapability.mockResolvedValue(true); mockResolvedConfig = { skillSync: { github: { enabled: false, sources: [] } } }; + mockGetAppConfig.mockResolvedValue({ skillSync: undefined }); }); function createApp() { @@ -112,7 +118,7 @@ describe('admin skills sync routes', () => { expect(req.skillSyncAllowServerCredentials).toBe(false); }); - it('allows tenant admins to run resolved override sync without server credentials', async () => { + it('prevents tenant admins from running overrides that require server credentials', async () => { const skillSync = { github: { enabled: true, @@ -137,12 +143,12 @@ describe('admin skills sync routes', () => { } return true; }); + mockGetAppConfig.mockResolvedValue({ skillSync: undefined }); const app = createApp(); - await request(app).post('/api/admin/skills/sync/run').expect(200); + await request(app).post('/api/admin/skills/sync/run').expect(403); - const req = mockHandlers.runSync.mock.calls[0][0]; - expect(req.skillSyncAllowServerCredentials).toBe(false); + expect(mockHandlers.runSync).not.toHaveBeenCalled(); }); it('prevents tenant admins from manually running base skill sync config', async () => { @@ -163,7 +169,8 @@ describe('admin skills sync routes', () => { ], }, }; - mockResolvedConfig = { skillSync, config: { skillSync } }; + mockResolvedConfig = { skillSync }; + mockGetAppConfig.mockResolvedValue({ skillSync }); mockHasCapability.mockImplementation(async (user, capability) => { if (capability === 'manage:skills') { return Boolean(user.tenantId); @@ -195,12 +202,14 @@ describe('admin skills sync routes', () => { ], }, }; - mockResolvedConfig = { skillSync, config: { skillSync } }; + mockResolvedConfig = { skillSync }; + mockGetAppConfig.mockResolvedValue({ skillSync }); const app = createApp(); await request(app).post('/api/admin/skills/sync/run').expect(200); const req = mockHandlers.runSync.mock.calls[0][0]; expect(req.skillSyncAllowServerCredentials).toBe(true); + expect(req.config.config.skillSync).toEqual(skillSync); }); }); diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index aaf73ac9dd..1e2f940206 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -57,6 +57,22 @@ async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId, loadAppConfi }; } +function withBaseSkillSyncConfig(req, baseConfig) { + if (!req?.config || req.config.config?.skillSync !== undefined) { + return req; + } + return { + ...req, + config: { + ...req.config, + config: { + ...(req.config.config ?? {}), + skillSync: baseConfig?.skillSync, + }, + }, + }; +} + function createRunner({ getConfig, loadAppConfig, allowServerCredentials = true } = {}) { const resolveAppConfig = loadAppConfig ?? loadCurrentAppConfig; const resolveConfig = getConfig ?? (() => getSyncConfig(resolveAppConfig)); @@ -155,11 +171,12 @@ const triggerOrchestrator = createSkillSyncTriggerOrchestrator({ }); function getGitHubSkillSyncRunnerForRequest(req) { - return triggerOrchestrator.getRunnerForAdminRequest(req); + return triggerOrchestrator.getRunnerForAdminRequest(withBaseSkillSyncConfig(req, appConfigRef)); } async function maybeRunGitHubSkillSyncForRequest(req) { - return triggerOrchestrator.maybeRunForRequest(req); + const baseConfig = await loadCurrentAppConfig(); + return triggerOrchestrator.maybeRunForRequest(withBaseSkillSyncConfig(req, baseConfig)); } function initializeGitHubSkillSync(appConfig) { diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index cc6cf5d30f..be40e15ec2 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -269,15 +269,17 @@ describe('GitHub skill sync service', () => { ], }, }; + mockGetAppConfig.mockResolvedValue({ skillSync }); const service = require('./sync'); const started = await service.maybeRunGitHubSkillSyncForRequest({ - config: { skillSync, config: { skillSync } }, + config: { skillSync }, user: { id: 'user-1', tenantId: 'tenant-a' }, }); expect(started).toBe(false); expect(mockCreatedRunners).toHaveLength(0); + expect(mockGetAppConfig).toHaveBeenCalledWith({ baseOnly: true }); }); it('creates an admin request runner from resolved skillSync config overrides', async () => { @@ -317,6 +319,40 @@ describe('GitHub skill sync service', () => { ); }); + it('preserves base admin runner tenant scope when request config has no nested base copy', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: true, + sources: [ + { + id: 'base-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + tenantId: 'base-tenant', + }, + ], + }, + }; + + const service = require('./sync'); + service.initializeGitHubSkillSync({ skillSync }); + service.getGitHubSkillSyncRunnerForRequest({ + config: { skillSync }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: true, + }); + const config = await mockCreatedRunners[1].deps.getConfig(); + + expect(config.github.sources[0]).toEqual( + expect.objectContaining({ id: 'base-skills', tenantId: 'base-tenant' }), + ); + }); + it('does not allow request-built admin override runners to use server credentials by default', async () => { const skillSync = { github: { diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 03dd5d6b5b..08400339f7 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -201,6 +201,23 @@ describe('createSkillSyncTriggerOrchestrator', () => { ); }); + it('preserves configured tenant scope for admin base skillSync runs', async () => { + const config = skillSync(); + const { orchestrator, runners } = createHarness(); + + orchestrator.getRunnerForAdminRequest({ + config: { skillSync: config, config: { skillSync: config } }, + user: { tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: true, + }); + const runnerConfig = await runners[0].input.getConfig(); + + expect(runnerConfig?.github?.runOnStartup).toBe(true); + expect(runnerConfig?.github?.sources[0]).toEqual( + expect.objectContaining({ id: 'tenant-skills', tenantId: 'other-tenant' }), + ); + }); + it('does not allow admin override runners to use server credentials by default', async () => { const config = skillSync(); const { orchestrator, runners } = createHarness();