diff --git a/docs/plans/2026-04-15-002-feat-ip-allowlist-plan.md b/docs/plans/2026-04-15-002-feat-ip-allowlist-plan.md index 679fa825b..a9e203b58 100644 --- a/docs/plans/2026-04-15-002-feat-ip-allowlist-plan.md +++ b/docs/plans/2026-04-15-002-feat-ip-allowlist-plan.md @@ -11,7 +11,7 @@ deepened: 2026-04-18 ## Overview -Add configurable IP address allowlisting to the fabro server so users without network-level controls (VPN, firewall) can restrict access by client IP. The allowlist is configured via `[server.ip_allowlist]` in settings.toml, validated at startup (fail-closed), and enforced as request middleware after tracing but before auth. To preserve TOML compatibility for an upcoming webhook endpoint, the config shape also reserves an optional `[server.ip_allowlist.incoming_webhooks]` override plus a reserved `github_meta_hooks` entry keyword for GitHub-origin webhook traffic. +Add configurable IP address allowlisting to the fabro server so users without network-level controls (VPN, firewall) can restrict access by client IP. The allowlist is configured via `[server.ip_allowlist]` in settings.toml, validated at startup (fail-closed), and enforced as request middleware after tracing but before auth. To preserve TOML compatibility for an upcoming GitHub webhook endpoint, the config shape also reserves an optional GitHub-specific webhook overlay at `[server.integrations.github.webhooks.ip_allowlist]` plus a reserved `github_meta_hooks` entry keyword for GitHub-origin webhook traffic. ## Problem Frame @@ -31,10 +31,11 @@ Some fabro users deploy on infrastructure without firewalls or VPNs. They need f - R10. Log rejected requests at warn level with client IP and path - R11. Unix socket + allowlist requires trusted_proxy_count > 0 or server refuses to start - R12. Invalid entries (malformed CIDR) cause startup failure (fail-closed) -- R13. The TOML schema must support a webhook-specific IP allowlist override without breaking existing `[server.ip_allowlist]` configs +- R13. The TOML schema must support provider-specific webhook IP allowlist overrides in integration-owned namespaces without breaking existing `[server.ip_allowlist]` configs - R14. Webhook-specific entries may include the reserved keyword `github_meta_hooks`, which resolves to GitHub's current webhook delivery ranges from `GET /meta` -- R15. `github_meta_hooks` is only valid in `[server.ip_allowlist.incoming_webhooks].entries`; using it in the default scope is a config error -- R16. `[server.ip_allowlist.incoming_webhooks].trusted_proxy_count` inherits from `[server.ip_allowlist].trusted_proxy_count` unless explicitly overridden +- R15. `github_meta_hooks` is only valid in `[server.integrations.github.webhooks.ip_allowlist].entries`; using it outside the GitHub webhook allowlist is a config error +- R16. `[server.integrations.github.webhooks.ip_allowlist].trusted_proxy_count` inherits from `[server.ip_allowlist].trusted_proxy_count` unless explicitly overridden +- R17. GitHub webhook IP allowlist config falls back to `[server.ip_allowlist]` when the GitHub-specific block or one of its fields is omitted, so webhook routes inherit the server allowlist by default ## Scope Boundaries @@ -43,7 +44,7 @@ Some fabro users deploy on infrastructure without firewalls or VPNs. They need f - No blocklist (deny-list) - No hot-reload of IP rules - No implicit loopback exemption -- This plan does not add the webhook endpoint itself; it only reserves the config and resolution shape needed so webhook-specific enforcement can be wired later without a TOML break +- This plan does not add the webhook endpoint itself; it only reserves the config and resolution shape needed so GitHub webhook-specific enforcement can be wired later without a TOML break - `/health/diagnostics` is NOT exempt (requires auth, probes server internals) ## Context & Research @@ -51,6 +52,7 @@ Some fabro users deploy on infrastructure without firewalls or VPNs. They need f ### Relevant Code and Patterns - **Config layer/resolved pattern**: `ServerLayer` (sparse, `deny_unknown_fields`) -> `resolve_server()` -> `ServerSettings` (fully populated) in `lib/crates/fabro-types/src/settings/server.rs` and `lib/crates/fabro-config/src/resolve/server.rs` +- **Nested integration webhook config already exists**: GitHub webhook delivery strategy already lives under `[server.integrations.github.webhooks]`, so a provider-specific IP allowlist fits the existing integration-owned namespace - **Middleware pattern**: `middleware::from_fn_with_state` used for `cookie_and_demo_middleware` at `lib/crates/fabro-server/src/server.rs:971` - **Router structure**: `/health` registered on outer router (line 948), dispatch service_fn routes to demo/real sub-routers (line 914) - **Serve paths**: Three listener branches in `lib/crates/fabro-server/src/serve.rs:492` — Unix, TCP+TLS, TCP plain. None currently use `ConnectInfo` @@ -73,22 +75,23 @@ Some fabro users deploy on infrastructure without firewalls or VPNs. They need f - **Generic 403 response body**: Use `"Access denied."` (matching existing `ApiError::forbidden()`) rather than a message that reveals the filtering mechanism - **TLS ConnectInfo must use Axum newtype**: The TLS accept loop must inject `axum::extract::ConnectInfo` (the wrapper type), not a bare `SocketAddr`, or the middleware extractor will silently fail - **`ipnet` as direct dependency**: Present in Cargo.lock as a transitive dependency (v2.11.0); added as a direct dependency to fabro-types, fabro-config, and fabro-server for CIDR parsing and matching -- **`[server.ip_allowlist]` is the default scope, with an optional `[server.ip_allowlist.incoming_webhooks]` override**: Both scopes expose `entries` and `trusted_proxy_count`, but the root table remains the default policy for all normal server traffic. If `incoming_webhooks` is absent, future webhook requests fall back to the default scope. If `incoming_webhooks` is present but omits `trusted_proxy_count`, it inherits the default scope's value +- **`[server.ip_allowlist]` remains the global default scope, and GitHub webhook overrides live under `[server.integrations.github.webhooks.ip_allowlist]`**: This matches the existing integration-owned webhook config shape and avoids inventing a parallel top-level webhook namespace +- **The GitHub webhook allowlist is an overlay on the global allowlist, not a separate root policy**: If `[server.integrations.github.webhooks.ip_allowlist]` is absent, GitHub webhook requests use `[server.ip_allowlist]`. If the GitHub block is present but omits `entries` or `trusted_proxy_count`, the omitted field inherits from `[server.ip_allowlist]` - **`entries` resolves to a typed enum, not raw strings**: User-provided strings parse into `IpAllowEntry`, with `Literal(IpNet)` for direct IP/CIDR values and `GitHubMetaHooks` for the reserved keyword `github_meta_hooks` -- **`github_meta_hooks` is webhook-only**: The reserved keyword is accepted only in `[server.ip_allowlist.incoming_webhooks].entries`, where it means "expand to GitHub's current `/meta` `hooks` ranges". Using it in the default scope is invalid config +- **`github_meta_hooks` is GitHub-webhook-only**: The reserved keyword is accepted only in `[server.integrations.github.webhooks.ip_allowlist].entries`, where it means "expand to GitHub's current `/meta` `hooks` ranges". Using it elsewhere is invalid config - **GitHub meta expansion should use startup-time fetch plus cache-aware reuse**: Resolve `github_meta_hooks` at startup, using `GET /meta` and the `hooks` array. Reuse cached data on `304 Not Modified`; if GitHub is unavailable and there is no usable cache, fail startup - **Middleware after TraceLayer, before cookie middleware**: Rejected requests still get trace spans (including 403 status), but no cookie/demo processing for blocked IPs - **Health exemption inside middleware**: The middleware checks the request path for `/health` rather than selectively applying to route sets, keeping router structure simple - **IPv4-mapped IPv6 normalization**: Convert `::ffff:x.x.x.x` to IPv4 equivalent before matching, so `10.0.0.0/8` matches regardless of how the OS reports the address -- **Land the schema before the webhook route if convenient**: The default-scope middleware can ship first. The `incoming_webhooks` override and `github_meta_hooks` resolution model should land now so the later webhook feature can wire into it without a TOML migration +- **Land the schema before the webhook route if convenient**: The default-scope middleware can ship first. The GitHub-specific webhook overlay and `github_meta_hooks` resolution model should land now so the later webhook feature can wire into it without a TOML migration ## Open Questions ### Resolved During Planning - **Which Axum ecosystem tools for IP detection?** Custom implementation. `axum-client-ip` doesn't support trusted-proxy-count. `ConnectInfo` for TCP remote address, manual X-Forwarded-For parsing for proxy support -- **Config shape?** `[server.ip_allowlist]` is the default scope. `[server.ip_allowlist.incoming_webhooks]` is an optional child override for the future webhook route. Both scopes carry `entries` and `trusted_proxy_count`, but `incoming_webhooks.trusted_proxy_count` inherits from the default scope when omitted, which fits the existing subdomain pattern and `deny_unknown_fields` constraint -- **How should GitHub webhook origins be represented?** As the reserved keyword `github_meta_hooks` inside `[server.ip_allowlist.incoming_webhooks].entries`, resolved into concrete IP nets from GitHub `GET /meta` +- **Config shape?** `[server.ip_allowlist]` is the global default scope. `[server.integrations.github.webhooks.ip_allowlist]` is an optional GitHub-specific overlay, which fits the existing integration-owned webhook namespace and `deny_unknown_fields` constraint +- **How should GitHub webhook origins be represented?** As the reserved keyword `github_meta_hooks` inside `[server.integrations.github.webhooks.ip_allowlist].entries`, resolved into concrete IP nets from GitHub `GET /meta` - **Which health endpoints to exempt?** Only `/health`. Not `/health/diagnostics` (requires auth, exposes server internals) - **IPv4-mapped IPv6 handling?** Normalize to IPv4 before matching - **X-Forwarded-For position?** Rightmost-minus-N where N = `trusted_proxy_count`. If N=0, use TCP remote address (ConnectInfo) @@ -109,7 +112,16 @@ Representative TOML: entries = ["10.0.0.0/8"] trusted_proxy_count = 1 - [server.ip_allowlist.incoming_webhooks] + [server.integrations.github] + strategy = "app" + app_id = "123456" + client_id = "Iv1.abc123" + slug = "fabro-app" + + [server.integrations.github.webhooks] + strategy = "tailscale_funnel" + + [server.integrations.github.webhooks.ip_allowlist] entries = ["github_meta_hooks"] # trusted_proxy_count inherits as 1 unless overridden here @@ -119,8 +131,9 @@ Request flow with IP allowlist: -> [TraceLayer] -> trace span created -> [IP Allowlist] -> select active scope - current routes: default `[server.ip_allowlist]` - - future webhook route: `[server.ip_allowlist.incoming_webhooks]` - or default fallback when absent + - future GitHub webhook route: + `[server.integrations.github.webhooks.ip_allowlist]` + overlaid on `[server.ip_allowlist]` if path == "/health": pass through else: extract client IP (ConnectInfo or X-Forwarded-For rightmost-minus-N) @@ -133,13 +146,18 @@ Request flow with IP allowlist: Startup validation: settings.toml -> parse [server.ip_allowlist] + -> parse optional [server.integrations.github.webhooks.ip_allowlist] -> parse each entry string into IpAllowEntry - IP/CIDR -> Literal(IpNet) - `github_meta_hooks` -> GitHubMetaHooks - -> reject `github_meta_hooks` in the default scope - -> when resolving a webhook scope containing `GitHubMetaHooks`, + -> reject `github_meta_hooks` outside the GitHub webhook allowlist + -> when resolving a GitHub webhook scope containing `GitHubMetaHooks`, call `GET /meta`, read `hooks`, parse into IpNet list - -> if Unix socket && active scope has entries && trusted_proxy_count == 0: error + -> for GitHub webhook requests, build the effective scope by overlaying + `[server.integrations.github.webhooks.ip_allowlist]` on + `[server.ip_allowlist]` + -> if Unix socket && active scope has entries && trusted_proxy_count == 0: + error -> resolve to IpAllowlistConfig (Arc, read-once) -> pass to build_router_with_options ``` @@ -148,9 +166,9 @@ Startup validation: - [ ] **Unit 1: Config types and resolution** -**Goal:** Define the settings.toml schema for `[server.ip_allowlist]` plus the future webhook override, and wire up typed entry parsing/validation. +**Goal:** Define the settings.toml schema for `[server.ip_allowlist]` plus the GitHub webhook overlay, and wire up typed entry parsing/validation. -**Requirements:** R4, R5, R8, R11, R12, R13, R14, R15, R16 +**Requirements:** R4, R5, R8, R11, R12, R13, R14, R15, R16, R17 **Dependencies:** None @@ -162,33 +180,40 @@ Startup validation: - Test: `lib/crates/fabro-config/src/resolve/server.rs` (inline tests) **Approach:** -- Add `ServerIpAllowlistScopeLayer` with `entries: Option>` and `trusted_proxy_count: Option`. Reuse that shape for the root scope and the nested `incoming_webhooks` child table -- Add `ServerIpAllowlistLayer` with root-scope fields plus `incoming_webhooks: Option`. Apply `#[serde(deny_unknown_fields)]` +- Add `ServerIpAllowlistLayer` for the root `[server.ip_allowlist]` table with `entries: Option>` and `trusted_proxy_count: Option`. Apply `#[serde(deny_unknown_fields)]` +- Add `ServerIpAllowlistOverrideLayer` with the same fields for provider-local webhook overlays - Add `ip_allowlist: Option` to `ServerLayer` +- Add `ip_allowlist: Option` to `IntegrationWebhooksLayer` so GitHub webhook config can live at `[server.integrations.github.webhooks.ip_allowlist]` - Add `IpAllowEntry` enum with `Literal(IpNet)` and `GitHubMetaHooks` -- Add `ServerIpAllowlistScopeSettings` with `entries: Vec` and `trusted_proxy_count: u32` -- Add `ServerIpAllowlistSettings` with `default_scope: ServerIpAllowlistScopeSettings` and `incoming_webhooks: Option` +- Add `ServerIpAllowlistSettings` with `entries: Vec` and `trusted_proxy_count: u32` for the global scope +- Add `ServerIpAllowlistOverrideSettings` with `entries: Option>` and `trusted_proxy_count: Option` for GitHub webhook overlay settings - Default when absent: empty `entries` vec, `trusted_proxy_count: 0` -- Add `resolve_ip_allowlist_scope()` to parse each entry string into `IpAllowEntry`, pushing `ResolveError` for invalid entries (fail-closed) -- Add `resolve_ip_allowlist()` to build the default scope plus optional webhook override, reject `github_meta_hooks` when it appears outside `incoming_webhooks.entries`, and resolve `incoming_webhooks.trusted_proxy_count` by inheriting `default_scope.trusted_proxy_count` when the child field is unset -- Add `ip_allowlist` field to `ServerSettings`, call resolver from `resolve_server()` -- Perform cross-domain validation in `resolve_server()` after both `resolve_listen()` and `resolve_ip_allowlist()` return: check Unix socket + active scope with non-empty entries + resolved `trusted_proxy_count == 0` → push error. The webhook override follows the same rule once that scope is actually wired into a route +- Add `resolve_ip_allowlist()` to parse the global scope into `ServerIpAllowlistSettings` +- Add `resolve_ip_allowlist_override()` to parse provider-local overlays into `ServerIpAllowlistOverrideSettings`, preserving omitted fields as `None` +- Extend `resolve_integrations()` to parse `server.integrations.github.webhooks.ip_allowlist` into the resolved GitHub webhook settings +- Reject `github_meta_hooks` outside the GitHub webhook allowlist path +- Add `ip_allowlist` field to `ServerSettings`, and extend the resolved integration webhook settings to carry the optional overlay +- Perform cross-domain validation in `resolve_server()` after `resolve_listen()`, `resolve_ip_allowlist()`, and `resolve_integrations()` return: compute the effective GitHub webhook scope by overlaying the GitHub webhook config on the global allowlist, then check Unix socket + effective scope with non-empty entries + resolved `trusted_proxy_count == 0` → push error **Patterns to follow:** - `ServerAuthLayer` / `ServerAuthSettings` in same file for layer/resolved struct pattern +- `IntegrationWebhooksLayer` / `IntegrationWebhooksSettings` in the same file for nested integration-owned config - `resolve_auth()` in `lib/crates/fabro-config/src/resolve/server.rs` for validation with `ResolveError` **Test scenarios:** - Happy path: valid IPv4, IPv6, and CIDR entries parse correctly into `IpAllowEntry::Literal` - Happy path: absent `[server.ip_allowlist]` section resolves to empty entries (R2) - Happy path: `trusted_proxy_count` defaults to 0 when not specified -- Happy path: `[server.ip_allowlist.incoming_webhooks]` resolves independently from the default scope -- Happy path: `incoming_webhooks.trusted_proxy_count` inherits the default scope value when omitted -- Happy path: `incoming_webhooks.trusted_proxy_count` overrides the default scope value when explicitly set -- Happy path: `github_meta_hooks` is accepted in `incoming_webhooks.entries` +- Happy path: `[server.integrations.github.webhooks.ip_allowlist]` resolves as an optional overlay independent from the global scope +- Happy path: GitHub webhook entries fall back to the global scope when the GitHub block is absent +- Happy path: GitHub webhook entries fall back to the global scope when the GitHub block omits `entries` +- Happy path: `server.integrations.github.webhooks.ip_allowlist.trusted_proxy_count` inherits the global value when omitted +- Happy path: `server.integrations.github.webhooks.ip_allowlist.trusted_proxy_count` overrides the global value when explicitly set +- Happy path: `github_meta_hooks` is accepted in `server.integrations.github.webhooks.ip_allowlist.entries` - Error path: malformed CIDR string (e.g., `10.0.0.0/33`) produces ResolveError - Error path: unparseable address (e.g., `not-an-ip`) produces ResolveError - Error path: `github_meta_hooks` in the default scope produces ResolveError +- Error path: `github_meta_hooks` outside the GitHub webhook allowlist path produces ResolveError - Error path: Unix socket listener + non-empty allowlist + `trusted_proxy_count: 0` produces ResolveError - Edge case: empty `entries` list (present but empty) resolves to empty vec (equivalent to no filtering) @@ -197,11 +222,11 @@ Startup validation: - `cargo nextest run -p fabro-types` passes - Invalid config entries cause resolution errors, not panics -- [ ] **Unit 2: Entry resolution, IP matching, and client IP extraction** +- [ ] **Unit 2: Effective scope resolution, entry expansion, IP matching, and client IP extraction** -**Goal:** Create the core IP matching logic, dynamic entry expansion, and client IP extraction (from ConnectInfo or proxy headers). +**Goal:** Create the core IP matching logic, effective scope overlay logic, dynamic entry expansion, and client IP extraction (from ConnectInfo or proxy headers). -**Requirements:** R4, R5, R6, R7, R14 +**Requirements:** R4, R5, R6, R7, R14, R17 **Dependencies:** Unit 1 (needs `ServerIpAllowlistSettings`) @@ -214,7 +239,10 @@ Startup validation: **Approach:** - `IpAllowlist` struct wrapping `Vec` with a `contains(&IpAddr) -> bool` method -- Add a resolution step that turns `Vec` into concrete `Vec` for a chosen scope: +- Add a helper that computes an effective scope from the global allowlist plus an optional provider-specific overlay: + - `entries = override.entries.unwrap_or(global.entries.clone())` + - `trusted_proxy_count = override.trusted_proxy_count.unwrap_or(global.trusted_proxy_count)` +- Add a resolution step that turns the effective scope's `Vec` into concrete `Vec` for a chosen route: - `Literal(IpNet)` passes through directly - `GitHubMetaHooks` fetches GitHub `GET /meta`, reads the `hooks` array, and parses each item as `IpNet` - Use cache-aware startup fetch for `GitHubMetaHooks`: send `If-None-Match` when cache exists, reuse cached ranges on `304`, and fail startup when the source cannot be resolved and no usable cache exists @@ -223,7 +251,7 @@ Startup validation: - If `trusted_proxy_count > 0`: parse `X-Forwarded-For` header, split by comma, take the entry at zero-indexed position `len - 1 - trusted_proxy_count` (skipping N trusted proxy entries from the right to reach the client IP). If `X-Forwarded-For` is absent or has fewer entries than `trusted_proxy_count + 1`, return `None` (fail-closed) - If `trusted_proxy_count == 0`: extract `ConnectInfo` from request extensions, return `addr.ip()` - `IpAllowlist::is_empty()` method to efficiently skip checking when no allowlist is configured (R2) -- `IpAllowlistConfig` struct containing `IpAllowlist` + `trusted_proxy_count: u32`, constructed from a chosen `ServerIpAllowlistScopeSettings`. This is the type used as middleware state (`Arc`) in Units 3, 4, and the later webhook wiring step +- `IpAllowlistConfig` struct containing `IpAllowlist` + `trusted_proxy_count: u32`, constructed from the effective scope for a chosen route. This is the type used as middleware state (`Arc`) in Units 3, 4, and the later GitHub webhook wiring step **Patterns to follow:** - `lib/crates/fabro-server/src/jwt_auth.rs` for module structure and inline test organization @@ -232,6 +260,10 @@ Startup validation: - Happy path: IPv4 address matches an individual IPv4 entry - Happy path: IPv4 address matches a CIDR range (e.g., `10.1.2.3` matches `10.0.0.0/8`) - Happy path: IPv6 address matches an IPv6 CIDR range +- Happy path: effective GitHub webhook scope falls back to global entries when the GitHub overlay is absent +- Happy path: effective GitHub webhook scope falls back to global entries when the GitHub overlay omits `entries` +- Happy path: effective GitHub webhook scope inherits the global `trusted_proxy_count` when the GitHub overlay omits it +- Happy path: effective GitHub webhook scope overrides `trusted_proxy_count` when the GitHub overlay sets it - Happy path: `GitHubMetaHooks` expands to the `hooks` ranges from a mocked `/meta` response - Happy path: cached GitHub metadata is reused on `304 Not Modified` - Happy path: `extract_client_ip` with `trusted_proxy_count: 0` returns ConnectInfo IP @@ -314,7 +346,7 @@ Startup validation: - In `tls.rs` accept loop (line 78): inject `axum::extract::ConnectInfo` (the Axum newtype wrapper, NOT a bare `SocketAddr`) as a request extension before calling the router. If the wrong type is inserted, `extract_client_ip` will silently fail and all requests will be rejected - In `serve.rs` Unix socket path (line 498): no ConnectInfo change needed — the startup validation (Unit 1) ensures `trusted_proxy_count > 0` when using Unix socket with an allowlist, so the middleware will use proxy headers - IP allowlist config is NOT added to `AppState` or `shared_settings` — it is captured by the middleware closure at construction time, ensuring R9 (no hot-reload) -- Do not wire `incoming_webhooks` here yet unless the webhook route already exists in the same change; the root middleware should continue to represent the default scope for current routes +- Do not wire `server.integrations.github.webhooks.ip_allowlist` here yet unless the GitHub webhook route already exists in the same change; the root middleware should continue to represent the global default scope for current routes **Patterns to follow:** - `auth_mode` resolution at `serve.rs:315` for resolve-once-at-startup pattern @@ -337,51 +369,50 @@ Startup validation: - [ ] **Unit 5: Webhook scope wiring** -**Goal:** When the webhook endpoint is added, route those requests through the webhook-specific override without changing the TOML schema. +**Goal:** When the GitHub webhook endpoint is added, route those requests through the GitHub-specific overlay without changing the TOML schema. -**Requirements:** R1, R7, R9, R13, R14, R15, R16 +**Requirements:** R1, R7, R9, R13, R14, R15, R16, R17 **Dependencies:** Unit 1 (typed config), Unit 2 (dynamic entry resolution), webhook endpoint implementation **Files:** - Modify: `lib/crates/fabro-server/src/server.rs` -- Modify: webhook handler/router files added by the webhook feature -- Test: webhook integration tests alongside the webhook endpoint +- Modify: `lib/crates/fabro-server/src/github_webhooks.rs` and/or webhook handler/router files added by the webhook feature +- Test: GitHub webhook integration tests alongside the webhook endpoint **Approach:** - Select the active allowlist scope by route: - - normal routes use `resolved_server_settings.ip_allowlist.default_scope` - - webhook routes use `resolved_server_settings.ip_allowlist.incoming_webhooks` when present, otherwise fall back to `default_scope` -- Resolve `incoming_webhooks.trusted_proxy_count` during config resolution, not at request time, so the webhook middleware sees a fully populated inherited-or-overridden value + - normal routes use `resolved_server_settings.ip_allowlist` + - GitHub webhook routes compute an effective scope by overlaying `resolved_server_settings.integrations.github.webhooks.ip_allowlist` on `resolved_server_settings.ip_allowlist` - Reuse the same middleware and `IpAllowlistConfig` shape; only the chosen scope changes -- If the webhook scope contains `GitHubMetaHooks`, resolve it at startup the same way as any other typed entry before constructing the webhook middleware state -- Keep the webhook route's HMAC verification; IP allowlisting is additive defense, not a replacement +- If the GitHub webhook scope contains `GitHubMetaHooks`, resolve it at startup the same way as any other typed entry before constructing the webhook middleware state +- Keep the GitHub webhook route's HMAC verification; IP allowlisting is additive defense, not a replacement **Patterns to follow:** - Reuse the default-scope middleware wiring rather than introducing a second bespoke path - Keep route selection explicit near the router definition so reviewers can see which requests use which scope **Test scenarios:** -- Happy path: webhook request allowed by `incoming_webhooks` but not by the default scope still succeeds -- Happy path: webhook request falls back to the default scope when `incoming_webhooks` is absent -- Happy path: webhook request uses the inherited default `trusted_proxy_count` when the child scope omits it -- Happy path: webhook request uses the child `trusted_proxy_count` when explicitly configured -- Happy path: `github_meta_hooks` allows a webhook request from a GitHub hook range -- Error path: webhook request from a non-allowlisted IP returns 403 before webhook auth/HMAC handling -- Integration: non-webhook routes continue using the default scope even when `incoming_webhooks` is configured +- Happy path: GitHub webhook request uses the global allowlist when `server.integrations.github.webhooks.ip_allowlist` is absent +- Happy path: GitHub webhook request uses global entries when the GitHub overlay omits `entries` +- Happy path: GitHub webhook request uses the inherited global `trusted_proxy_count` when the GitHub overlay omits it +- Happy path: GitHub webhook request uses the overlay `trusted_proxy_count` when explicitly configured +- Happy path: `github_meta_hooks` allows a GitHub webhook request from a GitHub hook range +- Error path: GitHub webhook request from a non-allowlisted IP returns 403 before webhook auth/HMAC handling +- Integration: non-webhook routes continue using the global scope even when GitHub webhook IP allowlist config is present **Verification:** -- Webhook integration tests pass once the endpoint exists +- GitHub webhook integration tests pass once the endpoint exists - Existing non-webhook allowlist tests still pass unchanged ## System-Wide Impact -- **Interaction graph:** The default middleware sits between TraceLayer and cookie/demo middleware. It reads `ConnectInfo` extensions (set by `into_make_service_with_connect_info` or manually in TLS path). The webhook-specific override reuses the same machinery once the webhook route exists +- **Interaction graph:** The default middleware sits between TraceLayer and cookie/demo middleware. It reads `ConnectInfo` extensions (set by `into_make_service_with_connect_info` or manually in TLS path). The GitHub webhook-specific overlay reuses the same machinery once the GitHub webhook route exists - **Error propagation:** 403 responses from the middleware short-circuit the request pipeline. They appear in TraceLayer output as normal 403 responses. The middleware also emits its own `warn!` log - **State lifecycle risks:** None — the allowlist is immutable after startup (Arc, no hot-reload). No shared mutable state -- **API surface parity:** The default scope applies uniformly to all current routes (except `/health`). A later webhook route can opt into the reserved override without changing the config surface +- **API surface parity:** The global scope applies uniformly to all current routes (except `/health`). A later GitHub webhook route can opt into the reserved provider-local overlay without changing the global config surface - **Integration coverage:** Key scenario: full request through TCP listener → ConnectInfo injection → IP filter → handler. This must be tested via `oneshot()` with manually injected `ConnectInfo` extensions -- **Unchanged invariants:** Auth middleware (extractors), demo mode dispatch, SSE streaming, and existing API routes remain unchanged. The webhook endpoint is still added separately; this plan only reserves and later wires the scope it will use +- **Unchanged invariants:** Auth middleware (extractors), demo mode dispatch, SSE streaming, and existing API routes remain unchanged. The GitHub webhook endpoint is still added separately; this plan only reserves and later wires the scope it will use ## Risks & Dependencies @@ -392,7 +423,7 @@ Startup validation: | IPv4-mapped IPv6 normalization bugs | Comprehensive test coverage in Unit 2 with explicit mapped-address test cases | | Existing tests may break if they rely on the serve signature | Tests use `oneshot()` against the router, not `axum::serve`, so they are unaffected | | `github_meta_hooks` makes startup depend on an external service | Use GitHub's ETag-capable `/meta` endpoint with cached reuse; fail closed only when the keyword is configured and no usable data can be resolved | -| A future webhook-specific schema could force a TOML migration | Reserve `[server.ip_allowlist.incoming_webhooks]` now so the later webhook feature only consumes existing config rather than renaming it | +| A future GitHub webhook-specific schema could force a TOML migration | Reserve `[server.integrations.github.webhooks.ip_allowlist]` now so the later GitHub webhook feature only consumes existing config rather than renaming it | ## Sources & References diff --git a/docs/plans/2026-04-18-001-feat-webhook-strategy-plan.md b/docs/plans/2026-04-18-001-feat-webhook-strategy-plan.md new file mode 100644 index 000000000..4e74a5a30 --- /dev/null +++ b/docs/plans/2026-04-18-001-feat-webhook-strategy-plan.md @@ -0,0 +1,75 @@ +# Feat: gate GitHub webhook exposure behind explicit strategy + +Today the Rust server silently spawns a separate webhook listener on a random loopback port, shells out to `tailscale funnel`, and mutates the GitHub App's webhook URL whenever `integrations.github.strategy = "app"` and `integrations.github.webhooks` is present. Side effects are host-wide and externally visible. Require explicit opt-in, move the handler onto the main API router at `POST /api/v1/webhooks/github`, and add a `server_url` strategy for operators with a stable public URL. + +## 1. Settings (`fabro-types/src/settings/server.rs`) + +- Extend `WebhookStrategy` enum: + - `TailscaleFunnel` (existing, serde `tailscale_funnel`) + - `ServerUrl` (new, serde `server_url`) +- `IntegrationWebhooksSettings` shape unchanged: `strategy: Option`. + +## 2. Config resolve (`fabro-config/src/resolve/server.rs`) + +- Thread new variant through layer merge. +- Validation: `ServerUrl` requires `server.api.url` to be set. Missing → resolve error at startup. + +## 3. OpenAPI + main router + +- `docs/api-reference/fabro-api.yaml`: add `POST /api/v1/webhooks/github`. + - Request body: `application/json`, raw (opaque bytes — verified by HMAC before parse). + - Responses: `200` OK, `401` unauthorized. + - No auth security scheme (signature-verified). +- `cargo build -p fabro-api` regenerates Rust types + client. +- `server.rs::build_router_with_options`: mount route outside the auth middleware layer. All `AuthMode` values still serve the endpoint; HMAC is the only gate. +- Handler: reject missing/invalid `X-Hub-Signature-256` with 401; on success parse metadata, log event, 200. +- Webhook secret plumbed into `AppState` from `server_secrets["GITHUB_APP_WEBHOOK_SECRET"]`. Route is mounted iff secret is present. + +## 4. Rename + slim `github_webhooks.rs` + +- Rename `WebhookManager` → `TailscaleFunnelManager` (single-purpose now). +- Delete `spawn_webhook_listener`, `WebhookListener`, standalone router — handler lives on main router. +- `TailscaleFunnelManager::start(main_server_port, app_id, private_key_pem)`: + - `tailscale funnel ` + - Parse funnel URL from `tailscale funnel status` + - Best-effort `PATCH https://api.github.com/app/hook/config` with `{funnel_url}/api/v1/webhooks/github` + - Log but don't fail on GitHub API errors +- `shutdown()`: `tailscale funnel off `. +- Keep `verify_signature` and `parse_event_metadata` as `pub(crate)` helpers consumed by the new main-router handler. + +## 5. `serve.rs` gating (`fabro-server/src/serve.rs:373-422`) + +Match on `resolved_server_settings.integrations.github.webhooks.strategy`: + +- `None` → no-op. (Route still mounted if secret present, but no funnel, no GitHub App mutation.) +- `Some(TailscaleFunnel)` → `TailscaleFunnelManager::start(main_port, app_id, private_key_pem)`. Keep current best-effort error handling. +- `Some(ServerUrl)` → best-effort `update_github_app_webhook({server.api.url}/api/v1/webhooks/github)`. Log failure, don't fail startup. No funnel, no manager. + +Requires wiring the main server's bound port into this block. + +## 6. Tests + +- Router test (new, in `server.rs` or adjacent module): + - Valid HMAC → 200 + - Missing `X-Hub-Signature-256` → 401 + - Invalid signature → 401 + - Works regardless of `AuthMode` (bearer header absent, wrong, and correct all behave identically for this route) +- Delete `spawn_listener_serves_route` in `github_webhooks.rs` tests. +- Settings resolve test: `strategy = "server_url"` without `server.api.url` → error. +- Existing OpenAPI conformance test picks up spec addition automatically. + +## 7. Docs + +- `docs/integrations/github.mdx`: document both strategies, side effects (funnel, App URL mutation), recommended choice per environment. +- `docs/administration/server-configuration.mdx`: reference `[server.integrations.github.webhooks] strategy` field. +- `docs/changelog/2026-04-18.mdx`: **breaking** — funnel no longer auto-enables on App strategy with webhooks config present. Operators must set `strategy = "tailscale_funnel"` explicitly to restore prior behavior. New `server_url` strategy for production deployments with a stable public URL. + +## 8. Migration + +- None automated. Existing users implicitly relying on auto-funnel must add `strategy = "tailscale_funnel"` to `[server.integrations.github.webhooks]`. Call out in changelog. + +## Unresolved questions + +1. Is `AppState` the right home for the webhook secret, or should it be injected into the handler via router state layering? +2. Should `ServerUrl`'s GitHub App URL update run on every `serve.rs` start, or only when the current App URL differs? (Rate-limit/noise concern.) +3. Where does the webhook route sit relative to `web_enabled` gating — always on, or only when `integrations.github.enabled`?