swadhin / opencodehub public

Add path-scoped checks and inline comments to single review API

swadhinbiswas committed 8 months ago
031c099
1 parent 6569299
commit 031c099
3 files changed
+251 -9
Showing 3 changed files+251-9
src/lib/openapi.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews.tstests/integration/reviews-route-path-permissions.test.ts
▼src/lib/openapi.ts
+54-0
@@ -174,6 +174,32 @@ export const openApiSpec = {
174
174
},
175
175
required: ["body"],
176
176
},
177
+ PullRequestReviewRequest: {
178
+ type: "object",
179
+ properties: {
180
+ state: { type: "string", enum: ["APPROVED", "CHANGES_REQUESTED", "COMMENTED"] },
181
+ body: { type: "string" },
182
+ commitSha: { type: "string" },
183
+ comments: {
184
+ type: "array",
185
+ items: {
186
+ type: "object",
187
+ properties: {
188
+ body: { type: "string" },
189
+ path: { type: "string" },
190
+ line: { type: "integer" },
191
+ side: { type: "string", enum: ["LEFT", "RIGHT"] },
192
+ startLine: { type: "integer" },
193
+ commitSha: { type: "string" },
194
+ inReplyToId: { type: "string" },
195
+ suggestedChange: { type: "string" },
196
+ },
197
+ required: ["body"],
198
+ },
199
+ },
200
+ },
201
+ required: ["state"],
202
+ },
177
203
WorkflowStateCreateRequest: {
178
204
type: "object",
179
205
properties: {
@@ -641,6 +667,34 @@ export const openApiSpec = {
641
667
},
642
668
},
643
669
},
670
+ "/repos/{owner}/{repo}/pulls/{number}/reviews": {
671
+ post: {
672
+ tags: ["Pull Requests"],
673
+ summary: "Submit pull request review",
674
+ description: "Submit a review state and optional inline comments with path-scoped permission enforcement.",
675
+ security: [{ bearerAuth: [] }],
676
+ parameters: [
677
+ { name: "owner", in: "path", required: true, schema: { type: "string" } },
678
+ { name: "repo", in: "path", required: true, schema: { type: "string" } },
679
+ { name: "number", in: "path", required: true, schema: { type: "integer" } },
680
+ ],
681
+ requestBody: {
682
+ required: true,
683
+ content: {
684
+ "application/json": {
685
+ schema: { $ref: "#/components/schemas/PullRequestReviewRequest" },
686
+ },
687
+ },
688
+ },
689
+ responses: {
690
+ 200: { description: "Review submitted" },
691
+ 400: { description: "Invalid review payload or state", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
692
+ 401: { description: "Unauthorized", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
693
+ 403: { description: "Forbidden", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
694
+ 404: { description: "Repository or pull request not found", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
695
+ },
696
+ },
697
+ },
644
698
"/repos/{owner}/{repo}/workflow/states": {
645
699
get: {
646
700
tags: ["Workflow"],
647
701
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews.ts
+69-9
@@ -6,8 +6,32 @@ import type { APIRoute } from "astro";
6
6
import { and, eq } from "drizzle-orm";
7
7
import { withErrorHandler } from "@/lib/errors";
8
8
import { logger } from "@/lib/logger";
9
-import { unauthorized, badRequest, notFound, success, forbidden } from "@/lib/api";
9
+import { unauthorized, badRequest, notFound, success, forbidden, parseBody } from "@/lib/api";
10
10
import { nanoid } from "nanoid";
11
+import { z } from "zod";
12
+import { checkPathPermissions } from "@/lib/path-scoping";
13
+import crypto from "crypto";
14
+
15
+const reviewSchema = z.object({
16
+ state: z.enum(["APPROVED", "CHANGES_REQUESTED", "COMMENTED"]),
17
+ body: z.string().max(10000).optional(),
18
+ commitSha: z.string().optional(),
19
+ comments: z
20
+ .array(
21
+ z.object({
22
+ body: z.string().min(1).max(20000),
23
+ path: z.string().optional(),
24
+ line: z.number().int().positive().optional(),
25
+ side: z.enum(["LEFT", "RIGHT"]).optional(),
26
+ startLine: z.number().int().positive().optional(),
27
+ commitSha: z.string().optional(),
28
+ inReplyToId: z.string().optional(),
29
+ suggestedChange: z.string().optional(),
30
+ })
31
+ )
32
+ .max(100)
33
+ .optional(),
34
+});
11
35
12
36
// POST: Submit a review
13
37
export const POST: APIRoute = withErrorHandler(async ({ params, request, locals }) => {
@@ -20,11 +44,9 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request, locals
20
44
return badRequest("Missing parameters");
21
45
}
22
46
23
- const { state, body, commitSha } = await request.json();
24
-
25
- if (!["APPROVED", "CHANGES_REQUESTED", "COMMENTED"].includes(state)) {
26
- return badRequest("Invalid review state");
27
- }
47
+ const parsed = await parseBody(request, reviewSchema);
48
+ if ("error" in parsed) return parsed.error;
49
+ const { state, body, commitSha, comments = [] } = parsed.data;
28
50
29
51
const db = getDatabase() as NodePgDatabase<typeof schema>;
30
52
const repoOwner = await db.query.users.findFirst({
@@ -63,17 +85,55 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request, locals
63
85
return badRequest("Authors cannot approve or request changes on their own PR");
64
86
}
65
87
88
+ const scopedPaths = Array.from(
89
+ new Set(
90
+ comments
91
+ .map((comment) => comment.path?.trim())
92
+ .filter((path): path is string => Boolean(path))
93
+ )
94
+ );
95
+ if (scopedPaths.length > 0) {
96
+ const permission = await checkPathPermissions(user.id, repo.id, scopedPaths, "write");
97
+ if (!permission.allowed) {
98
+ return forbidden(permission.reason || "Insufficient path permissions for one or more review comments");
99
+ }
100
+ }
101
+
102
+ const now = new Date();
66
103
const reviewId = nanoid();
67
104
await db.insert(schema.pullRequestReviews).values({
68
105
id: reviewId,
69
106
pullRequestId: pr.id,
70
107
reviewerId: user.id,
71
108
state,
72
- body,
109
+ body: body || "Review submitted",
73
110
commitSha,
74
- submittedAt: new Date()
111
+ submittedAt: now
75
112
});
76
113
114
+ for (const commentInput of comments) {
115
+ let commentBody = commentInput.body;
116
+ if (commentInput.suggestedChange) {
117
+ commentBody += `\n\n\`\`\`suggestion\n${commentInput.suggestedChange}\n\`\`\``;
118
+ }
119
+ const commentId = `comment_${crypto.randomBytes(8).toString("hex")}`;
120
+ await db.insert(schema.pullRequestComments).values({
121
+ id: commentId,
122
+ pullRequestId: pr.id,
123
+ reviewId,
124
+ authorId: user.id,
125
+ body: commentBody,
126
+ path: commentInput.path ?? null,
127
+ line: commentInput.line ?? null,
128
+ side: commentInput.side || "RIGHT",
129
+ startLine: commentInput.startLine ?? null,
130
+ commitSha: commentInput.commitSha || commitSha || null,
131
+ inReplyToId: commentInput.inReplyToId ?? null,
132
+ createdAt: now,
133
+ updatedAt: now,
134
+ });
135
+ }
136
+
77
137
// Trigger automation
78
138
import("@/lib/automations").then(({ triggerAutomation }) => {
79
139
let triggerEvent: "pr_approved" | "pr_changes_requested" | "comment_added" | null = null;
@@ -97,5 +157,5 @@ export const POST: APIRoute = withErrorHandler(async ({ params, request, locals
97
157
98
158
logger.info({ userId: user.id, prId: pr.id, reviewId, state }, "PR review submitted");
99
159
100
- return success({ id: reviewId, state });
160
+ return success({ id: reviewId, state, commentCount: comments.length });
101
161
});
102
162
▼tests/integration/reviews-route-path-permissions.test.ts
+128-0
@@ -0,0 +1,128 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const { canReadRepoMock, canWriteRepoMock, checkPathPermissionsMock, fakeSchema } = vi.hoisted(() => ({
4
+ canReadRepoMock: vi.fn(async () => true),
5
+ canWriteRepoMock: vi.fn(async () => true),
6
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
7
+ fakeSchema: {
8
+ users: { username: {} },
9
+ repositories: { ownerId: {}, name: {}, id: {} },
10
+ pullRequests: { repositoryId: {}, number: {}, id: {} },
11
+ pullRequestReviews: { id: {} },
12
+ pullRequestComments: { id: {} },
13
+ } as any,
14
+}));
15
+
16
+let mockDb: any;
17
+
18
+vi.mock("@/db", () => ({
19
+ getDatabase: () => mockDb,
20
+ schema: fakeSchema,
21
+}));
22
+
23
+vi.mock("@/lib/permissions", () => ({
24
+ canReadRepo: canReadRepoMock,
25
+ canWriteRepo: canWriteRepoMock,
26
+}));
27
+
28
+vi.mock("@/lib/path-scoping", () => ({
29
+ checkPathPermissions: checkPathPermissionsMock,
30
+}));
31
+
32
+vi.mock("@/lib/automations", () => ({
33
+ triggerAutomation: vi.fn(async () => undefined),
34
+}));
35
+
36
+import { POST as reviewPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/reviews";
37
+
38
+function makeDb() {
39
+ const owner = { id: "owner-1", username: "acme" };
40
+ const repo = { id: "repo-1", ownerId: "owner-1", name: "demo" };
41
+ const pr = { id: "pr-1", repositoryId: "repo-1", number: 42, authorId: "author-1" };
42
+
43
+ const insertCalls: Array<{ table: unknown; value: unknown }> = [];
44
+
45
+ return {
46
+ query: {
47
+ users: {
48
+ findFirst: vi.fn(async () => owner),
49
+ },
50
+ repositories: {
51
+ findFirst: vi.fn(async () => repo),
52
+ },
53
+ pullRequests: {
54
+ findFirst: vi.fn(async () => pr),
55
+ },
56
+ },
57
+ insert: vi.fn((table: unknown) => ({
58
+ values: vi.fn(async (value: unknown) => {
59
+ insertCalls.push({ table, value });
60
+ }),
61
+ })),
62
+ __state: {
63
+ insertCalls,
64
+ },
65
+ };
66
+}
67
+
68
+async function readJson(response: Response): Promise<any> {
69
+ return response.json();
70
+}
71
+
72
+describe("single review route path permissions", () => {
73
+ beforeEach(() => {
74
+ mockDb = makeDb();
75
+ canReadRepoMock.mockResolvedValue(true);
76
+ canWriteRepoMock.mockResolvedValue(true);
77
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
78
+ });
79
+
80
+ it("returns 403 when review comment path is denied by path-scoped permissions", async () => {
81
+ checkPathPermissionsMock.mockResolvedValue({
82
+ allowed: false,
83
+ deniedPaths: ["secure/secret.ts"],
84
+ reason: "Insufficient permissions for paths: secure/secret.ts",
85
+ });
86
+
87
+ const response = await reviewPost({
88
+ params: { owner: "acme", repo: "demo", number: "42" },
89
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/reviews", {
90
+ method: "POST",
91
+ headers: { "content-type": "application/json" },
92
+ body: JSON.stringify({
93
+ state: "COMMENTED",
94
+ comments: [{ body: "secure note", path: "secure/secret.ts", line: 5 }],
95
+ }),
96
+ }),
97
+ locals: { user: { id: "reviewer-1" } },
98
+ } as any);
99
+
100
+ const body = await readJson(response);
101
+ expect(response.status).toBe(403);
102
+ expect(body?.error?.code).toBe("FORBIDDEN");
103
+ expect(mockDb.__state.insertCalls).toHaveLength(0);
104
+ });
105
+
106
+ it("creates review and inline comment when path permissions pass", async () => {
107
+ const response = await reviewPost({
108
+ params: { owner: "acme", repo: "demo", number: "42" },
109
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/reviews", {
110
+ method: "POST",
111
+ headers: { "content-type": "application/json" },
112
+ body: JSON.stringify({
113
+ state: "COMMENTED",
114
+ body: "looks good",
115
+ comments: [{ body: "inline", path: "src/app.ts", line: 10 }],
116
+ }),
117
+ }),
118
+ locals: { user: { id: "reviewer-1" } },
119
+ } as any);
120
+
121
+ const body = await readJson(response);
122
+ expect(response.status).toBe(200);
123
+ expect(body?.data?.commentCount).toBe(1);
124
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith("reviewer-1", "repo-1", ["src/app.ts"], "write");
125
+ expect(mockDb.__state.insertCalls.length).toBe(2);
126
+ });
127
+});
128
+
0
129