swadhin / opencodehub public

Add path-scoped filtering for PR review listings

swadhinbiswas committed 8 months ago
193a0f2
1 parent 611aaa9
commit 193a0f2
4 files changed
+154 -3
Showing 4 changed files+154-3
doc/feature_audit.mdsrc/lib/openapi.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews.tstests/integration/reviews-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, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke and read-filter flows, PR rewrite operations, and PR impact scans; 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, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke and read-filter flows, PR rewrite operations, and PR impact scans; 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
+16-0
@@ -780,6 +780,22 @@ export const openApiSpec = {
780
780
},
781
781
},
782
782
"/repos/{owner}/{repo}/pulls/{number}/reviews": {
783
+ get: {
784
+ tags: ["Pull Requests"],
785
+ summary: "List pull request reviews",
786
+ description: "Returns PR reviews with inline comments filtered by path-scoped read permissions, including `hiddenCommentCount` metadata.",
787
+ security: [{ bearerAuth: [] }],
788
+ parameters: [
789
+ { name: "owner", in: "path", required: true, schema: { type: "string" } },
790
+ { name: "repo", in: "path", required: true, schema: { type: "string" } },
791
+ { name: "number", in: "path", required: true, schema: { type: "integer" } },
792
+ ],
793
+ responses: {
794
+ 200: { description: "Reviews returned" },
795
+ 401: { description: "Unauthorized", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
796
+ 404: { description: "Repository or pull request not found", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
797
+ },
798
+ },
783
799
post: {
784
800
tags: ["Pull Requests"],
785
801
summary: "Submit pull request review",
786
802
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews.ts
+85-0
@@ -33,6 +33,91 @@ const reviewSchema = z.object({
33
33
.optional(),
34
34
});
35
35
36
+export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
37
+ const { owner: ownerName, repo: repoName, number } = params;
38
+ const user = locals.user;
39
+
40
+ if (!user) return unauthorized();
41
+ if (!ownerName || !repoName || !number) return badRequest("Missing parameters");
42
+
43
+ const db = getDatabase() as NodePgDatabase<typeof schema>;
44
+ const repoOwner = await db.query.users.findFirst({
45
+ where: eq(schema.users.username, ownerName),
46
+ });
47
+ if (!repoOwner) return notFound("Repository not found");
48
+
49
+ const repo = await db.query.repositories.findFirst({
50
+ where: and(
51
+ eq(schema.repositories.ownerId, repoOwner.id),
52
+ eq(schema.repositories.name, repoName)
53
+ ),
54
+ });
55
+ if (!repo) return notFound("Repository not found");
56
+ if (!(await canReadRepo(user.id, repo, { isAdmin: user.isAdmin }))) return notFound("Repository not found");
57
+
58
+ const pr = await db.query.pullRequests.findFirst({
59
+ where: and(
60
+ eq(schema.pullRequests.repositoryId, repo.id),
61
+ eq(schema.pullRequests.number, parseInt(number))
62
+ )
63
+ });
64
+ if (!pr) return notFound("Pull request not found");
65
+
66
+ const reviews = await db.query.pullRequestReviews.findMany({
67
+ where: eq(schema.pullRequestReviews.pullRequestId, pr.id),
68
+ with: {
69
+ reviewer: {
70
+ columns: {
71
+ id: true,
72
+ username: true,
73
+ displayName: true,
74
+ },
75
+ },
76
+ comments: {
77
+ with: {
78
+ author: {
79
+ columns: {
80
+ id: true,
81
+ username: true,
82
+ displayName: true,
83
+ },
84
+ },
85
+ },
86
+ },
87
+ },
88
+ });
89
+
90
+ const scopedPaths = Array.from(
91
+ new Set(
92
+ reviews
93
+ .flatMap((review) => review.comments || [])
94
+ .map((comment) => comment.path)
95
+ .filter((path): path is string => Boolean(path))
96
+ )
97
+ );
98
+ const deniedPaths = new Set<string>();
99
+ if (scopedPaths.length > 0) {
100
+ const permission = await checkPathPermissions(user.id, repo.id, scopedPaths, "read");
101
+ for (const path of permission.deniedPaths || []) deniedPaths.add(path);
102
+ }
103
+
104
+ let hiddenCommentCount = 0;
105
+ const filteredReviews = reviews.map((review) => {
106
+ const comments = (review.comments || []).filter((comment) => {
107
+ if (!comment.path) return true;
108
+ const visible = !deniedPaths.has(comment.path);
109
+ if (!visible) hiddenCommentCount += 1;
110
+ return visible;
111
+ });
112
+ return {
113
+ ...review,
114
+ comments,
115
+ };
116
+ });
117
+
118
+ return success({ reviews: filteredReviews, hiddenCommentCount });
119
+});
120
+
36
121
// POST: Submit a review
37
122
export const POST: APIRoute = withErrorHandler(async ({ params, request, locals }) => {
38
123
const { owner: ownerName, repo: repoName, number } = params;
39
124
▼tests/integration/reviews-route-path-permissions.test.ts
+52-2
@@ -33,12 +33,34 @@ vi.mock("@/lib/automations", () => ({
33
33
triggerAutomation: vi.fn(async () => undefined),
34
34
}));
35
35
36
-import { POST as reviewPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews";
36
+import { GET as reviewGet, POST as reviewPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews";
37
37
38
38
function makeDb() {
39
39
const owner = { id: "owner-1", username: "acme" };
40
40
const repo = { id: "repo-1", ownerId: "owner-1", name: "demo" };
41
41
const pr = { id: "pr-1", repositoryId: "repo-1", number: 42, authorId: "author-1" };
42
+ const reviews = [
43
+ {
44
+ id: "rev-1",
45
+ pullRequestId: "pr-1",
46
+ state: "COMMENTED",
47
+ reviewer: { id: "reviewer-1", username: "reviewer-1", displayName: "Reviewer One" },
48
+ comments: [
49
+ {
50
+ id: "comment-1",
51
+ path: "src/app.ts",
52
+ body: "visible",
53
+ author: { id: "reviewer-1", username: "reviewer-1", displayName: "Reviewer One" },
54
+ },
55
+ {
56
+ id: "comment-2",
57
+ path: "secure/secret.ts",
58
+ body: "hidden",
59
+ author: { id: "reviewer-2", username: "reviewer-2", displayName: "Reviewer Two" },
60
+ },
61
+ ],
62
+ },
63
+ ];
42
64
43
65
const insertCalls: Array<{ table: unknown; value: unknown }> = [];
44
66
@@ -53,6 +75,9 @@ function makeDb() {
53
75
pullRequests: {
54
76
findFirst: vi.fn(async () => pr),
55
77
},
78
+ pullRequestReviews: {
79
+ findMany: vi.fn(async () => reviews),
80
+ },
56
81
},
57
82
insert: vi.fn((table: unknown) => ({
58
83
values: vi.fn(async (value: unknown) => {
@@ -103,6 +128,32 @@ describe("single review route path permissions", () => {
103
128
expect(mockDb.__state.insertCalls).toHaveLength(0);
104
129
});
105
130
131
+ it("filters inline review comments for denied read paths", async () => {
132
+ checkPathPermissionsMock.mockResolvedValue({
133
+ allowed: false,
134
+ deniedPaths: ["secure/secret.ts"],
135
+ reason: "Insufficient permissions for paths: secure/secret.ts",
136
+ });
137
+
138
+ const response = await reviewGet({
139
+ params: { owner: "acme", repo: "demo", number: "42" },
140
+ locals: { user: { id: "reviewer-1", isAdmin: false } },
141
+ } as any);
142
+
143
+ const body = await readJson(response);
144
+ expect(response.status).toBe(200);
145
+ expect(body?.data?.reviews).toHaveLength(1);
146
+ expect(body?.data?.reviews?.[0]?.comments).toHaveLength(1);
147
+ expect(body?.data?.reviews?.[0]?.comments?.[0]?.path).toBe("src/app.ts");
148
+ expect(body?.data?.hiddenCommentCount).toBe(1);
149
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith(
150
+ "reviewer-1",
151
+ "repo-1",
152
+ ["src/app.ts", "secure/secret.ts"],
153
+ "read"
154
+ );
155
+ });
156
+
106
157
it("creates review and inline comment when path permissions pass", async () => {
107
158
const response = await reviewPost({
108
159
params: { owner: "acme", repo: "demo", number: "42" },
@@ -125,4 +176,3 @@ describe("single review route path permissions", () => {
125
176
expect(mockDb.__state.insertCalls.length).toBe(2);
126
177
});
127
178
});
128
-
129
179