swadhin / opencodehub public

Enforce path permissions for PR impact scans

swadhinbiswas committed 8 months ago
8f48521
1 parent 677e3c0
commit 8f48521
3 files changed
+255 -1
Showing 3 changed files+255-1
doc/feature_audit.mdsrc/pages/api/repos/[owner]/[repo]/pulls/[number]/impact.tstests/integration/pr-impact-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, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke flows, and PR rewrite operations; broader endpoint coverage still remains |
39
+| File-level permissions | ⚠️ | Path-scoped enforcement now covers PR comment create/edit/delete, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke 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/pages/api/repos/[owner]/[repo]/pulls/[number]/impact.ts
+120-0
@@ -0,0 +1,120 @@
1
+import type { APIRoute } from "astro";
2
+import type { NodePgDatabase } from "drizzle-orm/node-postgres";
3
+import { and, eq } from "drizzle-orm";
4
+import { z } from "zod";
5
+import { getDatabase, schema } from "@/db";
6
+import { badRequest, forbidden, notFound, parseBody, success, unauthorized } from "@/lib/api";
7
+import { withErrorHandler } from "@/lib/errors";
8
+import { canReadRepo, canWriteRepo } from "@/lib/permissions";
9
+import { compareBranches } from "@/lib/git";
10
+import { resolveRepoPath } from "@/lib/git-storage";
11
+import { checkPathPermissions } from "@/lib/path-scoping";
12
+import {
13
+ analyzeImpact,
14
+ detectBreakingChanges,
15
+ detectMigrations,
16
+} from "@/lib/dependency-awareness";
17
+import { detectIaCFiles, triggerIaCHooks } from "@/lib/iac-hooks";
18
+
19
+const scanSchema = z.object({
20
+ persist: z.boolean().optional().default(true),
21
+ runIaCHooks: z.boolean().optional().default(false),
22
+ iacAction: z.enum(["plan", "apply"]).optional().default("plan"),
23
+ iacRunId: z.string().optional(),
24
+});
25
+
26
+async function resolveRepoAndPr(owner: string, repoName: string, number: number) {
27
+ const db = getDatabase() as NodePgDatabase<typeof schema>;
28
+ const repoOwner = await db.query.users.findFirst({
29
+ where: eq(schema.users.username, owner),
30
+ });
31
+ if (!repoOwner) return null;
32
+ const repository = await db.query.repositories.findFirst({
33
+ where: and(
34
+ eq(schema.repositories.ownerId, repoOwner.id),
35
+ eq(schema.repositories.name, repoName)
36
+ ),
37
+ });
38
+ if (!repository) return null;
39
+ const pr = await db.query.pullRequests.findFirst({
40
+ where: and(
41
+ eq(schema.pullRequests.repositoryId, repository.id),
42
+ eq(schema.pullRequests.number, number)
43
+ ),
44
+ });
45
+ if (!pr) return null;
46
+ return { repository, pr };
47
+}
48
+
49
+export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
50
+ const owner = params.owner;
51
+ const repoName = params.repo;
52
+ const number = Number(params.number);
53
+ if (!owner || !repoName || Number.isNaN(number)) return badRequest("Invalid route parameters");
54
+
55
+ const resolved = await resolveRepoAndPr(owner, repoName, number);
56
+ if (!resolved) return notFound("Pull request not found");
57
+
58
+ if (!(await canReadRepo(locals.user?.id, resolved.repository, { isAdmin: locals.user?.isAdmin }))) {
59
+ return notFound("Pull request not found");
60
+ }
61
+
62
+ const impact = await analyzeImpact(resolved.pr.id);
63
+ return success(impact);
64
+});
65
+
66
+export const POST: APIRoute = withErrorHandler(async ({ params, locals, request }) => {
67
+ const owner = params.owner;
68
+ const repoName = params.repo;
69
+ const number = Number(params.number);
70
+ if (!owner || !repoName || Number.isNaN(number)) return badRequest("Invalid route parameters");
71
+ if (!locals.user) return unauthorized();
72
+
73
+ const parsed = await parseBody(request, scanSchema);
74
+ if ("error" in parsed) return parsed.error;
75
+
76
+ const resolved = await resolveRepoAndPr(owner, repoName, number);
77
+ if (!resolved) return notFound("Pull request not found");
78
+ if (!(await canWriteRepo(locals.user.id, resolved.repository, { isAdmin: locals.user.isAdmin }))) {
79
+ return forbidden();
80
+ }
81
+
82
+ const repoPath = await resolveRepoPath(resolved.repository.diskPath);
83
+ const { diffs } = await compareBranches(repoPath, resolved.pr.baseBranch, resolved.pr.headBranch);
84
+ const changedFiles = diffs.map((d) => d.file).filter(Boolean);
85
+
86
+ if (changedFiles.length > 0) {
87
+ const permission = await checkPathPermissions(
88
+ locals.user.id,
89
+ resolved.repository.id,
90
+ changedFiles,
91
+ "write"
92
+ );
93
+ if (!permission.allowed) {
94
+ return forbidden(permission.reason || "Insufficient path permissions for one or more changed files");
95
+ }
96
+ }
97
+
98
+ const breaking = await detectBreakingChanges(resolved.pr.id);
99
+ const migrations = await detectMigrations(resolved.pr.id, changedFiles);
100
+ const impact = await analyzeImpact(resolved.pr.id);
101
+ const iacFiles = detectIaCFiles(changedFiles);
102
+ const iacHookResults =
103
+ parsed.data.runIaCHooks && iacFiles.length > 0
104
+ ? await triggerIaCHooks({
105
+ repositoryId: resolved.repository.id,
106
+ action: parsed.data.iacAction ?? "plan",
107
+ runId: parsed.data.iacRunId,
108
+ message: `Triggered from PR #${resolved.pr.number} impact scan`,
109
+ })
110
+ : [];
111
+
112
+ return success({
113
+ breakingDetected: breaking.length,
114
+ migrationsDetected: migrations.length,
115
+ iacFilesDetected: iacFiles.length,
116
+ iacHookRuns: iacHookResults,
117
+ impact,
118
+ persisted: parsed.data.persist,
119
+ });
120
+});
0
121
▼tests/integration/pr-impact-path-permissions.test.ts
+134-0
@@ -0,0 +1,134 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const {
4
+ canWriteRepoMock,
5
+ checkPathPermissionsMock,
6
+ compareBranchesMock,
7
+ analyzeImpactMock,
8
+ detectBreakingChangesMock,
9
+ detectMigrationsMock,
10
+ detectIaCFilesMock,
11
+ triggerIaCHooksMock,
12
+ fakeSchema,
13
+} = vi.hoisted(() => ({
14
+ canWriteRepoMock: vi.fn(async () => true),
15
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
16
+ compareBranchesMock: vi.fn(async () => ({ diffs: [{ file: "src/app.ts" }] })),
17
+ analyzeImpactMock: vi.fn(async () => ({ breakingChanges: [], migrations: [] })),
18
+ detectBreakingChangesMock: vi.fn(async () => []),
19
+ detectMigrationsMock: vi.fn(async () => []),
20
+ detectIaCFilesMock: vi.fn(() => []),
21
+ triggerIaCHooksMock: vi.fn(async () => []),
22
+ fakeSchema: {
23
+ users: { username: {} },
24
+ repositories: { ownerId: {}, name: {}, id: {} },
25
+ pullRequests: { repositoryId: {}, number: {}, id: {} },
26
+ } as any,
27
+}));
28
+
29
+let mockDb: any;
30
+
31
+vi.mock("@/db", () => ({
32
+ getDatabase: () => mockDb,
33
+ schema: fakeSchema,
34
+}));
35
+
36
+vi.mock("@/lib/permissions", () => ({
37
+ canReadRepo: vi.fn(async () => true),
38
+ canWriteRepo: canWriteRepoMock,
39
+}));
40
+
41
+vi.mock("@/lib/git-storage", () => ({
42
+ resolveRepoPath: vi.fn(async () => "/tmp/repo"),
43
+}));
44
+
45
+vi.mock("@/lib/git", () => ({
46
+ compareBranches: compareBranchesMock,
47
+}));
48
+
49
+vi.mock("@/lib/path-scoping", () => ({
50
+ checkPathPermissions: checkPathPermissionsMock,
51
+}));
52
+
53
+vi.mock("@/lib/dependency-awareness", () => ({
54
+ analyzeImpact: analyzeImpactMock,
55
+ detectBreakingChanges: detectBreakingChangesMock,
56
+ detectMigrations: detectMigrationsMock,
57
+}));
58
+
59
+vi.mock("@/lib/iac-hooks", () => ({
60
+ detectIaCFiles: detectIaCFilesMock,
61
+ triggerIaCHooks: triggerIaCHooksMock,
62
+}));
63
+
64
+import { POST as impactPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/impact";
65
+
66
+function makeDb() {
67
+ return {
68
+ query: {
69
+ users: {
70
+ findFirst: vi.fn(async () => ({ id: "owner-1" })),
71
+ },
72
+ repositories: {
73
+ findFirst: vi.fn(async () => ({ id: "repo-1", ownerId: "owner-1", name: "demo", diskPath: "/tmp/repo" })),
74
+ },
75
+ pullRequests: {
76
+ findFirst: vi.fn(async () => ({
77
+ id: "pr-1",
78
+ repositoryId: "repo-1",
79
+ number: 42,
80
+ baseBranch: "main",
81
+ headBranch: "feature",
82
+ })),
83
+ },
84
+ },
85
+ };
86
+}
87
+
88
+describe("PR impact path permissions", () => {
89
+ beforeEach(() => {
90
+ vi.clearAllMocks();
91
+ mockDb = makeDb();
92
+ canWriteRepoMock.mockResolvedValue(true);
93
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
94
+ compareBranchesMock.mockResolvedValue({ diffs: [{ file: "src/app.ts" }] });
95
+ });
96
+
97
+ it("blocks impact scan when changed files are not path-authorized", async () => {
98
+ checkPathPermissionsMock.mockResolvedValue({
99
+ allowed: false,
100
+ deniedPaths: ["src/secret.ts"],
101
+ reason: "Insufficient permissions for paths: src/secret.ts",
102
+ });
103
+ compareBranchesMock.mockResolvedValue({ diffs: [{ file: "src/secret.ts" }] });
104
+
105
+ const response = await impactPost({
106
+ params: { owner: "owner-1", repo: "demo", number: "42" },
107
+ locals: { user: { id: "user-1", isAdmin: false } },
108
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/impact", {
109
+ method: "POST",
110
+ headers: { "content-type": "application/json" },
111
+ body: JSON.stringify({}),
112
+ }),
113
+ } as any);
114
+
115
+ expect(response.status).toBe(403);
116
+ expect(detectBreakingChangesMock).not.toHaveBeenCalled();
117
+ });
118
+
119
+ it("runs impact scan when path permissions pass", async () => {
120
+ const response = await impactPost({
121
+ params: { owner: "owner-1", repo: "demo", number: "42" },
122
+ locals: { user: { id: "user-1", isAdmin: false } },
123
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/impact", {
124
+ method: "POST",
125
+ headers: { "content-type": "application/json" },
126
+ body: JSON.stringify({}),
127
+ }),
128
+ } as any);
129
+
130
+ expect(response.status).toBe(200);
131
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith("user-1", "repo-1", ["src/app.ts"], "write");
132
+ expect(detectBreakingChangesMock).toHaveBeenCalledWith("pr-1");
133
+ });
134
+});
0
135