fix: preserve skill sync request scope

This commit is contained in:
Danny Avila 2026-06-06 13:04:06 -04:00
parent c1c8aa1e41
commit 89dad75d35
5 changed files with 116 additions and 10 deletions

View file

@ -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);

View file

@ -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);
});
});

View file

@ -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) {

View file

@ -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: {

View file

@ -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();