swadhin / opencodehub public

Filter AI review suggestions by read path scope

swadhinbiswas committed 8 months ago

f41974f

1 parent 08b9d18

commit f41974f

4 files changed

+121 -2

Files

doc

feature_audit.md +1 -1

src

lib

openapi.ts +1

pages

api

repos

[owner]

[repo]

pulls

[number]

ai-review.ts +14 -1

tests

integration

ai-review-route-path-permissions.test.ts +105

Showing 4 changed files+121-2

doc/feature_audit.md src/lib/openapi.ts src/pages/api/repos/[owner]/[repo]/pulls/[number]/ai-review.ts tests/integration/ai-review-route-path-permissions.test.ts

▼doc/feature_audit.md

+1-1

@@ -36,7 +36,7 @@
36	36	\| Branch protection rules \| ✅ \| Schema + UI + API in place \|
37	37	\| Repository templates \| ⚠️ \| Template creation and listing are implemented with access-aware discovery filters (`/repos/templates?q=&owner=&visibility=`) including collaborator-visible private templates; dedicated template governance UX remains partial \|
38	38	\| Repository Wiki \| ✅ \| Implemented (Phase 8) \|
39		-\| File-level permissions \| ⚠️ \| Path-scoped enforcement now covers PR comment create/edit/delete/list filtering, PR review list comment filtering, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke and read-filter flows, PR rewrite operations, PR impact scan execution checks, and PR impact read-result file-path filtering; broader endpoint coverage still remains \|
	39	+\| File-level permissions \| ⚠️ \| Path-scoped enforcement now covers PR comment create/edit/delete/list filtering, PR review list comment filtering, AI review suggestion read filtering, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke and read-filter flows, PR rewrite operations, PR impact scan execution checks, and PR impact read-result file-path filtering; broader endpoint coverage still remains \|
40	40	\| Commit signing (GPG) \| ✅ \| GPG key management UI exists \|
41	41	\| Commit signing (SSH) \| ✅ \| SSH key management UI exists \|
42	42
43	43

▼src/lib/openapi.ts

+1-0

