swadhin / opencodehub public

Enforce path permissions for PR rewrite operations

swadhinbiswas committed 8 months ago
677e3c0
1 parent bae5051
commit 677e3c0
4 files changed
+170 -2
Showing 4 changed files+170-2
doc/feature_audit.mdsrc/lib/git-rewrite.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/rewrite.tstests/integration/pulls-rewrite-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, and partial file-approval create/revoke flows; 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, and PR rewrite operations; 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/lib/git-rewrite.ts
+25-0
@@ -87,6 +87,31 @@ export async function rewriteBranchHistory(
87
87
}
88
88
}
89
89
90
+export async function getRewriteOperationFiles(
91
+ repoOwner: string,
92
+ repoName: string,
93
+ operations: RewriteOperation[]
94
+): Promise<string[]> {
95
+ const repoPath = await acquireRepo(repoOwner, repoName);
96
+ const git = simpleGit(repoPath);
97
+
98
+ try {
99
+ const files = new Set<string>();
100
+
101
+ for (const op of operations) {
102
+ const output = await git.raw(["show", "--pretty=format:", "--name-only", op.hash]);
103
+ for (const line of output.split("\n")) {
104
+ const path = line.trim();
105
+ if (path.length > 0) files.add(path);
106
+ }
107
+ }
108
+
109
+ return [...files.values()];
110
+ } finally {
111
+ await releaseRepo(repoOwner, repoName, false);
112
+ }
113
+}
114
+
90
115
async function readCommitMessage(git: SimpleGit, rev: string): Promise<string> {
91
116
return (await git.raw(["show", "-s", "--format=%B", rev])).trim();
92
117
}
93
118
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/rewrite.ts
+24-1
@@ -3,7 +3,12 @@ import type { APIRoute } from "astro";
3
3
import { getDatabase, schema } from "@/db";
4
4
import { eq, and } from "drizzle-orm";
5
5
import { canWriteRepo } from "@/lib/permissions";
6
-import { rewriteBranchHistory, type RewriteOperation } from "@/lib/git-rewrite";
6
+import { checkPathPermissions } from "@/lib/path-scoping";
7
+import {
8
+ getRewriteOperationFiles,
9
+ rewriteBranchHistory,
10
+ type RewriteOperation
11
+} from "@/lib/git-rewrite";
7
12
8
13
export const POST: APIRoute = async ({ params, request, locals }) => {
9
14
const { owner, repo: repoName, number } = params;
@@ -52,6 +57,24 @@ export const POST: APIRoute = async ({ params, request, locals }) => {
52
57
return new Response("Invalid operations", { status: 400 });
53
58
}
54
59
60
+ const touchedFiles = await getRewriteOperationFiles(
61
+ repository.owner.username,
62
+ repository.name,
63
+ operations
64
+ );
65
+
66
+ if (touchedFiles.length > 0) {
67
+ const permission = await checkPathPermissions(
68
+ user.id,
69
+ repository.id,
70
+ touchedFiles,
71
+ "write"
72
+ );
73
+ if (!permission.allowed) {
74
+ return new Response(permission.reason || "Insufficient path permissions", { status: 403 });
75
+ }
76
+ }
77
+
55
78
// 3. Execute Rewrite
56
79
await rewriteBranchHistory(
57
80
repository.owner.username,
58
81
▼tests/integration/pulls-rewrite-path-permissions.test.ts
+120-0
@@ -0,0 +1,120 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const {
4
+ canWriteRepoMock,
5
+ checkPathPermissionsMock,
6
+ getRewriteOperationFilesMock,
7
+ rewriteBranchHistoryMock,
8
+ fakeSchema,
9
+} = vi.hoisted(() => ({
10
+ canWriteRepoMock: vi.fn(async () => true),
11
+ checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })),
12
+ getRewriteOperationFilesMock: vi.fn(async () => ["src/app.ts"]),
13
+ rewriteBranchHistoryMock: vi.fn(async () => undefined),
14
+ fakeSchema: {
15
+ repositories: { name: {}, ownerId: {}, id: {} },
16
+ users: { username: {} },
17
+ pullRequests: { repositoryId: {}, number: {} },
18
+ } as any,
19
+}));
20
+
21
+let mockDb: any;
22
+
23
+vi.mock("@/db", () => ({
24
+ getDatabase: () => mockDb,
25
+ schema: fakeSchema,
26
+}));
27
+
28
+vi.mock("@/lib/permissions", () => ({
29
+ canWriteRepo: canWriteRepoMock,
30
+}));
31
+
32
+vi.mock("@/lib/path-scoping", () => ({
33
+ checkPathPermissions: checkPathPermissionsMock,
34
+}));
35
+
36
+vi.mock("@/lib/git-rewrite", () => ({
37
+ getRewriteOperationFiles: getRewriteOperationFilesMock,
38
+ rewriteBranchHistory: rewriteBranchHistoryMock,
39
+}));
40
+
41
+import { POST as rewritePost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/rewrite";
42
+
43
+function makeDb() {
44
+ return {
45
+ query: {
46
+ users: {
47
+ findFirst: vi.fn(async () => ({ id: "owner-1" })),
48
+ },
49
+ repositories: {
50
+ findFirst: vi.fn(async () => ({
51
+ id: "repo-1",
52
+ name: "demo",
53
+ ownerId: "owner-1",
54
+ owner: { username: "owner-1" },
55
+ })),
56
+ },
57
+ pullRequests: {
58
+ findFirst: vi.fn(async () => ({
59
+ id: "pr-1",
60
+ number: 42,
61
+ repositoryId: "repo-1",
62
+ baseBranch: "main",
63
+ headBranch: "feature",
64
+ })),
65
+ },
66
+ },
67
+ };
68
+}
69
+
70
+describe("pull rewrite path permissions", () => {
71
+ beforeEach(() => {
72
+ vi.clearAllMocks();
73
+ mockDb = makeDb();
74
+ canWriteRepoMock.mockResolvedValue(true);
75
+ checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] });
76
+ getRewriteOperationFilesMock.mockResolvedValue(["src/app.ts"]);
77
+ });
78
+
79
+ it("blocks rewrite when touched paths are denied", async () => {
80
+ checkPathPermissionsMock.mockResolvedValue({
81
+ allowed: false,
82
+ deniedPaths: ["src/secret.ts"],
83
+ reason: "Insufficient permissions for paths: src/secret.ts",
84
+ });
85
+
86
+ const response = await rewritePost({
87
+ params: { owner: "owner-1", repo: "demo", number: "42" },
88
+ locals: { user: { id: "user-1" } },
89
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/rewrite", {
90
+ method: "POST",
91
+ headers: { "content-type": "application/json" },
92
+ body: JSON.stringify({
93
+ operations: [{ type: "pick", hash: "abc123" }],
94
+ }),
95
+ }),
96
+ } as any);
97
+
98
+ expect(response.status).toBe(403);
99
+ expect(rewriteBranchHistoryMock).not.toHaveBeenCalled();
100
+ });
101
+
102
+ it("executes rewrite when touched paths are allowed", async () => {
103
+ const response = await rewritePost({
104
+ params: { owner: "owner-1", repo: "demo", number: "42" },
105
+ locals: { user: { id: "user-1" } },
106
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42/rewrite", {
107
+ method: "POST",
108
+ headers: { "content-type": "application/json" },
109
+ body: JSON.stringify({
110
+ operations: [{ type: "pick", hash: "abc123" }],
111
+ }),
112
+ }),
113
+ } as any);
114
+
115
+ expect(response.status).toBe(200);
116
+ expect(getRewriteOperationFilesMock).toHaveBeenCalled();
117
+ expect(checkPathPermissionsMock).toHaveBeenCalledWith("user-1", "repo-1", ["src/app.ts"], "write");
118
+ expect(rewriteBranchHistoryMock).toHaveBeenCalled();
119
+ });
120
+});
0
121