Skip to content

Commit ddea515

Browse files
fix: validate reviewAgentLogPath to prevent path injection (#1134)
* fix: validate reviewAgentLogPath to prevent path injection Add path validation to invokeDiffReviewLlm to ensure the log path stays within the expected DATA_CACHE_DIR/review-agent directory. This addresses CodeQL alert #19 (js/path-injection) by resolving the path and verifying it does not escape the log directory. The validation is performed before each fs.appendFileSync call to prevent path traversal attacks even if the call chain changes in the future. Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * chore: add CHANGELOG entry for path injection fix Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * refactor: share REVIEW_AGENT_LOG_DIR constant between app.ts and invokeDiffReviewLlm.ts Export the log directory constant from invokeDiffReviewLlm.ts and import it in app.ts to ensure a single source of truth for the review agent log directory path. Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * docs: add guidance to link PRs to Linear issues in PR description Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * fix: use lazy evaluation for REVIEW_AGENT_LOG_DIR to fix build Change REVIEW_AGENT_LOG_DIR from a top-level constant to a getReviewAgentLogDir() function to avoid evaluating env.DATA_CACHE_DIR at module load time, which fails during Next.js build when environment variables are not yet available. Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * docs: add requirement to run tests and build before pushing Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com> * Revert "docs: add requirement to run tests and build before pushing" This reverts commit 476081e. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Michael Sukkarieh <msukkari@users.noreply.github.com>
1 parent da92ca1 commit ddea515

4 files changed

Lines changed: 19 additions & 1 deletion

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1010
### Fixed
1111
- Fixed revision selection so the 64-revision cap prefers the newest matching branches and tags instead of pruning by ref-name order. [#1122](https://github.com/sourcebot-dev/sourcebot/pull/1122)
1212
- Fixed infinite pagination loop in Gitea/Forgejo when an API token can only see a subset of org repos (the `x-total-count` header reports org total while token returns fewer items). [#1130](https://github.com/sourcebot-dev/sourcebot/pull/1130)
13+
- Fixed path injection vulnerability (CodeQL js/path-injection) in review agent log writing by validating paths stay within the expected log directory. [#1134](https://github.com/sourcebot-dev/sourcebot/pull/1134)
1314
- Fixed missing workflow permissions in `docs-broken-links.yml` by adding explicit `permissions: {}` to follow least privilege principle. [#1131](https://github.com/sourcebot-dev/sourcebot/pull/1131)
1415
- Fixed CodeQL missing-workflow-permissions alert by adding explicit empty permissions to `deploy-railway.yml`. [#1132](https://github.com/sourcebot-dev/sourcebot/pull/1132)
1516

CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,7 @@ You can optionally include a scope to indicate which package is affected:
276276

277277
PR description:
278278
- If a GitHub issue number was provided, include `Fixes #<github_issue_number>` in the PR description
279+
- If a Linear issue ID was provided (e.g., SOU-123), include `Fixes SOU-123` at the top of the PR description to auto-link the PR to the Linear issue
279280

280281
After the PR is created:
281282
- Update CHANGELOG.md with an entry under `[Unreleased]` linking to the new PR. New entries should be placed at the bottom of their section.

packages/web/src/features/agents/review-agent/app.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { Octokit } from "octokit";
22
import { generatePrReviews } from "@/features/agents/review-agent/nodes/generatePrReview";
33
import { githubPushPrReviews } from "@/features/agents/review-agent/nodes/githubPushPrReviews";
44
import { githubPrParser } from "@/features/agents/review-agent/nodes/githubPrParser";
5+
import { getReviewAgentLogDir } from "@/features/agents/review-agent/nodes/invokeDiffReviewLlm";
56
import { env } from "@sourcebot/shared";
67
import { GitHubPullRequest } from "@/features/agents/review-agent/types";
78
import path from "path";
@@ -30,7 +31,7 @@ export async function processGitHubPullRequest(octokit: Octokit, pullRequest: Gi
3031

3132
let reviewAgentLogPath: string | undefined;
3233
if (env.REVIEW_AGENT_LOGGING_ENABLED) {
33-
const reviewAgentLogDir = path.join(env.DATA_CACHE_DIR, "review-agent");
34+
const reviewAgentLogDir = getReviewAgentLogDir();
3435
if (!fs.existsSync(reviewAgentLogDir)) {
3536
fs.mkdirSync(reviewAgentLogDir, { recursive: true });
3637
}

packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,23 @@ import OpenAI from "openai";
22
import { sourcebot_file_diff_review, sourcebot_file_diff_review_schema } from "@/features/agents/review-agent/types";
33
import { env } from "@sourcebot/shared";
44
import fs from "fs";
5+
import path from "path";
56
import { createLogger } from "@sourcebot/shared";
67

78
const logger = createLogger('invoke-diff-review-llm');
89

10+
export const getReviewAgentLogDir = (): string => {
11+
return path.join(env.DATA_CACHE_DIR, 'review-agent');
12+
};
13+
14+
const validateLogPath = (logPath: string): void => {
15+
const resolved = path.resolve(logPath);
16+
const logDir = getReviewAgentLogDir();
17+
if (!resolved.startsWith(logDir + path.sep)) {
18+
throw new Error('reviewAgentLogPath escapes log directory');
19+
}
20+
};
21+
922
export const invokeDiffReviewLlm = async (reviewAgentLogPath: string | undefined, prompt: string): Promise<sourcebot_file_diff_review> => {
1023
logger.debug("Executing invoke_diff_review_llm");
1124

@@ -19,6 +32,7 @@ export const invokeDiffReviewLlm = async (reviewAgentLogPath: string | undefined
1932
});
2033

2134
if (reviewAgentLogPath) {
35+
validateLogPath(reviewAgentLogPath);
2236
fs.appendFileSync(reviewAgentLogPath, `\n\nPrompt:\n${prompt}`);
2337
}
2438

@@ -32,6 +46,7 @@ export const invokeDiffReviewLlm = async (reviewAgentLogPath: string | undefined
3246

3347
const openaiResponse = completion.choices[0].message.content;
3448
if (reviewAgentLogPath) {
49+
validateLogPath(reviewAgentLogPath);
3550
fs.appendFileSync(reviewAgentLogPath, `\n\nResponse:\n${openaiResponse}`);
3651
}
3752

0 commit comments

Comments
 (0)