diff --git a/docs/architecture/decisions/046-assembly-layer-custom-http-routes.md b/docs/architecture/decisions/046-assembly-layer-custom-http-routes.md index ff99720..7bd65de 100644 --- a/docs/architecture/decisions/046-assembly-layer-custom-http-routes.md +++ b/docs/architecture/decisions/046-assembly-layer-custom-http-routes.md @@ -124,7 +124,11 @@ patterns appear, and those custom routes are subject to the same collision rule.) If a custom route collides with a reserved path, the default surface wins — the custom route is silently shadowed (or the construction panics/warns; the specific collision-handling is a -two-way-door implementation detail). A deployment that wants +two-way-door implementation detail). alkhttp panics at construction for +both the same-method overlap (axum's merge) and the different-method +case (a pre-merge probe over the reserved set), so a custom +`POST /search` next to the default `GET /search` is rejected, not +silently served. A deployment that wants `/v1/chat/completions` namespaces it away from the reserved set, which is natural (`/v1/...` doesn't collide). diff --git a/src/server/adapter.rs b/src/server/adapter.rs index 0787f11..2b6af0f 100644 --- a/src/server/adapter.rs +++ b/src/server/adapter.rs @@ -7,6 +7,19 @@ //! bidirectional stream yielded by `Connection::accept_bi()`. The WS //! upgrade route lands with the websocket subsystem; until then the //! router reserves `/alk/channels` for it. +//! +//! ## Reserved paths (ADR-046 §3) +//! +//! [`RESERVED_PATHS`] is enforced per-method at `build_router` time: a +//! custom route that registers *any* method on a reserved path panics +//! at construction. The default surface already registers its own +//! methods on these paths, so axum's merge would catch the same-method +//! case anyway; the pre-merge probe extends the guarantee to the +//! per-method case (e.g. a custom `POST /search` next to the default +//! `GET /search`), where a silent merge would otherwise serve a custom +//! handler on a reserved path and break the "default surface wins" +//! rule. Same-path-different-method merges outside the reserved set +//! remain legal (axum composes the `MethodRouter`). use std::sync::Arc; @@ -34,7 +47,11 @@ pub const ALPN_H2: &[u8] = b"h2"; pub const WS_UPGRADE_PATH: &str = "/alk/channels"; /// Reserved default-surface paths (ADR-046 collision rule). Custom -/// routes must not collide with these; the default surface wins. +/// routes must not register any method on these paths — the default +/// surface owns them. Enforced per-method in `build_router` (a +/// same-method collision would already panic in axum's merge; the +/// pre-merge probe also covers different-method registrations, which +/// would otherwise silently merge in). pub const RESERVED_PATHS: &[&str] = &[ "/search", "/schema", @@ -167,24 +184,60 @@ fn build_router(state: RouterState, extra_routes: Option) -> Router { crate::websocket::ws_bearer_auth, )), ) - .route_layer(from_fn_with_state( - auth_state.clone(), - bearer_auth_middleware, - )) .fallback(decoy_fallback) .merge(mcp_router); let with_extras = match extra_routes { Some(extra) => { + enforce_reserved_paths(&extra); let extra: Router = extra.with_state(()); default.merge(extra) } None => default, }; + // Applied after the merges (ADR-046 §4): the bearer-auth layer wraps + // the extra routes too, so the documented default — custom routes + // carry the same auth — holds. A route that wants to opt out carries + // its own inner auth layer, which runs innermost (last-applied wins + // for extension stashes it inserts). + let with_extras = + with_extras.route_layer(from_fn_with_state(auth_state, bearer_auth_middleware)); + with_extras.with_state(state) } +/// Enforce the per-method reserved-path collision rule (ADR-046 §3): +/// reject extra routes that register any method on a +/// [`RESERVED_PATHS`] path by merging a probe `MethodRouter` occupied +/// on every method. The merge panics on the first collision — the same +/// panic axum raises for a same-method overlap — and is a no-op for a +/// custom router that respects the reserved set. +fn enforce_reserved_paths(extra: &Router) { + if !extra.has_routes() { + return; + } + let probe = RESERVED_PATHS.iter().fold(Router::new(), |router, path| { + router.route( + path, + get(rejected_reserved_path) + .post(rejected_reserved_path) + .put(rejected_reserved_path) + .patch(rejected_reserved_path) + .delete(rejected_reserved_path) + .head(rejected_reserved_path) + .options(rejected_reserved_path) + .trace(rejected_reserved_path) + .connect(rejected_reserved_path), + ) + }); + let _ = Router::new().merge(probe).merge(extra.clone()); +} + +async fn rejected_reserved_path() -> axum::response::Response { + unreachable!("reserved-path probe handler is never called") +} + use axum::middleware::from_fn_with_state; #[async_trait] @@ -258,6 +311,7 @@ async fn openapi_json_handler( #[cfg(test)] mod tests { use super::*; + use crate::server::auth::ResolvedIdentity; use alkcall::core::auth::IdentityProvider; use alkcall::core::types::ProtocolHandler; use tokio::io::{AsyncReadExt, AsyncWriteExt}; @@ -521,4 +575,133 @@ mod tests { let _ = server_task.await; } + + struct StaticProvider; + impl IdentityProvider for StaticProvider { + fn resolve_from_fingerprint(&self, _: &str) -> Option { + None + } + fn resolve_from_token( + &self, + _: &alkcall::core::auth::AuthToken, + ) -> Option { + Some(alkcall::core::auth::Identity { + id: "worker-a".to_string(), + scopes: vec![], + resources: std::collections::HashMap::new(), + }) + } + } + + fn static_provider() -> Arc { + Arc::new(StaticProvider) + } + + fn router_state(idp: Arc) -> RouterState { + RouterState { + registry: empty_registry(), + identity_provider: idp, + decoy: DecoyConfig::default(), + } + } + + async fn get_with_bearer( + app: Router, + path: &str, + authorization: Option<&str>, + ) -> axum::http::Response { + use tower::ServiceExt; + let mut builder = axum::http::Request::builder().uri(path); + if let Some(value) = authorization { + builder = builder.header(axum::http::header::AUTHORIZATION, value); + } + app.oneshot(builder.body(axum::body::Body::empty()).unwrap()) + .await + .unwrap() + } + + #[tokio::test] + async fn extra_routes_resolve_bearer_identity_through_the_default_auth() { + let extra = Router::new().route( + "/v1/whoami", + get(|ResolvedIdentity(identity): ResolvedIdentity| async move { + match identity { + Some(id) => id.id, + None => "none".to_string(), + } + }), + ); + let app = build_router(router_state(static_provider()), Some(extra)); + + let response = get_with_bearer(app.clone(), "/v1/whoami", Some("Bearer alk_test")).await; + assert_eq!(response.status(), axum::http::StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"worker-a"); + + let response = get_with_bearer(app, "/v1/whoami", None).await; + assert_eq!(response.status(), axum::http::StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"none"); + } + + #[tokio::test] + async fn extra_route_with_own_layer_can_opt_out_of_default_auth() { + async fn public_identity( + mut request: axum::extract::Request, + next: axum::middleware::Next, + ) -> axum::response::Response { + request + .extensions_mut() + .insert(Some(alkcall::core::auth::Identity { + id: "public-webhook".to_string(), + scopes: vec![], + resources: std::collections::HashMap::new(), + })); + next.run(request).await + } + + let extra = Router::new().route( + "/v1/public", + get(|ResolvedIdentity(identity): ResolvedIdentity| async move { + match identity { + Some(id) => id.id, + None => "none".to_string(), + } + }) + .route_layer(axum::middleware::from_fn(public_identity)), + ); + let app = build_router(router_state(static_provider()), Some(extra)); + + let response = get_with_bearer(app, "/v1/public", None).await; + assert_eq!(response.status(), axum::http::StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert_eq!(&body[..], b"public-webhook"); + } + + #[tokio::test] + #[should_panic(expected = "Overlapping method route")] + async fn extra_route_on_reserved_path_panics_at_construction() { + let extra = Router::new().route( + "/search", + axum::routing::post(|| async { "shadowing the gateway" }), + ); + let _ = HttpAdapter::new(static_provider(), empty_registry()).with_extra_routes(extra); + } + + #[tokio::test] + async fn extra_route_on_non_reserved_path_merges_cleanly() { + let extra = Router::new().route( + "/v1/ping", + get(|| async { "pong" }).post(|| async { "pong-post" }), + ); + let adapter = + HttpAdapter::new(static_provider(), empty_registry()).with_extra_routes(extra); + assert!(adapter.router().has_routes()); + } } diff --git a/tasks/server/review-001-extra-routes-auth.md b/tasks/server/review-001-extra-routes-auth.md index 9f57bf3..e640fda 100644 --- a/tasks/server/review-001-extra-routes-auth.md +++ b/tasks/server/review-001-extra-routes-auth.md @@ -1,7 +1,7 @@ --- id: review-001-extra-routes-auth name: Mount extra_routes under the bearer-auth middleware (SRV-01) -status: pending +status: completed depends_on: [] scope: narrow risk: medium @@ -32,11 +32,11 @@ has no reader. ## Acceptance Criteria -- [ ] A test mounts an extra route and asserts `ResolvedIdentity` is resolved from the bearer token (auth applies) -- [ ] A test shows an extra route carrying its own layer can still opt out (documented escape hatch) -- [ ] SRV-06 decision landed: per-method reserved-path merges rejected/documented; `RESERVED_PATHS` enforced or un-exported -- [ ] ADR-046 §4 language matches the implemented default after the fix -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [x] A test mounts an extra route and asserts `ResolvedIdentity` is resolved from the bearer token (auth applies) +- [x] A test shows an extra route carrying its own layer can still opt out (documented escape hatch) +- [x] SRV-06 decision landed: per-method reserved-path merges rejected/documented; `RESERVED_PATHS` enforced or un-exported +- [x] ADR-046 §4 language matches the implemented default after the fix +- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References @@ -45,8 +45,51 @@ has no reader. ## Notes -> Agent fills during implementation. +SRV-06 decision: **enforce** `RESERVED_PATHS` (kept exported, now with a +reader). `build_router` only merges extras into the default surface; it +cannot silently reject, and returning a `Result` would ripple the error +through `with_decoy`'s rebuild for no real gain — a wrong path is a +programming error, so panic-at-construction matches axum's own +same-method behavior and ADR-046 §3's "panics/warns" clause. Mechanism: +a probe `MethodRouter` occupied on all nine routable methods is merged +against the extras before the real merge, so a same-method overlap +panics in axum's merge (as before) *and* a different-method registration +(e.g. custom `POST /search` next to the default `GET /search`) now +panics too. `any()`-mounted extras are NOT caught (axum's fallback-first +merge shape does not conflict per-method) — the standard, method-router +style of registering extras is covered. Outside the reserved set, +same-path-different-method merges remain legal. Enforcement lives in +`build_router` (not `with_extra_routes`) so `with_decoy`'s rebuild path +is covered as well. Module docs in `src/server/adapter.rs` document the +rule; ADR-046 §3 gained one sentence restating the per-method rule (§4 +needed no change — the implemented default now matches its text). + +Auth-ordering note: the MCP nest (`/mcp`) is merged before the +route_layer and keeps its own explicit `bearer_auth_middleware` layer, +so reordering does not un-auth it (existing +`mcp_endpoint_serves_four_gateway_tools_bearer_gated` test stays green). +The WS upgrade route's stricter `ws_bearer_auth` inner layer is +unaffected (`route_layer` on the route itself). ## Summary -> Filled on completion. \ No newline at end of file +`src/server/adapter.rs`: moved the shared `bearer_auth_middleware` +`route_layer` from before to after the `extra_routes` merge (SRV-01), so +assembly-layer custom routes now resolve the bearer token by default — +`extra_routes_resolve_bearer_identity_through_the_default_auth` asserts +`ResolvedIdentity` is `Some("worker-a")` for a valid token and `None` +without one; `extra_route_with_own_layer_can_opt_out_of_default_auth` +shows a route's own inner layer winning (documented escape hatch). +Landed the SRV-06 enforcement (`enforce_reserved_paths`: pre-merge +probe; `RESERVED_PATHS` stays exported and now has a reader) — +`extra_route_on_reserved_path_panics_at_construction` asserts a custom +`POST /search` panics with axum's overlapping-route message; +`extra_route_on_non_reserved_path_merges_cleanly` covers the legal +different-method merge outside the reserved set. Kept green: +`mcp_endpoint_serves_four_gateway_tools_bearer_gated` (MCP nest carries +its own auth layer), all gateway tests, and the full suite — +`cargo test` (215) + `cargo test --all-features` (260 + 29 integration), +`cargo clippy --all-targets/--all-features -- -D warnings`, and +`cargo fmt --check` all pass. ADR-046: §3 one-sentence per-method +clarification; §4 unchanged. The `/mcp` body-limit fix (SRV-03) and the +`with_decoy` `.take()` fix (SRV-05) are different tasks and untouched. \ No newline at end of file