Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
34 changes: 29 additions & 5 deletions src/lib/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,25 @@ db.exec(`
repo TEXT NOT NULL,
pr_number INTEGER NOT NULL,
review_comment_id INTEGER NOT NULL,
finding_fingerprint TEXT,
dismissed_by TEXT NOT NULL,
head_sha TEXT NOT NULL,
dismissed_at TEXT NOT NULL DEFAULT (datetime('now')),
PRIMARY KEY (owner, repo, pr_number, review_comment_id)
);
`);

const dismissalColumns = db
.prepare("PRAGMA table_info(pr_finding_dismissals)")
.all() as Array<{ name: string }>;
if (!dismissalColumns.some((column) => column.name === "finding_fingerprint")) {
db.exec("ALTER TABLE pr_finding_dismissals ADD COLUMN finding_fingerprint TEXT");
}
db.exec(`
CREATE INDEX IF NOT EXISTS idx_pr_finding_dismissals_fingerprint
ON pr_finding_dismissals (owner, repo, pr_number, finding_fingerprint)
`);

logger.info({ path: env.dbPath }, "Database initialized");

export const queries: Record<string, Statement> = {
Expand Down Expand Up @@ -132,15 +144,16 @@ export const queries: Record<string, Statement> = {

dismissPrFinding: db.prepare(`
INSERT INTO pr_finding_dismissals (
owner, repo, pr_number, review_comment_id, dismissed_by, head_sha
owner, repo, pr_number, review_comment_id, finding_fingerprint, dismissed_by, head_sha
)
VALUES (
@owner, @repo, @prNumber, @reviewCommentId, @dismissedBy, @headSha
@owner, @repo, @prNumber, @reviewCommentId, @findingFingerprint, @dismissedBy, @headSha
)
ON CONFLICT(owner, repo, pr_number, review_comment_id) DO UPDATE SET
dismissed_by = @dismissedBy,
head_sha = @headSha,
dismissed_at = datetime('now')
finding_fingerprint = @findingFingerprint,
dismissed_by = @dismissedBy,
head_sha = @headSha,
dismissed_at = datetime('now')
`),

isPrFindingDismissed: db.prepare(`
Expand All @@ -153,6 +166,17 @@ export const queries: Record<string, Statement> = {
LIMIT 1
`),

isPrFindingFingerprintDismissed: db.prepare(`
SELECT 1
FROM pr_finding_dismissals
WHERE owner = @owner
AND repo = @repo
AND pr_number = @prNumber
AND finding_fingerprint = @findingFingerprint
AND head_sha = @headSha
LIMIT 1
`),

clearPrFindingDismissals: db.prepare(`
DELETE FROM pr_finding_dismissals
WHERE owner = @owner AND repo = @repo AND pr_number = @prNumber
Expand Down
3 changes: 2 additions & 1 deletion src/lib/types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export const SUPERAGENT_URL = "https://superagent.sh";

export const CHECK_NAMES = {
PR_SCAN: "Security scan",
CONTRIBUTOR_TRUST: "Contributor trust",
Expand Down Expand Up @@ -64,7 +66,6 @@ export interface ContributorResult {
verdict?: string;
confidence?: string;
name?: string;
url?: string;
threats?: Array<{ type: string; detail: string; severity: string }>;
sub_scores?: {
identity?: number;
Expand Down
42 changes: 42 additions & 0 deletions src/services/__tests__/checkRuns.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { describe, expect, it, vi } from "vitest";
import { SUPERAGENT_URL } from "../../lib/types.js";
import { completeCheck, createInProgressCheck } from "../checkRuns.js";

describe("checkRuns", () => {
it("sets Superagent as the check run details URL", async () => {
const octokit = {
rest: {
checks: {
create: vi.fn().mockResolvedValue({ data: { id: 42 } }),
update: vi.fn().mockResolvedValue({}),
},
},
};

const checkRunId = await createInProgressCheck(
octokit as never,
"acme",
"repo",
"abc123",
"Security scan",
);

expect(checkRunId).toBe(42);
expect(octokit.rest.checks.create).toHaveBeenCalledWith(
expect.objectContaining({
details_url: SUPERAGENT_URL,
}),
);

await completeCheck(octokit as never, "acme", "repo", checkRunId, "success", {
title: "PR scan passed",
summary: "No suspicious PR changes were detected.",
});

expect(octokit.rest.checks.update).toHaveBeenCalledWith(
expect.objectContaining({
details_url: SUPERAGENT_URL,
}),
);
});
});
15 changes: 1 addition & 14 deletions src/services/__tests__/comments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ describe("renderContributorTrustComment", () => {
score: 25,
verdict: "suspicious",
name: "sketchy-user",
url: "https://superagent.sh/contributor/sketchy-user",
threats: [
{ type: "new_account", severity: "high", detail: "Account created less than 30 days ago" },
],
Expand All @@ -80,7 +79,7 @@ describe("renderContributorTrustComment", () => {
expect(body).toContain("| Behavior | 30 |");
expect(body).toContain("| Content | 40 |");
expect(body).not.toContain("| Graph |");
expect(body).toContain("[Full profile](https://superagent.sh/contributor/sketchy-user?details=true)");
expect(body).toContain("Analyzed by [Superagent]");
});

it("renders without threats section when empty", () => {
Expand All @@ -107,18 +106,6 @@ describe("renderContributorTrustComment", () => {
expect(body).not.toContain("| Graph |");
});

it("omits full profile link when url is missing", () => {
const result: ContributorResult = {
score: 40,
verdict: "caution",
name: "some-user",
};
const body = renderContributorTrustComment(result);

expect(body).not.toContain("Full profile");
expect(body).toContain("Analyzed by [Superagent]");
});

it("uses correct emoji for each verdict", () => {
expect(renderContributorTrustComment({ score: 30, verdict: "caution", name: "a" }))
.toContain("\u26A0\uFE0F");
Expand Down
53 changes: 50 additions & 3 deletions src/services/__tests__/findingDismissal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
import { CHECK_NAMES, LABEL_DEFS, MARKERS } from "../../lib/types.js";

const dismissedFindingIds = vi.hoisted(() => new Set<number>());
const dismissPrFindingMock = vi.hoisted(() => vi.fn());

vi.mock("../prFindingDismissals.js", () => ({
dismissPrFinding: ({ reviewCommentId }: { reviewCommentId: number }) => {
dismissPrFinding: (params: { reviewCommentId: number }) => {
dismissPrFindingMock(params);
const { reviewCommentId } = params;
dismissedFindingIds.add(reviewCommentId);
},
isPrFindingDismissed: (
Expand Down Expand Up @@ -47,6 +50,7 @@ import { isTrustedRepoContributor } from "../trustedContributor.js";
import {
handleFindingThreadResolved,
handleFindingReply,
persistReviewedFindingDismissals,
reconcilePrScanAfterDismissals,
} from "../findingDismissal.js";

Expand Down Expand Up @@ -265,13 +269,56 @@ describe("reconcilePrScanAfterDismissals", () => {
});
});

describe("persistReviewedFindingDismissals", () => {
beforeEach(() => {
vi.clearAllMocks();
dismissedFindingIds.clear();
});

it("records reviewed findings against the review comment commit SHA", async () => {
const octokit = mockOctokit({
findingComments: [
{
id: 10,
body: `${MARKERS.PR_FINDING}\n**P2:** one`,
path: "a.ts",
line: 1,
commit_id: "comment-sha",
},
{
id: 11,
body: MARKERS.PR_FINDING_ACK,
in_reply_to_id: 10,
user: { login: "superagent-security[bot]", type: "Bot" },
},
],
});

await persistReviewedFindingDismissals(octokit, {
owner: "acme",
repo: "repo",
prNumber: 12,
headSha: "current-sha",
});

expect(dismissPrFindingMock).toHaveBeenCalledWith(
expect.objectContaining({
reviewCommentId: 10,
headSha: "comment-sha",
}),
);
});
});

function mockOctokit(options?: {
findingComments?: Array<{
id: number;
body: string;
path: string;
line: number;
path?: string;
line?: number;
commit_id?: string;
in_reply_to_id?: number;
user?: { login?: string; type?: string };
}>;
}) {
const findingComments = options?.findingComments ?? [
Expand Down
28 changes: 28 additions & 0 deletions src/services/__tests__/prFindings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from "vitest";
import { MARKERS } from "../../lib/types.js";
import {
isPrFindingComment,
listAcknowledgedFindingCommentIds,
resolveFindingForUserReply,
resolveRootFindingComment,
} from "../prFindings.js";
Expand Down Expand Up @@ -48,6 +49,33 @@ describe("resolveRootFindingComment", () => {
});
});

describe("listAcknowledgedFindingCommentIds", () => {
it("only trusts bot-authored acknowledgment markers", async () => {
const listReviewComments = vi.fn();
const octokit = {
paginate: vi.fn().mockResolvedValue([
{
id: 11,
body: MARKERS.PR_FINDING_ACK,
in_reply_to_id: 10,
user: { login: "malicious-user", type: "User" },
},
{
id: 12,
body: MARKERS.PR_FINDING_ACK,
in_reply_to_id: 20,
user: { login: "superagent-security[bot]", type: "Bot" },
},
]),
rest: { pulls: { listReviewComments } },
} as any;

const ids = await listAcknowledgedFindingCommentIds(octokit, "acme", "repo", 12);

expect([...ids]).toEqual([20]);
});
});

describe("resolveFindingForUserReply", () => {
it("matches a finding on the same file line when the reply has no in_reply_to_id", async () => {
const octokit = {
Expand Down
58 changes: 58 additions & 0 deletions src/services/__tests__/prScan.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
import { CHECK_NAMES, DEFAULT_CONFIG } from "../../lib/types.js";
import { runPrScan } from "../prScan.js";

const isPrFindingFingerprintDismissedMock = vi.hoisted(() => vi.fn());

vi.mock("../prFindingDismissals.js", () => ({
clearPrFindingDismissals: vi.fn(),
isPrFindingFingerprintDismissed: isPrFindingFingerprintDismissedMock,
}));

vi.mock("../../lib/logger.js", () => ({
Expand All @@ -14,10 +17,16 @@ vi.mock("../../lib/logger.js", () => ({
}),
}));

vi.mock("../findingDismissal.js", () => ({
persistReviewedFindingDismissals: vi.fn().mockResolvedValue([]),
scheduleDismissalReconcile: vi.fn(),
}));

describe("runPrScan", () => {
beforeEach(() => {
vi.clearAllMocks();
vi.unstubAllGlobals();
isPrFindingFingerprintDismissedMock.mockReturnValue(false);
});

it("comments and flags the PR when local scanner returns findings", async () => {
Expand Down Expand Up @@ -92,6 +101,55 @@ describe("runPrScan", () => {
expect(body).not.toContain("Recommended fix");
});

it("does not recreate comments for previously dismissed findings", async () => {
isPrFindingFingerprintDismissedMock.mockReturnValue(true);
stubScanFetch({
findings: [
{
category: "malicious_intent",
severity: "high",
title: "Suspicious lifecycle hook",
file: "package.json",
line: 1,
evidence: "postinstall executes a remote script.",
recommendation: "Remove the hook and vendor reviewed setup code.",
short_evidence: "A new postinstall hook executes a remote script.",
short_recommendation: "Remove the lifecycle hook or replace it with reviewed local setup code.",
},
],
});
const octokit = mockOctokit();

await runPrScan(octokit, {
owner: "acme",
repo: "repo",
prNumber: 12,
headSha: "abc123",
config: DEFAULT_CONFIG,
});

expect(octokit.rest.checks.update).toHaveBeenCalledWith(
expect.objectContaining({
conclusion: "success",
output: expect.objectContaining({
summary: "All detected findings were previously reviewed and dismissed.",
}),
}),
);
expect(octokit.rest.issues.setLabels).toHaveBeenCalledWith(
expect.objectContaining({ labels: ["keep", "pr:verified"] }),
);
expect(octokit.rest.pulls.createReview).not.toHaveBeenCalled();
expect(octokit.rest.pulls.deleteReviewComment).not.toHaveBeenCalled();
expect(isPrFindingFingerprintDismissedMock).toHaveBeenCalledWith(
"acme",
"repo",
12,
expect.any(String),
"abc123",
);
});

it("verifies the PR and removes stale comments when no findings are found", async () => {
stubScanFetch({ findings: [] });
const octokit = mockOctokit([{ id: 99, body: "<!-- brin-pr-scan --> old comment" }]);
Expand Down
3 changes: 3 additions & 0 deletions src/services/checkRuns.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Octokit } from "octokit";
import { SUPERAGENT_URL } from "../lib/types.js";

export interface CheckOutput {
title: string;
Expand Down Expand Up @@ -31,6 +32,7 @@ export async function createInProgressCheck(
head_sha: headSha,
status: "in_progress",
started_at: new Date().toISOString(),
details_url: SUPERAGENT_URL,
});
return data.id;
}
Expand All @@ -50,6 +52,7 @@ export async function completeCheck(
status: "completed",
conclusion,
completed_at: new Date().toISOString(),
details_url: SUPERAGENT_URL,
output,
});
}
Loading
Loading