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.
This commit is contained in:
Bryan Helmkamp 2026-04-27 10:49:53 -07:00
parent bac0d4a601
commit b61f69fd6e
No known key found for this signature in database
4 changed files with 77 additions and 43 deletions

View file

@ -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();

View file

@ -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 {

View file

@ -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<String, String>>,
_cancel_token: Option<tokio_util::sync::CancellationToken>,
) -> std::result::Result<ExecResult, String> {
) -> SandboxResult<ExecResult> {
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<usize>,
_limit: Option<usize>,
) -> std::result::Result<String, String> {
) -> SandboxResult<String> {
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<bool, String> {
async fn file_exists(&self, _: &str) -> SandboxResult<bool> {
unimplemented!()
}
async fn list_directory(
&self,
_path: &str,
_depth: Option<usize>,
) -> std::result::Result<Vec<fabro_sandbox::DirEntry>, String> {
) -> SandboxResult<Vec<fabro_sandbox::DirEntry>> {
unimplemented!()
}
async fn grep(
@ -1903,34 +1903,30 @@ rename to docs/NOTES.md
_pattern: &str,
_path: &str,
_options: &fabro_sandbox::GrepOptions,
) -> std::result::Result<Vec<String>, String> {
) -> SandboxResult<Vec<String>> {
unimplemented!()
}
async fn glob(
&self,
_pattern: &str,
_path: Option<&str>,
) -> std::result::Result<Vec<String>, String> {
async fn glob(&self, _pattern: &str, _path: Option<&str>) -> SandboxResult<Vec<String>> {
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 {

View file

@ -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<IpAllowlistConfig>) -> Router {