@@ -847,6 +847,7 @@ export const openApiSpec = {
847	847	get: {
848	848	tags: ["Pull Requests"],
849	849	summary: "Get latest AI review for pull request",
	850	+ description: "Returns latest AI review suggestions filtered by path-scoped read permissions, including `hiddenSuggestions` metadata.",
850	851	security: [{ bearerAuth: [] }],
851	852	parameters: [
852	853	{ name: "owner", in: "path", required: true, schema: { type: "string" } },
853	854

▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/ai-review.ts

+14-1

@@ -11,6 +11,7 @@ import type { AIProvider } from "@/lib/ai-review";
11	11	import { logger } from "@/lib/logger";
12	12	import { parseAIConfigFromStorage } from "@/lib/ai-config";
13	13	import { canReadRepo } from "@/lib/permissions";
	14	+import { checkPathPermissions } from "@/lib/path-scoping";
14	15
15	16	export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
16	17	const { owner, repo: repoName, number } = params;
@@ -60,7 +61,19 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
60	61	where: eq(schema.aiReviewSuggestions.aiReviewId, review.id),
61	62	});
62	63
63		- return success({ review, suggestions });
	64	+ const scopedPaths = Array.from(new Set(suggestions.map((item) => item.path).filter(Boolean)));
	65	+ const deniedPaths = new Set<string>();
	66	+ if (scopedPaths.length > 0) {
	67	+ const permission = await checkPathPermissions(user?.id \|\| "__anonymous__", repository.id, scopedPaths, "read");
	68	+ for (const path of permission.deniedPaths \|\| []) deniedPaths.add(path);
	69	+ }
	70	+ const visibleSuggestions = suggestions.filter((item) => !deniedPaths.has(item.path));
	71	+
	72	+ return success({
	73	+ review,
	74	+ suggestions: visibleSuggestions,
	75	+ hiddenSuggestions: suggestions.length - visibleSuggestions.length,
	76	+ });
64	77	});
65	78
66	79	export const POST: APIRoute = withErrorHandler(async ({ request, params }) => {
67	80

▼tests/integration/ai-review-route-path-permissions.test.ts

+105-0

@@ -0,0 +1,105 @@
	1	+import { beforeEach, describe, expect, it, vi } from "vitest";
	2	+
	3	+const {
	4	+ canReadRepoMock,
	5	+ checkPathPermissionsMock,
	6	+ fakeSchema,
	7	+} = vi.hoisted(() => ({
	8	+ canReadRepoMock: vi.fn(async () => true),
	9	+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
	10	+ fakeSchema: {
	11	+ users: { username: {}, id: {} },
	12	+ repositories: { ownerId: {}, name: {}, id: {} },
	13	+ pullRequests: { repositoryId: {}, number: {}, id: {} },
	14	+ aiReviews: { pullRequestId: {}, createdAt: {} },
	15	+ aiReviewSuggestions: { aiReviewId: {}, path: {} },
	16	+ } as any,
	17	+}));
	18	+
	19	+let mockDb: any;
	20	+
	21	+vi.mock("@/db", () => ({
	22	+ getDatabase: () => mockDb,
	23	+ schema: fakeSchema,
	24	+}));
	25	+
	26	+vi.mock("@/lib/permissions", () => ({
	27	+ canReadRepo: canReadRepoMock,
	28	+}));
	29	+
	30	+vi.mock("@/lib/path-scoping", () => ({
	31	+ checkPathPermissions: checkPathPermissionsMock,
	32	+}));
	33	+
	34	+vi.mock("@/lib/auth", () => ({
	35	+ getUserFromRequest: vi.fn(async () => ({ userId: "user-1", isAdmin: false })),
	36	+}));
	37	+
	38	+vi.mock("@/lib/ai-review", () => ({
	39	+ triggerAIReview: vi.fn(async () => ({ id: "review-1", status: "pending" })),
	40	+}));
	41	+
	42	+vi.mock("@/lib/ai-config", () => ({
	43	+ parseAIConfigFromStorage: vi.fn(() => ({ provider: "openai", model: "gpt-4-turbo", apiKeys: {} })),
	44	+}));
	45	+
	46	+import { GET as aiReviewGet } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/ai-review";
	47	+
	48	+function makeDb() {
	49	+ return {
	50	+ query: {
	51	+ users: {
	52	+ findFirst: vi.fn(async () => ({ id: "owner-1" })),
	53	+ },
	54	+ repositories: {
	55	+ findFirst: vi.fn(async () => ({ id: "repo-1", ownerId: "owner-1", name: "demo", owner: { username: "owner-1" } })),
	56	+ },
	57	+ pullRequests: {
	58	+ findFirst: vi.fn(async () => ({ id: "pr-1", repositoryId: "repo-1", number: 42 })),
	59	+ },
	60	+ aiReviews: {
	61	+ findFirst: vi.fn(async () => ({ id: "review-1", pullRequestId: "pr-1" })),
	62	+ },
	63	+ aiReviewSuggestions: {
	64	+ findMany: vi.fn(async () => ([
	65	+ { id: "s1", aiReviewId: "review-1", path: "src/app.ts", message: "ok" },
	66	+ { id: "s2", aiReviewId: "review-1", path: "secure/secret.ts", message: "sensitive" },
	67	+ ])),
	68	+ },
	69	+ },
	70	+ };
	71	+}
	72	+
	73	+describe("AI review route path permissions", () => {
	74	+ beforeEach(() => {
	75	+ vi.clearAllMocks();
	76	+ mockDb = makeDb();
	77	+ canReadRepoMock.mockResolvedValue(true);
	78	+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
	79	+ });
	80	+
	81	+ it("filters suggestion paths denied by read permissions", async () => {
	82	+ checkPathPermissionsMock.mockResolvedValue({
	83	+ allowed: false,
	84	+ deniedPaths: ["secure/secret.ts"],
	85	+ reason: "denied",
	86	+ });
	87	+
	88	+ const response = await aiReviewGet({
	89	+ params: { owner: "owner-1", repo: "demo", number: "42" },
	90	+ locals: { user: { id: "user-1", isAdmin: false } },
	91	+ } as any);
	92	+
	93	+ const body = await response.json();
	94	+ expect(response.status).toBe(200);
	95	+ expect(body?.data?.suggestions).toHaveLength(1);
	96	+ expect(body?.data?.suggestions?.[0]?.path).toBe("src/app.ts");
	97	+ expect(body?.data?.hiddenSuggestions).toBe(1);
	98	+ expect(checkPathPermissionsMock).toHaveBeenCalledWith(
	99	+ "user-1",
	100	+ "repo-1",
	101	+ ["src/app.ts", "secure/secret.ts"],
	102	+ "read"
	103	+ );
	104	+ });
	105	+});
0	106