mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
fix(security): restrict query-token auth to SSE endpoints only (#528)
* fix(security): restrict query-token auth to SSE endpoints only Query-string `?token=xxx` auth was accepted on all endpoints, exposing the main auth token in server logs, Referer headers, and browser history for state-changing routes. Now only GET /api/chat/events and GET /api/logs/events accept query tokens; all other endpoints require the Authorization header. Supersedes #364. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: add WebSocket endpoint to query-token allowlist, add URL-encoding tests The WS upgrade at /api/chat/ws also can't set custom headers, so it needs query-token auth like the SSE endpoints. Also adds tests for URL-encoded token values to cover the form_urlencoded parser. Addresses review feedback from Gemini (partially, /api/jobs/{id}/events is a JSON endpoint not SSE, so it correctly stays excluded) and Copilot (URL-encoded token test). Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Illia Polosukhin
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
e24c33ff90
commit
cbcd5adcc0
+148
-30
@@ -2,7 +2,7 @@
|
||||
|
||||
use axum::{
|
||||
extract::{Request, State},
|
||||
http::{HeaderMap, StatusCode},
|
||||
http::{HeaderMap, Method, StatusCode},
|
||||
middleware::Next,
|
||||
response::{IntoResponse, Response},
|
||||
};
|
||||
@@ -14,10 +14,44 @@ pub struct AuthState {
|
||||
pub token: String,
|
||||
}
|
||||
|
||||
/// Whether query-string token auth is allowed for this request.
|
||||
///
|
||||
/// Only GET requests to streaming endpoints may use `?token=xxx`. This
|
||||
/// minimizes token-in-URL exposure on state-changing routes, where the token
|
||||
/// would leak via server logs, Referer headers, and browser history.
|
||||
///
|
||||
/// Allowed endpoints:
|
||||
/// - SSE: `/api/chat/events`, `/api/logs/events` (EventSource can't set headers)
|
||||
/// - WebSocket: `/api/chat/ws` (WS upgrade can't set custom headers)
|
||||
///
|
||||
/// If you add a new SSE or WebSocket endpoint, add its path here.
|
||||
fn allows_query_token_auth(request: &Request) -> bool {
|
||||
if request.method() != Method::GET {
|
||||
return false;
|
||||
}
|
||||
|
||||
matches!(
|
||||
request.uri().path(),
|
||||
"/api/chat/events" | "/api/logs/events" | "/api/chat/ws"
|
||||
)
|
||||
}
|
||||
|
||||
/// Extract the `token` query parameter value, URL-decoded.
|
||||
fn query_token(request: &Request) -> Option<String> {
|
||||
let query = request.uri().query()?;
|
||||
url::form_urlencoded::parse(query.as_bytes()).find_map(|(k, v)| {
|
||||
if k == "token" {
|
||||
Some(v.into_owned())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
/// Auth middleware that validates bearer token from header or query param.
|
||||
///
|
||||
/// SSE connections can't set headers from `EventSource`, so we also accept
|
||||
/// `?token=xxx` as a query parameter.
|
||||
/// `?token=xxx` as a query parameter, but only on SSE endpoints.
|
||||
pub async fn auth_middleware(
|
||||
State(auth): State<AuthState>,
|
||||
headers: HeaderMap,
|
||||
@@ -35,15 +69,12 @@ pub async fn auth_middleware(
|
||||
return next.run(request).await;
|
||||
}
|
||||
|
||||
// Fall back to query parameter for SSE EventSource (constant-time comparison)
|
||||
if let Some(query) = request.uri().query() {
|
||||
for pair in query.split('&') {
|
||||
if let Some(token) = pair.strip_prefix("token=")
|
||||
&& bool::from(token.as_bytes().ct_eq(auth.token.as_bytes()))
|
||||
{
|
||||
return next.run(request).await;
|
||||
}
|
||||
}
|
||||
// Fall back to query parameter, but only for SSE endpoints (constant-time comparison).
|
||||
if allows_query_token_auth(&request)
|
||||
&& let Some(token) = query_token(&request)
|
||||
&& bool::from(token.as_bytes().ct_eq(auth.token.as_bytes()))
|
||||
{
|
||||
return next.run(request).await;
|
||||
}
|
||||
|
||||
(StatusCode::UNAUTHORIZED, "Invalid or missing auth token").into_response()
|
||||
@@ -62,24 +93,28 @@ mod tests {
|
||||
assert_eq!(cloned.token, "test-token");
|
||||
}
|
||||
|
||||
// === QA Plan - Web gateway auth tests ===
|
||||
|
||||
use axum::Router;
|
||||
use axum::body::Body;
|
||||
use axum::middleware;
|
||||
use axum::routing::get;
|
||||
use axum::routing::{get, post};
|
||||
use tower::ServiceExt;
|
||||
|
||||
async fn dummy_handler() -> &'static str {
|
||||
"ok"
|
||||
}
|
||||
|
||||
/// Router with streaming endpoints (query auth allowed) and regular
|
||||
/// endpoints (query auth rejected).
|
||||
fn test_app(token: &str) -> Router {
|
||||
let state = AuthState {
|
||||
token: token.to_string(),
|
||||
};
|
||||
Router::new()
|
||||
.route("/test", get(dummy_handler))
|
||||
.route("/api/chat/events", get(dummy_handler))
|
||||
.route("/api/logs/events", get(dummy_handler))
|
||||
.route("/api/chat/ws", get(dummy_handler))
|
||||
.route("/api/chat/history", get(dummy_handler))
|
||||
.route("/api/chat/send", post(dummy_handler))
|
||||
.layer(middleware::from_fn_with_state(state, auth_middleware))
|
||||
}
|
||||
|
||||
@@ -87,7 +122,7 @@ mod tests {
|
||||
async fn test_valid_bearer_token_passes() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "Bearer secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
@@ -99,7 +134,7 @@ mod tests {
|
||||
async fn test_invalid_bearer_token_rejected() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "Bearer wrong-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
@@ -108,10 +143,10 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_missing_auth_header_falls_through_to_query() {
|
||||
async fn test_query_token_allowed_for_chat_events() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test?token=secret-token")
|
||||
.uri("/api/chat/events?token=secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
@@ -119,10 +154,80 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_param_invalid_token_rejected() {
|
||||
async fn test_query_token_allowed_for_logs_events() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test?token=wrong-token")
|
||||
.uri("/api/logs/events?token=secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::OK);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_allowed_for_ws_upgrade() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/ws?token=secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::OK);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_url_encoded() {
|
||||
// Token with characters that get percent-encoded in URLs.
|
||||
let raw_token = "tok+en/with spaces";
|
||||
let app = test_app(raw_token);
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/events?token=tok%2Ben%2Fwith%20spaces")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::OK);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_url_encoded_mismatch() {
|
||||
let app = test_app("real-token");
|
||||
// Encoded value decodes to "wrong-token", not "real-token".
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/events?token=wrong%2Dtoken")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_rejected_for_non_sse_get() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/history?token=secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_rejected_for_post() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.method(Method::POST)
|
||||
.uri("/api/chat/send?token=secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_query_token_invalid_rejected() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/events?token=wrong-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
@@ -132,17 +237,32 @@ mod tests {
|
||||
#[tokio::test]
|
||||
async fn test_no_auth_at_all_rejected() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder().uri("/test").body(Body::empty()).unwrap();
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/events")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_bearer_prefix_case_insensitive() {
|
||||
// RFC 6750 Section 2.1: auth-scheme comparison must be case-insensitive.
|
||||
async fn test_bearer_header_works_for_post() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.method(Method::POST)
|
||||
.uri("/api/chat/send")
|
||||
.header("Authorization", "Bearer secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
let resp = app.oneshot(req).await.unwrap();
|
||||
assert_eq!(resp.status(), StatusCode::OK);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_bearer_prefix_case_insensitive() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "bearer secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
@@ -154,7 +274,7 @@ mod tests {
|
||||
async fn test_bearer_prefix_mixed_case() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "BEARER secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
@@ -166,7 +286,7 @@ mod tests {
|
||||
async fn test_empty_bearer_token_rejected() {
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "Bearer ")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
@@ -176,11 +296,9 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_token_with_whitespace_rejected() {
|
||||
// Extra space after "Bearer " means the token value starts with a space,
|
||||
// which should not match the expected token.
|
||||
let app = test_app("secret-token");
|
||||
let req = Request::builder()
|
||||
.uri("/test")
|
||||
.uri("/api/chat/events")
|
||||
.header("Authorization", "Bearer secret-token")
|
||||
.body(Body::empty())
|
||||
.unwrap();
|
||||
|
||||
Reference in New Issue
Block a user