diff --git a/packages/docs/public/llms.txt b/packages/docs/public/llms.txt index aa87a9ec..f837ea85 100644 --- a/packages/docs/public/llms.txt +++ b/packages/docs/public/llms.txt @@ -192,7 +192,7 @@ Skills define what Warden analyzes and when. | `ignorePaths` | Files to exclude (glob patterns) | | `failOn` | Minimum severity to fail: `critical`, `high`, `medium`, `low`, `info`, `off` | | `reportOn` | Minimum severity to report | -| `remote` | GitHub repository for remote skills: `owner/repo` or `owner/repo@sha` | +| `remote` | GitHub repository for remote skills: `owner/repo` or `owner/repo@sha`. Hosted private remotes use `github-token` auth on `github.com`. | | `model` | Model override | | `maxTurns` | Max agentic turns per hunk | @@ -473,7 +473,7 @@ jobs: | Input | Default | Description | |-------|---------|-------------| -| `github-token` | `GITHUB_TOKEN` | GitHub token for posting comments | +| `github-token` | `GITHUB_TOKEN` | GitHub token for posting comments and private remote-skill fetch auth in hosted runs | | `anthropic-api-key` | - | Anthropic API key (falls back to `WARDEN_ANTHROPIC_API_KEY`) | | `config-path` | `warden.toml` | Path to config file | | `fail-on` | - | Minimum severity to fail the check | diff --git a/packages/docs/src/pages/config.astro b/packages/docs/src/pages/config.astro index 72c01707..300dfa68 100644 --- a/packages/docs/src/pages/config.astro +++ b/packages/docs/src/pages/config.astro @@ -62,7 +62,10 @@ actions = ["opened", "synchronize"]`}
reportOn
Minimum severity to report
remote
-
GitHub repository for remote skills: owner/repo or owner/repo@sha
+
+ GitHub repository for remote skills: owner/repo or owner/repo@sha. + In hosted GitHub Actions runs, private remotes use github-token auth on github.com. +
model
Model override (optional)
maxTurns
@@ -448,7 +451,7 @@ jobs:
github-token
-
GitHub token for posting comments. Default: GITHUB_TOKEN
+
GitHub token for posting comments and private remote-skill fetch auth in hosted runs. Default: GITHUB_TOKEN
anthropic-api-key
Anthropic API key (falls back to WARDEN_ANTHROPIC_API_KEY)
config-path
@@ -466,5 +469,13 @@ jobs:
parallel
Maximum concurrent trigger executions. Default: 5
+

Private Remote Troubleshooting

+ + diff --git a/specs/private-remote-git-auth.md b/specs/private-remote-git-auth.md new file mode 100644 index 00000000..a8f0ffd0 --- /dev/null +++ b/specs/private-remote-git-auth.md @@ -0,0 +1,165 @@ +# Private Remote Git Auth + +## Problem + +Remote skills can be sourced from GitHub repositories via `skills[].remote`. + +That works for public repositories and for private repositories when the runtime can use SSH credentials. It does not work reliably in hosted GitHub Actions runs, where: + +- the checkout token is available as `GITHUB_TOKEN` +- SSH keys are usually not configured +- remote skill repositories may be private + +The result is that private remote skills are difficult to use in hosted runs even when GitHub already issued a token with the right repository access. + +## Goals + +- Allow hosted GitHub Action runs to fetch private remote skills from `github.com` using the existing `github-token` input. +- Preserve current behavior for public remotes and for non-GitHub remotes. +- Avoid embedding credentials in clone URLs, persisted cache state, or user-visible error messages. +- Keep remote resolution behavior consistent across PR and schedule workflows. + +## Non-Goals + +- Add a general-purpose credential system for arbitrary git hosts. +- Persist credentials in git config, state files, or cache directories. +- Change CLI auth flows beyond using the options already threaded into remote resolution. +- Add token discovery from new environment variables as part of this change. + +## User-Facing Behavior + +### Configuration + +No config format changes are required. + +Remote skills continue to use: + +- `owner/repo` +- `owner/repo@sha` +- `https://github.com/owner/repo.git` +- `git@github.com:owner/repo.git` + +### Hosted GitHub Actions + +When Warden resolves a remote skill or agent in GitHub Actions: + +- the action passes `github-token` into remote resolution +- if the remote resolves to `github.com` and the token is non-empty, fetches use one-shot git auth env injection +- authenticated runtime transport uses `https://github.com/owner/repo.git` +- configured remote syntax is still preserved in cache metadata for future non-authenticated refreshes + +### Non-GitHub Remotes + +If a cached or configured remote is not a `github.com` remote: + +- no GitHub auth env is injected +- existing clone/fetch behavior is preserved +- failures from that host should surface as the underlying git failure, not as GitHub-token guidance + +## Auth Model + +### Token Source + +This change relies on the existing action input: + +- `github-token`, which already defaults to `GITHUB_TOKEN` + +Whitespace-only values are treated as unset. + +### Git Transport + +For authenticated `github.com` fetches: + +- Warden does not rewrite the configured remote reference +- Warden builds a per-command git environment using `http.https://github.com/.extraheader` +- the Authorization header is sent only for that git subprocess +- the token is never placed in the clone URL + +### Persistence Rules + +Warden may persist the original remote URL form in cache state for refresh behavior. + +Warden must not persist: + +- the token +- an authenticated URL +- an auth header + +## Resolution Rules + +### Remote Detection + +GitHub auth is used only when both conditions hold: + +1. A non-empty `githubToken` is present. +2. The effective remote is a `github.com` remote. + +The effective remote is determined from: + +- the explicit remote URL from the current ref, if present +- otherwise the stored `cloneUrl` in cache state, if present +- otherwise shorthand `owner/repo`, which is treated as GitHub + +### Refresh Semantics + +For unpinned remotes: + +- cached explicit remote URLs remain authoritative for refresh behavior +- cached SSH remotes continue to refresh via SSH unless GitHub auth is actively being used for that fetch +- authenticated GitHub fetches reset to `FETCH_HEAD` + +For pinned remotes: + +- the cached SHA remains authoritative +- the repository is only fetched when not cached or when force-refresh behavior requires it + +## Error Semantics + +### Authenticated GitHub Fetches + +When the authenticated GitHub path is in use and git returns an auth-shaped failure, Warden should raise a high-signal loader error: + +- `Failed to authenticate when cloning owner/repo. Ensure the provided GitHub token has read access ...` + +The original error must be preserved as `cause`. + +### Unauthenticated Shorthand HTTPS Fetches + +When a shorthand GitHub remote falls back to HTTPS without a token and git cannot prompt for credentials, Warden should raise guidance that points to: + +- providing a GitHub token +- or using the SSH remote form + +### Non-GitHub Failures + +If GitHub auth was not used, Warden should not rewrite failures as GitHub token problems merely because a token happened to be present. + +## Integration Points + +The token must be threaded through: + +- action input parsing +- PR workflow trigger execution +- schedule workflow skill execution +- loader remote resolution for both skills and agents + +This change does not require schema changes in `warden.toml`. + +## Tests + +Required coverage: + +- token is threaded from action workflows into remote resolution +- whitespace-only tokens are ignored +- GitHub SSH remotes use runtime HTTPS plus auth env when a token is present +- non-GitHub remotes do not receive GitHub auth env +- non-GitHub failures are not rewritten into GitHub-specific auth errors +- auth-related errors preserve the original cause +- tokens do not appear in thrown messages +- concurrent fetches keep auth env isolated per command + +## Operational Notes + +- GitHub token must have repository read access to the remote skill repository. +- For GitHub App tokens, the app must be installed on both the code repository and the remote skill repository. +- This spec intentionally limits authenticated remote support to `github.com`. diff --git a/src/action/triggers/executor.test.ts b/src/action/triggers/executor.test.ts index b262f9f4..1318e5e4 100644 --- a/src/action/triggers/executor.test.ts +++ b/src/action/triggers/executor.test.ts @@ -31,6 +31,7 @@ vi.mock('../../output/renderer.js', () => ({ import { runSkillTask } from '../../cli/output/tasks.js'; import { createSkillCheck, updateSkillCheck, failSkillCheck } from '../../output/github-checks.js'; import { renderSkillReport } from '../../output/renderer.js'; +import { resolveSkillAsync } from '../../skills/loader.js'; describe('executeTrigger', () => { // Suppress console output during tests @@ -75,6 +76,7 @@ describe('executeTrigger', () => { context: mockContext, config: mockConfig, anthropicApiKey: 'test-key', + githubToken: 'gh-token', claudePath: '/test/claude', globalMaxFindings: 10, }; @@ -102,7 +104,12 @@ describe('executeTrigger', () => { vi.mocked(updateSkillCheck).mockResolvedValue(undefined); vi.mocked(renderSkillReport).mockReturnValue(mockRenderResult); - const result = await executeTrigger(mockTrigger, mockDeps); + const triggerWithRemote: ResolvedTrigger = { + ...mockTrigger, + remote: 'owner/repo', + }; + + const result = await executeTrigger(triggerWithRemote, mockDeps); expect(result.triggerName).toBe('test-trigger'); expect(result.report).toBe(mockReport); @@ -122,6 +129,14 @@ describe('executeTrigger', () => { minConfidence: 'medium', failCheck: undefined, }); + + const taskOptions = vi.mocked(runSkillTask).mock.calls[0]?.[0]; + expect(taskOptions).toBeDefined(); + await taskOptions?.resolveSkill(); + expect(resolveSkillAsync).toHaveBeenCalledWith('test-skill', '/test/path', { + remote: 'owner/repo', + githubToken: 'gh-token', + }); }); it('executes a trigger successfully with no findings', async () => { diff --git a/src/action/triggers/executor.ts b/src/action/triggers/executor.ts index 42c77e46..90a19761 100644 --- a/src/action/triggers/executor.ts +++ b/src/action/triggers/executor.ts @@ -44,6 +44,7 @@ export interface TriggerExecutorDeps { context: EventContext; config: WardenConfig; anthropicApiKey: string; + githubToken?: string; claudePath: string; /** Global fail-on from action inputs (trigger-specific takes precedence) */ globalFailOn?: SeverityThreshold; @@ -131,6 +132,7 @@ export async function executeTrigger( failOn, resolveSkill: () => resolveSkillAsync(trigger.skill, context.repoPath, { remote: trigger.remote, + githubToken: deps.githubToken, }), context: filterContextByPaths(context, trigger.filters), runnerOptions: { diff --git a/src/action/workflow/pr-workflow.ts b/src/action/workflow/pr-workflow.ts index 07a94018..bcbd7a68 100644 --- a/src/action/workflow/pr-workflow.ts +++ b/src/action/workflow/pr-workflow.ts @@ -265,6 +265,7 @@ async function executeAllTriggers( context, config, anthropicApiKey: inputs.anthropicApiKey, + githubToken: inputs.githubToken, claudePath, globalFailOn: inputs.failOn, globalReportOn: inputs.reportOn, @@ -413,7 +414,7 @@ async function evaluateFixesAndResolveStale( logAction(`Resolved ${resolvedCount} comments via fix evaluation`); } // Track only actually resolved comments for allResolved check - resolvedIds.forEach((id) => commentsResolvedByFixEval.add(id)); + for (const id of resolvedIds) commentsResolvedByFixEval.add(id); } // Post replies for failed fixes and track them so stale pass doesn't override @@ -477,7 +478,7 @@ async function evaluateFixesAndResolveStale( emitStaleResolutionMetric(count, skill); } } - resolvedIds.forEach((id) => commentsResolvedByStale.add(id)); + for (const id of resolvedIds) commentsResolvedByStale.add(id); } } catch (error) { Sentry.captureException(error, { tags: { operation: 'resolve_stale_comments' } }); diff --git a/src/action/workflow/schedule.test.ts b/src/action/workflow/schedule.test.ts index 5591bccb..49ddebe1 100644 --- a/src/action/workflow/schedule.test.ts +++ b/src/action/workflow/schedule.test.ts @@ -289,6 +289,10 @@ describe('runScheduleWorkflow', () => { await runScheduleWorkflow(mockOctokit, createDefaultInputs(), SCHEDULE_FIXTURES); + expect(mockResolveSkillAsync).toHaveBeenCalledWith('test-skill', SCHEDULE_FIXTURES, { + remote: undefined, + githubToken: 'test-github-token', + }); expect(mockRunSkill).toHaveBeenCalledTimes(1); expect(mockCreateOrUpdateIssue).toHaveBeenCalledWith( mockOctokit, diff --git a/src/action/workflow/schedule.ts b/src/action/workflow/schedule.ts index 36e499e3..2aa2eeec 100644 --- a/src/action/workflow/schedule.ts +++ b/src/action/workflow/schedule.ts @@ -146,6 +146,7 @@ export async function runScheduleWorkflow( // Run skill const skill = await resolveSkillAsync(resolved.skill, repoPath, { remote: resolved.remote, + githubToken: inputs.githubToken, }); const claudePath = await findClaudeCodeExecutable(); const report = await runSkill(skill, context, { diff --git a/src/skills/auth-options.ts b/src/skills/auth-options.ts new file mode 100644 index 00000000..dcda4488 --- /dev/null +++ b/src/skills/auth-options.ts @@ -0,0 +1,15 @@ +/** + * Optional authentication options for remote skill/agent fetches. + */ +export interface RemoteAuthOptions { + /** + * GitHub token for authenticating private remote skill/agent fetches. + * + * Accepts: PAT (classic/fine-grained), GitHub App token, or GITHUB_TOKEN. + * Must have repository read access to the remote skill repo. + * For GitHub Apps, the app must be installed on the remote repo. + * + * Whitespace-only values are treated as unset (useful for CI env vars). + */ + githubToken?: string; +} diff --git a/src/skills/index.ts b/src/skills/index.ts index 5fea4746..e6557be2 100644 --- a/src/skills/index.ts +++ b/src/skills/index.ts @@ -22,6 +22,8 @@ export type { ResolveSkillOptions, } from './loader.js'; +export type { RemoteAuthOptions } from './auth-options.js'; + export { parseRemoteRef, formatRemoteRef, diff --git a/src/skills/loader.test.ts b/src/skills/loader.test.ts index 36960082..586103a7 100644 --- a/src/skills/loader.test.ts +++ b/src/skills/loader.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterAll } from 'vitest'; +import { describe, it, expect, beforeEach, afterAll, vi } from 'vitest'; import { join } from 'node:path'; import { homedir } from 'node:os'; import { writeFileSync, unlinkSync, mkdtempSync, mkdirSync, rmSync } from 'node:fs'; @@ -18,6 +18,13 @@ import { AGENT_MARKER_FILE, } from './loader.js'; +vi.mock('./remote.js', () => ({ + resolveRemoteSkill: vi.fn(), + resolveRemoteAgent: vi.fn(), +})); + +import { resolveRemoteSkill, resolveRemoteAgent } from './remote.js'; + describe('loadSkillFromFile', () => { it('rejects unsupported file types', async () => { await expect(loadSkillFromFile('/path/to/skill.json')).rejects.toThrow(SkillLoaderError); @@ -41,6 +48,46 @@ describe('resolveSkillAsync', () => { await expect(resolveSkillAsync('nonexistent-skill')).rejects.toThrow(SkillLoaderError); await expect(resolveSkillAsync('nonexistent-skill')).rejects.toThrow('Skill not found'); }); + + it('forwards githubToken and offline to remote skill resolution', async () => { + vi.mocked(resolveRemoteSkill).mockResolvedValue({ + name: 'remote-skill', + description: 'from remote', + prompt: 'prompt', + }); + + await resolveSkillAsync('remote-skill', '/tmp/repo', { + remote: 'owner/repo', + offline: true, + githubToken: 'test-token', + }); + + expect(resolveRemoteSkill).toHaveBeenCalledWith('owner/repo', 'remote-skill', { + offline: true, + githubToken: 'test-token', + }); + }); +}); + +describe('resolveAgentAsync', () => { + it('forwards githubToken and offline to remote agent resolution', async () => { + vi.mocked(resolveRemoteAgent).mockResolvedValue({ + name: 'remote-agent', + description: 'from remote', + prompt: 'prompt', + }); + + await resolveAgentAsync('remote-agent', '/tmp/repo', { + remote: 'owner/repo', + offline: true, + githubToken: 'test-token', + }); + + expect(resolveRemoteAgent).toHaveBeenCalledWith('owner/repo', 'remote-agent', { + offline: true, + githubToken: 'test-token', + }); + }); }); describe('skills caching', () => { diff --git a/src/skills/loader.ts b/src/skills/loader.ts index 091cb3e6..3969cb61 100644 --- a/src/skills/loader.ts +++ b/src/skills/loader.ts @@ -4,6 +4,7 @@ import { existsSync } from 'node:fs'; import { homedir } from 'node:os'; import type { SkillDefinition, ToolName } from '../config/schema.js'; import { ToolNameSchema } from '../config/schema.js'; +import type { RemoteAuthOptions } from './auth-options.js'; export class SkillLoaderError extends Error { constructor(message: string, options?: { cause?: unknown }) { @@ -139,7 +140,7 @@ function parseMarkdownFrontmatter(content: string): { frontmatter: Record { - const { remote, offline } = options ?? {}; + const { remote, offline, githubToken } = options ?? {}; // 1. Remote repository resolution takes priority when specified if (remote) { // Dynamic import to avoid circular dependencies const { resolveRemoteSkill, resolveRemoteAgent } = await import('./remote.js'); const resolver = config.kind === 'skill' ? resolveRemoteSkill : resolveRemoteAgent; - return resolver(remote, nameOrPath, { offline }); + return resolver(remote, nameOrPath, { offline, githubToken }); } // 2. Direct path resolution diff --git a/src/skills/remote-auth.test.ts b/src/skills/remote-auth.test.ts new file mode 100644 index 00000000..f45da8cd --- /dev/null +++ b/src/skills/remote-auth.test.ts @@ -0,0 +1,229 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { join } from 'node:path'; +import { mkdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; + +vi.mock('../utils/exec.js', () => ({ + execGitNonInteractive: vi.fn(), +})); + +import { execGitNonInteractive } from '../utils/exec.js'; +import { fetchRemote, getRemotePath, saveState } from './remote.js'; +import { SkillLoaderError } from './loader.js'; + +describe('fetchRemote auth behavior', () => { + const originalStateDir = process.env['WARDEN_STATE_DIR']; + let stateDir: string; + + beforeEach(() => { + stateDir = join(tmpdir(), `warden-remote-auth-${Date.now()}-${Math.random().toString(36).slice(2)}`); + process.env['WARDEN_STATE_DIR'] = stateDir; + mkdirSync(stateDir, { recursive: true }); + + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'clone') { + const targetDir = args[args.length - 1]; + if (typeof targetDir === 'string') { + mkdirSync(targetDir, { recursive: true }); + } + return ''; + } + if (args[0] === 'rev-parse') return 'deadbeef'; + return ''; + }); + }); + + afterEach(() => { + if (originalStateDir === undefined) { + delete process.env['WARDEN_STATE_DIR']; + } else { + process.env['WARDEN_STATE_DIR'] = originalStateDir; + } + rmSync(stateDir, { recursive: true, force: true }); + vi.clearAllMocks(); + }); + + it('uses runtime HTTPS clone URL and auth env for GitHub SSH remotes when token is set', async () => { + await fetchRemote('git@github.com:owner/repo.git', { githubToken: 'test-token' }); + + const cloneCall = vi.mocked(execGitNonInteractive).mock.calls.find((call) => call[0][0] === 'clone'); + expect(cloneCall).toBeDefined(); + expect(cloneCall?.[0]).toEqual(['clone', '--depth=1', '--', 'https://github.com/owner/repo.git', expect.any(String)]); + + const cloneOptions = cloneCall?.[1]; + expect(cloneOptions?.env?.['GIT_CONFIG_COUNT']).toBe('1'); + expect(cloneOptions?.env?.['GIT_CONFIG_KEY_0']).toBe('http.https://github.com/.extraheader'); + expect(cloneOptions?.env?.['GIT_CONFIG_VALUE_0']).toContain('AUTHORIZATION: basic '); + expect(cloneCall?.[0].join(' ')).not.toContain('test-token'); + }); + + it('treats whitespace-only tokens as unset', async () => { + await fetchRemote('git@github.com:owner/repo.git', { githubToken: ' ' }); + + const cloneCall = vi.mocked(execGitNonInteractive).mock.calls.find((call) => call[0][0] === 'clone'); + expect(cloneCall).toBeDefined(); + expect(cloneCall?.[0]).toEqual(['clone', '--depth=1', '--', 'git@github.com:owner/repo.git', expect.any(String)]); + + const cloneOptions = cloneCall?.[1]; + expect(cloneOptions?.env?.['GIT_CONFIG_COUNT']).toBeUndefined(); + expect(cloneOptions?.env?.['GIT_CONFIG_KEY_0']).toBeUndefined(); + expect(cloneOptions?.env?.['GIT_CONFIG_VALUE_0']).toBeUndefined(); + }); + + it('does not apply github auth env for persisted non-github clone URLs', async () => { + const remotePath = getRemotePath('owner/repo'); + mkdirSync(remotePath, { recursive: true }); + saveState({ + remotes: { + 'owner/repo': { + sha: 'abc123', + fetchedAt: new Date().toISOString(), + cloneUrl: 'https://example.com/owner/repo.git', + }, + }, + }); + + await fetchRemote('owner/repo', { githubToken: 'test-token', force: true }); + + const fetchCall = vi.mocked(execGitNonInteractive).mock.calls.find((call) => call[0][0] === 'fetch'); + expect(fetchCall).toBeDefined(); + expect(fetchCall?.[0]).toEqual(['fetch', '--depth=1', '--', 'https://example.com/owner/repo.git']); + const fetchOptions = fetchCall?.[1]; + expect(fetchOptions?.env?.['GIT_CONFIG_COUNT']).toBeUndefined(); + }); + + it('sanitizes token from auth-failure error messages', async () => { + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'clone') { + throw new Error('authentication failed for test-token'); + } + return ''; + }); + + await expect(fetchRemote('owner/repo', { githubToken: 'test-token' })) + .rejects.toThrow('Failed to authenticate when cloning owner/repo'); + await expect(fetchRemote('owner/repo', { githubToken: 'test-token' })) + .rejects.not.toThrow('test-token'); + }); + + it('preserves original auth failure as cause for tokenized fetches', async () => { + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'clone') { + throw new Error('fatal: authentication failed'); + } + return ''; + }); + + try { + await fetchRemote('owner/repo', { githubToken: 'test-token' }); + throw new Error('expected fetchRemote to throw'); + } catch (error) { + expect(error).toBeInstanceOf(SkillLoaderError); + expect((error as Error).cause).toBeInstanceOf(Error); + expect(((error as Error).cause as Error).message).toContain('authentication failed'); + } + }); + + it('preserves helpful auth guidance on github cache refresh failures', async () => { + const remotePath = getRemotePath('owner/repo'); + mkdirSync(remotePath, { recursive: true }); + saveState({ + remotes: { + 'owner/repo': { + sha: 'abc123', + fetchedAt: new Date().toISOString(), + }, + }, + }); + + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'fetch') { + throw new Error('fatal: authentication failed'); + } + if (args[0] === 'rev-parse') return 'deadbeef'; + return ''; + }); + + try { + await fetchRemote('owner/repo', { githubToken: 'test-token', force: true }); + throw new Error('expected fetchRemote to throw'); + } catch (error) { + expect(error).toBeInstanceOf(SkillLoaderError); + expect((error as Error).message).toContain('Failed to authenticate when fetching owner/repo'); + expect((error as Error).cause).toBeInstanceOf(Error); + expect(((error as Error).cause as Error).message).toContain('authentication failed'); + } + }); + + it('preserves original HTTPS prompt failure as cause for unauthenticated shorthand refs', async () => { + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'clone') { + throw new Error('fatal: could not read Username for \'https://github.com\': terminal prompts disabled'); + } + return ''; + }); + + try { + await fetchRemote('owner/repo'); + throw new Error('expected fetchRemote to throw'); + } catch (error) { + expect(error).toBeInstanceOf(SkillLoaderError); + expect((error as Error).cause).toBeInstanceOf(Error); + expect(((error as Error).cause as Error).message).toContain('terminal prompts disabled'); + } + }); + + it('does not remap auth-like failures for non-github remotes when a github token is present', async () => { + const remotePath = getRemotePath('owner/repo'); + mkdirSync(remotePath, { recursive: true }); + saveState({ + remotes: { + 'owner/repo': { + sha: 'abc123', + fetchedAt: new Date().toISOString(), + cloneUrl: 'https://example.com/owner/repo.git', + }, + }, + }); + + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'fetch') { + throw new Error('fatal: authentication failed'); + } + if (args[0] === 'rev-parse') return 'deadbeef'; + return ''; + }); + + await expect(fetchRemote('owner/repo', { githubToken: 'test-token', force: true })) + .rejects.toThrow('Git command failed: git fetch --depth=1 -- https://example.com/owner/repo.git: fatal: authentication failed'); + await expect(fetchRemote('owner/repo', { githubToken: 'test-token', force: true })) + .rejects.not.toThrow('Failed to authenticate when cloning owner/repo'); + }); + + it('suggests a generic github token for unauthenticated shorthand https failures', async () => { + vi.mocked(execGitNonInteractive).mockImplementation((args: string[]) => { + if (args[0] === 'clone') { + throw new Error('fatal: could not read Username for \'https://github.com\': terminal prompts disabled'); + } + return ''; + }); + + await expect(fetchRemote('owner/repo')) + .rejects.toThrow('provide a GitHub token or use the SSH URL'); + await expect(fetchRemote('owner/repo')) + .rejects.not.toThrow('WARDEN_GITHUB_TOKEN'); + }); + + it('keeps per-command auth env isolated across concurrent fetches', async () => { + await Promise.all([ + fetchRemote('owner/repo-a', { githubToken: 'token-a' }), + fetchRemote('owner/repo-b', { githubToken: 'token-b' }), + ]); + + const cloneCalls = vi.mocked(execGitNonInteractive).mock.calls.filter((call) => call[0][0] === 'clone'); + expect(cloneCalls).toHaveLength(2); + const headers = cloneCalls.map((call) => call[1]?.env?.['GIT_CONFIG_VALUE_0']); + expect(headers[0]).not.toBe(headers[1]); + expect(headers.every((h) => typeof h === 'string' && h.startsWith('AUTHORIZATION: basic '))).toBe(true); + }); +}); diff --git a/src/skills/remote.test.ts b/src/skills/remote.test.ts index 6348537c..b84f758a 100644 --- a/src/skills/remote.test.ts +++ b/src/skills/remote.test.ts @@ -694,6 +694,23 @@ metadata: expect(skills[0]?.name).toBe('good-skill'); expect(skills[0]?.pluginName).toBe('legit'); }); + + it('ignores plugins with absolute source paths', async () => { + const remotePath = getRemotePath('test/repo'); + createFileTree(remotePath, { + '.claude-plugin/marketplace.json': marketplaceJson([ + { name: 'absolute', source: '/tmp/absolute-plugin' }, + { name: 'legit', source: './plugins/legit' }, + ]), + 'plugins/legit/skills/good-skill/SKILL.md': skillMd('good-skill', 'Legit skill'), + }); + + const skills = await discoverRemoteSkills('test/repo'); + + expect(skills.length).toBe(1); + expect(skills[0]?.name).toBe('good-skill'); + expect(skills[0]?.pluginName).toBe('legit'); + }); }); }); diff --git a/src/skills/remote.ts b/src/skills/remote.ts index 5ac73568..56951342 100644 --- a/src/skills/remote.ts +++ b/src/skills/remote.ts @@ -1,9 +1,10 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync, rmSync, renameSync, readdirSync, statSync } from 'node:fs'; import { homedir } from 'node:os'; -import { dirname, join, resolve } from 'node:path'; +import { dirname, isAbsolute, join, relative, resolve } from 'node:path'; import { z } from 'zod'; import { execGitNonInteractive } from '../utils/exec.js'; import { loadSkillFromMarkdown, SkillLoaderError, AGENT_MARKER_FILE } from './loader.js'; +import type { RemoteAuthOptions } from './auth-options.js'; import type { SkillDefinition } from '../config/schema.js'; /** Default TTL for unpinned remote skills: 24 hours */ @@ -257,8 +258,8 @@ export function saveState(state: RemoteState): void { export function getCacheTtlSeconds(): number { const envTtl = process.env['WARDEN_SKILL_CACHE_TTL']; if (envTtl) { - const parsed = parseInt(envTtl, 10); - if (!isNaN(parsed) && parsed > 0) { + const parsed = Number.parseInt(envTtl, 10); + if (!Number.isNaN(parsed) && parsed > 0) { return parsed; } } @@ -289,7 +290,7 @@ export function shouldRefresh(ref: string, state: RemoteState): boolean { return now - fetchedAt > ttl; } -export interface FetchRemoteOptions { +export interface FetchRemoteOptions extends RemoteAuthOptions { /** Force refresh even if cache is valid */ force?: boolean; /** Skip network operations - only use cache */ @@ -298,14 +299,68 @@ export interface FetchRemoteOptions { onProgress?: (message: string) => void; } +/** + * Normalize token input from env/action inputs. + * Empty-string values in CI should behave as "unset". + */ +function normalizeToken(token?: string): string | undefined { + const normalized = token?.trim(); + return normalized ? normalized : undefined; +} + +/** + * Build one-shot git auth environment for GitHub HTTPS requests. + * Uses a transient extraheader, avoiding tokenized URLs or persisted git config. + */ +function buildGitHubAuthEnv(token: string): Record { + const basic = Buffer.from(`x-access-token:${token}`).toString('base64'); + return { + GIT_CONFIG_COUNT: '1', + GIT_CONFIG_KEY_0: 'http.https://github.com/.extraheader', + GIT_CONFIG_VALUE_0: `AUTHORIZATION: basic ${basic}`, + }; +} + +/** + * Check whether this remote points to github.com. + */ +function isGitHubRemote(parsed: ParsedRemoteRef, cloneUrl?: string): boolean { + // owner/repo shorthand is GitHub unless an explicit non-GitHub URL is persisted in state. + const effectiveUrl = cloneUrl ?? parsed.cloneUrl; + if (!effectiveUrl) return true; + return normalizeGitHubUrl(effectiveUrl) !== null; +} + +/** + * Runtime clone URL for authenticated GitHub fetches. + * Deliberately separate from stored cloneUrl/state. + */ +function getGitHubHttpsUrl(parsed: ParsedRemoteRef): string { + return `https://github.com/${parsed.owner}/${parsed.repo}.git`; +} + +function isGitAuthFailure(message: string): boolean { + const lower = message.toLowerCase(); + return ( + lower.includes('authentication failed') || + lower.includes('could not read username') || + lower.includes('terminal prompts disabled') || + lower.includes('repository not found') || + lower.includes('http basic: access denied') || + lower.includes('permission denied') || + lower.includes('access denied') || + lower.includes('403') + ); +} + /** * Execute a git command and return stdout. * Uses non-interactive mode to prevent SSH passphrase prompts. * Throws SkillLoaderError on failure. */ -function execGit(args: string[], options?: { cwd?: string }): string { +function execGit(args: string[], options?: { cwd?: string; env?: Record }): string { try { - return execGitNonInteractive(args, { cwd: options?.cwd }); + return execGitNonInteractive(args, { cwd: options?.cwd, env: options?.env }); } catch (error) { const message = error instanceof Error ? error.message : String(error); throw new SkillLoaderError(`Git command failed: git ${args.join(' ')}: ${message}`, { cause: error }); @@ -351,8 +406,15 @@ export async function fetchRemote(ref: string, options: FetchRemoteOptions = {}) return stateEntry.sha; } - // Use the original clone URL if provided, fall back to stored URL from state, then HTTPS - const repoUrl = parsed.cloneUrl ?? stateEntry?.cloneUrl ?? `https://github.com/${parsed.owner}/${parsed.repo}.git`; + const sourceCloneUrl = parsed.cloneUrl ?? stateEntry?.cloneUrl; + const token = normalizeToken(options.githubToken); + const useGitHubAuth = !!token && isGitHubRemote(parsed, sourceCloneUrl); + const gitAuthEnv = token && useGitHubAuth ? buildGitHubAuthEnv(token) : undefined; + + // Use runtime HTTPS for authenticated GitHub fetches. Otherwise preserve existing URL semantics. + const repoUrl = useGitHubAuth + ? getGitHubHttpsUrl(parsed) + : (sourceCloneUrl ?? `https://github.com/${parsed.owner}/${parsed.repo}.git`); // Clone or update if (!isCached) { @@ -369,28 +431,35 @@ export async function fetchRemote(ref: string, options: FetchRemoteOptions = {}) const { sha } = parsed; if (sha) { // For pinned refs, shallow clone then checkout the specific SHA - execGit(['clone', '--depth=1', '--', repoUrl, remotePath]); + execGit(['clone', '--depth=1', '--', repoUrl, remotePath], { env: gitAuthEnv }); try { // Try to fetch the pinned SHA directly - execGit(['fetch', '--depth=1', 'origin', '--', sha], { cwd: remotePath }); + execGit(['fetch', '--depth=1', 'origin', '--', sha], { cwd: remotePath, env: gitAuthEnv }); execGit(['checkout', sha], { cwd: remotePath }); } catch { // If SHA not found in shallow history, do a full fetch and retry - execGit(['fetch', '--unshallow'], { cwd: remotePath }); + execGit(['fetch', '--unshallow'], { cwd: remotePath, env: gitAuthEnv }); execGit(['checkout', sha], { cwd: remotePath }); } } else { // For unpinned refs, shallow clone of default branch - execGit(['clone', '--depth=1', '--', repoUrl, remotePath]); + execGit(['clone', '--depth=1', '--', repoUrl, remotePath], { env: gitAuthEnv }); } } catch (error) { const message = error instanceof Error ? error.message : String(error); - // Detect HTTPS auth failures and suggest SSH URL - if (!parsed.cloneUrl && (message.includes('terminal prompts disabled') || message.includes('could not read Username'))) { + if (useGitHubAuth && isGitAuthFailure(message)) { throw new SkillLoaderError( - `Failed to clone ${stateKey} via HTTPS. ` + - `For private repos, use the SSH URL: warden add --remote git@github.com:${parsed.owner}/${parsed.repo}.git` + `Failed to authenticate when cloning ${stateKey}. ` + + `Ensure the provided GitHub token has read access to ${parsed.owner}/${parsed.repo}.`, + { cause: error } + ); + } + // Unauthenticated shorthand HTTPS failure: provide explicit auth guidance. + if (!token && !parsed.cloneUrl && (message.includes('terminal prompts disabled') || message.includes('could not read Username'))) { + throw new SkillLoaderError( + `Failed to clone ${stateKey} via HTTPS. For private repos, provide a GitHub token or use the SSH URL: warden add --remote git@github.com:${parsed.owner}/${parsed.repo}.git`, + { cause: error } ); } throw error; @@ -400,9 +469,26 @@ export async function fetchRemote(ref: string, options: FetchRemoteOptions = {}) onProgress?.(`Updating ${ref}...`); if (!isPinned) { - // For unpinned refs, pull latest - execGit(['fetch', '--depth=1', 'origin'], { cwd: remotePath }); - execGit(['reset', '--hard', 'origin/HEAD'], { cwd: remotePath }); + try { + // For unpinned refs, pull latest from the explicit remote URL so cached SSH remotes + // keep their transport semantics across refreshes. + if (useGitHubAuth) { + execGit(['fetch', '--depth=1', '--', repoUrl], { cwd: remotePath, env: gitAuthEnv }); + } else { + execGit(['fetch', '--depth=1', '--', repoUrl], { cwd: remotePath }); + } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (useGitHubAuth && isGitAuthFailure(message)) { + throw new SkillLoaderError( + `Failed to authenticate when fetching ${stateKey}. ` + + `Ensure the provided GitHub token has read access to ${parsed.owner}/${parsed.repo}.`, + { cause: error } + ); + } + throw error; + } + execGit(['reset', '--hard', 'FETCH_HEAD'], { cwd: remotePath }); } // Pinned refs don't need updates - SHA is immutable } @@ -537,7 +623,8 @@ async function discoverMarketplaceSkills( // Security: Ensure plugin source doesn't escape the repo directory via path traversal const resolvedSkillsPath = resolve(skillsPath); const resolvedRemotePath = resolve(remotePath); - if (!resolvedSkillsPath.startsWith(resolvedRemotePath + '/')) { + const relativePath = relative(resolvedRemotePath, resolvedSkillsPath); + if (relativePath === '..' || relativePath.startsWith('..') || isAbsolute(relativePath)) { continue; // Silently skip — attacker-controlled marketplace.json, don't leak info }