also looking at this
fix: prevent generated dependency cache PRs
#1366
16 files
+356
-72
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -3,6 +3,7 @@ import type { FileChange, GitProvider, Issue, IssueOptions, PullRequest, PullReq | ||
| 3 | 3 | import { Buffer } from 'node:buffer' |
| 4 | 4 | import { spawn } from 'node:child_process' |
| 5 | 5 | import process from 'node:process' |
| 6 | import { assertUpdateTargetsExist, FileChangeValidationError, normalizeRepositoryPath } from '../utils/file-changes' | |
| 6 | 7 | import { detectRequiredPackageManagers, getAllLockFilePaths, regenerateLockFile } from '../utils/lock-file' |
| 7 | 8 | |
| 8 | 9 | // Match GitHub token formats (ghp_*, gho_*, ghs_*, ghu_*, ghr_*, github_pat_*) |
| @@ -141,7 +142,7 @@ export class GitHubProvider implements GitProvider { | ||
| 141 | 142 | // Lockfile regeneration failures must NOT fall through to the API path — |
| 142 | 143 | // the API path can't regenerate lockfiles either, so it would just produce |
| 143 | 144 | // a PR with an updated manifest and stale lockfile. Bubble the error up. |
| 144 | if (gitError instanceof LockfileRegenerationError) | |
| 145 | if (gitError instanceof LockfileRegenerationError || gitError instanceof FileChangeValidationError) | |
| 145 | 146 | throw gitError |
| 146 | 147 | |
| 147 | 148 | console.warn(`⚠️ Git CLI commit failed, falling back to GitHub API: ${gitError}`) |
| @@ -240,9 +241,19 @@ export class GitHubProvider implements GitProvider { | ||
| 240 | 241 | await this.runCommand('git', ['clean', '-fd']) |
| 241 | 242 | console.log(`✅ Reset ${branchName} to origin/${baseBranch}`) |
| 242 | 243 | |
| 244 | await assertUpdateTargetsExist(files, async (cleanPath) => { | |
| 245 | try { | |
| 246 | await this.runCommand('git', ['cat-file', '-e', `origin/${baseBranch}:${cleanPath}`]) | |
| 247 | return true | |
| 248 | } | |
| 249 | catch { | |
| 250 | return false | |
| 251 | } | |
| 252 | }) | |
| 253 | ||
| 243 | 254 | // Apply file changes |
| 244 | 255 | for (const file of files) { |
| 245 | const cleanPath = file.path.replace(/^\.\//, '').replace(/^\/+/, '') | |
| 256 | const cleanPath = normalizeRepositoryPath(file.path) | |
| 246 | 257 | |
| 247 | 258 | // Safety check: prevent writing to sensitive files during tests |
| 248 | 259 | if (process.env.NODE_ENV === 'test' || process.env.BUN_ENV === 'test') { |
| @@ -395,11 +406,23 @@ export class GitHubProvider implements GitProvider { | ||
| 395 | 406 | const baseCommit = await this.apiRequest(`GET /repos/${this.owner}/${this.repo}/git/commits/${baseSha}`) |
| 396 | 407 | const baseTreeSha = baseCommit.tree.sha |
| 397 | 408 | |
| 409 | await assertUpdateTargetsExist(files, async (cleanPath) => { | |
| 410 | const encodedPath = cleanPath.split('/').map(segment => encodeURIComponent(segment)).join('/') | |
| 411 | try { | |
| 412 | await this.apiRequest(`GET /repos/${this.owner}/${this.repo}/contents/${encodedPath}?ref=${encodeURIComponent(baseBranch)}`) | |
| 413 | return true | |
| 414 | } | |
| 415 | catch (error: any) { | |
| 416 | if (error.message?.includes('404')) | |
| 417 | return false | |
| 418 | throw error | |
| 419 | } | |
| 420 | }) | |
| 421 | ||
| 398 | 422 | // Create new tree with file changes |
| 399 | 423 | const tree = [] |
| 400 | 424 | for (const file of files) { |
| 401 | // Ensure path doesn't start with ./ or have leading slashes (GitHub API requires clean relative paths) | |
| 402 | const cleanPath = file.path.replace(/^\.\//, '').replace(/^\/+/, '') | |
| 425 | const cleanPath = normalizeRepositoryPath(file.path) | |
| 403 | 426 | |
| 404 | 427 | if (file.type === 'delete') { |
| 405 | 428 | tree.push({ |
| @@ -0,0 +1,41 @@ | ||
| 1 | import type { FileChange } from '../types' | |
| 2 | import { posix } from 'node:path' | |
| 3 | ||
| 4 | export class FileChangeValidationError extends Error { | |
| 5 | constructor(message: string) { | |
| 6 | super(message) | |
| 7 | this.name = 'FileChangeValidationError' | |
| 8 | } | |
| 9 | } | |
| 10 | ||
| 11 | /** | |
| 12 | * Convert a user or scanner supplied path into a safe repository-relative path. | |
| 13 | */ | |
| 14 | export function normalizeRepositoryPath(filePath: string): string { | |
| 15 | const normalized = posix.normalize(filePath.replaceAll('\\', '/').replace(/^\.\/+/, '')) | |
| 16 | ||
| 17 | if (!normalized || normalized === '.' || normalized === '..' || normalized.startsWith('../') || posix.isAbsolute(normalized)) { | |
| 18 | throw new FileChangeValidationError(`Refusing unsafe repository path: ${JSON.stringify(filePath)}`) | |
| 19 | } | |
| 20 | ||
| 21 | return normalized | |
| 22 | } | |
| 23 | ||
| 24 | /** | |
| 25 | * An update must modify a file that already exists on the base branch. Treating | |
| 26 | * a missing update target as a create is how generated cache paths can leak | |
| 27 | * into dependency PRs. | |
| 28 | */ | |
| 29 | export async function assertUpdateTargetsExist( | |
| 30 | files: FileChange[], | |
| 31 | targetExists: (path: string) => boolean | Promise<boolean>, | |
| 32 | ): Promise<void> { | |
| 33 | for (const file of files) { | |
| 34 | const cleanPath = normalizeRepositoryPath(file.path) | |
| 35 | if (file.type === 'update' && !(await targetExists(cleanPath))) { | |
| 36 | throw new FileChangeValidationError( | |
| 37 | `Refusing to create missing update target: ${cleanPath}. Use change type "create" for intentional new files.`, | |
| 38 | ) | |
| 39 | } | |
| 40 | } | |
| 41 | } | |
| @@ -0,0 +1,39 @@ | ||
| 1 | import type { FileChange } from '../src/types' | |
| 2 | import { describe, expect, it } from 'bun:test' | |
| 3 | import { | |
| 4 | assertUpdateTargetsExist, | |
| 5 | FileChangeValidationError, | |
| 6 | normalizeRepositoryPath, | |
| 7 | } from '../src/utils/file-changes' | |
| 8 | ||
| 9 | describe('file change validation', () => { | |
| 10 | it('normalizes repository-relative paths', () => { | |
| 11 | expect(normalizeRepositoryPath('./packages\\app/package.json')).toBe('packages/app/package.json') | |
| 12 | }) | |
| 13 | ||
| 14 | it('rejects paths outside the repository', () => { | |
| 15 | expect(() => normalizeRepositoryPath('../package.json')).toThrow(FileChangeValidationError) | |
| 16 | expect(() => normalizeRepositoryPath('/tmp/package.json')).toThrow(FileChangeValidationError) | |
| 17 | }) | |
| 18 | ||
| 19 | it('allows updates only when the base target exists', async () => { | |
| 20 | const files: FileChange[] = [ | |
| 21 | { path: 'package.json', content: '{}', type: 'update' }, | |
| 22 | { path: 'generated.json', content: '{}', type: 'create' }, | |
| 23 | ] | |
| 24 | ||
| 25 | await expect(assertUpdateTargetsExist(files, path => path === 'package.json')).resolves.toBeUndefined() | |
| 26 | }) | |
| 27 | ||
| 28 | it('rejects an update that would create a missing file', async () => { | |
| 29 | const files: FileChange[] = [{ | |
| 30 | path: './pantry/install/cache/acorn/package.json', | |
| 31 | content: '{}', | |
| 32 | type: 'update', | |
| 33 | }] | |
| 34 | ||
| 35 | await expect(assertUpdateTargetsExist(files, () => false)).rejects.toThrow( | |
| 36 | 'Refusing to create missing update target: pantry/install/cache/acorn/package.json', | |
| 37 | ) | |
| 38 | }) | |
| 39 | }) | |
| @@ -46,6 +46,30 @@ describe('No-op commit prevention', () => { | ||
| 46 | 46 | expect(hasPush).toBeTrue() |
| 47 | 47 | }) |
| 48 | 48 | |
| 49 | it('does not fall back to an API create when an update target is missing from base', async () => { | |
| 50 | const prov = new GitHubProvider('token', 'owner', 'repo', true) as any | |
| 51 | const commands: Array<{ cmd: string, args: string[] }> = [] | |
| 52 | let apiCalled = false | |
| 53 | ||
| 54 | prov.runCommand = async (command: string, args: string[]) => { | |
| 55 | commands.push({ cmd: command, args }) | |
| 56 | if (command === 'git' && args[0] === 'cat-file') | |
| 57 | throw new Error('missing from base') | |
| 58 | return '' | |
| 59 | } | |
| 60 | prov.apiRequest = async () => { | |
| 61 | apiCalled = true | |
| 62 | return {} | |
| 63 | } | |
| 64 | ||
| 65 | await expect(prov.commitChanges('buddy/test-branch', 'test message', filesChanged)).rejects.toThrow( | |
| 66 | 'Refusing to create missing update target: test-package.json', | |
| 67 | ) | |
| 68 | ||
| 69 | expect(apiCalled).toBeFalse() | |
| 70 | expect(commands.some(command => command.args[0] === 'add')).toBeFalse() | |
| 71 | }) | |
| 72 | ||
| 49 | 73 | it('skips API commit when new tree equals current tree (API fallback path)', async () => { |
| 50 | 74 | const prov = new GitHubProvider('token', 'owner', 'repo', true) as any |
| 51 | 75 | const apiCalls: Array<{ endpoint: string, data: any }> = [] |