swadhin / opencodehub public

Enforce path-scoped permissions for PR comment submissions

swadhinbiswas committed 8 months ago
2a813ed
1 parent 54e9ce2
commit 2a813ed
4 files changed
+172 -2
Showing 4 changed files+172-2
src/pages/api/repos/[owner]/[repo]/pulls/[number]/comments.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews/batch.tstests/integration/pr-comments-path-permissions.test.tstests/integration/review-batch-route.test.ts
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/comments.ts
+14-1
@@ -8,9 +8,10 @@ import type { NodePgDatabase } from "drizzle-orm/node-postgres";
8
8
import { eq, and, desc, isNull } from "drizzle-orm";
9
9
import { getDatabase, schema } from "@/db";
10
10
import { getUserFromRequest } from "@/lib/auth";
11
-import { parseBody, unauthorized, badRequest, notFound, success, serverError } from "@/lib/api";
11
+import { parseBody, unauthorized, badRequest, notFound, success, forbidden } from "@/lib/api";
12
12
import { z } from "zod";
13
13
import crypto from "crypto";
14
+import { checkPathPermissions } from "@/lib/path-scoping";
14
15
15
16
const createCommentSchema = z.object({
16
17
body: z.string().min(1),
@@ -111,6 +112,18 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request }) => {
111
112
return notFound("Pull request not found");
112
113
}
113
114
115
+ if (parsed.data.path) {
116
+ const permission = await checkPathPermissions(
117
+ tokenPayload.userId,
118
+ pr.repositoryId,
119
+ [parsed.data.path],
120
+ "write"
121
+ );
122
+ if (!permission.allowed) {
123
+ return forbidden(permission.reason || "Insufficient path permissions for this comment");
124
+ }
125
+ }
126
+
114
127
const now = new Date();
115
128
const commentId = `comment_${crypto.randomBytes(8).toString("hex")}`;
116
129
117
130
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews/batch.ts
+15-0
@@ -9,6 +9,7 @@ import { withErrorHandler } from "@/lib/errors";
9
9
import { logger } from "@/lib/logger";
10
10
import { canReadRepo, canWriteRepo } from "@/lib/permissions";
11
11
import { badRequest, forbidden, notFound, parseBody, success, unauthorized } from "@/lib/api";
12
+import { checkPathPermissions } from "@/lib/path-scoping";
12
13
13
14
const batchReviewSchema = z.object({
14
15
state: z.enum(["APPROVED", "CHANGES_REQUESTED", "COMMENTED"]).default("COMMENTED"),
@@ -68,6 +69,20 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request, locals
68
69
return badRequest("Authors cannot approve or request changes on their own PR");
69
70
}
70
71
72
+ const scopedPaths = Array.from(
73
+ new Set(
74
+ parsed.data.comments
75
+ .map((comment) => comment.path?.trim())
76
+ .filter((path): path is string => Boolean(path))
77
+ )
78
+ );
79
+ if (scopedPaths.length > 0) {
80
+ const permission = await checkPathPermissions(userId, repo.id, scopedPaths, "write");
81
+ if (!permission.allowed) {
82
+ return forbidden(permission.reason || "Insufficient path permissions for one or more review comments");
83
+ }
84
+ }
85
+
71
86
const reviewId = nanoid();
72
87
const now = new Date();
73
88
const createdCommentIds: string[] = [];
74
89
▼tests/integration/pr-comments-path-permissions.test.ts
+104-0
@@ -0,0 +1,104 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const { getUserFromRequestMock, checkPathPermissionsMock, fakeSchema } = vi.hoisted(() => ({
4
+ getUserFromRequestMock: vi.fn(async () => ({ userId: "user-1", isAdmin: false })),
5
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
6
+ fakeSchema: {
7
+ pullRequests: { number: {}, repositoryId: {} },
8
+ pullRequestComments: { id: {}, pullRequestId: {} },
9
+ } as any,
10
+}));
11
+
12
+let mockDb: any;
13
+
14
+vi.mock("@/db", () => ({
15
+ getDatabase: () => mockDb,
16
+ schema: fakeSchema,
17
+}));
18
+
19
+vi.mock("@/lib/auth", () => ({
20
+ getUserFromRequest: getUserFromRequestMock,
21
+}));
22
+
23
+vi.mock("@/lib/path-scoping", () => ({
24
+ checkPathPermissions: checkPathPermissionsMock,
25
+}));
26
+
27
+import { POST as createCommentPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/comments";
28
+
29
+function makeDb() {
30
+ const pr = {
31
+ id: "pr-1",
32
+ repositoryId: "repo-1",
33
+ repository: { id: "repo-1", ownerId: "owner-1" },
34
+ };
35
+ const insertCalls: unknown[] = [];
36
+
37
+ return {
38
+ query: {
39
+ pullRequests: {
40
+ findFirst: vi.fn(async () => pr),
41
+ },
42
+ pullRequestComments: {
43
+ findFirst: vi.fn(async () => ({ id: "comment-1", author: { username: "user-1" } })),
44
+ },
45
+ },
46
+ insert: vi.fn(() => ({
47
+ values: vi.fn(async (value: unknown) => {
48
+ insertCalls.push(value);
49
+ }),
50
+ })),
51
+ __state: { insertCalls },
52
+ };
53
+}
54
+
55
+async function readJson(response: Response): Promise<any> {
56
+ return response.json();
57
+}
58
+
59
+describe("PR comments path permissions", () => {
60
+ beforeEach(() => {
61
+ mockDb = makeDb();
62
+ getUserFromRequestMock.mockResolvedValue({ userId: "user-1", isAdmin: false });
63
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
64
+ });
65
+
66
+ it("blocks inline comment creation when path write permission is denied", async () => {
67
+ checkPathPermissionsMock.mockResolvedValue({
68
+ allowed: false,
69
+ deniedPaths: ["secure/config.yml"],
70
+ reason: "Insufficient permissions for paths: secure/config.yml",
71
+ });
72
+
73
+ const response = await createCommentPost({
74
+ params: { owner: "owner-1", repo: "demo", number: "42" },
75
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/comments", {
76
+ method: "POST",
77
+ headers: { "content-type": "application/json" },
78
+ body: JSON.stringify({ body: "blocked", path: "secure/config.yml", line: 1 }),
79
+ }),
80
+ } as any);
81
+
82
+ const body = await readJson(response);
83
+ expect(response.status).toBe(403);
84
+ expect(body?.error?.code).toBe("FORBIDDEN");
85
+ expect(mockDb.__state.insertCalls).toHaveLength(0);
86
+ });
87
+
88
+ it("allows comment creation when path permission check passes", async () => {
89
+ const response = await createCommentPost({
90
+ params: { owner: "owner-1", repo: "demo", number: "42" },
91
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/comments", {
92
+ method: "POST",
93
+ headers: { "content-type": "application/json" },
94
+ body: JSON.stringify({ body: "allowed", path: "src/app.ts", line: 3 }),
95
+ }),
96
+ } as any);
97
+
98
+ const body = await readJson(response);
99
+ expect(response.status).toBe(200);
100
+ expect(body?.success).toBe(true);
101
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith("user-1", "repo-1", ["src/app.ts"], "write");
102
+ expect(mockDb.__state.insertCalls).toHaveLength(1);
103
+ });
104
+});
0
105
▼tests/integration/review-batch-route.test.ts
+39-1
@@ -1,9 +1,10 @@
1
1
import { beforeEach, describe, expect, it, vi } from "vitest";
2
2
3
-const { canReadRepoMock, canWriteRepoMock, triggerAutomationMock, fakeSchema } = vi.hoisted(() => ({
3
+const { canReadRepoMock, canWriteRepoMock, triggerAutomationMock, checkPathPermissionsMock, fakeSchema } = vi.hoisted(() => ({
4
4
canReadRepoMock: vi.fn(async () => true),
5
5
canWriteRepoMock: vi.fn(async () => true),
6
6
triggerAutomationMock: vi.fn(async () => undefined),
7
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
7
8
fakeSchema: {
8
9
users: { username: {} },
9
10
repositories: { ownerId: {}, name: {}, id: {} },
@@ -29,6 +30,10 @@ vi.mock("@/lib/automations", () => ({
29
30
triggerAutomation: triggerAutomationMock,
30
31
}));
31
32
33
+vi.mock("@/lib/path-scoping", () => ({
34
+ checkPathPermissions: checkPathPermissionsMock,
35
+}));
36
+
32
37
import { POST as batchReviewPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews/batch";
33
38
34
39
function makeDb(options?: { failCommentInsert?: boolean }) {
@@ -88,6 +93,7 @@ describe("batch review route", () => {
88
93
canReadRepoMock.mockResolvedValue(true);
89
94
canWriteRepoMock.mockResolvedValue(true);
90
95
triggerAutomationMock.mockClear();
96
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
91
97
});
92
98
93
99
it("returns 401 when user is not authenticated", async () => {
@@ -163,6 +169,38 @@ describe("batch review route", () => {
163
169
expect(commentInserts).toHaveLength(2);
164
170
});
165
171
172
+ it("returns 403 when path-scoped permissions deny any review comment path", async () => {
173
+ mockDb = makeDb();
174
+ checkPathPermissionsMock.mockResolvedValue({
175
+ allowed: false,
176
+ deniedPaths: ["secure/secret.ts"],
177
+ reason: "Insufficient permissions for paths: secure/secret.ts",
178
+ });
179
+
180
+ const response = await batchReviewPost({
181
+ params: { owner: "acme", repo: "demo", number: "42" },
182
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/reviews/batch", {
183
+ method: "POST",
184
+ headers: { "content-type": "application/json" },
185
+ body: JSON.stringify({
186
+ state: "COMMENTED",
187
+ comments: [{ body: "secure change", path: "secure/secret.ts", line: 10 }],
188
+ }),
189
+ }),
190
+ locals: { user: { id: "reviewer-1" } },
191
+ } as any);
192
+
193
+ const body = await readJson(response);
194
+ expect(response.status).toBe(403);
195
+ expect(body?.error?.code).toBe("FORBIDDEN");
196
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith(
197
+ "reviewer-1",
198
+ "repo-1",
199
+ ["secure/secret.ts"],
200
+ "write"
201
+ );
202
+ });
203
+
166
204
it("rolls back created rows when comment insert fails mid-batch", async () => {
167
205
mockDb = makeDb({ failCommentInsert: true });
168
206
169
207