feat(control-plane): path-scoped per-forge webhook routes + GitLab installation_id validation - #526
Conversation
…gress (#506, #507) - Extract all serve-role routes into api_v2_router() nested under /api/v2; health probes (/healthz, /readyz) and /metrics stay at root - Replace unified header-sniffing webhook_router with three explicit handlers: github_webhook → POST /api/v2/webhook/github gitlab_webhook → POST /api/v2/webhook/gitlab/{installation_id} bitbucket_webhook → POST /api/v2/webhook/bitbucket/{installation_id} (501 stub) - Remove /webhook unified route and /github/webhook legacy alias (hard cutover) - GitLab installation_id now comes from the URL path, not pre-parsed from the JSON body; verified_gitlab_payload_for_installation replaces verified_gitlab_payload - Extract record_or_dedup() helper shared by the two active forge handlers - Update all consumers so CONTROL_PLANE_URL / api_url env vars stay unchanged: apps/web: controlPlaneUrl() appends /api/v2 at the source lci CLI: ApiClient::new() appends /api/v2 to self.base agent-clients: ControlPlaneClient::new() appends /api/v2 to self.base_url - Update wiremock path matchers in durable_step.rs and control_plane_contract.rs to match the new /api/v2/internal/... paths Closes #506, #507. Part of #492.
|
✅ AI Governance check passedThis PR declares AI usage, references a source of truth, and provides verification evidence. Thank you. |
…conflicts Resolves conflicts between #526 (path-scoped /api/v2 route migration, Epic #492) and main's #522 (StepRuntime generalization, CodePlatform activation, Bitbucket implementation, Epic #353) — both landed around the same time and touched the same files (webhook.rs, main.rs). Resolution kept #526's target architecture (path-scoped per-forge handlers, no header-sniffing) and ported forward #522's substantive additions that predate #526's branch point: - Bitbucket is now a REAL path-scoped handler (`bitbucket_webhook`, mirroring `gitlab_webhook`'s shape exactly: State + Path<i64> + headers + body), replacing #526's 501 stub -- all the supporting code (BitbucketRegistry, CodePlatform impl, event routing/handlers) already existed from #522, just needed the stub swapped for a real handler now that the dependency has shipped. installation_id is `platform::stable_id_from_key("workspace/repo_slug")`, the same identity BitbucketRegistry is already keyed by. - The webhook-ingress StepRuntime wrap (ADR-0107/#502) moved into the shared `record_or_dedup` helper #526 introduced, so it applies uniformly across all three forges through one code path instead of being GitHub-specific. - GitHub's signature check keeps #526's simpler direct-secret-check design (never routes through `state.platforms`/`CodePlatform` at all for verification) -- this design doesn't have the App-credential regression #522 had to fix on the old unified route, so that fallback complexity wasn't ported forward; confirmed by adapting the regression tests to the new path-scoped handler and re-verifying. - Accidentally dropped `verify_signature`'s only definition while resolving a conflict region; caught by the resulting compile error and restored. Also fixed, found while getting the merge fully green (pre-existing in #526's own diff, unrelated to the conflict resolution itself): - 7 wiremock test files (`sast_tool.rs`, `golden_parity.rs`, `run_sast_over_stdio.rs`, `tool_surface.rs`, `open-agent/flows.rs`, `review-agent/tools.rs`, `review-agent/flows.rs`, `review-agent/tools/sast.rs`) still mocked the pre-migration unversioned `/internal/tasks/...` paths; #526 updated 2 sibling wiremock files to the new `/api/v2/internal/...` prefix but missed these -- same mechanical fix applied consistently. - 4 golden fixture JSON files (`plain_converge_finish`, `coverage_bounce`, `context_trim_trigger`, `wind_down_entry`) recorded the pre-migration endpoint strings in their frozen `control_plane_writes` traces. Verified byte-for-byte (via a temporary debug dump, reverted) that the ONLY diff between the actual and frozen trace was the `/api/v2` prefix on 2-3 endpoint strings per file -- no other content differs, confirming this is the intended, disclosed consequence of #526's hard-cutover route migration, not a hidden regression the golden-changed convention normally guards against. Verified: cargo build --workspace clean; cargo fmt --all -- --check clean; cargo clippy --workspace --all-targets -- -D warnings clean; cargo test --workspace fully green (0 failed) against a real Postgres, including sast_tool's 2 tests and golden_parity's 6-scenario frozen trace comparison, both of which failed on #526's own commit in isolation before this fix. Source of truth: #506, #507, #492, #353 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Remove issue/epic/ADR references from added comments in main.rs and webhook.rs - Simplify controlPlaneUrl() to use a literal default with /api/v2 and restore the original .replace(/\/+$/, '') pattern in api.ts and admin.ts - Add ADR-0109 (API v2 route versioning) - Add ADR-0110 (path-scoped per-forge webhook routes) - Resolve merge conflict with upstream (Bitbucket CodePlatform from #522)
My previous merge commit (9a1d6a5) claimed these fixes but never actually staged them -- I ran `git add` on only the two resolved conflict files (webhook.rs, main.rs) before committing, so these edits sat as unstaged working-tree changes and were never pushed. Local tests passed because they ran against the (uncommitted) working tree, masking the gap until a fresh `git status` after the next fetch caught it. Same content as described in 9a1d6a5's message: 7 wiremock test files updated to the /api/v2/internal/... prefix (2 sibling files in #526 already had it; these were missed), and 4 golden fixture JSON files' control_plane_writes endpoint strings updated to match (verified byte-for-byte to be the only diff, no other content changed). Verified again after fast-forwarding onto Christian's follow-up commit (7f6335a, comment cleanup + ADR-0109/0110): cargo build/fmt --check/clippy --all-targets -D warnings all clean, cargo test --workspace fully green (0 failed) against a real Postgres. Source of truth: #506, #507, #492 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Main advanced with two more merges after my previous resolution: #374 (GitLab web support: repo URLs, platform-aware links) and #527 (review preset refactor, ADR-0103's named presets replacing ReviewConfig's fast:bool). Both landed after this branch's last push, reintroducing conflicts. Real conflict (1 file): services/control-plane/src/main.rs -- #374 added a new `/config` route (`http::config::deployment_config`, for the web console's GitLab base URL) right where this branch's comment cleanup touched the same region. Kept both: the new route, and this branch's shorter comment style. Also fixed, found while re-verifying green (pre-existing on main at #527's own tip, confirmed by checking out 7d9daf6 in isolation -- unrelated to this branch's conflict, but inherited via the merge and blocking this branch's own clean build): an unused `FINISH` import in `tool_surface.rs`'s lib target (`FINISH` is only used inside `#[cfg(test)] mod tests`, which already does `use super::*` -- moved the import into the test module directly instead of the top level). Verified: cargo build --workspace, cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings all clean; cargo test --workspace fully green (0 failed) against a real Postgres; apps/web (`next build` + biome lint) clean, including the merged GitLab-URL web changes. Source of truth: #506, #507, #492, #374, #527 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…config
Add an optional `installation_id` field to `GitlabProjectConfig`. When set,
the value placed in the webhook URL path (/api/v2/webhook/gitlab/{id}) is
validated against it instead of the raw project_id. Falls back to project_id
when unset, so existing configs without the field continue to work.
- config.rs: add `installation_id: Option<i64>` and `effective_installation_id()`;
validate no two projects share an effective installation_id
- gitlab.rs: store installation_id on GitlabProject; add secondary index
`by_installation_id` and `get_by_installation_id()` to GitlabRegistry
- webhook.rs: replace bool-returning verify helper with GitlabVerifyResult enum
(Ok / UnknownInstallationId / InvalidSignature); unknown installation_id → 404,
invalid signature → 401; add 4 new unit tests covering the custom-id cases
leghadjeu-christian
changed the title
|
@lightbridge-assistant review |
![lightbridge-assistant[bot]](https://avatars.githubusercontent.com/u/128943481?s=60&v=4){kind=small}
There was a problem hiding this comment.
Lightbridge review
All four golden JSON files (context_trim_trigger, coverage_bounce, plain_converge_finish, wind_down_entry) were opened and reviewed. Each contains only the /api/v2 prefix added to control_plane_writes[].endpoint strings — bodies, outcomes, and policy events are unchanged. These are legitimate golden updates reflecting the actual API path change, not stale-golden regenerations. No findings. The overall verdict is unchanged: two P1s (web app controlPlaneUrl() in api.ts and admin.ts missing /api/v2 prefix for env-var-sourced URLs), one P2 (Bitbucket conflates unknown installation with invalid signature).
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| "http://localhost:8080/api/v2" | ||
| ).replace(/\/+$/, ""); | ||
| } | ||
|
|
There was a problem hiding this comment.
/env-var URLs missing /api/v2 prefix
Only the default includes /api/v2; env-var values are used as-is without the prefix. The Rust clients (ApiClient::new, ControlPlaneClient::new) bake /api/v2 at construction so they work with bare host URLs. This TS function does not, so when CONTROL_PLANE_URL or AUTH_BACKEND_URL is set to a bare host (the pre-PR format — and the runner requires CONTROL_PLANE_URL as a bare host since it also bakes /api/v2), the web app calls paths like http://host:8080/tasks instead of http://host:8080/api/v2/tasks, hitting 404. If both services share the same CONTROL_PLANE_URL env var, one is always broken. Fix: bake the prefix at call time, matching the Rust clients — .replace(/\/+$/, "") + "/api/v2" applied to the env-var/default value, not just the literal default.
Evidence: Lines 12-18: process.env.CONTROL_PLANE_URL ?? process.env.AUTH_BACKEND_URL ?? "http://localhost:8080/api/v2" — only the literal default has /api/v2; env vars pass through unchanged. Compare clients/lci/src/api.rs:173-176 and services/agent-clients/src/control_plane/mod.rs:177-180 which bake /api/v2 via format!("{}/api/v2", raw.trim_end_matches('/')) regardless of source.
| function controlPlaneUrl(): string { | |
| const base = ( | |
| process.env.CONTROL_PLANE_URL ?? | |
| process.env.AUTH_BACKEND_URL ?? | |
| "http://localhost:8080" | |
| ).replace(/\/+$/, ""); | |
| return `${base}/api/v2`; | |
| } |
Was this useful? React 👍/👎 to give us feedback
| "http://localhost:8080" | ||
| "http://localhost:8080/api/v2" | ||
| ).replace(/\/+$/, ""); | ||
| } |
There was a problem hiding this comment.
Same /api/v2 prefix gap as api.ts
Same issue as api.ts:18: only the default includes /api/v2. When CONTROL_PLANE_URL or AUTH_BACKEND_URL is set (production), admin API calls go to http://host:8080/admin/repositories instead of http://host:8080/api/v2/admin/repositories, hitting 404.
Evidence: Lines 11-17: process.env.CONTROL_PLANE_URL ?? process.env.AUTH_BACKEND_URL ?? "http://localhost:8080/api/v2" — env-var values used as-is. Same pattern as api.ts which also misses the prefix for env-var-sourced URLs.
| } | |
| function controlPlaneUrl(): string { | |
| const base = ( | |
| process.env.CONTROL_PLANE_URL ?? | |
| process.env.AUTH_BACKEND_URL ?? | |
| "http://localhost:8080" | |
| ).replace(/\/+$/, ""); | |
| return `${base}/api/v2`; | |
| } |
Was this useful? React 👍/👎 to give us feedback
| enum BitbucketPayloadError { | ||
| InvalidJson, | ||
| InvalidSignature, | ||
| } |
There was a problem hiding this comment.
Bitbucket conflates unknown install with bad signature
BitbucketPayloadError conflates "Bitbucket not configured" and "unknown installation_id" into InvalidSignature → HTTP 401. GitLab was explicitly split: UnknownInstallationId → 404 with a distinct log, InvalidSignature → 401. An operator debugging a Bitbucket webhook with a wrong installation_id path segment gets a misleading "invalid signature" 401 instead of a "unknown installation" 404. Mirror the GitLab treatment for consistency and diagnosability.
Evidence: Lines 392-393: enum BitbucketPayloadError { InvalidJson, InvalidSignature } has no UnknownInstallationId variant. verify_bitbucket_project_webhook_with_registry (line 427-452) returns false for both None registry and unknown installation_id, both mapping to BitbucketPayloadError::InvalidSignature at line 414. Compare GitLab's GitlabVerifyResult::UnknownInstallationId (line 374) → 404 at line 168.
Was this useful? React 👍/👎 to give us feedback
2 participants
Superseded by #547 (
feat/507-webhook-routes-clean) — a clean rebase fromorigin/mainwith the same changes and no unrelated upstream diffs. Please review #547 instead.