-
Notifications
You must be signed in to change notification settings - Fork 0
Codex/merge security fixes #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2ffce8c
3bba8bc
5df5987
8a15689
ca98e68
6d5a61d
58e8636
434f992
7cf0976
9972b90
624e06c
a60adbe
70eaae5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |||||
| * Instrumented with W&B Weave for observability. | ||||||
| */ | ||||||
|
|
||||||
| import { execSync } from 'child_process'; | ||||||
| import { execFileSync } from 'child_process'; | ||||||
| import * as fs from 'fs'; | ||||||
| import * as path from 'path'; | ||||||
| import type { | ||||||
|
|
@@ -38,6 +38,8 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| private autoCommit: boolean; | ||||||
| private currentFailureReport?: FailureReport; | ||||||
|
|
||||||
| private static readonly COMMIT_MESSAGE_MAX_LENGTH = 200; | ||||||
|
|
||||||
| constructor(projectRoot: string = process.cwd(), options: VerifierOptions = {}) { | ||||||
| this.projectRoot = projectRoot; | ||||||
| this.useRedis = options.useRedis ?? true; | ||||||
|
|
@@ -179,9 +181,14 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| */ | ||||||
| private async commitFix(patch: Patch): Promise<void> { | ||||||
| try { | ||||||
| const commitMessage = `fix: ${patch.description}\n\nApplied by PatchPilot`; | ||||||
| execSync(`git add ${patch.file}`, { cwd: this.projectRoot, stdio: 'pipe' }); | ||||||
| execSync(`git commit -m "${commitMessage}"`, { | ||||||
| const safeFilePath = this.getValidatedFilePath(patch.file); | ||||||
| const commitMessage = `fix: ${this.sanitizeCommitDescription(patch.description)}\n\nApplied by QAgent`; | ||||||
|
|
||||||
| execFileSync('git', ['add', '--', safeFilePath], { | ||||||
| cwd: this.projectRoot, | ||||||
| stdio: 'pipe', | ||||||
| }); | ||||||
| execFileSync('git', ['commit', '-m', commitMessage], { | ||||||
|
Comment on lines
+184
to
+191
|
||||||
| cwd: this.projectRoot, | ||||||
| stdio: 'pipe', | ||||||
| }); | ||||||
|
|
@@ -210,7 +217,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| } | ||||||
|
|
||||||
| private async deploy(): Promise<string> { | ||||||
| execSync('git push', { | ||||||
| execFileSync('git', ['push'], { | ||||||
| cwd: this.projectRoot, | ||||||
| stdio: 'pipe', | ||||||
| }); | ||||||
|
|
@@ -266,7 +273,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| */ | ||||||
| private async applyPatch(patch: Patch): Promise<boolean> { | ||||||
| try { | ||||||
| const fullPath = path.join(this.projectRoot, patch.file); | ||||||
| const fullPath = this.getValidatedFilePath(patch.file); | ||||||
| const sourceCode = fs.readFileSync(fullPath, 'utf-8'); | ||||||
|
|
||||||
| // Parse the diff to get the change details | ||||||
|
|
@@ -313,7 +320,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| * Create a backup of a file | ||||||
| */ | ||||||
| private async backupFile(filePath: string): Promise<string> { | ||||||
| const fullPath = path.join(this.projectRoot, filePath); | ||||||
| const fullPath = this.getValidatedFilePath(filePath); | ||||||
| const backupPath = `${fullPath}.backup.${Date.now()}`; | ||||||
| fs.copyFileSync(fullPath, backupPath); | ||||||
| return backupPath; | ||||||
|
|
@@ -323,7 +330,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| * Restore a file from backup | ||||||
| */ | ||||||
| private async restoreFile(filePath: string, backupPath: string): Promise<void> { | ||||||
| const fullPath = path.join(this.projectRoot, filePath); | ||||||
| const fullPath = this.getValidatedFilePath(filePath); | ||||||
| fs.copyFileSync(backupPath, fullPath); | ||||||
| this.cleanupBackup(backupPath); | ||||||
| } | ||||||
|
|
@@ -344,7 +351,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| */ | ||||||
| private async validateSyntax(filePath: string): Promise<boolean> { | ||||||
| try { | ||||||
| const fullPath = path.join(this.projectRoot, filePath); | ||||||
| const fullPath = this.getValidatedFilePath(filePath); | ||||||
| const content = fs.readFileSync(fullPath, 'utf-8'); | ||||||
|
|
||||||
| // Basic bracket balance check | ||||||
|
|
@@ -371,7 +378,7 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| // Try to run TypeScript check using project config | ||||||
| try { | ||||||
| // Use project's tsconfig to ensure JSX and other settings are applied | ||||||
| execSync(`npx tsc --noEmit --project tsconfig.json 2>&1`, { | ||||||
| execFileSync('npx', ['tsc', '--noEmit', '--project', 'tsconfig.json'], { | ||||||
| cwd: this.projectRoot, | ||||||
| stdio: 'pipe', | ||||||
| }); | ||||||
|
|
@@ -441,6 +448,41 @@ export class VerifierAgent implements IVerifierAgent { | |||||
| console.error('Error recording fix in knowledge base:', error); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Resolve and validate that patch file paths stay within the repository. | ||||||
| */ | ||||||
| private getValidatedFilePath(filePath: string): string { | ||||||
| if (!filePath || typeof filePath !== 'string') { | ||||||
| throw new Error('Invalid patch file path'); | ||||||
| } | ||||||
|
|
||||||
| const normalizedPath = path.normalize(filePath); | ||||||
| if (path.isAbsolute(normalizedPath)) { | ||||||
| throw new Error('Absolute file paths are not allowed in patches'); | ||||||
| } | ||||||
|
|
||||||
| const resolvedPath = path.resolve(this.projectRoot, normalizedPath); | ||||||
| const rootPath = path.resolve(this.projectRoot); | ||||||
|
|
||||||
| if (resolvedPath !== rootPath && !resolvedPath.startsWith(`${rootPath}${path.sep}`)) { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Path validation allows the project root directory itself ( Prompt for AI agents
Suggested change
|
||||||
| throw new Error('Patch file path resolves outside project root'); | ||||||
| } | ||||||
|
|
||||||
| return resolvedPath; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Keep commit subjects safe, readable, and bounded. | ||||||
| */ | ||||||
| private sanitizeCommitDescription(description: string): string { | ||||||
| const fallback = 'Bug fix'; | ||||||
| const normalized = description.trim().replace(/[\r\n]+/g, ' '); | ||||||
| if (!normalized) { | ||||||
| return fallback; | ||||||
| } | ||||||
| return normalized.slice(0, VerifierAgent.COMMIT_MESSAGE_MAX_LENGTH); | ||||||
| } | ||||||
|
Comment on lines
+452
to
+485
|
||||||
| } | ||||||
|
|
||||||
| export default VerifierAgent; | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,9 @@ import { executeAdHocRun } from '@/lib/queue/ad-hoc-runner'; | |||||||||||||||
| export const dynamic = 'force-dynamic'; | ||||||||||||||||
|
|
||||||||||||||||
| export async function GET(request: NextRequest) { | ||||||||||||||||
| const session = await getSession(); | ||||||||||||||||
| const userId = session?.user?.id; | ||||||||||||||||
|
|
||||||||||||||||
| const { searchParams } = request.nextUrl; | ||||||||||||||||
| const page = Math.max(1, parseInt(searchParams.get('page') || '1', 10)); | ||||||||||||||||
| const limit = Math.min(100, Math.max(1, parseInt(searchParams.get('limit') || '20', 10))); | ||||||||||||||||
|
|
@@ -22,6 +25,10 @@ export async function GET(request: NextRequest) { | |||||||||||||||
|
|
||||||||||||||||
| let runs = await getAllRunsAsync(); | ||||||||||||||||
|
|
||||||||||||||||
| if (userId !== undefined) { | ||||||||||||||||
| runs = runs.filter((r) => r.ownerId === userId); | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Scope Prompt for AI agents |
||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Reject unauthenticated GET requests here; when Prompt for AI agents
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| if (statusFilter) { | ||||||||||||||||
| runs = runs.filter((run) => run.status === statusFilter); | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -86,12 +93,17 @@ export async function POST(request: NextRequest) { | |||||||||||||||
| return NextResponse.json({ error: 'Repository is required' }, { status: 400 }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const session = cloudMode ? await getSession() : null; | ||||||||||||||||
| const githubToken = session?.accessToken || undefined; | ||||||||||||||||
| const session = await getSession(); | ||||||||||||||||
| const githubToken = cloudMode ? session?.accessToken || undefined : undefined; | ||||||||||||||||
|
|
||||||||||||||||
| if (cloudMode && !githubToken) { | ||||||||||||||||
| return NextResponse.json({ error: 'GitHub authentication required' }, { status: 401 }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const resolvedRepoId = repoId || (cloudMode ? repoName : 'local'); | ||||||||||||||||
| const resolvedRepoName = repoName || 'Demo App'; | ||||||||||||||||
| const run = createRun({ | ||||||||||||||||
| ownerId: session?.user?.id, | ||||||||||||||||
| repoId: resolvedRepoId, | ||||||||||||||||
| repoName: resolvedRepoName, | ||||||||||||||||
| testSpecs, | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| const DEV_SESSION_SECRET_GLOBAL_KEY = '__PATCHPILOT_DEV_SESSION_SECRET__'; | ||
| const TEST_FALLBACK_SECRET = 'test-session-secret'; | ||
|
|
||
| type GlobalWithSecret = typeof globalThis & { | ||
| [DEV_SESSION_SECRET_GLOBAL_KEY]?: string; | ||
| }; | ||
|
|
||
| function getDevSessionSecret(): string { | ||
| const globalWithSecret = globalThis as GlobalWithSecret; | ||
| if (!globalWithSecret[DEV_SESSION_SECRET_GLOBAL_KEY]) { | ||
| globalWithSecret[DEV_SESSION_SECRET_GLOBAL_KEY] = crypto.randomUUID(); | ||
| } | ||
|
|
||
| return globalWithSecret[DEV_SESSION_SECRET_GLOBAL_KEY]; | ||
| } | ||
|
|
||
| export function getSessionSecret(): string { | ||
| const envSecret = process.env.SESSION_SECRET; | ||
| if (envSecret) { | ||
| return envSecret; | ||
| } | ||
|
|
||
| if (process.env.NODE_ENV === 'test') { | ||
| return TEST_FALLBACK_SECRET; | ||
| } | ||
|
|
||
| if (process.env.NODE_ENV === 'production') { | ||
| throw new Error('SESSION_SECRET environment variable is required in production'); | ||
| } | ||
|
|
||
| return getDevSessionSecret(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,9 @@ | ||
| import { cookies } from 'next/headers'; | ||
| import { SignJWT, jwtVerify } from 'jose'; | ||
| import type { Session, GitHubUser, GitHubRepo, SessionPayload } from '@/lib/types'; | ||
| import { getSessionSecret } from '@/lib/auth/session-secret'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This import changes session signing to use a runtime-local dev fallback secret, which can make middleware reject valid local sessions when Prompt for AI agents |
||
|
|
||
| const SESSION_COOKIE = 'qagent_session'; | ||
| const TEST_FALLBACK_SECRET = 'test-session-secret'; | ||
|
|
||
| function getSessionSecret(): string { | ||
| const secret = process.env.SESSION_SECRET; | ||
| if (secret) { | ||
| return secret; | ||
| } | ||
|
|
||
| if (process.env.NODE_ENV === 'test') { | ||
| return TEST_FALLBACK_SECRET; | ||
| } | ||
|
|
||
| throw new Error('SESSION_SECRET environment variable is required'); | ||
| } | ||
|
|
||
| export function getSessionKey(): Uint8Array { | ||
| return new TextEncoder().encode(getSessionSecret()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,7 +113,10 @@ export async function installDependencies( | |
| const pm = detectPackageManager(repoPath); | ||
| console.log(`[GitClone] Installing dependencies with ${pm}...`); | ||
|
|
||
| const installCommand = pm === 'npm' ? 'npm install' : `${pm} install`; | ||
| const installCommand = | ||
| pm === 'npm' | ||
| ? 'npm install --ignore-scripts' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Skipping install scripts here can leave cloned repos unusable for Prompt for AI agents |
||
| : `${pm} install --ignore-scripts`; | ||
|
|
||
| try { | ||
| execSync(installCommand, { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P0: Disabling only the build step still executes untrusted repository code via ESLint config/plugins.
Prompt for AI agents