From a4e51ecc7d42fa18a8fd60b1b80722bce3b57e00 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 4 Jun 2026 22:03:20 -0400 Subject: [PATCH] fix: tighten skill sync trigger safeguards --- api/server/services/Skills/sync.test.js | 57 +++++++++++++++++++ packages/api/src/skills/sync/github.spec.ts | 23 ++++---- packages/api/src/skills/sync/github.ts | 21 ++++--- .../api/src/skills/sync/orchestrator.spec.ts | 47 +++++++++++++-- packages/api/src/skills/sync/orchestrator.ts | 3 + .../data-schemas/src/methods/skill.spec.ts | 17 ++++++ packages/data-schemas/src/schema/skill.ts | 2 + 7 files changed, 145 insertions(+), 25 deletions(-) diff --git a/api/server/services/Skills/sync.test.js b/api/server/services/Skills/sync.test.js index d06ec23b3e..cc6cf5d30f 100644 --- a/api/server/services/Skills/sync.test.js +++ b/api/server/services/Skills/sync.test.js @@ -32,6 +32,9 @@ jest.mock('@librechat/api', () => { provider: 'github', sourceId: source.id, status: 'idle', + credentialPresent: + deps.allowServerCredentials !== false && + Boolean(source.credentialKey || source.token), owner: source.owner, repo: source.repo, ref: source.ref, @@ -175,6 +178,28 @@ describe('GitHub skill sync service', () => { ], }, }; + mockRunnerStatus = { + enabled: true, + intervalMinutes: 60, + runOnStartup: false, + sources: [ + { + provider: 'github', + sourceId: 'tenant-skills', + status: 'idle', + credentialPresent: true, + owner: 'LibreChat', + repo: 'skills', + ref: 'main', + paths: ['skills'], + syncedSkillCount: 0, + syncedFileCount: 0, + deletedSkillCount: 0, + deletedFileCount: 0, + }, + ], + credentials: [], + }; const service = require('./sync'); const started = await service.maybeRunGitHubSkillSyncForRequest({ @@ -196,6 +221,36 @@ describe('GitHub skill sync service', () => { ); }); + it('does not auto-start request-scoped sync when server credentials are unavailable', 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'); + const started = await service.maybeRunGitHubSkillSyncForRequest({ + config: { skillSync, config: {} }, + user: { id: 'user-1', tenantId: 'tenant-a' }, + }); + + expect(started).toBe(false); + expect(mockCreatedRunners[0].deps.allowServerCredentials).toBe(false); + expect(mockCreatedRunners[0].runner.runOnce).not.toHaveBeenCalled(); + }); + it('does not start a request-scoped sync for base YAML skillSync config', async () => { const skillSync = { github: { @@ -317,6 +372,7 @@ describe('GitHub skill sync service', () => { provider: 'github', sourceId: 'tenant-skills', status: 'running', + credentialPresent: true, owner: 'LibreChat', repo: 'skills', ref: 'main', @@ -368,6 +424,7 @@ describe('GitHub skill sync service', () => { provider: 'github', sourceId: 'tenant-skills', status: 'running', + credentialPresent: true, owner: 'LibreChat', repo: 'skills', ref: 'main', diff --git a/packages/api/src/skills/sync/github.spec.ts b/packages/api/src/skills/sync/github.spec.ts index af61d7001b..7cde5c9ad0 100644 --- a/packages/api/src/skills/sync/github.spec.ts +++ b/packages/api/src/skills/sync/github.spec.ts @@ -433,7 +433,7 @@ describe('createGitHubSkillSyncRunner', () => { } }); - it('does not list or resolve server credentials when server credentials are disabled', async () => { + it('does not list or resolve server credentials and skips runs when they 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'); @@ -489,22 +489,22 @@ describe('createGitHubSkillSyncRunner', () => { credentialPresent: false, }), ]); - expect(result.status).toBe('failed'); + expect(result.status).toBe('skipped'); + expect(result.message).toBe( + 'GitHub skill sync credentials are not available for this runner', + ); expect(result.sources).toEqual([ - expect.objectContaining({ - sourceId: 'librechat-skills', - status: 'failed', - errorCode: 'MISSING_CREDENTIAL', - }), + expect.objectContaining({ sourceId: 'librechat-skills', credentialPresent: false }), expect.objectContaining({ sourceId: 'stored-credential-skills', - status: 'failed', - errorCode: 'MISSING_CREDENTIAL', + credentialPresent: false, }), ]); expect(listCredentials).not.toHaveBeenCalled(); expect(getCredentialToken).not.toHaveBeenCalled(); expect(deps.fetchFn).not.toHaveBeenCalled(); + expect(deps.tryAcquireLock).not.toHaveBeenCalled(); + expect(deps.upsertStatus).not.toHaveBeenCalled(); } finally { if (previousToken == null) { delete process.env.GITHUB_SKILLS_TOKEN; @@ -592,9 +592,8 @@ describe('createGitHubSkillSyncRunner', () => { expect(deps.upsertStatus).toHaveBeenCalledWith( expect.objectContaining({ sourceId: 'librechat-skills', tenantId: 'tenant-a' }), ); - expect(deps.tryAcquireLock).toHaveBeenCalledWith( - expect.objectContaining({ tenantId: 'tenant-a' }), - ); + const [lockParams] = (deps.tryAcquireLock as jest.Mock).mock.calls[0]; + expect(lockParams).not.toHaveProperty('tenantId'); }); it('matches stored source status by tenant and source id', async () => { diff --git a/packages/api/src/skills/sync/github.ts b/packages/api/src/skills/sync/github.ts index 82b2f1f71e..bbea40fee6 100644 --- a/packages/api/src/skills/sync/github.ts +++ b/packages/api/src/skills/sync/github.ts @@ -760,11 +760,6 @@ function makeStatusKey(sourceId: string, tenantId?: string): string { return `${tenantId ?? ''}:${sourceId}`; } -function getLockTenantId(sources: SkillSyncGitHubSourceConfig[]): string | undefined { - const tenantIds = new Set(sources.map((source) => source.tenantId).filter(Boolean)); - return tenantIds.size === 1 ? [...tenantIds][0] : undefined; -} - async function ensurePublicViewer( deps: GitHubSkillSyncDeps, skillId: Types.ObjectId, @@ -1540,13 +1535,22 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { if (!github.enabled || github.sources.length === 0) { return { status: 'skipped', message: 'GitHub skill sync is disabled', sources: [] }; } + const allowServerCredentials = deps.allowServerCredentials !== false; + if (!allowServerCredentials) { + const status = await getStatus(); + if (!status.sources.some((source) => source.credentialPresent)) { + return { + status: 'skipped', + message: 'GitHub skill sync credentials are not available for this runner', + sources: status.sources, + }; + } + } const lockOwner = `${lockOwnerPrefix}:${crypto.randomUUID().replace(/-/g, '').slice(0, 12)}`; - const lockTenantId = getLockTenantId(github.sources); const acquired = await deps.tryAcquireLock({ provider: PROVIDER, lockOwner, leaseMs: LOCK_LEASE_MS, - tenantId: lockTenantId, }); if (!acquired) { const status = await getStatus(); @@ -1569,7 +1573,6 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { provider: PROVIDER, lockOwner, leaseMs: LOCK_LEASE_MS, - tenantId: lockTenantId, }) .then((refreshed) => { if (!refreshed) { @@ -1603,7 +1606,7 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) { }; } finally { clearInterval(refreshTimer); - await deps.releaseLock({ provider: PROVIDER, lockOwner, tenantId: lockTenantId }); + await deps.releaseLock({ provider: PROVIDER, lockOwner }); } } diff --git a/packages/api/src/skills/sync/orchestrator.spec.ts b/packages/api/src/skills/sync/orchestrator.spec.ts index 604a5a239e..a6b952be47 100644 --- a/packages/api/src/skills/sync/orchestrator.spec.ts +++ b/packages/api/src/skills/sync/orchestrator.spec.ts @@ -30,7 +30,10 @@ function skillSync( }; } -function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus { +function statusFromConfig( + config: SkillSyncConfig | undefined, + { allowServerCredentials = true }: { allowServerCredentials?: boolean } = {}, +): RunnerStatus { const github = config?.github; return { enabled: github?.enabled ?? false, @@ -43,7 +46,9 @@ function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus { tenantId: configuredSource.tenantId, status: 'idle', credentialKey: configuredSource.credentialKey, - credentialPresent: Boolean(configuredSource.credentialKey || configuredSource.token), + credentialPresent: + allowServerCredentials && + Boolean(configuredSource.credentialKey || configuredSource.token), owner: configuredSource.owner, repo: configuredSource.repo, ref: configuredSource.ref, @@ -66,6 +71,16 @@ function statusFromConfig(config: SkillSyncConfig | undefined): RunnerStatus { }; } +function withRunnableCredentials(status: RunnerStatus): RunnerStatus { + return { + ...status, + sources: status.sources.map((configuredSource) => ({ + ...configuredSource, + credentialPresent: true, + })), + }; +} + function completedRun(): RunnerRunResult { return { status: 'completed', sources: [] }; } @@ -99,7 +114,13 @@ function createHarness( }; const createRunner = jest.fn((input: SkillSyncTriggerRunnerFactoryInput) => { const runner: GitHubSkillSyncRunner = { - getStatus: jest.fn(async () => options.status ?? statusFromConfig(await input.getConfig())), + getStatus: jest.fn( + async () => + options.status ?? + statusFromConfig(await input.getConfig(), { + allowServerCredentials: input.allowServerCredentials !== false, + }), + ), runOnce: jest.fn(options.runOnce ?? (async () => completedRun())), }; runners.push({ input, runner }); @@ -115,7 +136,9 @@ function createHarness( describe('createSkillSyncTriggerOrchestrator', () => { it('starts request sync from resolved admin skillSync config and derives tenant from the request', async () => { const config = skillSync(); - const { orchestrator, runners } = createHarness(); + const { orchestrator, runners } = createHarness({ + status: withRunnableCredentials(statusFromConfig(config)), + }); const started = await orchestrator.maybeRunForRequest({ config: { skillSync: config, config: {} }, @@ -132,6 +155,20 @@ describe('createSkillSyncTriggerOrchestrator', () => { ); }); + it('does not auto-start request sync when only server credentials are configured', async () => { + const config = skillSync(); + const { orchestrator, runners } = createHarness(); + + const started = await orchestrator.maybeRunForRequest({ + config: { skillSync: config, config: {} }, + user: { tenantId: 'tenant-a' }, + }); + + expect(started).toBe(false); + expect(runners[0].input.allowServerCredentials).toBe(false); + expect(runners[0].runner.runOnce).not.toHaveBeenCalled(); + }); + it('does not start request sync for base YAML skillSync config', async () => { const config = skillSync(); const { createRunner, orchestrator } = createHarness(); @@ -209,6 +246,7 @@ describe('createSkillSyncTriggerOrchestrator', () => { { ...statusFromConfig(config).sources[0], status: 'running', + credentialPresent: true, startedAt: new Date(Date.now() - 40 * 60 * 1000), }, ], @@ -228,6 +266,7 @@ describe('createSkillSyncTriggerOrchestrator', () => { const pendingRun = deferred(); const config = skillSync({ runOnStartup: false }); const { orchestrator, runners } = createHarness({ + status: withRunnableCredentials(statusFromConfig(config)), runOnce: () => pendingRun.promise, }); diff --git a/packages/api/src/skills/sync/orchestrator.ts b/packages/api/src/skills/sync/orchestrator.ts index 49710f715f..59fc10e9e6 100644 --- a/packages/api/src/skills/sync/orchestrator.ts +++ b/packages/api/src/skills/sync/orchestrator.ts @@ -173,6 +173,9 @@ function shouldRunRequestSync( const intervalMs = Math.max(minIntervalMs, (status.intervalMinutes ?? 60) * 60 * 1000); const now = Date.now(); return status.sources.some((source) => { + if (!source.credentialPresent) { + return false; + } if (source.status === 'running') { const startedAt = toTimestamp(source.startedAt); return Boolean(startedAt && now - startedAt >= staleRunningMs); diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index 0731ca4c57..3886140e1c 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -139,6 +139,23 @@ function makeSkillInput(overrides: Record = {}) { }; } +describe('Skill schema indexes', () => { + it('supports GitHub sync source metadata lookups', () => { + const indexSpecs = Skill.schema.indexes().map(([fields]) => fields); + + expect(indexSpecs).toContainEqual({ + source: 1, + 'sourceMetadata.upstreamId': 1, + tenantId: 1, + }); + expect(indexSpecs).toContainEqual({ + source: 1, + 'sourceMetadata.sourceId': 1, + tenantId: 1, + }); + }); +}); + describe('skill validation helpers', () => { it('rejects names starting with reserved brand prefixes', () => { expect(validateSkillName('anthropic-helper').some((i) => i.code === 'RESERVED_PREFIX')).toBe( diff --git a/packages/data-schemas/src/schema/skill.ts b/packages/data-schemas/src/schema/skill.ts index 0580d921f7..9ece6d82cc 100644 --- a/packages/data-schemas/src/schema/skill.ts +++ b/packages/data-schemas/src/schema/skill.ts @@ -229,5 +229,7 @@ skillSchema.index({ author: 1, tenantId: 1 }); skillSchema.index({ category: 1, updatedAt: -1 }); skillSchema.index({ updatedAt: -1, _id: 1 }); skillSchema.index({ name: 1, author: 1, tenantId: 1 }, { unique: true }); +skillSchema.index({ source: 1, 'sourceMetadata.upstreamId': 1, tenantId: 1 }); +skillSchema.index({ source: 1, 'sourceMetadata.sourceId': 1, tenantId: 1 }); export default skillSchema;