diff --git a/backend/branches.go b/backend/branches.go index 070102c..4f591de 100644 --- a/backend/branches.go +++ b/backend/branches.go @@ -27,6 +27,9 @@ type createBranchResponse struct { func (p *githubPlugin) createBranch(req *plugin.Request, res *plugin.Response) { projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } type createBranchBody struct { RepoID string `json:"repo_id"` @@ -112,6 +115,9 @@ func (p *githubPlugin) createBranch(req *plugin.Request, res *plugin.Response) { func (p *githubPlugin) linkBranchToTask(req *plugin.Request, res *plugin.Response) { projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } type linkBranchToTaskBody struct { RepoID string `json:"repo_id"` @@ -199,24 +205,48 @@ func (p *githubPlugin) linkBranchToTask(req *plugin.Request, res *plugin.Respons // ─── GET /tasks/:taskId/github/branches ─────────────────────────────────────── func (p *githubPlugin) listTaskBranches(req *plugin.Request, res *plugin.Response) { + projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } - result, err := p.db.Query(` - SELECT id, task_id, repo_id, branch_name, created_at - FROM github_task_branches WHERE task_id = $1 ORDER BY created_at ASC - `, taskID) + result, err := p.db.Query( + `SELECT id, task_id, repo_id, branch_name, created_at FROM github_task_branches WHERE task_id = $1 ORDER BY created_at ASC`, + taskID, + ) if err != nil { apiError(res, 500, "INTERNAL_ERROR", err.Error()) return } + // github_task_branches has no project_id column of its own — only + // repo_id, which links to github_repositories (which does). Re-verify + // each branch's repo against the caller's project as defense-in-depth: + // taskBelongsToProject above already closes the main vector (a foreign + // taskId), but this also protects against any row a pre-fix caller + // might have already linked across projects. No SQL JOIN here (kept + // consistent with resolvePRForTask's style elsewhere in this plugin, + // which also resolves through separate single-table queries). items := make([]taskBranchResponse, 0, len(result.Rows)) for _, row := range result.Rows { sc := newRowScanner(result.Columns, row) + repoID := sc.str("repo_id") + repoResult, rErr := p.db.Query( + `SELECT id FROM github_repositories WHERE id = $1 AND project_id = $2`, + repoID, projectID, + ) + if rErr != nil { + apiError(res, 500, "INTERNAL_ERROR", rErr.Error()) + return + } + if len(repoResult.Rows) == 0 { + continue + } items = append(items, taskBranchResponse{ ID: sc.str("id"), TaskID: sc.str("task_id"), - RepoID: sc.str("repo_id"), + RepoID: repoID, BranchName: sc.str("branch_name"), CreatedAt: sc.str("created_at"), }) diff --git a/backend/plugin.go b/backend/plugin.go index 8bd8543..0010b64 100644 --- a/backend/plugin.go +++ b/backend/plugin.go @@ -98,6 +98,30 @@ func apiError(res *plugin.Response, code int, errCode, message string) { }) } +// taskBelongsToProject verifies taskID exists, is not deleted, and belongs +// to projectID — writing a 404 and returning false otherwise. Every handler +// that accepts a :taskId path param and uses it to read or write +// project-scoped GitHub data (PRs, branches) must call this first: the +// host's route-level permission check only verifies the caller belongs to +// the project in the URL, it has no way to also verify an arbitrary path +// param like :taskId belongs to that same project — that's this plugin's +// job, the same role resolvePRForTask plays for PR-specific resources. +func (p *githubPlugin) taskBelongsToProject(taskID, projectID string, res *plugin.Response) bool { + result, err := p.db.Query( + `SELECT id FROM tasks WHERE id = $1 AND project_id = $2 AND deleted_at IS NULL`, + taskID, projectID, + ) + if err != nil { + apiError(res, 500, "INTERNAL_ERROR", err.Error()) + return false + } + if len(result.Rows) == 0 { + apiError(res, 404, "TASK_NOT_FOUND", "Task not found") + return false + } + return true +} + // ─── event handlers ────────────────────────────────────────────────────────── // handleTaskDeleted cleans up branches and PR links when a task is deleted. diff --git a/backend/plugin_test.go b/backend/plugin_test.go index 94c8cca..cb2a3cd 100644 --- a/backend/plugin_test.go +++ b/backend/plugin_test.go @@ -1,6 +1,9 @@ package main import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" "encoding/json" "testing" @@ -224,6 +227,311 @@ func TestCreateBranch_MissingRepoID(t *testing.T) { } } +// ── Cross-project task ownership guards ────────────────────────────────────── +// +// Every handler below previously trusted the URL's :taskId without +// verifying it belongs to the caller's own project (req.Caller.ProjectID) — +// only the request body's repo_id/pr_number were project-checked. A caller +// with tasks.read/write on their own project could substitute a foreign +// taskId to read or attach records to a completely different project's +// task. taskBelongsToProject now runs before any of that, so these all +// resolve to a 404 before ever reaching a GitHub API call or a DB write — +// which is also what keeps them unit-testable, since the outbound GitHub +// call itself always errors outside a real WASM build (see the comment +// above the PR review/comment tests). + +const otherTaskID = "task-2" // seeded under a project the default caller is not a member of + +func foreignTaskReq() plugintest.Request { + return plugintest.Request{ + Caller: plugin.CallerIdentity{ + ProjectID: testProjectID, // the default caller's own project + CallerID: "member-1", + CallerRole: "PROJECT_MEMBER", + }, + PathParams: map[string]string{"taskId": otherTaskID}, // but a foreign task + } +} + +// setupWithForeignTask re-seeds tasks with both the default task and a +// second task belonging to a different project. +func setupWithForeignTask(t *testing.T) *plugintest.Context { + t.Helper() + tc := setupPlugin(t) + tc.DB.SeedRows("tasks", []string{"id", "project_id", "deleted_at"}, [][]any{ + {testTaskID, testProjectID, nil}, + {otherTaskID, "other-project", nil}, + }) + return tc +} + +func TestListTaskPRs_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("GET", "/tasks/:taskId/pull-requests", foreignTaskReq()) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +func TestListTaskBranches_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("GET", "/tasks/:taskId/branches", foreignTaskReq()) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +// TestListTaskBranches_ReturnsOwnProjectBranches is the happy-path +// counterpart to the cross-project test above — it exercises the handler's +// main query past taskBelongsToProject, which a rejection test alone +// wouldn't reach. +func TestListTaskBranches_ReturnsOwnProjectBranches(t *testing.T) { + tc := setupPlugin(t) + tc.DB.SeedRows("github_repositories", + []string{"id", "project_id", "integration_id", "owner", "repo_name", "full_name", + "webhook_id", "webhook_secret_enc", "default_branch", "created_at", "updated_at"}, + [][]any{{"repo-1", testProjectID, "integration-1", "octocat", "hello-world", "octocat/hello-world", + "wh-1", "enc-secret", "main", "now", "now"}}) + tc.DB.SeedRows("github_task_branches", + []string{"id", "task_id", "repo_id", "branch_name", "created_at"}, + [][]any{{"branch-1", testTaskID, "repo-1", "feature/x", "now"}}) + + res := tc.Call("GET", "/tasks/:taskId/branches", reqWithPathParams(map[string]string{"taskId": testTaskID})) + if res.StatusCode != 200 { + t.Fatalf("expected 200, got %d: %s", res.StatusCode, res.BodyString()) + } + var env struct { + Data []taskBranchResponse `json:"data"` + } + if err := json.Unmarshal(res.Body, &env); err != nil { + t.Fatal(err) + } + if len(env.Data) != 1 || env.Data[0].BranchName != "feature/x" { + t.Fatalf("expected the seeded branch to be returned, got %+v", env.Data) + } +} + +// TestListTaskBranches_SkipsRepoFromAnotherProject mirrors +// TestListTaskPRs_SkipsLinkFromAnotherProject: github_task_branches has no +// project_id of its own, only repo_id, so a branch row pointing at a repo +// that belongs to a different project must be filtered out even when the +// task itself is the caller's own. +func TestListTaskBranches_SkipsRepoFromAnotherProject(t *testing.T) { + tc := setupPlugin(t) + tc.DB.SeedRows("github_repositories", + []string{"id", "project_id", "integration_id", "owner", "repo_name", "full_name", + "webhook_id", "webhook_secret_enc", "default_branch", "created_at", "updated_at"}, + [][]any{{"repo-foreign", "other-project", "integration-1", "octocat", "other-repo", "octocat/other-repo", + "wh-1", "enc-secret", "main", "now", "now"}}) + tc.DB.SeedRows("github_task_branches", + []string{"id", "task_id", "repo_id", "branch_name", "created_at"}, + [][]any{{"branch-1", testTaskID, "repo-foreign", "feature/x", "now"}}) + + res := tc.Call("GET", "/tasks/:taskId/branches", reqWithPathParams(map[string]string{"taskId": testTaskID})) + if res.StatusCode != 200 { + t.Fatalf("expected 200, got %d: %s", res.StatusCode, res.BodyString()) + } + var env struct { + Data []taskBranchResponse `json:"data"` + } + if err := json.Unmarshal(res.Body, &env); err != nil { + t.Fatal(err) + } + if len(env.Data) != 0 { + t.Fatalf("expected the cross-project repo's branch to be filtered out, got %+v", env.Data) + } +} + +func TestCreateBranch_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("POST", "/tasks/:taskId/branches", + foreignTaskReq().WithJSONBody(map[string]string{ + "repo_id": "00000000-0000-0000-0000-000000000001", "branch_name": "dev", + })) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +func TestLinkBranchToTask_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("POST", "/tasks/:taskId/branches/link", + foreignTaskReq().WithJSONBody(map[string]string{ + "repo_id": "00000000-0000-0000-0000-000000000001", "branch_name": "dev", + })) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +func TestCreatePullRequest_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("POST", "/tasks/:taskId/pull-requests", + foreignTaskReq().WithJSONBody(map[string]string{ + "repo_id": "00000000-0000-0000-0000-000000000001", "title": "x", "head_branch": "a", "base_branch": "main", + })) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +func TestLinkPRToTask_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + res := tc.Call("POST", "/tasks/:taskId/pull-requests/link", + foreignTaskReq().WithJSONBody(map[string]any{ + "repo_id": "00000000-0000-0000-0000-000000000001", "pr_number": 1, + })) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +func TestUnlinkPRFromTask_CrossProjectTaskRejected(t *testing.T) { + tc := setupWithForeignTask(t) + req := foreignTaskReq() + req.PathParams["prId"] = "pr-1" + res := tc.Call("DELETE", "/tasks/:taskId/pull-requests/:prId", req) + if res.StatusCode != 404 { + t.Fatalf("expected 404, got %d: %s", res.StatusCode, res.BodyString()) + } +} + +// TestListTaskPRs_SkipsLinkFromAnotherProject covers the defense-in-depth +// layer on top of taskBelongsToProject: even for the caller's own, +// legitimate task, a github_task_pr_links row pointing at a PR that +// actually belongs to a different project (e.g. one created by a pre-fix +// caller exploiting the taskId substitution above) must not be returned. +func TestListTaskPRs_SkipsLinkFromAnotherProject(t *testing.T) { + tc := setupPlugin(t) + tc.DB.SeedRows("github_pull_requests", + []string{"id", "project_id", "repo_id", "pr_number", "github_pr_id", "title", + "state", "html_url", "head_branch", "base_branch", "author", "merged_at", "created_at", "updated_at"}, + [][]any{ + {"pr-foreign", "other-project", "repo-1", 1, int64(1), "Someone else's PR", + "open", "https://example.com", "feature", "main", "octocat", nil, "now", "now"}, + }) + tc.DB.SeedRows("github_task_pr_links", + []string{"id", "task_id", "pull_request_id", "created_at"}, + [][]any{{"link-1", testTaskID, "pr-foreign", "now"}}) + + res := tc.Call("GET", "/tasks/:taskId/pull-requests", reqWithPathParams(map[string]string{"taskId": testTaskID})) + if res.StatusCode != 200 { + t.Fatalf("expected 200, got %d: %s", res.StatusCode, res.BodyString()) + } + var env struct { + Data []pullRequestResponse `json:"data"` + } + if err := json.Unmarshal(res.Body, &env); err != nil { + t.Fatal(err) + } + if len(env.Data) != 0 { + t.Fatalf("expected the cross-project-linked PR to be filtered out, got %+v", env.Data) + } +} + +// ── Webhook: project scoping + signature verification ──────────────────────── +// +// receiveWebhook always responds 204 regardless of outcome (so GitHub +// doesn't retry on an application-level rejection), so the status code +// can't distinguish "accepted" from "rejected" the way it can for the +// authenticated API routes elsewhere in this file. These tests instead +// assert on tc.Log (plugintest.CapturingLogger), using the distinct log +// line each outcome takes — an event that reaches handleWebhookEvent's +// `default:` case only gets there after signature verification succeeds, +// which is what makes "unhandled event type" a reliable positive signal. + +const testEncryptionKey = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" // 64 hex chars = 32 bytes + +func signedWebhookRequest(projectID string, payload []byte, secret string) plugintest.Request { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(payload) + sig := "sha256=" + hex.EncodeToString(mac.Sum(nil)) + return plugintest.Request{ + PathParams: map[string]string{"projectId": projectID}, + Headers: map[string]string{ + "X-Github-Event": "workflow_run", // any type receiveWebhook doesn't specifically handle + "X-Hub-Signature-256": sig, + }, + Body: payload, + } +} + +func seedEncryptedRepo(t *testing.T, tc *plugintest.Context, repoID, projectID, fullName, secret string) { + t.Helper() + tc.Config.Set("ENCRYPTION_KEY", testEncryptionKey) + encSecret, err := encryptAES(secret, testEncryptionKey) + if err != nil { + t.Fatalf("failed to encrypt test secret: %v", err) + } + tc.DB.SeedRows("github_repositories", + []string{"id", "project_id", "integration_id", "owner", "repo_name", "full_name", + "webhook_id", "webhook_secret_enc", "default_branch", "created_at", "updated_at"}, + append(tc.DB.AllRows("github_repositories"), []any{ + repoID, projectID, "integration-1", "octocat", "hello-world", fullName, + "wh-1", encSecret, "main", "now", "now", + })) +} + +// TestReceiveWebhook_ScopesRepositoryLookupToURLProject pins the fix for a +// real cross-tenant interference bug: github_repositories.full_name is only +// unique per-project, so two projects can legitimately link the same repo. +// The old lookup (`WHERE full_name = $1`, no project_id) could resolve to +// whichever row Postgres happened to return first — using a DIFFERENT +// project's secret to verify a delivery meant for this one, which then +// fails signature verification and silently drops a legitimate delivery. +// This seeds the same full_name under two different projects with two +// different secrets, and confirms a delivery signed with project-1's own +// secret, addressed to project-1's URL, still verifies successfully even +// though project-2's row for the same repo full_name exists and was seeded +// first (so a naive full_name-only lookup would find it before project-1's). +func TestReceiveWebhook_ScopesRepositoryLookupToURLProject(t *testing.T) { + tc := setupPlugin(t) + seedEncryptedRepo(t, tc, "repo-other", "other-project", "octocat/hello-world", "other-projects-secret") + seedEncryptedRepo(t, tc, "repo-mine", testProjectID, "octocat/hello-world", "my-projects-secret") + + payload := []byte(`{"repository":{"full_name":"octocat/hello-world"}}`) + res := tc.Call("POST", "/webhook", signedWebhookRequest(testProjectID, payload, "my-projects-secret")) + if res.StatusCode != 204 { + t.Fatalf("expected 204, got %d: %s", res.StatusCode, res.BodyString()) + } + if tc.Log.HasMessage("invalid webhook signature") { + t.Fatal("signature verified against the wrong project's secret — repository lookup is not scoped to the URL's project") + } + if !tc.Log.HasMessage("unhandled event type") { + t.Fatalf("expected signature verification to succeed and reach event dispatch; log entries: %+v", tc.Log.Entries()) + } +} + +// TestReceiveWebhook_RejectsMissingSecret pins the fix for a fail-open path: +// a repository row with no webhook secret configured previously skipped +// HMAC verification entirely and trusted the payload; it must now be +// refused instead, without ever reaching event dispatch. +func TestReceiveWebhook_RejectsMissingSecret(t *testing.T) { + tc := setupPlugin(t) + tc.DB.SeedRows("github_repositories", + []string{"id", "project_id", "integration_id", "owner", "repo_name", "full_name", + "webhook_id", "webhook_secret_enc", "default_branch", "created_at", "updated_at"}, + [][]any{{"repo-1", testProjectID, "integration-1", "octocat", "hello-world", "octocat/hello-world", + "wh-1", "", "main", "now", "now"}}) + + payload := []byte(`{"repository":{"full_name":"octocat/hello-world"}}`) + req := plugintest.Request{ + PathParams: map[string]string{"projectId": testProjectID}, + Headers: map[string]string{"X-Github-Event": "workflow_run", "X-Hub-Signature-256": "sha256=deadbeef"}, + Body: payload, + } + res := tc.Call("POST", "/webhook", req) + if res.StatusCode != 204 { + t.Fatalf("expected 204, got %d: %s", res.StatusCode, res.BodyString()) + } + if !tc.Log.HasMessage("no webhook secret configured") { + t.Fatalf("expected the missing-secret delivery to be refused before dispatch; log entries: %+v", tc.Log.Entries()) + } + if tc.Log.HasMessage("unhandled event type") { + t.Fatal("delivery with no configured secret reached event dispatch — HMAC verification was skipped instead of failing closed") + } +} + // ── overallCIState ───────────────────────────────────────────────────────────── func TestOverallCIState_NoChecks(t *testing.T) { diff --git a/backend/pull_requests.go b/backend/pull_requests.go index 13be30c..00b4652 100644 --- a/backend/pull_requests.go +++ b/backend/pull_requests.go @@ -30,26 +30,43 @@ type pullRequestResponse struct { // ─── GET /tasks/:taskId/github/pull-requests ────────────────────────────────── func (p *githubPlugin) listTaskPRs(req *plugin.Request, res *plugin.Response) { + projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } - result, err := p.db.Query(` - SELECT pr.id, pr.project_id, pr.repo_id, pr.pr_number, pr.github_pr_id, - pr.title, pr.state, pr.html_url, pr.head_branch, pr.base_branch, - pr.author, pr.merged_at, pr.created_at, pr.updated_at - FROM github_pull_requests pr - JOIN github_task_pr_links l ON l.pull_request_id = pr.id - WHERE l.task_id = $1 - ORDER BY l.created_at ASC - `, taskID) + linkResult, err := p.db.Query( + `SELECT pull_request_id FROM github_task_pr_links WHERE task_id = $1 ORDER BY created_at ASC`, + taskID, + ) if err != nil { apiError(res, 500, "INTERNAL_ERROR", err.Error()) return } - items := make([]pullRequestResponse, 0, len(result.Rows)) - for _, row := range result.Rows { - sc := newRowScanner(result.Columns, row) - pr := pullRequestResponse{ + // No SQL JOIN here (resolved via a separate query per link instead) — + // consistent with resolvePRForTask's style elsewhere in this plugin. + // taskBelongsToProject above already closes the main vector (a foreign + // taskId); re-verifying each linked PR's own project_id here is + // defense-in-depth against any row a pre-fix caller might have already + // linked across projects. + items := make([]pullRequestResponse, 0, len(linkResult.Rows)) + for _, linkRow := range linkResult.Rows { + prID := newRowScanner(linkResult.Columns, linkRow).str("pull_request_id") + prResult, pErr := p.db.Query( + `SELECT id, project_id, repo_id, pr_number, github_pr_id, title, state, html_url, head_branch, base_branch, author, merged_at, created_at, updated_at FROM github_pull_requests WHERE id = $1 AND project_id = $2`, + prID, projectID, + ) + if pErr != nil { + apiError(res, 500, "INTERNAL_ERROR", pErr.Error()) + return + } + if len(prResult.Rows) == 0 { + continue + } + sc := newRowScanner(prResult.Columns, prResult.Rows[0]) + items = append(items, pullRequestResponse{ ID: sc.str("id"), ProjectID: sc.str("project_id"), RepoID: sc.str("repo_id"), @@ -64,8 +81,7 @@ func (p *githubPlugin) listTaskPRs(req *plugin.Request, res *plugin.Response) { MergedAt: sc.strPtr("merged_at"), CreatedAt: sc.str("created_at"), UpdatedAt: sc.str("updated_at"), - } - items = append(items, pr) + }) } ok(res, items) } @@ -75,6 +91,9 @@ func (p *githubPlugin) listTaskPRs(req *plugin.Request, res *plugin.Response) { func (p *githubPlugin) linkPRToTask(req *plugin.Request, res *plugin.Response) { projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } type linkPRToTaskBody struct { RepoID string `json:"repo_id"` @@ -209,6 +228,9 @@ func (p *githubPlugin) linkPRToTask(req *plugin.Request, res *plugin.Response) { func (p *githubPlugin) createPullRequest(req *plugin.Request, res *plugin.Response) { projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } type createPullRequestBody struct { RepoID string `json:"repo_id"` @@ -334,8 +356,28 @@ func (p *githubPlugin) createPullRequest(req *plugin.Request, res *plugin.Respon // ─── DELETE /tasks/:taskId/github/pull-requests/:prId ──────────────────────── func (p *githubPlugin) unlinkPRFromTask(req *plugin.Request, res *plugin.Response) { + projectID := req.Caller.ProjectID taskID := req.PathParam("taskId") prID := req.PathParam("prId") + if !p.taskBelongsToProject(taskID, projectID, res) { + return + } + + // Re-verify the PR itself belongs to the caller's project (not just the + // task) before deleting the link — defense-in-depth against any link a + // pre-fix caller might have already created across projects. + prResult, err := p.db.Query( + `SELECT id FROM github_pull_requests WHERE id = $1 AND project_id = $2`, + prID, projectID, + ) + if err != nil { + apiError(res, 500, "INTERNAL_ERROR", err.Error()) + return + } + if len(prResult.Rows) == 0 { + apiError(res, 404, "GITHUB_PR_LINK_NOT_FOUND", "Pull request link not found") + return + } rowsAffected, err := p.db.Exec( `DELETE FROM github_task_pr_links WHERE task_id = $1 AND pull_request_id = $2`, diff --git a/backend/webhook.go b/backend/webhook.go index ad1d1be..39e7f8f 100644 --- a/backend/webhook.go +++ b/backend/webhook.go @@ -19,6 +19,12 @@ var branchTaskRefRe = regexp.MustCompile(`(?i)\b([A-Z][A-Z0-9]{1,19})-(\d{1,6})\ // ─── POST /webhook ──────────────────────────────────────────────────────────── func (p *githubPlugin) receiveWebhook(req *plugin.Request, res *plugin.Response) { + // projectId comes from the URL the integration registered with GitHub + // (.../projects/:projectId/webhook) — this route has no requirePermissions + // middleware (GitHub itself calls it, with no Paca auth), so + // req.Caller.ProjectID is never populated here; the path segment is the + // only source of truth for which project this delivery is for. + projectID := req.PathParam("projectId") event := req.Headers["X-Github-Event"] signature := req.Headers["X-Hub-Signature-256"] @@ -35,20 +41,27 @@ func (p *githubPlugin) receiveWebhook(req *plugin.Request, res *plugin.Response) } // Always 204 so GitHub does not retry on application errors. - if err := p.handleWebhookEvent(repoFullName, event, signature, body); err != nil { + if err := p.handleWebhookEvent(projectID, repoFullName, event, signature, body); err != nil { p.log.Error("github: webhook handler error: " + err.Error()) } res.NoContent() } -func (p *githubPlugin) handleWebhookEvent(repoFullName, event, signature string, payload []byte) error { +func (p *githubPlugin) handleWebhookEvent(projectID, repoFullName, event, signature string, payload []byte) error { p.log.Info("github: webhook received, repo=" + repoFullName + ", event=" + event) - // Look up the repository by full name. - result, err := p.db.Query(` - SELECT id, project_id, integration_id, owner, repo_name, full_name, default_branch, webhook_secret_enc - FROM github_repositories WHERE full_name = $1 - `, repoFullName) + // Look up the repository by (project_id, full_name) — full_name alone is + // only unique per-project (two projects can legitimately link the same + // repo), so scoping by the URL's own project_id avoids picking an + // arbitrary row when that happens; the previous full_name-only lookup + // could resolve to a different project's row, using its secret to + // verify a delivery meant for this project (which then just fails + // closed on the signature check) or its repo/default_branch for event + // processing. + result, err := p.db.Query( + `SELECT id, project_id, integration_id, owner, repo_name, full_name, default_branch, webhook_secret_enc FROM github_repositories WHERE full_name = $1 AND project_id = $2`, + repoFullName, projectID, + ) if err != nil { p.log.Error("github: failed to query repository: " + err.Error() + ", repo=" + repoFullName) return err @@ -59,20 +72,25 @@ func (p *githubPlugin) handleWebhookEvent(repoFullName, event, signature string, } sc := newRowScanner(result.Columns, result.Rows[0]) repoID := sc.str("id") - projectID := sc.str("project_id") + projectID = sc.str("project_id") webhookSecretEnc := sc.str("webhook_secret_enc") - // Verify HMAC signature. - if webhookSecretEnc != "" { - secret, dErr := p.decrypt(webhookSecretEnc) - if dErr != nil { - p.log.Error("github: failed to decrypt webhook secret: " + dErr.Error() + ", repo=" + repoFullName) - return dErr - } - if !verifyHMAC(payload, secret, signature) { - p.log.Info("github: invalid webhook signature, repo=" + repoFullName) - return nil // silently drop invalid signatures - } + // Verify HMAC signature. A missing secret fails closed rather than + // skipping verification — an unsigned/unverifiable delivery must never + // be trusted, even though every repo linked through the normal API + // always has a secret generated for it today. + if webhookSecretEnc == "" { + p.log.Error("github: repository has no webhook secret configured, refusing to process delivery, repo=" + repoFullName) + return nil + } + secret, dErr := p.decrypt(webhookSecretEnc) + if dErr != nil { + p.log.Error("github: failed to decrypt webhook secret: " + dErr.Error() + ", repo=" + repoFullName) + return dErr + } + if !verifyHMAC(payload, secret, signature) { + p.log.Info("github: invalid webhook signature, repo=" + repoFullName) + return nil // silently drop invalid signatures } switch event { diff --git a/plugin.json b/plugin.json index 9b27990..0dc7d6e 100644 --- a/plugin.json +++ b/plugin.json @@ -2,10 +2,18 @@ "id": "com.paca.github", "displayName": "GitHub Integration", "description": "Integrates GitHub repositories, pull requests, and branches with Paca projects and tasks.", - "version": "0.3.5", + "version": "0.4.0", "minCoreVersion": "v0.13.3", "capabilities": ["repository"], "permissions": ["db.read", "db.write"], + "customPermissions": [ + { + "key": "github.manage", + "label": "Manage GitHub integration", + "description": "View and manage this project's GitHub integration: connect or disconnect the access token, and link or unlink repositories. Does not affect PR/branch info on individual tasks, which any project member with task access can already see.", + "scope": "project" + } + ], "backend": { "allowedConfigKeys": ["ENCRYPTION_KEY", "PUBLIC_URL"], "allowedOutboundDomains": ["api.github.com"], @@ -23,7 +31,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.read"] + "permissions": ["github.manage"] } ] }, @@ -36,7 +44,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.write"] + "permissions": ["github.manage"] } ] }, @@ -49,7 +57,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.write"] + "permissions": ["github.manage"] } ] }, @@ -62,7 +70,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.read"] + "permissions": ["github.manage"] } ] }, @@ -75,7 +83,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.read"] + "permissions": ["github.manage"] } ] }, @@ -88,7 +96,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.write"] + "permissions": ["github.manage"] } ] }, @@ -101,7 +109,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.write"] + "permissions": ["github.manage"] } ] }, @@ -114,7 +122,7 @@ { "name": "requirePermissions", "scope": "project", - "permissions": ["projects.read"] + "permissions": ["github.manage"] } ] }, @@ -263,7 +271,7 @@ }, { "method": "POST", - "path": "/webhook", + "path": "/projects/:projectId/webhook", "middlewares": [{ "name": "optionalAuthn" }] } ] @@ -275,7 +283,8 @@ "point": "project.settings.tab", "component": "GitHubSettingsTab", "label": "GitHub", - "order": 100 + "order": 100, + "requiredPermission": "github.manage" }, { "point": "task.detail.section",