Harden AI callback auth and enforce suggestion path permissions

swadhinbiswas committed 8 months ago
019051f
1 parent aea2e5e
commit 019051f
6 files changed
+431 -18
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/ai-review/callback.ts
+90-6
1
1
import type { APIRoute } from "astro";
2
2
import { and, eq } from "drizzle-orm";
3
3
import type { NodePgDatabase } from "drizzle-orm/node-postgres";
4
+import crypto from "crypto";
4
5
import { z } from "zod";
5
6
import { getDatabase, schema } from "@/db";
6
7
import { withErrorHandler } from "@/lib/errors";
43
44
rawResponse: z.unknown().optional(),
44
45
});
45
46
46
-function isAuthorized(request: Request): boolean {
47
+const CALLBACK_TIMESTAMP_TOLERANCE_MS = 5 * 60 * 1000;
48
+const CALLBACK_EVENT_TTL_MS = 10 * 60 * 1000;
49
+const seenEventIds = new Map<string, number>();
50
+
51
+function timingSafeMatch(expected: string, actual: string): boolean {
52
+ const expectedBuffer = Buffer.from(expected);
53
+ const actualBuffer = Buffer.from(actual);
54
+ if (expectedBuffer.length !== actualBuffer.length) return false;
55
+ return crypto.timingSafeEqual(expectedBuffer, actualBuffer);
56
+}
57
+
58
+function parseTimestampToMs(timestampHeader: string | null): number | null {
59
+ if (!timestampHeader) return null;
60
+ const parsed = Number.parseInt(timestampHeader, 10);
61
+ if (!Number.isFinite(parsed)) return null;
62
+ // Accept unix seconds or milliseconds for compatibility.
63
+ return parsed < 1_000_000_000_000 ? parsed * 1000 : parsed;
64
+}
65
+
66
+function purgeExpiredEvents(now: number): void {
67
+ for (const [eventId, expiresAt] of seenEventIds.entries()) {
68
+ if (expiresAt <= now) {
69
+ seenEventIds.delete(eventId);
70
+ }
71
+ }
72
+}
73
+
74
+function registerEventId(eventId: string, now: number): boolean {
75
+ purgeExpiredEvents(now);
76
+ if (seenEventIds.has(eventId)) {
77
+ return false;
78
+ }
79
+ seenEventIds.set(eventId, now + CALLBACK_EVENT_TTL_MS);
80
+ return true;
81
+}
82
+
83
+function isAuthorized(request: Request, rawBody: string): { ok: boolean; reason?: string } {
47
84
const expected =
48
85
process.env.EXTERNAL_AGENT_CALLBACK_SECRET || process.env.INTERNAL_HOOK_SECRET;
49
- if (!expected) return false;
50
- return request.headers.get("Authorization") === `Bearer ${expected}`;
86
+ if (!expected) return { ok: false, reason: "Callback secret is not configured" };
87
+
88
+ const bearerAuth = request.headers.get("authorization");
89
+ if (bearerAuth === `Bearer ${expected}`) {
90
+ return { ok: true };
91
+ }
92
+
93
+ const timestampHeader = request.headers.get("x-opencodehub-timestamp");
94
+ const signatureHeader = request.headers.get("x-opencodehub-signature");
95
+ const eventId =
96
+ request.headers.get("x-opencodehub-event-id") || request.headers.get("x-request-id");
97
+
98
+ if (!timestampHeader || !signatureHeader || !eventId) {
99
+ return { ok: false, reason: "Missing callback authentication headers" };
100
+ }
101
+
102
+ const timestampMs = parseTimestampToMs(timestampHeader);
103
+ if (!timestampMs) {
104
+ return { ok: false, reason: "Invalid callback timestamp" };
105
+ }
106
+
107
+ const now = Date.now();
108
+ if (Math.abs(now - timestampMs) > CALLBACK_TIMESTAMP_TOLERANCE_MS) {
109
+ return { ok: false, reason: "Callback timestamp is outside allowed window" };
110
+ }
111
+
112
+ const signedPayload = `${timestampHeader}.${rawBody}`;
113
+ const expectedSignature = `sha256=${crypto
114
+ .createHmac("sha256", expected)
115
+ .update(signedPayload)
116
+ .digest("hex")}`;
117
+ if (!timingSafeMatch(expectedSignature, signatureHeader)) {
118
+ return { ok: false, reason: "Invalid callback signature" };
119
+ }
120
+
121
+ if (!registerEventId(eventId, now)) {
122
+ return { ok: false, reason: "Duplicate callback event" };
123
+ }
124
+
125
+ return { ok: true };
51
126
}
52
127
53
128
export const POST: APIRoute = withErrorHandler(async ({ params, request }) => {
54
- if (!isAuthorized(request)) {
55
- return unauthorized();
129
+ const rawBody = await request.text();
130
+ const auth = isAuthorized(request, rawBody);
131
+ if (!auth.ok) {
132
+ return unauthorized(auth.reason);
56
133
}
57
134
58
135
const { owner, repo: repoName, number } = params;
61
138
return badRequest("Invalid repository or pull request");
62
139
}
63
140
64
- const parsed = callbackSchema.safeParse(await request.json());
141
+ let payload: unknown;
142
+ try {
143
+ payload = JSON.parse(rawBody);
144
+ } catch {
145
+ return badRequest("Invalid callback payload");
146
+ }
147
+
148
+ const parsed = callbackSchema.safeParse(payload);
65
149
if (!parsed.success) {
66
150
return badRequest("Invalid callback payload");
67
151
}
68
152
▼src/pages/api/repos/[owner]/[repo]/pulls/[pullNumber]/suggestions/apply.ts
+19-3
5
5
6
6
import type { APIRoute } from "astro";
7
7
import type { NodePgDatabase } from "drizzle-orm/node-postgres";
8
-import { eq } from "drizzle-orm";
8
+import { and, eq } from "drizzle-orm";
9
9
import { getDatabase, schema } from "@/db";
10
10
import { getUserFromRequest } from "@/lib/auth";
11
11
import { withErrorHandler } from "@/lib/errors";
12
12
import { parseBody, unauthorized, badRequest, notFound, forbidden, success } from "@/lib/api";
13
13
import { applySuggestion, batchApplySuggestions, canApplySuggestions } from "@/lib/suggestions";
14
+import { checkPathPermissions } from "@/lib/path-scoping";
14
15
import { z } from "zod";
15
16
import { logger } from "@/lib/logger";
16
17
43
44
}
44
45
45
46
const repository = await db.query.repositories.findFirst({
46
- where: eq(schema.repositories.ownerId, repoOwner.id),
47
+ where: and(
48
+ eq(schema.repositories.ownerId, repoOwner.id),
49
+ eq(schema.repositories.name, repo as string)
50
+ ),
47
51
});
48
52
49
53
if (!repository) {
52
56
53
57
// Find PR
54
58
const pr = await db.query.pullRequests.findFirst({
55
- where: eq(schema.pullRequests.number, parseInt(pullNumber as string)),
59
+ where: and(
60
+ eq(schema.pullRequests.repositoryId, repository.id),
61
+ eq(schema.pullRequests.number, parseInt(pullNumber as string))
62
+ ),
56
63
});
57
64
58
65
if (!pr) {
77
84
return badRequest(`Invalid comment IDs: ${invalidIds.join(", ")}`);
78
85
}
79
86
87
+ const pathScopedComments = comments.filter(c => commentIds.includes(c.id) && !!c.path);
88
+ if (pathScopedComments.length > 0) {
89
+ const scopedPaths = [...new Set(pathScopedComments.map(c => c.path as string))];
90
+ const permission = await checkPathPermissions(user.userId, repository.id, scopedPaths, "write");
91
+ if (!permission.allowed) {
92
+ return forbidden(permission.reason || "Insufficient path permissions to apply these suggestions");
93
+ }
94
+ }
95
+
80
96
// Apply suggestions
81
97
if (commentIds.length === 1) {
82
98
const result = await applySuggestion(commentIds[0], user.userId);
83
99
▼src/pages/api/repos/[owner]/[repo]/settings/states.ts
+50-6
3
3
import { withErrorHandler, success, badRequest, unauthorized } from "@/lib/api";
4
4
import { eq, and, asc } from "drizzle-orm";
5
5
import { nanoid } from "nanoid";
6
+import { z } from "zod";
7
+
8
+const reviewerSchema = z.object({
9
+ userId: z.string().optional(),
10
+ teamId: z.string().optional(),
11
+ requiredCount: z.number().int().positive().optional(),
12
+}).refine((value) => Boolean(value.userId) !== Boolean(value.teamId), {
13
+ message: "Each reviewer rule requires exactly one of userId or teamId",
14
+});
15
+
16
+const createStateSchema = z.object({
17
+ name: z.string().min(1),
18
+ displayName: z.string().min(1),
19
+ color: z.string().optional(),
20
+ description: z.string().optional(),
21
+ icon: z.string().optional(),
22
+ isFinal: z.boolean().optional(),
23
+ allowMerge: z.boolean().optional(),
24
+ requireCodeOwner: z.boolean().optional(),
25
+ reviewers: z.array(reviewerSchema).optional(),
26
+});
6
27
7
28
export const GET: APIRoute = withErrorHandler(async ({ params, locals }: APIContext) => {
8
29
const { repo } = locals as any;
11
32
const states = await db.query.prStateDefinitions.findMany({
12
33
where: eq(schema.prStateDefinitions.repositoryId, repo.id),
13
34
orderBy: asc(schema.prStateDefinitions.order),
35
+ with: {
36
+ reviewers: true,
37
+ },
14
38
});
15
39
16
40
return success(states);
26
50
return unauthorized("You must be an admin to manage PR states");
27
51
}
28
52
29
- const body = await request.json();
30
- const { name, displayName, color, description, icon, isFinal, allowMerge } = body;
31
-
32
- if (!name || !displayName) {
33
- return badRequest("Name and Display Name are required");
53
+ const body = await request.json().catch(() => null);
54
+ const parsed = createStateSchema.safeParse(body);
55
+ if (!parsed.success) {
56
+ return badRequest(parsed.error.issues[0]?.message || "Invalid state payload");
34
57
}
58
+ const { name, displayName, color, description, icon, isFinal, allowMerge, requireCodeOwner, reviewers } = parsed.data;
35
59
36
60
const id = nanoid();
37
61
52
76
icon,
53
77
isFinal: isFinal || false,
54
78
allowMerge: allowMerge || false,
79
+ requireCodeOwner: requireCodeOwner || false,
55
80
order: nextOrder,
56
81
}).returning();
57
82
58
- return success(newState);
83
+ if (reviewers && reviewers.length > 0) {
84
+ await (db as any).insert(schema.prStateReviewers).values(
85
+ reviewers.map((reviewer) => ({
86
+ id: nanoid(),
87
+ stateDefinitionId: id,
88
+ userId: reviewer.userId || null,
89
+ teamId: reviewer.teamId || null,
90
+ requiredCount: reviewer.requiredCount || 1,
91
+ }))
92
+ );
93
+ }
94
+
95
+ const created = await db.query.prStateDefinitions.findFirst({
96
+ where: eq(schema.prStateDefinitions.id, id),
97
+ with: {
98
+ reviewers: true,
99
+ },
100
+ });
101
+
102
+ return success(created || newState);
59
103
});
60
104
▼src/pages/api/repos/[owner]/[repo]/settings/states/[id].ts
+55-3
2
2
import { getDatabase, schema } from "@/db";
3
3
import { withErrorHandler, success, badRequest, unauthorized, notFound } from "@/lib/api";
4
4
import { eq, and } from "drizzle-orm";
5
+import { z } from "zod";
6
+import { nanoid } from "nanoid";
7
+
8
+const reviewerSchema = z.object({
9
+ userId: z.string().optional(),
10
+ teamId: z.string().optional(),
11
+ requiredCount: z.number().int().positive().optional(),
12
+}).refine((value) => Boolean(value.userId) !== Boolean(value.teamId), {
13
+ message: "Each reviewer rule requires exactly one of userId or teamId",
14
+});
15
+
16
+const updateStateSchema = z.object({
17
+ name: z.string().min(1).optional(),
18
+ displayName: z.string().min(1).optional(),
19
+ color: z.string().optional(),
20
+ description: z.string().optional(),
21
+ icon: z.string().optional(),
22
+ isFinal: z.boolean().optional(),
23
+ allowMerge: z.boolean().optional(),
24
+ requireCodeOwner: z.boolean().optional(),
25
+ order: z.number().int().optional(),
26
+ reviewers: z.array(reviewerSchema).optional(),
27
+});
5
28
6
29
export const PUT: APIRoute = withErrorHandler(async ({ params, request, locals }: APIContext) => {
7
30
const { repo, user } = locals as any;
16
39
return unauthorized("You must be an admin to manage PR states");
17
40
}
18
41
19
- const body = await request.json();
20
- const { name, displayName, color, description, icon, isFinal, allowMerge, order } = body;
42
+ const body = await request.json().catch(() => null);
43
+ const parsed = updateStateSchema.safeParse(body);
44
+ if (!parsed.success) {
45
+ return badRequest(parsed.error.issues[0]?.message || "Invalid state payload");
46
+ }
47
+ const { name, displayName, color, description, icon, isFinal, allowMerge, requireCodeOwner, order, reviewers } = parsed.data;
21
48
22
49
const existing = await db.query.prStateDefinitions.findFirst({
23
50
where: and(
37
64
icon: icon ?? existing.icon,
38
65
isFinal: isFinal ?? existing.isFinal,
39
66
allowMerge: allowMerge ?? existing.allowMerge,
67
+ requireCodeOwner: requireCodeOwner ?? existing.requireCodeOwner,
40
68
order: order ?? existing.order,
41
69
updatedAt: new Date(),
42
70
})
43
71
.where(eq(schema.prStateDefinitions.id, id))
44
72
.returning();
45
73
46
- return success(updated);
74
+ if (reviewers) {
75
+ await (db as any).delete(schema.prStateReviewers)
76
+ .where(eq(schema.prStateReviewers.stateDefinitionId, id));
77
+
78
+ if (reviewers.length > 0) {
79
+ await (db as any).insert(schema.prStateReviewers).values(
80
+ reviewers.map((reviewer) => ({
81
+ id: nanoid(),
82
+ stateDefinitionId: id,
83
+ userId: reviewer.userId || null,
84
+ teamId: reviewer.teamId || null,
85
+ requiredCount: reviewer.requiredCount || 1,
86
+ }))
87
+ );
88
+ }
89
+ }
90
+
91
+ const hydrated = await db.query.prStateDefinitions.findFirst({
92
+ where: eq(schema.prStateDefinitions.id, id),
93
+ with: {
94
+ reviewers: true,
95
+ },
96
+ });
97
+
98
+ return success(hydrated || updated);
47
99
});
48
100
49
101
export const DELETE: APIRoute = withErrorHandler(async ({ params, locals }: APIContext) => {
50
102
▼tests/integration/ai-review-callback-route.test.ts
+86-0
1
1
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2
+import crypto from "crypto";
2
3
3
4
const { fakeSchema } = vi.hoisted(() => ({
4
5
fakeSchema: {
73
74
return response.json();
74
75
}
75
76
77
+function buildSignedHeaders(secret: string, payload: string, timestamp: string, eventId: string) {
78
+ const signature = `sha256=${crypto
79
+ .createHmac("sha256", secret)
80
+ .update(`${timestamp}.${payload}`)
81
+ .digest("hex")}`;
82
+
83
+ return {
84
+ "content-type": "application/json",
85
+ "x-opencodehub-timestamp": timestamp,
86
+ "x-opencodehub-signature": signature,
87
+ "x-opencodehub-event-id": eventId,
88
+ };
89
+}
90
+
76
91
describe("AI review callback route", () => {
77
92
beforeEach(() => {
78
93
mockDb = makeDb();
159
174
expect(mockDb.__state.insertCalls.length).toBe(0);
160
175
expect(mockDb.__state.updateCalls.length).toBe(1);
161
176
});
177
+
178
+ it("accepts signed callback requests without bearer token", async () => {
179
+ const payload = JSON.stringify({
180
+ reviewId: "review-1",
181
+ status: "completed",
182
+ suggestions: [],
183
+ });
184
+ const timestamp = String(Math.floor(Date.now() / 1000));
185
+
186
+ const response = await callbackPost({
187
+ params: { owner: "acme", repo: "demo", number: "42" },
188
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/ai-review/callback", {
189
+ method: "POST",
190
+ headers: buildSignedHeaders("callback-secret", payload, timestamp, "evt-signed-1"),
191
+ body: payload,
192
+ }),
193
+ } as any);
194
+
195
+ expect(response.status).toBe(200);
196
+ });
197
+
198
+ it("rejects signed callback with stale timestamp", async () => {
199
+ const payload = JSON.stringify({
200
+ reviewId: "review-1",
201
+ status: "completed",
202
+ suggestions: [],
203
+ });
204
+ const staleTimestamp = String(Math.floor((Date.now() - 10 * 60 * 1000) / 1000));
205
+
206
+ const response = await callbackPost({
207
+ params: { owner: "acme", repo: "demo", number: "42" },
208
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/ai-review/callback", {
209
+ method: "POST",
210
+ headers: buildSignedHeaders("callback-secret", payload, staleTimestamp, "evt-stale-1"),
211
+ body: payload,
212
+ }),
213
+ } as any);
214
+
215
+ expect(response.status).toBe(401);
216
+ });
217
+
218
+ it("rejects duplicate signed callback event ids", async () => {
219
+ const payload = JSON.stringify({
220
+ reviewId: "review-1",
221
+ status: "completed",
222
+ suggestions: [],
223
+ });
224
+ const timestamp = String(Math.floor(Date.now() / 1000));
225
+ const headers = buildSignedHeaders("callback-secret", payload, timestamp, "evt-dup-1");
226
+
227
+ const first = await callbackPost({
228
+ params: { owner: "acme", repo: "demo", number: "42" },
229
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/ai-review/callback", {
230
+ method: "POST",
231
+ headers,
232
+ body: payload,
233
+ }),
234
+ } as any);
235
+
236
+ const second = await callbackPost({
237
+ params: { owner: "acme", repo: "demo", number: "42" },
238
+ request: new Request("http://localhost/api/repos/acme/demo/pulls/42/ai-review/callback", {
239
+ method: "POST",
240
+ headers,
241
+ body: payload,
242
+ }),
243
+ } as any);
244
+
245
+ expect(first.status).toBe(200);
246
+ expect(second.status).toBe(401);
247
+ });
162
248
});
163
249
▼tests/integration/suggestions-apply-route.test.ts
+131-0
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const {
4
+ getUserFromRequestMock,
5
+ canApplySuggestionsMock,
6
+ applySuggestionMock,
7
+ batchApplySuggestionsMock,
8
+ checkPathPermissionsMock,
9
+ fakeSchema,
10
+} = vi.hoisted(() => ({
11
+ getUserFromRequestMock: vi.fn(async () => ({ userId: "user-1" })),
12
+ canApplySuggestionsMock: vi.fn(async () => true),
13
+ applySuggestionMock: vi.fn(async () => ({ success: true, commitSha: "abc123" })),
14
+ batchApplySuggestionsMock: vi.fn(async () => ({ success: true, applied: [], failed: [] })),
15
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
16
+ fakeSchema: {
17
+ users: { username: {}, id: {} },
18
+ repositories: { ownerId: {}, name: {}, id: {} },
19
+ pullRequests: { repositoryId: {}, number: {}, id: {} },
20
+ pullRequestComments: { pullRequestId: {}, id: {} },
21
+ } as any,
22
+}));
23
+
24
+let mockDb: any;
25
+
26
+vi.mock("@/db", () => ({
27
+ getDatabase: () => mockDb,
28
+ schema: fakeSchema,
29
+}));
30
+
31
+vi.mock("@/lib/auth", () => ({
32
+ getUserFromRequest: getUserFromRequestMock,
33
+}));
34
+
35
+vi.mock("@/lib/suggestions", () => ({
36
+ canApplySuggestions: canApplySuggestionsMock,
37
+ applySuggestion: applySuggestionMock,
38
+ batchApplySuggestions: batchApplySuggestionsMock,
39
+}));
40
+
41
+vi.mock("@/lib/path-scoping", () => ({
42
+ checkPathPermissions: checkPathPermissionsMock,
43
+}));
44
+
45
+import { POST as applySuggestionsPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[pullNumber]/suggestions/apply";
46
+
47
+function makeDb() {
48
+ const owner = { id: "owner-1" };
49
+ const repository = { id: "repo-1" };
50
+ const pr = { id: "pr-1" };
51
+ const comments = [
52
+ { id: "comment-1", path: "secure/config.yml" },
53
+ { id: "comment-2", path: "src/app.ts" },
54
+ ];
55
+
56
+ return {
57
+ query: {
58
+ users: {
59
+ findFirst: vi.fn(async () => owner),
60
+ },
61
+ repositories: {
62
+ findFirst: vi.fn(async () => repository),
63
+ },
64
+ pullRequests: {
65
+ findFirst: vi.fn(async () => pr),
66
+ },
67
+ pullRequestComments: {
68
+ findMany: vi.fn(async () => comments),
69
+ },
70
+ },
71
+ };
72
+}
73
+
74
+async function readJson(response: Response): Promise<any> {
75
+ return response.json();
76
+}
77
+
78
+describe("suggestions apply route", () => {
79
+ beforeEach(() => {
80
+ mockDb = makeDb();
81
+ getUserFromRequestMock.mockResolvedValue({ userId: "user-1" });
82
+ canApplySuggestionsMock.mockResolvedValue(true);
83
+ applySuggestionMock.mockResolvedValue({ success: true, commitSha: "abc123" });
84
+ batchApplySuggestionsMock.mockResolvedValue({ success: true, applied: ["comment-1"], failed: [] });
85
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
86
+ });
87
+
88
+ it("blocks apply when path-scoped write permission is denied", async () => {
89
+ checkPathPermissionsMock.mockResolvedValue({
90
+ allowed: false,
91
+ deniedPaths: ["secure/config.yml"],
92
+ reason: "Insufficient permissions for paths: secure/config.yml",
93
+ });
94
+
95
+ const response = await applySuggestionsPost({
96
+ params: { owner: "owner-1", repo: "demo", pullNumber: "42" },
97
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/suggestions/apply", {
98
+ method: "POST",
99
+ headers: { "content-type": "application/json" },
100
+ body: JSON.stringify({ commentIds: ["comment-1"] }),
101
+ }),
102
+ } as any);
103
+
104
+ const body = await readJson(response);
105
+ expect(response.status).toBe(403);
106
+ expect(body?.error?.code).toBe("FORBIDDEN");
107
+ expect(applySuggestionMock).not.toHaveBeenCalled();
108
+ });
109
+
110
+ it("applies suggestion when permission checks pass", async () => {
111
+ const response = await applySuggestionsPost({
112
+ params: { owner: "owner-1", repo: "demo", pullNumber: "42" },
113
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/suggestions/apply", {
114
+ method: "POST",
115
+ headers: { "content-type": "application/json" },
116
+ body: JSON.stringify({ commentIds: ["comment-1"] }),
117
+ }),
118
+ } as any);
119
+
120
+ const body = await readJson(response);
121
+ expect(response.status).toBe(200);
122
+ expect(body?.success).toBe(true);
123
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith(
124
+ "user-1",
125
+ "repo-1",
126
+ ["secure/config.yml"],
127
+ "write"
128
+ );
129
+ expect(applySuggestionMock).toHaveBeenCalledWith("comment-1", "user-1");
130
+ });
131
+});
0
132