fix(server): auth-cover extra_routes + reserved-path rule (SRV-01, SRV-06)
- apply bearer_auth_middleware route_layer AFTER the extra_routes merge, so assembly-layer custom routes resolve the bearer token by default (ADR-046 §4); per-route opt-out via the route's own layer remains - enforce RESERVED_PATHS per-method at build time: a probe MethodRouter occupied on all methods is pre-merged against extras, so a custom POST /search panics like a same-method overlap (ADR-046 §3) - tests: auth resolves through an extra route; an extra route with its own layer opts out; reserved-path merge panics; non-reserved different-method merge stays legal; MCP bearer-gate test stays green - ADR-046 §3: one sentence restating the per-method rejection rule Verification: cargo test (215) ok, cargo test --all-features (260 + integration) ok, clippy -D warnings (default + all-features) ok, cargo fmt --check ok.
This commit is contained in:
@@ -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
|
collision rule.) If a custom route collides with a reserved path, the
|
||||||
default surface wins — the custom route is silently shadowed (or the
|
default surface wins — the custom route is silently shadowed (or the
|
||||||
construction panics/warns; the specific collision-handling is a
|
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
|
`/v1/chat/completions` namespaces it away from the reserved set, which
|
||||||
is natural (`/v1/...` doesn't collide).
|
is natural (`/v1/...` doesn't collide).
|
||||||
|
|
||||||
|
|||||||
+188
-5
@@ -7,6 +7,19 @@
|
|||||||
//! bidirectional stream yielded by `Connection::accept_bi()`. The WS
|
//! bidirectional stream yielded by `Connection::accept_bi()`. The WS
|
||||||
//! upgrade route lands with the websocket subsystem; until then the
|
//! upgrade route lands with the websocket subsystem; until then the
|
||||||
//! router reserves `/alk/channels` for it.
|
//! 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;
|
use std::sync::Arc;
|
||||||
|
|
||||||
@@ -34,7 +47,11 @@ pub const ALPN_H2: &[u8] = b"h2";
|
|||||||
pub const WS_UPGRADE_PATH: &str = "/alk/channels";
|
pub const WS_UPGRADE_PATH: &str = "/alk/channels";
|
||||||
|
|
||||||
/// Reserved default-surface paths (ADR-046 collision rule). Custom
|
/// 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] = &[
|
pub const RESERVED_PATHS: &[&str] = &[
|
||||||
"/search",
|
"/search",
|
||||||
"/schema",
|
"/schema",
|
||||||
@@ -167,24 +184,60 @@ fn build_router(state: RouterState, extra_routes: Option<Router>) -> Router {
|
|||||||
crate::websocket::ws_bearer_auth,
|
crate::websocket::ws_bearer_auth,
|
||||||
)),
|
)),
|
||||||
)
|
)
|
||||||
.route_layer(from_fn_with_state(
|
|
||||||
auth_state.clone(),
|
|
||||||
bearer_auth_middleware,
|
|
||||||
))
|
|
||||||
.fallback(decoy_fallback)
|
.fallback(decoy_fallback)
|
||||||
.merge(mcp_router);
|
.merge(mcp_router);
|
||||||
|
|
||||||
let with_extras = match extra_routes {
|
let with_extras = match extra_routes {
|
||||||
Some(extra) => {
|
Some(extra) => {
|
||||||
|
enforce_reserved_paths(&extra);
|
||||||
let extra: Router<RouterState> = extra.with_state(());
|
let extra: Router<RouterState> = extra.with_state(());
|
||||||
default.merge(extra)
|
default.merge(extra)
|
||||||
}
|
}
|
||||||
None => default,
|
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)
|
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;
|
use axum::middleware::from_fn_with_state;
|
||||||
|
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
@@ -258,6 +311,7 @@ async fn openapi_json_handler(
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
use crate::server::auth::ResolvedIdentity;
|
||||||
use alkcall::core::auth::IdentityProvider;
|
use alkcall::core::auth::IdentityProvider;
|
||||||
use alkcall::core::types::ProtocolHandler;
|
use alkcall::core::types::ProtocolHandler;
|
||||||
use tokio::io::{AsyncReadExt, AsyncWriteExt};
|
use tokio::io::{AsyncReadExt, AsyncWriteExt};
|
||||||
@@ -521,4 +575,133 @@ mod tests {
|
|||||||
|
|
||||||
let _ = server_task.await;
|
let _ = server_task.await;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
struct StaticProvider;
|
||||||
|
impl IdentityProvider for StaticProvider {
|
||||||
|
fn resolve_from_fingerprint(&self, _: &str) -> Option<alkcall::core::auth::Identity> {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
fn resolve_from_token(
|
||||||
|
&self,
|
||||||
|
_: &alkcall::core::auth::AuthToken,
|
||||||
|
) -> Option<alkcall::core::auth::Identity> {
|
||||||
|
Some(alkcall::core::auth::Identity {
|
||||||
|
id: "worker-a".to_string(),
|
||||||
|
scopes: vec![],
|
||||||
|
resources: std::collections::HashMap::new(),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn static_provider() -> Arc<dyn IdentityProvider> {
|
||||||
|
Arc::new(StaticProvider)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn router_state(idp: Arc<dyn IdentityProvider>) -> 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<axum::body::Body> {
|
||||||
|
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());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
---
|
---
|
||||||
id: review-001-extra-routes-auth
|
id: review-001-extra-routes-auth
|
||||||
name: Mount extra_routes under the bearer-auth middleware (SRV-01)
|
name: Mount extra_routes under the bearer-auth middleware (SRV-01)
|
||||||
status: pending
|
status: completed
|
||||||
depends_on: []
|
depends_on: []
|
||||||
scope: narrow
|
scope: narrow
|
||||||
risk: medium
|
risk: medium
|
||||||
@@ -32,11 +32,11 @@ has no reader.
|
|||||||
|
|
||||||
## Acceptance Criteria
|
## Acceptance Criteria
|
||||||
|
|
||||||
- [ ] A test mounts an extra route and asserts `ResolvedIdentity` is resolved from the bearer token (auth applies)
|
- [x] 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)
|
- [x] 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
|
- [x] 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
|
- [x] ADR-046 §4 language matches the implemented default after the fix
|
||||||
- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
||||||
|
|
||||||
## References
|
## References
|
||||||
|
|
||||||
@@ -45,8 +45,51 @@ has no reader.
|
|||||||
|
|
||||||
## Notes
|
## 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
|
## Summary
|
||||||
|
|
||||||
> Filled on completion.
|
`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.
|
||||||
Reference in New Issue
Block a user