diff --git a/packages/api/src/admin/skills.spec.ts b/packages/api/src/admin/skills.spec.ts index 43312c872e..ce5e970d5b 100644 --- a/packages/api/src/admin/skills.spec.ts +++ b/packages/api/src/admin/skills.spec.ts @@ -1,4 +1,5 @@ import { SystemCapabilities } from '@librechat/data-schemas'; +import type { ISkillSyncStatus } from '@librechat/data-schemas'; import type { NextFunction, Response } from 'express'; import { createAdminSkillsSyncAccess, createAdminSkillsSyncHandlers } from './skills'; @@ -17,6 +18,36 @@ function createNext(): NextFunction & jest.Mock { return jest.fn() as NextFunction & jest.Mock; } +type SourceStatus = ISkillSyncStatus & { credentialPresent: boolean }; + +function createSourceStatus(overrides: Partial = {}): SourceStatus { + return { + 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, + errorCode: undefined, + errorMessage: undefined, + startedAt: undefined, + finishedAt: undefined, + lastSuccessAt: undefined, + lastFailureAt: undefined, + createdAt: undefined, + updatedAt: undefined, + ...overrides, + }; +} + function createHandlers({ statusErrorCode, statusErrorMessage, @@ -27,30 +58,10 @@ function createHandlers({ intervalMinutes: 60, runOnStartup: false, sources: [ - { - provider: 'github' as const, - sourceId: 'tenant-skills', - tenantId: 'tenant-a', - status: 'idle' as const, - credentialKey: 'github-skills-prod', - credentialPresent: true, - owner: 'LibreChat', - repo: 'skills', - ref: 'main', - paths: ['skills'], - syncedSkillCount: 0, - syncedFileCount: 0, - deletedSkillCount: 0, - deletedFileCount: 0, + createSourceStatus({ errorCode: statusErrorCode, errorMessage: statusErrorMessage, - startedAt: undefined, - finishedAt: undefined, - lastSuccessAt: undefined, - lastFailureAt: undefined, - createdAt: undefined, - updatedAt: undefined, - }, + }), ], credentials: [ { @@ -65,26 +76,13 @@ function createHandlers({ runOnce: jest.fn(async () => ({ status: 'completed' as const, sources: [ - { - provider: 'github' as const, - sourceId: 'tenant-skills', - tenantId: 'tenant-a', - status: 'succeeded' as const, - credentialKey: 'github-skills-prod', - credentialPresent: true, + createSourceStatus({ + status: 'succeeded', syncedSkillCount: 1, syncedFileCount: 2, - deletedSkillCount: 0, - deletedFileCount: 0, errorCode: statusErrorCode, errorMessage: statusErrorMessage, - startedAt: undefined, - finishedAt: undefined, - lastSuccessAt: undefined, - lastFailureAt: undefined, - createdAt: undefined, - updatedAt: undefined, - }, + }), ], })), }; @@ -101,7 +99,13 @@ describe('createAdminSkillsSyncHandlers', () => { const { handlers } = createHandlers(); const res = createResponse(); - await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res); + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); expect(res.status).toHaveBeenCalledWith(200); expect(res.json).toHaveBeenCalledWith( @@ -121,6 +125,87 @@ describe('createAdminSkillsSyncHandlers', () => { ); }); + it('filters tenant-scoped status reads to the request tenant and unscoped sources', async () => { + const runner = { + getStatus: jest.fn(async () => ({ + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + createSourceStatus({ + sourceId: 'tenant-a-source', + tenantId: 'tenant-a', + status: 'succeeded', + syncedSkillCount: 4, + syncedFileCount: 8, + }), + createSourceStatus({ + sourceId: 'shared-source', + tenantId: undefined, + status: 'succeeded', + syncedSkillCount: 2, + syncedFileCount: 3, + }), + createSourceStatus({ + sourceId: 'tenant-b-source', + tenantId: 'tenant-b', + status: 'failed', + errorCode: 'PARSE_ERROR', + errorMessage: 'Failed to parse skills/private-b/SKILL.md', + syncedSkillCount: 7, + syncedFileCount: 11, + deletedSkillCount: 1, + deletedFileCount: 2, + }), + ], + credentials: [], + fineGrainedTokenRecommendation: 'Use a fine-grained token.', + })), + runOnce: jest.fn(), + }; + const handlers = createAdminSkillsSyncHandlers({ + runner, + upsertCredential: jest.fn(), + deleteCredential: jest.fn(), + }); + const res = createResponse(); + + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + sources: [ + expect.objectContaining({ + sourceId: 'tenant-a-source', + tenantId: 'tenant-a', + status: 'succeeded', + syncedSkillCount: 4, + syncedFileCount: 8, + }), + expect.objectContaining({ + sourceId: 'shared-source', + tenantId: undefined, + status: 'succeeded', + syncedSkillCount: 2, + syncedFileCount: 3, + }), + ], + }), + ); + const response = res.json.mock.calls[0]?.[0] as { sources: Array<{ tenantId?: string }> }; + expect(response.sources).toHaveLength(2); + expect(response.sources).toEqual([ + expect.objectContaining({ sourceId: 'tenant-a-source' }), + expect.objectContaining({ sourceId: 'shared-source', tenantId: undefined }), + ]); + }); + it('redacts credential-related errors from tenant-scoped status reads', async () => { const { handlers } = createHandlers({ statusErrorCode: 'MISSING_CREDENTIAL', @@ -128,7 +213,13 @@ describe('createAdminSkillsSyncHandlers', () => { }); const res = createResponse(); - await handlers.getSyncStatus({ skillSyncCanReadCredentials: false } as never, res); + await handlers.getSyncStatus( + { + user: { id: 'user-1', tenantId: 'tenant-a' }, + skillSyncCanReadCredentials: false, + } as never, + res, + ); expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ diff --git a/packages/api/src/admin/skills.ts b/packages/api/src/admin/skills.ts index 4588d0091e..8d481047f8 100644 --- a/packages/api/src/admin/skills.ts +++ b/packages/api/src/admin/skills.ts @@ -22,6 +22,7 @@ export type AdminSkillsRequest = Request & { user?: { _id?: Types.ObjectId; id?: string; + tenantId?: string; }; skillSyncAllowServerCredentials?: boolean; skillSyncCanReadCredentials?: boolean; @@ -153,6 +154,25 @@ function serializeSourceStatus( }; } +function getRequestTenantId(req: AdminSkillsRequest): string | undefined { + const tenantId = req.user?.tenantId; + return typeof tenantId === 'string' && tenantId ? tenantId : undefined; +} + +function getVisibleSourceStatuses( + req: AdminSkillsRequest, + sources: T[], +): T[] { + if (req.skillSyncCanReadCredentials !== false) { + return sources; + } + const tenantId = getRequestTenantId(req); + if (!tenantId) { + return []; + } + return sources.filter((source) => !source.tenantId || source.tenantId === tenantId); +} + function isCredentialKey(value: unknown): value is string { return typeof value === 'string' && CREDENTIAL_KEY_PATTERN.test(value); } @@ -332,7 +352,7 @@ export function createAdminSkillsSyncHandlers(deps: AdminSkillSyncDeps): AdminSk enabled: status.enabled, intervalMinutes: status.intervalMinutes, runOnStartup: status.runOnStartup, - sources: status.sources.map((source) => + sources: getVisibleSourceStatuses(req, status.sources).map((source) => serializeSourceStatus(source, { includeCredentialMetadata }), ), credentials: includeCredentialMetadata ? status.credentials.map(serializeCredential) : [], diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 231ba280d3..c86359e966 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -250,6 +250,83 @@ describe('createSkillSyncTriggerOrchestrator', () => { ); }); + it('filters admin base skillSync status sources without rewriting unscoped sources', async () => { + const config = skillSync({ + sources: [ + { + ...source, + id: 'tenant-a-source', + tenantId: 'tenant-a', + }, + { + ...source, + id: 'tenant-b-source', + tenantId: 'tenant-b', + }, + { + id: 'shared-source', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ], + }); + const { orchestrator, runners } = createHarness(); + + orchestrator.getRunnerForAdminRequest({ + config: { skillSync: config, config: { skillSync: config } }, + user: { tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: false, + }); + const runnerConfig = await runners[0].input.getConfig(); + + expect(runnerConfig?.github?.sources).toEqual([ + expect.objectContaining({ id: 'tenant-a-source', tenantId: 'tenant-a' }), + expect.objectContaining({ id: 'shared-source', tenantId: undefined }), + ]); + }); + + it('filters inherited base sources when admin override changes non-source fields', async () => { + const baseSources = [ + { + ...source, + id: 'tenant-a-source', + tenantId: 'tenant-a', + }, + { + ...source, + id: 'tenant-b-source', + tenantId: 'tenant-b', + }, + { + id: 'shared-source', + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + token: '${GITHUB_SKILLS_TOKEN}', + }, + ]; + const base = skillSync({ intervalMinutes: 60, sources: baseSources }); + const resolved = skillSync({ intervalMinutes: 30, sources: baseSources }); + const { orchestrator, runners } = createHarness(); + + orchestrator.getRunnerForAdminRequest({ + config: { skillSync: resolved, config: { skillSync: base } }, + user: { tenantId: 'tenant-a' }, + skillSyncAllowServerCredentials: false, + }); + const runnerConfig = await runners[0].input.getConfig(); + + expect(runnerConfig?.github?.intervalMinutes).toBe(30); + expect(runnerConfig?.github?.sources).toEqual([ + expect.objectContaining({ id: 'tenant-a-source', tenantId: 'tenant-a' }), + expect.objectContaining({ id: 'shared-source', tenantId: undefined }), + ]); + }); + it('does not allow admin override runners to use server credentials by default', async () => { const config = skillSync(); const { orchestrator, runners } = createHarness(); diff --git a/packages/api/src/skills/sync/orchestrator.ts b/packages/api/src/skills/sync/orchestrator.ts index 05ecd64ded..5181799f05 100644 --- a/packages/api/src/skills/sync/orchestrator.ts +++ b/packages/api/src/skills/sync/orchestrator.ts @@ -91,7 +91,7 @@ function getRequestTenantId(user: SkillSyncRequestUser | undefined): string | un function withRequestTenant( config: SkillSyncConfigWithGitHub, user: SkillSyncRequestUser | undefined, - { disableRunOnStartup = false } = {}, + { disableRunOnStartup = false }: { disableRunOnStartup?: boolean } = {}, ): SkillSyncConfig { const tenantId = getRequestTenantId(user); return { @@ -109,6 +109,44 @@ function withRequestTenant( }; } +function isSameGitHubSource( + left: ResolvedGitHubSkillSyncConfig['sources'][number] | undefined, + right: ResolvedGitHubSkillSyncConfig['sources'][number] | undefined, +): boolean { + return JSON.stringify(left ?? null) === JSON.stringify(right ?? null); +} + +function withAdminReadableTenantScope( + config: SkillSyncConfigWithGitHub, + user: SkillSyncRequestUser | undefined, + base: SkillSyncConfigWithGitHub | undefined, +): SkillSyncConfig { + const tenantId = getRequestTenantId(user); + const baseSourceById = new Map(base?.github.sources.map((source) => [source.id, source]) ?? []); + const sources = tenantId + ? config.github.sources.flatMap((source) => { + const baseSource = baseSourceById.get(source.id); + if (isSameGitHubSource(source, baseSource)) { + if (source.tenantId && source.tenantId !== tenantId) { + return []; + } + return [{ ...source, tenantId: source.tenantId ? tenantId : undefined }]; + } + if (source.tenantId && source.tenantId !== tenantId) { + return []; + } + return [{ ...source, tenantId }]; + }) + : []; + return { + ...config, + github: { + ...config.github, + sources, + }, + }; +} + function getRequestSkillSyncConfig( appConfig: SkillSyncAppConfigLike | undefined, user: SkillSyncRequestUser | undefined, @@ -135,6 +173,7 @@ function getAdminRequestSkillSyncConfig( appConfig: SkillSyncAppConfigLike | undefined, user: SkillSyncRequestUser | undefined, logger: SkillSyncTriggerLogger, + { allowServerCredentials = false }: { allowServerCredentials?: boolean } = {}, ): SkillSyncConfig | undefined { const resolved = parseSkillSyncConfig(appConfig?.skillSync, logger); if (!hasGitHubConfig(resolved)) { @@ -142,6 +181,10 @@ function getAdminRequestSkillSyncConfig( } const base = parseSkillSyncConfig(appConfig?.config?.skillSync, logger); + const baseWithGitHub = hasGitHubConfig(base) ? base : undefined; + if (!allowServerCredentials) { + return withAdminReadableTenantScope(resolved, user, baseWithGitHub); + } if (isSameSkillSyncConfig(resolved, base)) { return resolved; } @@ -212,11 +255,14 @@ export function createSkillSyncTriggerOrchestrator( const staleRunningMs = deps.staleRunningMs ?? REQUEST_SYNC_STALE_RUNNING_MS; function getRunnerForAdminRequest(request: SkillSyncRequestLike): GitHubSkillSyncRunner { - const config = getAdminRequestSkillSyncConfig(request.config, request.user, deps.logger); + const allowServerCredentials = Boolean(request.skillSyncAllowServerCredentials); + const config = getAdminRequestSkillSyncConfig(request.config, request.user, deps.logger, { + allowServerCredentials, + }); return deps.createRunner({ getConfig: async () => config, loadAppConfig: async () => request.config, - allowServerCredentials: Boolean(request.skillSyncAllowServerCredentials), + allowServerCredentials, }); }