From c0e6a68ca4e9664a47080dcecc325e556e5db70b Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Tue, 13 Oct 2026 22:32:39 -0400 Subject: [PATCH] fix(video): keep signed stream tokens out of logs and traces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The access log and the tracing root span's http.target both recorded the full /video/original/stream?token=… URL, i.e. a credential for a possibly sensitive file, in the journal and OTel exports. An outermost middleware now moves the token into request extensions and rewrites the URI to token=redacted before either logger sees it; the handler reads the token from extensions and fails closed (401) without the middleware. Co-Authored-By: Claude Opus 5.5 --- src/handlers/video.rs | 130 ++++++++++++++++++++++++++++++++++++++---- src/main.rs | 5 ++ 2 files changed, 125 insertions(+), 10 deletions(-) diff --git a/src/handlers/video.rs b/src/handlers/video.rs index 4506c12..cd97d29 100644 --- a/src/handlers/video.rs +++ b/src/handlers/video.rs @@ -8,13 +8,18 @@ use tracing_opentelemetry::OpenTelemetrySpanExt; use actix_files::NamedFile; use actix_web::{ - HttpRequest, HttpResponse, Responder, get, post, + HttpMessage, HttpRequest, HttpResponse, Responder, + body::MessageBody, + dev::{ServiceRequest, ServiceResponse}, + get, + middleware::Next, + post, web::{self, Data}, }; use log::{debug, error, info, warn}; use opentelemetry::KeyValue; use opentelemetry::trace::Status; -use serde::{Deserialize, Serialize}; +use serde::Serialize; use crate::content_hash; use crate::data::{ @@ -301,29 +306,71 @@ pub async fn original_video_url( span.set_status(Status::Ok); HttpResponse::Ok().json(OriginalVideoUrlResponse { - url: format!("/video/original/stream?token={}", token), + url: format!("{}?token={}", ORIGINAL_STREAM_PATH, token), expires_at, }) } -#[derive(Deserialize)] -struct OriginalStreamQuery { - token: String, +const ORIGINAL_STREAM_PATH: &str = "/video/original/stream"; + +/// A stream token lifted out of the request URI by +/// [`redact_original_stream_token`]. +struct OriginalStreamToken(String); + +/// Moves `token=` off `/video/original/stream` requests into request +/// extensions and rewrites the URI to `token=redacted`, so neither the +/// access log nor the tracing span's `http.target` (exported to OTel) +/// records a credential for a possibly sensitive file. Must be the +/// outermost `.wrap()` so it runs before both loggers; without it the +/// stream handler finds no token and 401s (fails closed). +pub async fn redact_original_stream_token( + mut req: ServiceRequest, + next: Next, +) -> Result, actix_web::Error> { + if req.path() == ORIGINAL_STREAM_PATH + && let Some((token, redacted_query)) = take_token_param(req.query_string()) + && let Ok(uri) = format!("{}?{}", ORIGINAL_STREAM_PATH, redacted_query).parse() + { + req.head_mut().uri = uri; + req.extensions_mut().insert(OriginalStreamToken(token)); + } + next.call(req).await +} + +/// Split the first `token=` value out of a query string, returning it and +/// the query with that value replaced by `redacted`. +fn take_token_param(query: &str) -> Option<(String, String)> { + let mut token = None; + let redacted: Vec<&str> = query + .split('&') + .map(|pair| match pair.strip_prefix("token=") { + Some(value) if token.is_none() => { + token = Some(value.to_string()); + "token=redacted" + } + _ => pair, + }) + .collect(); + token.map(|t| (t, redacted.join("&"))) } /// Stream an original video named by a token from `POST /video/original`. -/// Authenticated by the token alone (no Claims). `NamedFile` honours Range, -/// so players can seek. +/// Authenticated by the token alone (no Claims), which +/// [`redact_original_stream_token`] has already moved out of the URI. +/// `NamedFile` honours Range, so players can seek. #[get("/video/original/stream")] #[tracing::instrument(name = "stream_original_video", skip_all)] pub async fn stream_original_video( request: HttpRequest, - query: web::Query, app_state: Data, ) -> impl Responder { let span = tracing::Span::current(); - let Some(claims) = original_url::verify(&query.token) else { + let claims = request + .extensions() + .get::() + .and_then(|t| original_url::verify(&t.0)); + let Some(claims) = claims else { span.set_status(Status::error("bad token")); return HttpResponse::Unauthorized().finish(); }; @@ -745,6 +792,7 @@ mod tests { write_library_file(&app_state, "clips/a.mp4", b"0123456789"); let app = actix_web::test::init_service( App::new() + .wrap(actix_web::middleware::from_fn(redact_original_stream_token)) .service(original_video_url) .service(stream_original_video) .app_data(app_state), @@ -796,12 +844,74 @@ mod tests { assert_eq!(actix_web::test::call_service(&app, req).await.status(), 401); } + #[test] + fn take_token_param_redacts_only_the_token() { + assert_eq!( + take_token_param("token=abc.def.ghi"), + Some(("abc.def.ghi".into(), "token=redacted".into())) + ); + assert_eq!( + take_token_param("x=1&token=abc&y=2"), + Some(("abc".into(), "x=1&token=redacted&y=2".into())) + ); + assert_eq!(take_token_param("x=1&tokens=abc"), None); + assert_eq!(take_token_param(""), None); + } + + /// What the loggers (and handler) see after the middleware runs. + #[get("/video/original/stream")] + async fn echo_uri_and_token(request: HttpRequest) -> HttpResponse { + let token = request + .extensions() + .get::() + .map(|t| t.0.clone()) + .unwrap_or_default(); + HttpResponse::Ok().body(format!("{} {}", request.uri(), token)) + } + + #[actix_rt::test] + async fn redact_middleware_strips_token_from_uri() { + let app = actix_web::test::init_service( + App::new() + .wrap(actix_web::middleware::from_fn(redact_original_stream_token)) + .service(echo_uri_and_token), + ) + .await; + let req = actix_web::test::TestRequest::get() + .uri("/video/original/stream?token=secret.jwt.value") + .to_request(); + let body = actix_web::test::call_and_read_body(&app, req).await; + assert_eq!( + std::str::from_utf8(&body).unwrap(), + "/video/original/stream?token=redacted secret.jwt.value" + ); + } + + #[actix_rt::test] + async fn stream_original_video_fails_closed_without_middleware() { + let app_state = Data::new(AppState::test_state()); + write_library_file(&app_state, "clips/a.mp4", b"0123456789"); + let app = actix_web::test::init_service( + App::new() + .service(stream_original_video) + .app_data(app_state), + ) + .await; + let (token, _) = + original_url::sign("1", crate::libraries::PRIMARY_LIBRARY_ID, "clips/a.mp4"); + let req = actix_web::test::TestRequest::get() + .uri(&format!("/video/original/stream?token={}", token)) + .to_request(); + assert_eq!(actix_web::test::call_service(&app, req).await.status(), 401); + } + #[actix_rt::test] async fn stream_original_video_rejects_bad_tokens() { let app_state = Data::new(AppState::test_state()); write_library_file(&app_state, "clips/a.mp4", b"0123456789"); let app = actix_web::test::init_service( App::new() + .wrap(actix_web::middleware::from_fn(redact_original_stream_token)) .service(stream_original_video) .app_data(app_state), ) diff --git a/src/main.rs b/src/main.rs index 9b89ed3..27ef1a5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -318,6 +318,11 @@ fn main() -> std::io::Result<()> { .wrap(middleware::Logger::default()) .wrap(cors) .wrap(tracing_actix_web::TracingLogger::default()) + // Outermost: strips signed stream tokens from the URI before + // either logger above records it. + .wrap(middleware::from_fn( + handlers::video::redact_original_stream_token, + )) .service( web::resource("/login") .wrap(Governor::new(&governor_conf)) -- 2.54.0