From e3fe1845166cf703b794dc3169ed7d95847888a4 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 1 Jun 2026 22:17:12 -0400 Subject: [PATCH] fix: Restrict skill sync server credentials --- api/server/routes/admin/skills.js | 88 +++++++++-- api/server/routes/admin/skills.test.js | 144 ++++++++++++++++-- api/server/services/Skills/sync.js | 3 +- api/server/services/Skills/sync.test.js | 31 ++++ packages/api/src/admin/skills.spec.ts | 132 ++++++++++++++++ packages/api/src/admin/skills.ts | 23 ++- packages/api/src/skills/sync/github.spec.ts | 81 ++++++++++ packages/api/src/skills/sync/github.ts | 29 +++- .../api/src/skills/sync/orchestrator.spec.ts | 15 ++ packages/api/src/skills/sync/orchestrator.ts | 4 + 10 files changed, 514 insertions(+), 36 deletions(-) create mode 100644 packages/api/src/admin/skills.spec.ts diff --git a/api/server/routes/admin/skills.js b/api/server/routes/admin/skills.js index 4c2e8f7bde..5e27388e1c 100644 --- a/api/server/routes/admin/skills.js +++ b/api/server/routes/admin/skills.js @@ -1,4 +1,5 @@ const express = require('express'); +const { skillSyncConfigSchema } = require('librechat-data-provider'); const { createAdminSkillsSyncHandlers } = require('@librechat/api'); const { SystemCapabilities } = require('@librechat/data-schemas'); const { hasCapability, requireCapability } = require('~/server/middleware/roles/capabilities'); @@ -10,18 +11,51 @@ const configMiddleware = require('~/server/middleware/config/app'); const router = express.Router(); const requireAdminAccess = requireCapability(SystemCapabilities.ACCESS_ADMIN); +function getCapabilityUser(req, { platformOnly = false } = {}) { + const id = req.user?.id ?? req.user?._id?.toString?.(); + if (!id) { + return null; + } + return { + id, + role: req.user?.role ?? '', + ...(platformOnly ? {} : { tenantId: req.user?.tenantId }), + }; +} + +function parseSkillSyncConfig(raw) { + if (!raw || typeof raw !== 'object') { + return undefined; + } + const parsed = skillSyncConfigSchema.safeParse(raw); + return parsed.success ? parsed.data : undefined; +} + +function isSameSkillSyncConfig(left, right) { + return JSON.stringify(left ?? null) === JSON.stringify(right ?? null); +} + +function hasResolvedSkillSyncOverride(req) { + const resolved = parseSkillSyncConfig(req.config?.skillSync); + const base = parseSkillSyncConfig(req.config?.config?.skillSync); + return Boolean(resolved?.github && !isSameSkillSyncConfig(resolved, base)); +} + +async function hasSkillCapability(req, capability, { platformOnly = false } = {}) { + const user = getCapabilityUser(req, { platformOnly }); + if (!user) { + return false; + } + return hasCapability(user, capability); +} + function requireSkillCapability(capability, { platformOnly = false } = {}) { return async (req, res, next) => { try { - const id = req.user?.id ?? req.user?._id?.toString?.(); - if (!id) { + const user = getCapabilityUser(req, { platformOnly }); + if (!user) { return res.status(401).json({ message: 'Authentication required' }); } - const user = { - id, - role: req.user?.role ?? '', - ...(platformOnly ? {} : { tenantId: req.user?.tenantId }), - }; if (await hasCapability(user, capability)) { return next(); } @@ -32,8 +66,42 @@ function requireSkillCapability(capability, { platformOnly = false } = {}) { }; } +async function attachCredentialReadAccess(req, res, next) { + try { + const canReadCredentials = await hasSkillCapability(req, SystemCapabilities.READ_SKILLS, { + platformOnly: true, + }); + req.skillSyncCanReadCredentials = canReadCredentials; + req.skillSyncAllowServerCredentials = canReadCredentials; + return next(); + } catch { + return res.status(500).json({ message: 'Internal Server Error' }); + } +} + +async function requireSyncRunCapability(req, res, next) { + try { + const canManagePlatform = await hasSkillCapability(req, SystemCapabilities.MANAGE_SKILLS, { + platformOnly: true, + }); + if (canManagePlatform) { + req.skillSyncAllowServerCredentials = true; + return next(); + } + if ( + hasResolvedSkillSyncOverride(req) && + (await hasSkillCapability(req, SystemCapabilities.MANAGE_SKILLS)) + ) { + req.skillSyncAllowServerCredentials = false; + return next(); + } + return res.status(403).json({ message: 'Forbidden' }); + } catch { + return res.status(500).json({ message: 'Internal Server Error' }); + } +} + const requireReadSkills = requireSkillCapability(SystemCapabilities.READ_SKILLS); -const requireManageSkills = requireSkillCapability(SystemCapabilities.MANAGE_SKILLS); const requirePlatformManageSkills = requireSkillCapability(SystemCapabilities.MANAGE_SKILLS, { platformOnly: true, }); @@ -46,8 +114,8 @@ const handlers = createAdminSkillsSyncHandlers({ router.use(requireJwtAuth, requireAdminAccess, configMiddleware); -router.get('/sync/status', requireReadSkills, handlers.getSyncStatus); -router.post('/sync/run', requireManageSkills, handlers.runSync); +router.get('/sync/status', requireReadSkills, attachCredentialReadAccess, handlers.getSyncStatus); +router.post('/sync/run', requireSyncRunCapability, handlers.runSync); router.put('/sync/credentials/:credentialKey', requirePlatformManageSkills, handlers.setCredential); router.delete( '/sync/credentials/:credentialKey', diff --git a/api/server/routes/admin/skills.test.js b/api/server/routes/admin/skills.test.js index 2301c915af..92a56fa289 100644 --- a/api/server/routes/admin/skills.test.js +++ b/api/server/routes/admin/skills.test.js @@ -8,11 +8,18 @@ const mockRequireJwtAuth = jest.fn((req, res, next) => { const mockCapabilityMiddleware = jest.fn((req, res, next) => next()); const mockRequireCapability = jest.fn(() => mockCapabilityMiddleware); const mockHasCapability = jest.fn().mockResolvedValue(true); +let mockResolvedConfig = { skillSync: { github: { enabled: false, sources: [] } } }; const mockConfigMiddleware = jest.fn((req, res, next) => { - req.config = { skillSync: { github: { enabled: false, sources: [] } } }; + req.config = mockResolvedConfig; next(); }); const mockGetGitHubSkillSyncRunnerForRequest = jest.fn(); +const mockHandlers = { + getSyncStatus: jest.fn((req, res) => res.status(200).json({ ok: true })), + runSync: jest.fn((req, res) => res.status(200).json({ ok: true })), + setCredential: jest.fn((req, res) => res.status(200).json({ ok: true })), + deleteCredential: jest.fn((req, res) => res.status(200).json({ ok: true })), +}; jest.mock('@librechat/data-schemas', () => ({ SystemCapabilities: { @@ -23,12 +30,7 @@ jest.mock('@librechat/data-schemas', () => ({ })); jest.mock('@librechat/api', () => ({ - createAdminSkillsSyncHandlers: jest.fn(() => ({ - getSyncStatus: jest.fn((req, res) => res.status(200).json({ ok: true })), - runSync: jest.fn((req, res) => res.status(200).json({ ok: true })), - setCredential: jest.fn((req, res) => res.status(200).json({ ok: true })), - deleteCredential: jest.fn((req, res) => res.status(200).json({ ok: true })), - })), + createAdminSkillsSyncHandlers: jest.fn(() => mockHandlers), })); jest.mock('~/server/middleware/roles/capabilities', () => ({ @@ -52,11 +54,22 @@ jest.mock('~/server/services/Skills/sync', () => ({ })); describe('admin skills sync routes', () => { - it('requires JWT auth and admin capabilities for sync endpoints', async () => { + beforeEach(() => { + jest.clearAllMocks(); + mockHasCapability.mockResolvedValue(true); + mockResolvedConfig = { skillSync: { github: { enabled: false, sources: [] } } }; + }); + + function createApp() { const router = require('./skills'); const app = express(); app.use(express.json()); app.use('/api/admin/skills', router); + return app; + } + + it('requires JWT auth and admin capabilities for sync endpoints', async () => { + const app = createApp(); await request(app).get('/api/admin/skills/sync/status').expect(200); await request(app).post('/api/admin/skills/sync/run').expect(200); @@ -67,15 +80,12 @@ describe('admin skills sync routes', () => { expect(mockRequireJwtAuth).toHaveBeenCalled(); expect(mockCapabilityMiddleware).toHaveBeenCalled(); expect(mockConfigMiddleware).toHaveBeenCalled(); - expect(mockHasCapability).toHaveBeenCalledTimes(4); + expect(mockHasCapability).toHaveBeenCalledTimes(5); expect(mockHasCapability).toHaveBeenCalledWith( { id: 'user-1', role: 'ADMIN', tenantId: 'tenant-a' }, 'read:skills', ); - expect(mockHasCapability).toHaveBeenCalledWith( - { id: 'user-1', role: 'ADMIN', tenantId: 'tenant-a' }, - 'manage:skills', - ); + expect(mockHasCapability).toHaveBeenCalledWith({ id: 'user-1', role: 'ADMIN' }, 'read:skills'); expect(mockHasCapability).toHaveBeenCalledWith( { id: 'user-1', role: 'ADMIN' }, 'manage:skills', @@ -85,4 +95,112 @@ describe('admin skills sync routes', () => { expect.objectContaining({ getRunner: mockGetGitHubSkillSyncRunnerForRequest }), ); }); + + it('marks credential metadata hidden for tenant-scoped status reads', async () => { + mockHasCapability.mockImplementation(async (user, capability) => { + if (capability === 'read:skills') { + return Boolean(user.tenantId); + } + return true; + }); + const app = createApp(); + + await request(app).get('/api/admin/skills/sync/status').expect(200); + + const req = mockHandlers.getSyncStatus.mock.calls[0][0]; + expect(req.skillSyncCanReadCredentials).toBe(false); + expect(req.skillSyncAllowServerCredentials).toBe(false); + }); + + it('allows tenant admins to run resolved override sync without server credentials', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'tenant-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + mockResolvedConfig = { skillSync, config: {} }; + mockHasCapability.mockImplementation(async (user, capability) => { + if (capability === 'manage:skills') { + return Boolean(user.tenantId); + } + return true; + }); + 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(false); + }); + + it('prevents tenant admins from manually running base skill sync config', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'base-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + mockResolvedConfig = { skillSync, config: { skillSync } }; + mockHasCapability.mockImplementation(async (user, capability) => { + if (capability === 'manage:skills') { + return Boolean(user.tenantId); + } + return true; + }); + const app = createApp(); + + await request(app).post('/api/admin/skills/sync/run').expect(403); + + expect(mockHandlers.runSync).not.toHaveBeenCalled(); + }); + + it('allows platform admins to manually run base skill sync config with server credentials', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'base-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + mockResolvedConfig = { skillSync, config: { 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); + }); }); diff --git a/api/server/services/Skills/sync.js b/api/server/services/Skills/sync.js index eae40d26e7..aaf73ac9dd 100644 --- a/api/server/services/Skills/sync.js +++ b/api/server/services/Skills/sync.js @@ -57,7 +57,7 @@ async function getSyntheticReq({ userId = SYSTEM_USER_ID, tenantId, loadAppConfi }; } -function createRunner({ getConfig, loadAppConfig } = {}) { +function createRunner({ getConfig, loadAppConfig, allowServerCredentials = true } = {}) { const resolveAppConfig = loadAppConfig ?? loadCurrentAppConfig; const resolveConfig = getConfig ?? (() => getSyncConfig(resolveAppConfig)); const createdRunner = createGitHubSkillSyncRunner({ @@ -141,6 +141,7 @@ function createRunner({ getConfig, loadAppConfig } = {}) { file, ); }, + allowServerCredentials, }); return { getStatus: createdRunner.getStatus, diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index b6c74166ac..d06ec23b3e 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -185,6 +185,7 @@ describe('GitHub skill sync service', () => { const requestRunner = mockCreatedRunners[0].runner; const requestConfig = await mockCreatedRunners[0].deps.getConfig(); expect(started).toBe(true); + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false); expect(requestRunner.runOnce).toHaveBeenCalledTimes(1); expect(requestConfig.github.runOnStartup).toBe(false); expect(requestConfig.github.sources[0]).toEqual( @@ -248,17 +249,47 @@ describe('GitHub skill sync service', () => { const runner = service.getGitHubSkillSyncRunnerForRequest({ config: { skillSync, config: {} }, user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: true, }); const config = await mockCreatedRunners[0].deps.getConfig(); expect(runner.runOnce).toBe(mockCreatedRunners[0].runner.runOnce); expect(runner.getStatus).toBe(mockCreatedRunners[0].runner.getStatus); + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(true); expect(config.github.runOnStartup).toBe(true); expect(config.github.sources[0]).toEqual( expect.objectContaining({ id: 'tenant-skills', tenantId: 'tenant-a' }), ); }); + it('does not allow request-built admin override runners to use server credentials by default', async () => { + const skillSync = { + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: true, + sources: [ + { + id: 'tenant-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }, + }; + + const service = require('./sync'); + service.getGitHubSkillSyncRunnerForRequest({ + config: { skillSync, config: {} }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + }); + + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false); + }); + it('does not start a request-scoped sync when the configured source is already running', async () => { const skillSync = { github: { diff --git a/packages/api/src/admin/skills.spec.ts b/packages/api/src/admin/skills.spec.ts new file mode 100644 index 0000000000..3fecc668ec --- /dev/null +++ b/packages/api/src/admin/skills.spec.ts @@ -0,0 +1,132 @@ +import type { Response } from 'express'; +import { createAdminSkillsSyncHandlers } from './skills'; + +function createResponse() { + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }; + return res as unknown as Response & { + status: jest.Mock; + json: jest.Mock; + }; +} + +function createHandlers() { + const runner = { + getStatus: jest.fn(async () => ({ + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + provider: 'github', + sourceId: 'tenant-skills', + tenantId: 'tenant-a', + status: 'idle', + credentialKey: 'github-skills-prod', + credentialPresent: true, + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + }, + ], + credentials: [ + { + provider: 'github', + credentialKey: 'github-skills-prod', + credentialPresent: true, + tokenFingerprint: 'abc123', + }, + ], + fineGrainedTokenRecommendation: 'Use a fine-grained token.', + })), + runOnce: jest.fn(async () => ({ + status: 'completed', + sources: [ + { + provider: 'github', + sourceId: 'tenant-skills', + tenantId: 'tenant-a', + status: 'succeeded', + credentialKey: 'github-skills-prod', + credentialPresent: true, + syncedSkillCount: 1, + syncedFileCount: 2, + deletedSkillCount: 0, + deletedFileCount: 0, + }, + ], + })), + }; + const handlers = createAdminSkillsSyncHandlers({ + runner, + upsertCredential: jest.fn(), + deleteCredential: jest.fn(), + }); + return { handlers, runner }; +} + +describe('createAdminSkillsSyncHandlers', () => { + it('omits credential summaries and source credential metadata for tenant-scoped status reads', async () => { + const { handlers } = createHandlers(); + const res = createResponse(); + + await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res); + + expect(res.status).toHaveBeenCalledWith(200); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + credentials: [], + sources: [ + expect.objectContaining({ + credentialKey: undefined, + credentialPresent: false, + }), + ], + }), + ); + }); + + it('includes credential summaries and source credential metadata for platform status reads', async () => { + const { handlers } = createHandlers(); + const res = createResponse(); + + await handlers.getSyncStatus({ skillSyncCanReadCredentials: true } as never, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + credentials: [expect.objectContaining({ credentialKey: 'github-skills-prod' })], + sources: [ + expect.objectContaining({ + credentialKey: 'github-skills-prod', + credentialPresent: true, + }), + ], + }), + ); + }); + + it('omits source credential metadata from tenant-scoped manual run responses', async () => { + const { handlers } = createHandlers(); + const res = createResponse(); + + await handlers.runSync({ skillSyncAllowServerCredentials: false } as never, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + credentialKey: undefined, + credentialPresent: false, + }), + ], + }), + ); + }); +}); diff --git a/packages/api/src/admin/skills.ts b/packages/api/src/admin/skills.ts index c40e1c5d2f..78cea561d8 100644 --- a/packages/api/src/admin/skills.ts +++ b/packages/api/src/admin/skills.ts @@ -19,6 +19,8 @@ type AdminSkillsRequest = Request & { _id?: Types.ObjectId; id?: string; }; + skillSyncAllowServerCredentials?: boolean; + skillSyncCanReadCredentials?: boolean; }; export type AdminSkillSyncDeps = { @@ -52,14 +54,15 @@ function serializeCredential( function serializeSourceStatus( status: ISkillSyncStatus & { credentialPresent?: boolean }, + { includeCredentialMetadata = true }: { includeCredentialMetadata?: boolean } = {}, ): TGitHubSkillSyncSourceStatus { return { provider: status.provider, sourceId: status.sourceId, tenantId: status.tenantId, status: status.status, - credentialKey: status.credentialKey, - credentialPresent: status.credentialPresent ?? false, + credentialKey: includeCredentialMetadata ? status.credentialKey : undefined, + credentialPresent: includeCredentialMetadata ? (status.credentialPresent ?? false) : false, owner: status.owner, repo: status.repo, ref: status.ref, @@ -96,25 +99,31 @@ export function createAdminSkillsSyncHandlers(deps: AdminSkillSyncDeps) { return runner; } - async function getSyncStatus(req: Request, res: Response) { + async function getSyncStatus(req: AdminSkillsRequest, res: Response) { + const includeCredentialMetadata = req.skillSyncCanReadCredentials !== false; const status = await getRunner(req).getStatus(); const response: TGitHubSkillSyncStatusResponse = { enabled: status.enabled, intervalMinutes: status.intervalMinutes, runOnStartup: status.runOnStartup, - sources: status.sources.map(serializeSourceStatus), - credentials: status.credentials.map(serializeCredential), + sources: status.sources.map((source) => + serializeSourceStatus(source, { includeCredentialMetadata }), + ), + credentials: includeCredentialMetadata ? status.credentials.map(serializeCredential) : [], fineGrainedTokenRecommendation: status.fineGrainedTokenRecommendation, }; return res.status(200).json(response); } - async function runSync(req: Request, res: Response) { + async function runSync(req: AdminSkillsRequest, res: Response) { + const includeCredentialMetadata = req.skillSyncAllowServerCredentials === true; const result = await getRunner(req).runOnce(); const response: TGitHubSkillSyncManualRunResponse = { status: result.status, message: result.message, - sources: result.sources.map(serializeSourceStatus), + sources: result.sources.map((source) => + serializeSourceStatus(source, { includeCredentialMetadata }), + ), }; return res.status(result.status === 'skipped' ? 202 : 200).json(response); } diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index 5ab88a8a98..af61d7001b 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -433,6 +433,87 @@ describe('createGitHubSkillSyncRunner', () => { } }); + it('does not list or resolve server credentials when server credentials are disabled', async () => { + const previousToken = process.env.GITHUB_SKILLS_TOKEN; + process.env.GITHUB_SKILLS_TOKEN = 'github_pat_from_env'; + const getCredentialToken = jest.fn(async () => 'github_pat_from_db'); + const listCredentials = jest.fn(async () => [ + { + provider: 'github' as const, + credentialKey: 'github-skills-prod', + credentialPresent: true, + tokenFingerprint: 'abc123', + }, + ]); + const deps = createDeps({ + allowServerCredentials: false, + getCredentialToken, + listCredentials, + getConfig: () => ({ + github: { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + id: 'librechat-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + { + id: 'stored-credential-skills', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + credentialKey: 'github-skills-prod', + }, + ], + }, + }), + }); + const runner = createGitHubSkillSyncRunner(deps); + + try { + const status = await runner.getStatus(); + const result = await runner.runOnce(); + + expect(status.credentials).toEqual([]); + expect(status.sources).toEqual([ + expect.objectContaining({ sourceId: 'librechat-skills', credentialPresent: false }), + expect.objectContaining({ + sourceId: 'stored-credential-skills', + credentialPresent: false, + }), + ]); + expect(result.status).toBe('failed'); + expect(result.sources).toEqual([ + expect.objectContaining({ + sourceId: 'librechat-skills', + status: 'failed', + errorCode: 'MISSING_CREDENTIAL', + }), + expect.objectContaining({ + sourceId: 'stored-credential-skills', + status: 'failed', + errorCode: 'MISSING_CREDENTIAL', + }), + ]); + expect(listCredentials).not.toHaveBeenCalled(); + expect(getCredentialToken).not.toHaveBeenCalled(); + expect(deps.fetchFn).not.toHaveBeenCalled(); + } finally { + if (previousToken == null) { + delete process.env.GITHUB_SKILLS_TOKEN; + } else { + process.env.GITHUB_SKILLS_TOKEN = previousToken; + } + } + }); + it('preserves slash-delimited refs when fetching the GitHub commit', async () => { const baseFetch = githubFetch(); const fetchFn = jest.fn(async (input: RequestInfo | URL) => { diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index a04b9eb1eb..82b2f1f71e 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -208,6 +208,7 @@ export type GitHubSkillSyncDeps = { }) => Promise; fetchFn?: FetchFn; lockOwner?: string; + allowServerCredentials?: boolean; }; export type GitHubSkillSyncRunResult = { @@ -1178,6 +1179,9 @@ async function resolveGitHubToken( deps: GitHubSkillSyncDeps, source: SkillSyncGitHubSourceConfig, ): Promise { + if (deps.allowServerCredentials === false) { + return null; + } const tokenEnvVar = getTokenEnvVarName(source.token); if (tokenEnvVar) { return process.env[tokenEnvVar]?.trim() || null; @@ -1188,7 +1192,13 @@ async function resolveGitHubToken( return deps.getCredentialToken(PROVIDER, source.credentialKey); } -function getMissingCredentialMessage(source: SkillSyncGitHubSourceConfig): string { +function getMissingCredentialMessage( + source: SkillSyncGitHubSourceConfig, + allowServerCredentials: boolean, +): string { + if (!allowServerCredentials) { + return 'Server GitHub credentials are not available for this skill sync config'; + } const tokenEnvVar = getTokenEnvVarName(source.token); if (tokenEnvVar) { return `Missing GitHub token environment variable "${tokenEnvVar}"`; @@ -1207,10 +1217,14 @@ async function syncSource(params: { await deps.upsertStatus(makeStatusInput({ source, status: 'running', startedAt })); try { assertNotCancelled(); + const allowServerCredentials = deps.allowServerCredentials !== false; const token = await resolveGitHubToken(deps, source); assertNotCancelled(); if (!token) { - throw new SkillSyncError('MISSING_CREDENTIAL', getMissingCredentialMessage(source)); + throw new SkillSyncError( + 'MISSING_CREDENTIAL', + getMissingCredentialMessage(source, allowServerCredentials), + ); } const commit = await fetchCommit({ fetchFn, token, source }); assertNotCancelled(); @@ -1466,9 +1480,10 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { async function getStatus() { const github = getGithubConfig(await deps.getConfig()); + const allowServerCredentials = deps.allowServerCredentials !== false; const [storedStatuses, credentials] = await Promise.all([ deps.listStatuses(PROVIDER), - deps.listCredentials(PROVIDER), + allowServerCredentials ? deps.listCredentials(PROVIDER) : Promise.resolve([]), ]); const statusBySourceId = new Map( storedStatuses.map((status) => [makeStatusKey(status.sourceId, status.tenantId), status]), @@ -1478,9 +1493,13 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { ); const sources = github.sources.map((source) => { const stored = statusBySourceId.get(makeStatusKey(source.id, source.tenantId)); - const credential = source.credentialKey ? credentialByKey.get(source.credentialKey) : null; + const credential = + allowServerCredentials && source.credentialKey + ? credentialByKey.get(source.credentialKey) + : null; const tokenEnvVar = getTokenEnvVarName(source.token); - const envTokenPresent = tokenEnvVar ? Boolean(process.env[tokenEnvVar]?.trim()) : false; + const envTokenPresent = + allowServerCredentials && tokenEnvVar ? Boolean(process.env[tokenEnvVar]?.trim()) : false; return { provider: PROVIDER, sourceId: source.id, diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 702e08d269..604a5a239e 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -124,6 +124,7 @@ describe('createSkillSyncTriggerOrchestrator', () => { const requestConfig = await runners[0].input.getConfig(); expect(started).toBe(true); + expect(runners[0].input.allowServerCredentials).toBe(false); expect(runners[0].runner.runOnce).toHaveBeenCalledTimes(1); expect(requestConfig?.github?.runOnStartup).toBe(false); expect(requestConfig?.github?.sources[0]).toEqual( @@ -151,16 +152,30 @@ describe('createSkillSyncTriggerOrchestrator', () => { const runner = orchestrator.getRunnerForAdminRequest({ config: { skillSync: config, config: {} }, user: { tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: true, }); const runnerConfig = await runners[0].input.getConfig(); expect(runner).toBe(runners[0].runner); + expect(runners[0].input.allowServerCredentials).toBe(true); expect(runnerConfig?.github?.runOnStartup).toBe(true); expect(runnerConfig?.github?.sources[0]).toEqual( expect.objectContaining({ id: 'tenant-skills', tenantId: 'tenant-a' }), ); }); + it('does not allow admin override runners to use server credentials by default', async () => { + const config = skillSync(); + const { orchestrator, runners } = createHarness(); + + orchestrator.getRunnerForAdminRequest({ + config: { skillSync: config, config: {} }, + user: { tenantId: 'tenant-a' }, + }); + + expect(runners[0].input.allowServerCredentials).toBe(false); + }); + it('does not start request sync when the configured source is already running', async () => { const config = skillSync({ runOnStartup: false }); const { orchestrator, runners } = createHarness({ diff --git a/packages/api/src/skills/sync/orchestrator.ts b/packages/api/src/skills/sync/orchestrator.ts index 71a411dfb8..49710f715f 100644 --- a/packages/api/src/skills/sync/orchestrator.ts +++ b/packages/api/src/skills/sync/orchestrator.ts @@ -21,6 +21,7 @@ type SkillSyncRequestUser = { type SkillSyncRequestLike = { config?: SkillSyncAppConfigLike; user?: SkillSyncRequestUser; + skillSyncAllowServerCredentials?: boolean; }; type ResolvedSkillSyncConfig = NonNullable; @@ -39,6 +40,7 @@ type SkillSyncTriggerLogger = { export type SkillSyncTriggerRunnerFactoryInput = { getConfig: () => MaybePromise; loadAppConfig: () => MaybePromise; + allowServerCredentials?: boolean; }; export type SkillSyncTriggerOrchestratorDeps = { @@ -202,6 +204,7 @@ export function createSkillSyncTriggerOrchestrator(deps: SkillSyncTriggerOrchest return deps.createRunner({ getConfig: async () => config, loadAppConfig: async () => request.config, + allowServerCredentials: Boolean(request.skillSyncAllowServerCredentials), }); } @@ -219,6 +222,7 @@ export function createSkillSyncTriggerOrchestrator(deps: SkillSyncTriggerOrchest const requestRunner = deps.createRunner({ getConfig: async () => config, loadAppConfig: async () => request.config, + allowServerCredentials: false, }); const status = await requestRunner.getStatus(); if (!shouldRunRequestSync(status, { minIntervalMs, staleRunningMs })) {