From b61f69fd6e1caf0274934cceded07074fdee0dbc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Apr 2026 10:49:53 -0700 Subject: [PATCH] fix(server): tighten routing middleware behavior Keep demo dispatch scoped to API requests, add no-store defaults for install responses, and update the server test sandbox mock for typed sandbox errors. --- lib/crates/fabro-server/src/auth/translate.rs | 29 ++++++++++++ lib/crates/fabro-server/src/install.rs | 10 ++++ lib/crates/fabro-server/src/run_files.rs | 34 ++++++-------- lib/crates/fabro-server/src/server.rs | 47 +++++++++---------- 4 files changed, 77 insertions(+), 43 deletions(-) diff --git a/lib/crates/fabro-server/src/auth/translate.rs b/lib/crates/fabro-server/src/auth/translate.rs index 4a480ee22..2f86a27ed 100644 --- a/lib/crates/fabro-server/src/auth/translate.rs +++ b/lib/crates/fabro-server/src/auth/translate.rs @@ -614,6 +614,35 @@ methods = ["dev-token"] ); } + #[tokio::test] + async fn full_router_does_not_demo_dispatch_auth_routes() { + let state = test_state(); + let app = server::build_router_with_options( + state, + &auth_mode(), + Arc::new(crate::ip_allowlist::IpAllowlistConfig::default()), + RouterOptions::default(), + ); + + let response = app + .oneshot( + Request::builder() + .method("GET") + .uri("/auth/login/github") + .header(header::COOKIE, "fabro-demo=1") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_ne!( + response.status(), + StatusCode::NOT_FOUND, + "demo cookie must not steal /auth/* routes from the real router", + ); + } + #[tokio::test] async fn full_router_accepts_dev_token_bearer_when_web_is_disabled() { let state = test_state(); diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index edf3c9e0a..bbdb47577 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -582,6 +582,16 @@ pub fn build_install_router(state: InstallAppState) -> Router { } })) .layer(middleware::from_fn(security_headers::layer)) + .layer(middleware::from_fn(no_store_default)) +} + +async fn no_store_default(req: Request, next: middleware::Next) -> Response { + let mut response = next.run(req).await; + response + .headers_mut() + .entry(header::CACHE_CONTROL) + .or_insert(header::HeaderValue::from_static("no-store")); + response } struct InstallFinishGuard { diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index f664a70bb..162807361 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -1840,7 +1840,7 @@ rename to docs/NOTES.md // ── fetch_blob_table two-phase error isolation ───────────────────── use async_trait::async_trait; - use fabro_sandbox::ExecResult; + use fabro_sandbox::{Error as SandboxError, ExecResult, Result as SandboxResult}; /// Scripted sandbox for the two-phase tests — serves different /// `exec_command` responses for `cat-file --batch-check` vs @@ -1860,15 +1860,15 @@ rename to docs/NOTES.md _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> std::result::Result { + ) -> SandboxResult { if command.contains("cat-file --batch-check") { Ok(self.batch_check_result.clone()) } else if command.contains("cat-file --batch") { Ok(self.batch_result.clone()) } else { - Err(format!( + Err(SandboxError::message(format!( "unexpected command in ScriptedBlobSandbox: {command}" - )) + ))) } } @@ -1879,23 +1879,23 @@ rename to docs/NOTES.md _path: &str, _offset: Option, _limit: Option, - ) -> std::result::Result { + ) -> SandboxResult { unimplemented!() } - async fn write_file(&self, _: &str, _: &str) -> std::result::Result<(), String> { + async fn write_file(&self, _: &str, _: &str) -> SandboxResult<()> { unimplemented!() } - async fn delete_file(&self, _: &str) -> std::result::Result<(), String> { + async fn delete_file(&self, _: &str) -> SandboxResult<()> { unimplemented!() } - async fn file_exists(&self, _: &str) -> std::result::Result { + async fn file_exists(&self, _: &str) -> SandboxResult { unimplemented!() } async fn list_directory( &self, _path: &str, _depth: Option, - ) -> std::result::Result, String> { + ) -> SandboxResult> { unimplemented!() } async fn grep( @@ -1903,34 +1903,30 @@ rename to docs/NOTES.md _pattern: &str, _path: &str, _options: &fabro_sandbox::GrepOptions, - ) -> std::result::Result, String> { + ) -> SandboxResult> { unimplemented!() } - async fn glob( - &self, - _pattern: &str, - _path: Option<&str>, - ) -> std::result::Result, String> { + async fn glob(&self, _pattern: &str, _path: Option<&str>) -> SandboxResult> { unimplemented!() } async fn download_file_to_local( &self, _remote: &str, _local: &std::path::Path, - ) -> std::result::Result<(), String> { + ) -> SandboxResult<()> { unimplemented!() } async fn upload_file_from_local( &self, _local: &std::path::Path, _remote: &str, - ) -> std::result::Result<(), String> { + ) -> SandboxResult<()> { unimplemented!() } - async fn initialize(&self) -> std::result::Result<(), String> { + async fn initialize(&self) -> SandboxResult<()> { Ok(()) } - async fn cleanup(&self) -> std::result::Result<(), String> { + async fn cleanup(&self) -> SandboxResult<()> { Ok(()) } fn working_directory(&self) -> &'static str { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 1f10f03fb..84541b97a 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -108,7 +108,6 @@ use tokio::time::{sleep, timeout}; use tokio_stream::StreamExt; use tokio_stream::wrappers::{BroadcastStream, UnboundedReceiverStream}; use tower::{ServiceExt, service_fn}; -use tower_http::trace::TraceLayer; use tracing::{Instrument, debug, error, info, warn}; use ulid::Ulid; @@ -1013,7 +1012,10 @@ pub fn build_router_with_options( let demo = demo_router.clone(); let real = real_router.clone(); async move { - if web_enabled && req.headers().get("x-fabro-demo").is_some_and(|v| v == "1") { + let demo_active = web_enabled + && req.uri().path().starts_with("/api/") + && req.headers().get("x-fabro-demo").is_some_and(|v| v == "1"); + if demo_active { demo.oneshot(req).await } else { real.oneshot(req).await @@ -1021,27 +1023,6 @@ pub fn build_router_with_options( } }); - let trace_layer = TraceLayer::new_for_http() - .make_span_with(|req: &axum_extract::Request| { - let method = req.method().as_str(); - let path = req.uri().path(); - tracing::debug_span!("http_request", method, path) - }) - .on_request(|req: &axum_extract::Request, _span: &tracing::Span| { - debug!(method = %req.method(), path = %req.uri().path(), "HTTP request"); - }) - .on_response( - |response: &Response, latency: std::time::Duration, _span: &tracing::Span| { - let status = response.status().as_u16(); - let latency_ms = latency.as_millis(); - if status >= 500 { - error!(status, latency_ms, "HTTP response"); - } else { - info!(status, latency_ms, "HTTP response"); - } - }, - ); - let mut app_router = Router::new() .route("/health", get(health)) .fallback_service(service_fn(move |req: axum_extract::Request| { @@ -1099,7 +1080,25 @@ pub fn build_router_with_options( canonical_host::redirect_middleware, )) .layer(middleware::from_fn(security_headers::layer)) - .layer(trace_layer) + .layer(middleware::from_fn(http_log_middleware)) +} + +async fn http_log_middleware(req: axum_extract::Request, next: axum::middleware::Next) -> Response { + let method = req.method().clone(); + let path = req.uri().path().to_string(); + let start = std::time::Instant::now(); + let response = next.run(req).await; + if path.starts_with("/assets/") { + return response; + } + let status = response.status().as_u16(); + let latency_ms = start.elapsed().as_millis(); + if status >= 500 { + error!(%method, %path, status, latency_ms, "HTTP response"); + } else { + info!(%method, %path, status, latency_ms, "HTTP response"); + } + response } fn github_webhook_routes(secret: Arc<[u8]>, ip_allowlist_config: Arc) -> Router {