Codex/merge security fixes#25
Merged
rishabhcli merged 13 commits intomainfrom Mar 17, 2026
Merged
Conversation
…validation' into codex/merge-security-fixes
…-auth-bypass' into codex/merge-security-fixes # Conflicts: # lib/auth/session.ts # middleware.ts
…jection-vulnerability' into codex/merge-security-fixes # Conflicts: # agents/verifier/index.ts
…ization-vulnerability' into codex/merge-security-fixes # Conflicts: # app/api/runs/analyze/route.ts # app/api/runs/route.ts
…ity-vulnerability' into codex/merge-security-fixes # Conflicts: # app/api/runs/route.ts
…erability' into codex/merge-security-fixes # Conflicts: # app/api/auth/mobile/exchange/route.ts
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds owner-based run access control via session authentication, implements PKCE support for OAuth flows, refactors child process execution to use safer Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR focuses on tightening security across the Git/GitHub automation flows, session secret handling, and run access control by introducing safer process execution, path validation, and per-user run scoping.
Changes:
- Harden VerifierAgent git operations (move to
execFileSync, validate patch paths, sanitize commit messages) and update unit tests accordingly. - Introduce per-run ownership (
ownerId) and scope/api/runsresults and sensitive run session endpoints to the authenticated user. - Improve OAuth security by adding state/PKCE validation in the mobile auth flow and supporting optional PKCE/token-exchange params for GitHub token exchange.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/verifier.test.ts | Updates mocks/assertions for execFileSync-based git commands. |
| mobile/lib/auth/context.tsx | Adds OAuth state + PKCE verifier validation and sends verifier/state to backend exchange. |
| lib/types.ts | Adds ownerId?: number to Run. |
| lib/queue/ad-hoc-runner.ts | Constructs analyzer with build-script execution disabled for untrusted repos. |
| lib/git/clone.ts | Installs dependencies with --ignore-scripts to reduce supply-chain/script risk. |
| lib/dashboard/run-store.ts | Propagates ownerId into created runs. |
| lib/auth/session.ts | Uses shared getSessionSecret() implementation. |
| lib/auth/session-secret.ts | New env-aware session secret loader (prod-required, dev-random, test-fixed). |
| lib/auth/github.ts | Expands token exchange to accept optional redirectUri/PKCE/state parameters. |
| app/api/runs/route.ts | Scopes run listing to the authenticated user and requires GitHub auth for cloud runs. |
| app/api/runs/analyze/route.ts | Records ownerId on analysis runs. |
| app/api/runs/[runId]/session/route.ts | Restricts Browserbase session debug URLs to the run owner. |
| agents/verifier/index.ts | Security hardening: safer git invocation, path traversal protection, commit message sanitization. |
| agents/analyzer/index.ts | Adds allowBuildScripts switch to skip build checks for untrusted repos. |
Comments suppressed due to low confidence (2)
app/api/runs/route.ts:52
- After scoping
runsto the current user, the response still returnsstatscomputed bygetRunStatsAsync(repoId)which aggregates across all runs (not owner-scoped). This can leak other users’ activity in a multi-tenant deployment and also makes the stats inconsistent with the returned run list. Consider computing stats from the already-filteredruns, or extending the stats API to accept anownerIdand scope the aggregation accordingly.
if (userId !== undefined) {
runs = runs.filter((r) => r.ownerId === userId);
}
if (statusFilter) {
runs = runs.filter((run) => run.status === statusFilter);
}
if (repoIdFilter) {
runs = runs.filter((run) => run.repoId === repoIdFilter);
}
if (searchQuery) {
const normalizedQuery = searchQuery.toLowerCase();
runs = runs.filter(
(run) =>
run.repoName.toLowerCase().includes(normalizedQuery) ||
run.id.toLowerCase().includes(normalizedQuery)
);
}
const total = runs.length;
const totalPages = Math.ceil(total / limit);
const offset = (page - 1) * limit;
const paginatedRuns = runs.slice(offset, offset + limit);
const stats = await getRunStatsAsync(repoIdFilter || undefined);
agents/verifier/index.ts:403
validateSyntax()previously redirected stderr to stdout (2>&1) before parsing compiler output; withexecFileSync('npx', ['tsc', ...])that redirection is gone, but the catch block still only inspectserror.stdout. If TypeScript writes diagnostics tostderr,filteredOutputwill be empty and syntax/JSX errors may be missed. Consider mergingstdout+stderrwhen parsing, or explicitly checkingerror.stderras well.
execFileSync('npx', ['tsc', '--noEmit', '--project', 'tsconfig.json'], {
cwd: this.projectRoot,
stdio: 'pipe',
});
} catch (error) {
// Log TypeScript output for debugging but check if it's a syntax error
if (error instanceof Error && 'stdout' in error) {
const stdout = (error as { stdout?: Buffer }).stdout;
if (stdout) {
const output = stdout.toString();
// Filter out npm warnings and unrelated files
const filteredOutput = output
.split('\n')
.filter((line) =>
!line.includes('npm warn') &&
!line.includes('npm WARN') &&
line.includes(filePath))
.join('\n')
.trim();
// Only fail on actual syntax errors (TS1xxxx and TS17xxx are parse/JSX errors)
if (filteredOutput.includes('error TS1') || filteredOutput.includes('error TS17')) {
console.log(`Syntax error in ${filePath}:`, filteredOutput.slice(0, 300));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let runs = await getAllRunsAsync(); | ||
|
|
||
| if (userId !== undefined) { | ||
| runs = runs.filter((r) => r.ownerId === userId); |
Comment on lines
+452
to
+485
| /** | ||
| * 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}`)) { | ||
| 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
+184
to
+191
| 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], { |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/git/clone.ts">
<violation number="1" location="lib/git/clone.ts:118">
P1: Skipping install scripts here can leave cloned repos unusable for `setupLocalRepo`. Some projects need `prepare`/`postinstall` or native build steps to generate artifacts before `startDevServer()` runs, so this shared helper should not disable scripts unconditionally.</violation>
</file>
<file name="agents/verifier/index.ts">
<violation number="1" location="agents/verifier/index.ts:468">
P2: Path validation allows the project root directory itself (`"."`) to pass, which in `commitFix` would cause `git add` to stage the entire repository. Since this is a path traversal guard, it should reject paths that don't resolve to a file strictly inside the root.</violation>
</file>
<file name="lib/auth/session.ts">
<violation number="1" location="lib/auth/session.ts:4">
P2: This import changes session signing to use a runtime-local dev fallback secret, which can make middleware reject valid local sessions when `SESSION_SECRET` is unset.</violation>
</file>
<file name="app/api/runs/route.ts">
<violation number="1" location="app/api/runs/route.ts:28">
P1: Reject unauthenticated GET requests here; when `userId` is undefined this branch is skipped and the endpoint still returns every run.</violation>
<violation number="2" location="app/api/runs/route.ts:29">
P2: Scope `stats` to the same owner filter as `runs`; this response still leaks cross-user aggregate counts.</violation>
</file>
<file name="agents/analyzer/index.ts">
<violation number="1" location="agents/analyzer/index.ts:68">
P0: Disabling only the build step still executes untrusted repository code via ESLint config/plugins.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // 3. Build check | ||
| const buildIssues = await this.runBuildCheck(); | ||
| issues.push(...buildIssues); | ||
| if (this.allowBuildScripts) { |
There was a problem hiding this comment.
P0: Disabling only the build step still executes untrusted repository code via ESLint config/plugins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/analyzer/index.ts, line 68:
<comment>Disabling only the build step still executes untrusted repository code via ESLint config/plugins.</comment>
<file context>
@@ -63,8 +65,12 @@ export class CodeAnalyzerAgent {
// 3. Build check
- const buildIssues = await this.runBuildCheck();
- issues.push(...buildIssues);
+ if (this.allowBuildScripts) {
+ const buildIssues = await this.runBuildCheck();
+ issues.push(...buildIssues);
</file context>
| const installCommand = pm === 'npm' ? 'npm install' : `${pm} install`; | ||
| const installCommand = | ||
| pm === 'npm' | ||
| ? 'npm install --ignore-scripts' |
There was a problem hiding this comment.
P1: Skipping install scripts here can leave cloned repos unusable for setupLocalRepo. Some projects need prepare/postinstall or native build steps to generate artifacts before startDevServer() runs, so this shared helper should not disable scripts unconditionally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/git/clone.ts, line 118:
<comment>Skipping install scripts here can leave cloned repos unusable for `setupLocalRepo`. Some projects need `prepare`/`postinstall` or native build steps to generate artifacts before `startDevServer()` runs, so this shared helper should not disable scripts unconditionally.</comment>
<file context>
@@ -113,7 +113,10 @@ export async function installDependencies(
- const installCommand = pm === 'npm' ? 'npm install' : `${pm} install`;
+ const installCommand =
+ pm === 'npm'
+ ? 'npm install --ignore-scripts'
+ : `${pm} install --ignore-scripts`;
</file context>
Comment on lines
+28
to
+30
| if (userId !== undefined) { | ||
| runs = runs.filter((r) => r.ownerId === userId); | ||
| } |
There was a problem hiding this comment.
P1: Reject unauthenticated GET requests here; when userId is undefined this branch is skipped and the endpoint still returns every run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/runs/route.ts, line 28:
<comment>Reject unauthenticated GET requests here; when `userId` is undefined this branch is skipped and the endpoint still returns every run.</comment>
<file context>
@@ -22,6 +25,10 @@ export async function GET(request: NextRequest) {
let runs = await getAllRunsAsync();
+ if (userId !== undefined) {
+ runs = runs.filter((r) => r.ownerId === userId);
+ }
</file context>
Suggested change
| if (userId !== undefined) { | |
| runs = runs.filter((r) => r.ownerId === userId); | |
| } | |
| if (userId === undefined) { | |
| return NextResponse.json({ error: 'Authentication required' }, { status: 401 }); | |
| } | |
| runs = runs.filter((r) => r.ownerId === userId); |
| 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.
P2: Path validation allows the project root directory itself (".") to pass, which in commitFix would cause git add to stage the entire repository. Since this is a path traversal guard, it should reject paths that don't resolve to a file strictly inside the root.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At agents/verifier/index.ts, line 468:
<comment>Path validation allows the project root directory itself (`"."`) to pass, which in `commitFix` would cause `git add` to stage the entire repository. Since this is a path traversal guard, it should reject paths that don't resolve to a file strictly inside the root.</comment>
<file context>
@@ -441,6 +448,41 @@ export class VerifierAgent implements IVerifierAgent {
+ const resolvedPath = path.resolve(this.projectRoot, normalizedPath);
+ const rootPath = path.resolve(this.projectRoot);
+
+ if (resolvedPath !== rootPath && !resolvedPath.startsWith(`${rootPath}${path.sep}`)) {
+ throw new Error('Patch file path resolves outside project root');
+ }
</file context>
Suggested change
| if (resolvedPath !== rootPath && !resolvedPath.startsWith(`${rootPath}${path.sep}`)) { | |
| if (!resolvedPath.startsWith(`${rootPath}${path.sep}`)) { |
| 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.
P2: This import changes session signing to use a runtime-local dev fallback secret, which can make middleware reject valid local sessions when SESSION_SECRET is unset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/auth/session.ts, line 4:
<comment>This import changes session signing to use a runtime-local dev fallback secret, which can make middleware reject valid local sessions when `SESSION_SECRET` is unset.</comment>
<file context>
@@ -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';
const SESSION_COOKIE = 'qagent_session';
</file context>
| let runs = await getAllRunsAsync(); | ||
|
|
||
| if (userId !== undefined) { | ||
| runs = runs.filter((r) => r.ownerId === userId); |
There was a problem hiding this comment.
P2: Scope stats to the same owner filter as runs; this response still leaks cross-user aggregate counts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/api/runs/route.ts, line 29:
<comment>Scope `stats` to the same owner filter as `runs`; this response still leaks cross-user aggregate counts.</comment>
<file context>
@@ -22,6 +25,10 @@ export async function GET(request: NextRequest) {
let runs = await getAllRunsAsync();
+ if (userId !== undefined) {
+ runs = runs.filter((r) => r.ownerId === userId);
+ }
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Worked for 9m 18s
I merged the Codex branches into a single integration branch and pushed it to origin as codex/merge-security-fixes at 70eaae5.
I found 6 remote codex/* branches, not 5. One of them, origin/codex/fix-mobile-oauth-exchange-validation, was already patch-equivalent to a fix that had landed on origin/main, but I still merged it so all 6 are now ancestors of the integration branch. I kept main untouched.
Key conflict resolutions:
Combined the JWT secret hardening with the newer session code: production still requires SESSION_SECRET, tests keep a deterministic fallback, and dev gets a per-process random secret.
Preserved the secure, already-merged behavior that disables the deprecated mobile /api/auth/mobile/exchange endpoint.
Kept the run ownership protections for run listing and Browserbase session access.
Carried over the verifier command-injection hardening and aligned its tests.
Applied the untrusted-repo hardening to the current live code path by skipping install scripts and disabling build-script execution during code analysis.
Summary by cubic
Hardened security across verification, analysis, and auth to prevent command injection, path traversal, and untrusted script execution. Enforced run ownership and PKCE, and removed insecure session secret defaults; users now only see and access their own runs.
Bug Fixes
execSyncwithexecFileSync, validated patch file paths stay within the repo, usedgit add --, and sanitized/limited commit messages; also switched TypeScript checks toexecFileSync.allowBuildScriptsflag; ad‑hoc runs set it to false to skip build checks for untrusted repos.--ignore-scriptsto package manager install to block postinstall scripts.GET /api/runs/[runId]/sessionnow requires the requester to own the run; run creation storesownerId;GET /api/runsfilters byownerId; cloud runs require a GitHub token.exchangeCodeForTokensupportsredirectUri,codeVerifier, andstate; mobile app validates PKCEstateand sendscodeVerifierandstate.Migration
SESSION_SECRETin production; dev and test have safe defaults.Written for commit 70eaae5. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Security & Bug Fixes