swadhin / opencodehub public

Enforce repo and path permissions for comment and review mutations

swadhinbiswas committed 8 months ago
93f6489
1 parent 2de61fa
commit 93f6489
5 files changed
+237 -3
Showing 5 changed files+237-3
src/lib/graphql/resolvers.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/comments.tstests/integration/pr-comments-path-permissions.test.tstests/unit/graphql-add-comment-permissions.test.tstests/unit/graphql-review-permissions.test.ts
▼src/lib/graphql/resolvers.ts
+50-2
@@ -566,12 +566,46 @@ export const resolvers = {
566
566
if (!ctx.userId) throw new Error("Authentication required");
567
567
568
568
const { generateId } = await import("@/lib/utils");
569
+ const { checkPathPermissions } = await import("@/lib/path-scoping");
570
+
571
+ const pullRequest = await ctx.db.query.pullRequests.findFirst({
572
+ where: eq(schema.pullRequests.id, input.pullRequestId),
573
+ columns: {
574
+ id: true,
575
+ repositoryId: true,
576
+ },
577
+ });
578
+ if (!pullRequest) throw new Error("Pull request not found");
579
+
580
+ const repository = await ctx.db.query.repositories.findFirst({
581
+ where: eq(schema.repositories.id, pullRequest.repositoryId),
582
+ });
583
+ if (!repository || !(await canWriteRepo(ctx.userId, repository, { isAdmin: ctx.user?.isAdmin ?? undefined }))) {
584
+ throw new Error("Insufficient repository permissions");
585
+ }
586
+
587
+ const scopedPaths = [...new Set(
588
+ (input.comments || [])
589
+ .map((comment: { path?: string }) => comment.path)
590
+ .filter(Boolean) as string[]
591
+ )];
592
+ if (scopedPaths.length > 0) {
593
+ const permission = await checkPathPermissions(
594
+ ctx.userId,
595
+ repository.id,
596
+ scopedPaths,
597
+ "write"
598
+ );
599
+ if (!permission.allowed) {
600
+ throw new Error(permission.reason || "Insufficient path permissions");
601
+ }
602
+ }
569
603
570
604
const reviewId = generateId();
571
605
572
606
await ctx.db.insert(schema.pullRequestReviews).values({
573
607
id: reviewId,
574
- pullRequestId: input.pullRequestId,
608
+ pullRequestId: pullRequest.id,
575
609
reviewerId: ctx.userId,
576
610
state: input.event.toLowerCase(), // APPROVED, REQUEST_CHANGES, COMMENT, etc.
577
611
body: input.body,
@@ -585,7 +619,7 @@ export const resolvers = {
585
619
for (const comment of input.comments) {
586
620
await ctx.db.insert(schema.pullRequestComments).values({
587
621
id: generateId(),
588
- pullRequestId: input.pullRequestId,
622
+ pullRequestId: pullRequest.id,
589
623
reviewId: reviewId,
590
624
authorId: ctx.userId,
591
625
body: comment.body,
@@ -621,6 +655,13 @@ export const resolvers = {
621
655
});
622
656
623
657
if (issue) {
658
+ const repository = await ctx.db.query.repositories.findFirst({
659
+ where: eq(schema.repositories.id, issue.repositoryId),
660
+ });
661
+ if (!repository || !(await canWriteRepo(ctx.userId, repository, { isAdmin: ctx.user?.isAdmin ?? undefined }))) {
662
+ throw new Error("Insufficient repository permissions");
663
+ }
664
+
624
665
await ctx.db.insert(schema.issueComments).values({
625
666
id,
626
667
issueId: input.subjectId,
@@ -643,6 +684,13 @@ export const resolvers = {
643
684
});
644
685
645
686
if (pr) {
687
+ const repository = await ctx.db.query.repositories.findFirst({
688
+ where: eq(schema.repositories.id, pr.repositoryId),
689
+ });
690
+ if (!repository || !(await canWriteRepo(ctx.userId, repository, { isAdmin: ctx.user?.isAdmin ?? undefined }))) {
691
+ throw new Error("Insufficient repository permissions");
692
+ }
693
+
646
694
await ctx.db.insert(schema.pullRequestComments).values({
647
695
id,
648
696
pullRequestId: input.subjectId,
649
697
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/comments.ts
+13-0
@@ -12,6 +12,7 @@ import { parseBody, unauthorized, badRequest, notFound, success, forbidden } fro
12
12
import { z } from "zod";
13
13
import crypto from "crypto";
14
14
import { checkPathPermissions } from "@/lib/path-scoping";
15
+import { canReadRepo, canWriteRepo } from "@/lib/permissions";
15
16
16
17
const createCommentSchema = z.object({
17
18
body: z.string().min(1),
@@ -62,6 +63,9 @@ export const GET: APIRoute = withErrorHandler(async ({ params, request }) => {
62
63
const db = getDatabase() as NodePgDatabase<typeof schema>;
63
64
const repository = await getRepositoryByOwnerAndName(db, owner as string, repo as string);
64
65
if (!repository) return notFound("Repository not found");
66
+ if (!(await canReadRepo(tokenPayload.userId, repository, { isAdmin: tokenPayload.isAdmin }))) {
67
+ return notFound("Repository not found");
68
+ }
65
69
66
70
// Find PR
67
71
const pr = await db.query.pullRequests.findFirst({
@@ -122,6 +126,9 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request }) => {
122
126
const db = getDatabase() as NodePgDatabase<typeof schema>;
123
127
const repository = await getRepositoryByOwnerAndName(db, owner as string, repo as string);
124
128
if (!repository) return notFound("Repository not found");
129
+ if (!(await canWriteRepo(tokenPayload.userId, repository, { isAdmin: tokenPayload.isAdmin }))) {
130
+ return forbidden();
131
+ }
125
132
126
133
// Find PR
127
134
const pr = await db.query.pullRequests.findFirst({
@@ -198,6 +205,9 @@ export const PATCH: APIRoute = withErrorHandler(async ({ params, request }) => {
198
205
const db = getDatabase() as NodePgDatabase<typeof schema>;
199
206
const repository = await getRepositoryByOwnerAndName(db, owner as string, repo as string);
200
207
if (!repository) return notFound("Repository not found");
208
+ if (!(await canWriteRepo(tokenPayload.userId, repository, { isAdmin: tokenPayload.isAdmin }))) {
209
+ return forbidden();
210
+ }
201
211
202
212
// Find comment
203
213
const comment = await db.query.pullRequestComments.findFirst({
@@ -256,6 +266,9 @@ export const DELETE: APIRoute = withErrorHandler(async ({ params, request }) =>
256
266
const db = getDatabase() as NodePgDatabase<typeof schema>;
257
267
const repository = await getRepositoryByOwnerAndName(db, owner as string, repo as string);
258
268
if (!repository) return notFound("Repository not found");
269
+ if (!(await canWriteRepo(tokenPayload.userId, repository, { isAdmin: tokenPayload.isAdmin }))) {
270
+ return forbidden();
271
+ }
259
272
260
273
// Find comment
261
274
const comment = await db.query.pullRequestComments.findFirst({
262
275
▼tests/integration/pr-comments-path-permissions.test.ts
+28-1
@@ -1,8 +1,10 @@
1
1
import { beforeEach, describe, expect, it, vi } from "vitest";
2
2
3
-const { getUserFromRequestMock, checkPathPermissionsMock, fakeSchema } = vi.hoisted(() => ({
3
+const { getUserFromRequestMock, checkPathPermissionsMock, canReadRepoMock, canWriteRepoMock, fakeSchema } = vi.hoisted(() => ({
4
4
getUserFromRequestMock: vi.fn(async () => ({ userId: "user-1", isAdmin: false })),
5
5
checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
6
+ canReadRepoMock: vi.fn(async () => true),
7
+ canWriteRepoMock: vi.fn(async () => true),
6
8
fakeSchema: {
7
9
users: { username: {}, id: {} },
8
10
repositories: { ownerId: {}, name: {}, id: {} },
@@ -26,6 +28,11 @@ vi.mock("@/lib/path-scoping", () => ({
26
28
checkPathPermissions: checkPathPermissionsMock,
27
29
}));
28
30
31
+vi.mock("@/lib/permissions", () => ({
32
+ canReadRepo: canReadRepoMock,
33
+ canWriteRepo: canWriteRepoMock,
34
+}));
35
+
29
36
import { POST as createCommentPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/comments";
30
37
import { PATCH as updateCommentPatch } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/comments";
31
38
import { DELETE as deleteCommentDelete } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/comments";
@@ -98,6 +105,8 @@ describe("PR comments path permissions", () => {
98
105
mockDb = makeDb();
99
106
getUserFromRequestMock.mockResolvedValue({ userId: "user-1", isAdmin: false });
100
107
checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
108
+ canReadRepoMock.mockResolvedValue(true);
109
+ canWriteRepoMock.mockResolvedValue(true);
101
110
});
102
111
103
112
it("blocks inline comment creation when path write permission is denied", async () => {
@@ -122,6 +131,24 @@ describe("PR comments path permissions", () => {
122
131
expect(mockDb.__state.insertCalls).toHaveLength(0);
123
132
});
124
133
134
+ it("blocks comment creation when repository write permission is denied", async () => {
135
+ canWriteRepoMock.mockResolvedValue(false);
136
+
137
+ const response = await createCommentPost({
138
+ params: { owner: "owner-1", repo: "demo", number: "42" },
139
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/comments", {
140
+ method: "POST",
141
+ headers: { "content-type": "application/json" },
142
+ body: JSON.stringify({ body: "blocked", path: "src/app.ts", line: 1 }),
143
+ }),
144
+ } as any);
145
+
146
+ const body = await readJson(response);
147
+ expect(response.status).toBe(403);
148
+ expect(body?.error?.code).toBe("FORBIDDEN");
149
+ expect(mockDb.__state.insertCalls).toHaveLength(0);
150
+ });
151
+
125
152
it("allows comment creation when path permission check passes", async () => {
126
153
const response = await createCommentPost({
127
154
params: { owner: "owner-1", repo: "demo", number: "42" },
128
155
▼tests/unit/graphql-add-comment-permissions.test.ts
+66-0
@@ -0,0 +1,66 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const { canWriteRepoMock } = vi.hoisted(() => ({
4
+ canWriteRepoMock: vi.fn(async () => true),
5
+}));
6
+
7
+vi.mock("@/lib/permissions", async () => {
8
+ const actual = await vi.importActual<any>("@/lib/permissions");
9
+ return {
10
+ ...actual,
11
+ canWriteRepo: canWriteRepoMock,
12
+ };
13
+});
14
+
15
+import { resolvers } from "@/lib/graphql/resolvers";
16
+
17
+function makeContext() {
18
+ const insertCalls: unknown[] = [];
19
+ return {
20
+ db: {
21
+ query: {
22
+ issues: {
23
+ findFirst: vi.fn(async () => null),
24
+ },
25
+ pullRequests: {
26
+ findFirst: vi.fn(async () => ({ id: "pr-1", repositoryId: "repo-1" })),
27
+ },
28
+ repositories: {
29
+ findFirst: vi.fn(async () => ({ id: "repo-1" })),
30
+ },
31
+ pullRequestComments: {
32
+ findFirst: vi.fn(async () => ({ id: "comment-1" })),
33
+ },
34
+ },
35
+ insert: vi.fn(() => ({
36
+ values: vi.fn(async (value: unknown) => {
37
+ insertCalls.push(value);
38
+ }),
39
+ })),
40
+ __state: { insertCalls },
41
+ },
42
+ userId: "user-1",
43
+ user: { isAdmin: false },
44
+ } as any;
45
+}
46
+
47
+describe("GraphQL addComment permission enforcement", () => {
48
+ beforeEach(() => {
49
+ canWriteRepoMock.mockResolvedValue(true);
50
+ });
51
+
52
+ it("blocks PR comment creation when user lacks repository write permission", async () => {
53
+ const ctx = makeContext();
54
+ canWriteRepoMock.mockResolvedValue(false);
55
+
56
+ await expect(
57
+ resolvers.Mutation.addComment(
58
+ {},
59
+ { input: { subjectId: "pr-1", body: "test comment" } },
60
+ ctx
61
+ )
62
+ ).rejects.toThrow("Insufficient repository permissions");
63
+
64
+ expect(ctx.db.__state.insertCalls).toHaveLength(0);
65
+ });
66
+});
0
67
▼tests/unit/graphql-review-permissions.test.ts
+80-0
@@ -0,0 +1,80 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const { canWriteRepoMock, checkPathPermissionsMock } = vi.hoisted(() => ({
4
+ canWriteRepoMock: vi.fn(async () => true),
5
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
6
+}));
7
+
8
+vi.mock("@/lib/permissions", async () => {
9
+ const actual = await vi.importActual<any>("@/lib/permissions");
10
+ return {
11
+ ...actual,
12
+ canWriteRepo: canWriteRepoMock,
13
+ };
14
+});
15
+
16
+vi.mock("@/lib/path-scoping", () => ({
17
+ checkPathPermissions: checkPathPermissionsMock,
18
+}));
19
+
20
+import { resolvers } from "@/lib/graphql/resolvers";
21
+
22
+function makeContext() {
23
+ const insertCalls: unknown[] = [];
24
+ return {
25
+ db: {
26
+ query: {
27
+ pullRequests: {
28
+ findFirst: vi.fn(async () => ({ id: "pr-1", repositoryId: "repo-1" })),
29
+ },
30
+ repositories: {
31
+ findFirst: vi.fn(async () => ({ id: "repo-1" })),
32
+ },
33
+ pullRequestReviews: {
34
+ findFirst: vi.fn(async () => ({ id: "review-1" })),
35
+ },
36
+ },
37
+ insert: vi.fn(() => ({
38
+ values: vi.fn(async (value: unknown) => {
39
+ insertCalls.push(value);
40
+ }),
41
+ })),
42
+ __state: { insertCalls },
43
+ },
44
+ userId: "user-1",
45
+ user: { isAdmin: false },
46
+ } as any;
47
+}
48
+
49
+describe("GraphQL addPullRequestReview permission enforcement", () => {
50
+ beforeEach(() => {
51
+ canWriteRepoMock.mockResolvedValue(true);
52
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
53
+ });
54
+
55
+ it("blocks review submission when path write permission is denied", async () => {
56
+ const ctx = makeContext();
57
+ checkPathPermissionsMock.mockResolvedValue({
58
+ allowed: false,
59
+ deniedPaths: ["secure/config.yml"],
60
+ reason: "Insufficient permissions for paths: secure/config.yml",
61
+ });
62
+
63
+ await expect(
64
+ resolvers.Mutation.addPullRequestReview(
65
+ {},
66
+ {
67
+ input: {
68
+ pullRequestId: "pr-1",
69
+ event: "COMMENT",
70
+ body: "review",
71
+ comments: [{ body: "inline", path: "secure/config.yml", line: 3 }],
72
+ },
73
+ },
74
+ ctx
75
+ )
76
+ ).rejects.toThrow("Insufficient permissions for paths: secure/config.yml");
77
+
78
+ expect(ctx.db.__state.insertCalls).toHaveLength(0);
79
+ });
80
+});
0
81