Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 36 additions & 127 deletions .github/actions/llm-review/reviewer.js
Original file line number Diff line number Diff line change
@@ -1,144 +1,53 @@
const fs = require('fs');
const fetch = require('node-fetch');
const { Octokit } = require('@octokit/rest');
const path = require('path');

const [, , patchFile] = process.argv;
// Logic is now centralized in the main library
const { performReview } = require('../../../lib/reviewer');

if (!patchFile || !fs.existsSync(patchFile)) {
console.error("No patch file provided or file does not exist");
process.exit(1);
}

const patch = fs.readFileSync(patchFile, 'utf8');

const LLM_API_URL = process.env.LLM_API_URL;
const LLM_API_KEY = process.env.LLM_API_KEY;
const repo = process.env.GITHUB_REPOSITORY;
const prNumber = process.env.PR_NUMBER;
const octokit = new Octokit({ auth: process.env.GITHUB_TOKEN });

const [owner, repoName] = repo.split('/');

function buildPrompt(patch) {
return `
You are a precise code reviewer that outputs JSON matching this schema:
{
"summary": string,
"findings": [
{
"file": string,
"start_line": int,
"end_line": int,
"issue": string,
"severity": "INFO"|"LOW"|"MEDIUM"|"HIGH",
"confidence": float (0-1),
"suggestion": string
}
]
}

Context: the following is a git patch. Focus only on changed hunks. For each hunk, inspect for bugs, concurrency issues, insecure patterns, dead code, ignored exceptions, lint issues, suspicious tests, and missing resource cleanup. Provide only JSON.

Patch below:
-----
${patch}
-----

Return an empty findings list if nothing to report.
`;
}

async function callLLM(prompt) {
console.log("Calling LLM...");
// Adapt for specific LLM provider if needed. Using Generic POST here.
const res = await fetch(LLM_API_URL, {
method: "POST",
headers: {
"Content-Type": "application/json",
"x-goog-api-key": LLM_API_KEY,
},
body: JSON.stringify({
contents: [{
parts: [{ text: prompt }]
}],
generationConfig: {
temperature: 0,
response_mime_type: "application/json"
}
}),
});
(async () => {
try {

Check notice on line 8 in .github/actions/llm-review/reviewer.js

View workflow job for this annotation

GitHub Actions / Gemini AI Analysis

Gemini: Refactoring to use centralized review logic

Refactoring to use centralized review logic Why: The original `reviewer.js` file contained duplicated logic for calling the LLM and processing the results. This change moves that logic to `lib/reviewer.js`, making it easier to maintain and test. Impact: Improves code organization and reusability by centralizing the review logic in the `lib/reviewer.js` file.
Raw output
Ensure that all necessary environment variables are correctly passed to the `performReview` function.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring to use centralized review logic

Severity: INFO | Confidence: 80%

Why: The original reviewer.js file contained duplicated logic for calling the LLM and processing the results. This change moves that logic to lib/reviewer.js, making it easier to maintain and test.

Advice: Ensure that all necessary environment variables are correctly passed to the performReview function.

const [, , patchFile] = process.argv;

if (!res.ok) {
const errorBody = await res.text();
console.error("LLM error", errorBody);
throw new Error(`LLM request failed with status ${res.status}`);
}
if (!patchFile || !fs.existsSync(patchFile)) {
console.error("No patch file provided or file does not exist");
process.exit(1);
}

const json = await res.json();
// Extracting the text response from Gemini response format
try {
return json.candidates[0].content.parts[0].text;
} catch (e) {
console.error("Failed to parse LLM response format", json);
throw new Error("Invalid LLM response format");
}
}
const patch = fs.readFileSync(patchFile, 'utf8');

async function postSummary(summary) {
if (!summary) return;
console.log("Posting summary comment...");
await octokit.rest.issues.createComment({
owner,
repo: repoName,
issue_number: Number(prNumber),
body: `**LLM Review Summary**\n\n${summary}`
});
}
// Dynamic import for Octokit (ESM)
const { Octokit } = await import('@octokit/rest');

async function postFindingsAsReview(findings) {
if (!findings || findings.length === 0) return;
console.log(`Posting ${findings.length} findings as a review...`);
const LLM_API_KEY = process.env.LLM_API_KEY; // Corresponds to GEMINI_API_KEY
const repo = process.env.GITHUB_REPOSITORY;
const prNumber = process.env.PR_NUMBER;
const githubToken = process.env.GITHUB_TOKEN;

const comments = findings.map(f => ({
path: f.file,
body: `**${f.issue}** (severity: ${f.severity}, confidence: ${Math.round(f.confidence * 100)}%)\n\n${f.suggestion}`,
line: f.end_line,
side: "RIGHT"
}));
if (!githubToken) {
console.error("GITHUB_TOKEN is missing");
process.exit(1);
}

await octokit.rest.pulls.createReview({
owner,
repo: repoName,
pull_number: Number(prNumber),
event: "COMMENT",
comments
});
}
console.log(`Env Check: LLM_API_KEY: ${!!LLM_API_KEY}, REPO: ${repo}, PR: ${prNumber}`);

(async () => {
try {
const prompt = buildPrompt(patch);
const llmRaw = await callLLM(prompt);
const octokit = new Octokit({ auth: githubToken });

console.log("LLM Raw Response:", llmRaw);
console.log(`Starting review for ${repo} #${prNumber}`);

let result;
try {
result = JSON.parse(llmRaw);
} catch (e) {
// Fallback: try to find JSON in the string if it contains markdown markers
const jsonMatch = llmRaw.match(/```json\n([\s\S]*?)\n```/) || llmRaw.match(/{[\s\S]*}/);
if (jsonMatch) {
result = JSON.parse(jsonMatch[jsonMatch.length - 1]);
} else {
throw e;
}
}
// Reuse the same robust logic as the Probot App
const result = await performReview({
patch,
geminiKey: LLM_API_KEY,
octokit,
repo,
prNumber,
severity: 'Medium' // Default for Action
});

await postSummary(result.summary);
await postFindingsAsReview(result.findings);
console.log("Review completed successfully.");
console.log("Summary:", result.summary);
console.log("Findings:", result.findings ? result.findings.length : 0);

console.log("LLM review completed successfully");
} catch (err) {
console.error("Review failed", err);
process.exit(1);
Expand Down
6 changes: 5 additions & 1 deletion .github/workflows/llm-review.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
name: CI + LLM Review
on:
pull_request:
types: [opened, synchronize, reopened, ready_for_review]

Check notice on line 4 in .github/workflows/llm-review.yml

View workflow job for this annotation

GitHub Actions / Gemini AI Analysis

Gemini: Added permissions for the workflow

Added permissions for the workflow Why: The workflow needs these permissions to access the code, post comments on pull requests, and create check runs with annotations. Impact: Allows the workflow to read contents, write to pull requests, and write checks, which are necessary for the LLM review process.
Raw output
Verify that the permissions are sufficient for the workflow to function correctly.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added permissions for the workflow

Severity: INFO | Confidence: 90%

Why: The workflow needs these permissions to access the code, post comments on pull requests, and create check runs with annotations.

Advice: Verify that the permissions are sufficient for the workflow to function correctly.

permissions:
contents: read
pull-requests: write
checks: write
jobs:
llm-review:
runs-on: ubuntu-latest
Expand All @@ -26,7 +30,7 @@
env:
LLM_API_URL: ${{ secrets.LLM_API_URL }}
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GITHUB_TOKEN: ${{ github.token }}
GITHUB_REPOSITORY: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
run: |
Expand Down
11 changes: 11 additions & 0 deletions lib/email-validator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Email validator utility

function validateEmail(email) {
// VULNERABLE: Catastrophic backtracking regex
// This regex takes exponential time for inputs like "aaaaaaaaaaaaaaaaaaaa!"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regular expression is vulnerable to ReDoS (Regular Expression Denial of Service).

Severity: CRITICAL | Confidence: 90%

Why: The regex ^([a-zA-Z0-9_\-\.]+)@([a-zA-Z0-9_\-\.]+)\.([a-zA-Z]{2,5})$ is vulnerable to catastrophic backtracking. Specifically, the ([a-zA-Z0-9_\-\.]+) patterns can match many characters, and when combined with the . (dot) and the lack of clear boundaries, it can lead to exponential backtracking when an invalid email with many similar characters is provided. The test file lib/email-validator.test.js demonstrates this vulnerability.

Suggested Fix:

Suggested change
// This regex takes exponential time for inputs like "aaaaaaaaaaaaaaaaaaaa!"
const regex = /^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$/g;

const regex = /^([a-zA-Z0-9_\-\.]+)@([a-zA-Z0-9_\-\.]+)\.([a-zA-Z]{2,5})$/;

Check failure on line 7 in lib/email-validator.js

View workflow job for this annotation

GitHub Actions / Gemini AI Analysis

Gemini: Regular expression is vulnerable to Catastrophic Backtracking (ReDoS)

Regular expression is vulnerable to Catastrophic Backtracking (ReDoS) Why: The regular expression `^([a-zA-Z0-9_\-\.]+)@([a-zA-Z0-9_\-\.]+)\.([a-zA-Z]{2,5})$` is vulnerable to catastrophic backtracking. Specifically, the `([a-zA-Z0-9_\-\.]+)` patterns can cause the regex engine to explore many possible combinations when given a malicious input like 'aaaaaaaaaaaaaaaaaaaa!'. Impact: An attacker can cause a denial of service by providing a specially crafted email address that takes an extremely long time to validate.
Raw output
Replace the vulnerable regex with a more robust and secure email validation method. Consider using a well-vetted library or a simpler regex that avoids nested quantifiers.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regular expression is vulnerable to Catastrophic Backtracking (ReDoS)

Severity: CRITICAL | Confidence: 95%

Why: The regular expression ^([a-zA-Z0-9_\-\.]+)@([a-zA-Z0-9_\-\.]+)\.([a-zA-Z]{2,5})$ is vulnerable to catastrophic backtracking. Specifically, the ([a-zA-Z0-9_\-\.]+) patterns can cause the regex engine to explore many possible combinations when given a malicious input like 'aaaaaaaaaaaaaaaaaaaa!'.

Suggested Fix:

Suggested change
const validator = require('validator');
function validateEmail(email) {
return validator.isEmail(email);
}

return regex.test(email);
}

module.exports = { validateEmail };
13 changes: 13 additions & 0 deletions lib/email-validator.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const { validateEmail } = require('./email-validator');

console.log("Testing normal email:", validateEmail("test@example.com"));

// EXPLOIT: This payload causes the regex engine to hang
const attackPayload = "a".repeat(50) + "!";
console.log("Attempting ReDoS attack...");

const start = process.hrtime();
validateEmail(attackPayload);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code demonstrates a ReDoS attack against the email validator.

Severity: CRITICAL | Confidence: 90%

Why: The attackPayload is specifically crafted to trigger catastrophic backtracking in the vulnerable regex used in lib/email-validator.js. The repeated 'a' characters followed by an exclamation mark cause the regex engine to explore many possible matches, leading to exponential time complexity.

Suggested Fix:

Suggested change
validateEmail(attackPayload);
// Test that validation of long invalid strings completes in a reasonable time
const longInvalidEmail = "a".repeat(50) + "!";
const start = process.hrtime();
validateEmail(longInvalidEmail);
const end = process.hrtime(start);
const executionTime = end[1] / 1000000;
console.log(`Execution time for long invalid email: ${executionTime} ms`);
if (executionTime > 100) { // Adjust the threshold as needed
console.error("Validation of long invalid email took too long!");
}

const end = process.hrtime(start);

Check failure on line 12 in lib/email-validator.test.js

View workflow job for this annotation

GitHub Actions / Gemini AI Analysis

Gemini: Demonstrates ReDoS vulnerability in email validator

Demonstrates ReDoS vulnerability in email validator Why: The test case uses a crafted payload to trigger catastrophic backtracking in the email validator's regex. The execution time is measured to demonstrate the denial-of-service potential. Impact: Confirms the vulnerability in `lib/email-validator.js` and shows how an attacker can exploit it.
Raw output
Remove or disable this test after the vulnerability in `lib/email-validator.js` is fixed. Keeping it active serves as a regression test.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Demonstrates ReDoS vulnerability in email validator

Severity: CRITICAL | Confidence: 90%

Why: The test case uses a crafted payload to trigger catastrophic backtracking in the email validator's regex. The execution time is measured to demonstrate the denial-of-service potential.

Advice: Remove or disable this test after the vulnerability in lib/email-validator.js is fixed. Keeping it active serves as a regression test.

console.log(`Execution time: ${end[1] / 1000000} ms`);
Loading