fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(audit-2026-06-11/C-1): drop cookie auth on /api/v1/admin/*

Browse Source 
Same-origin CSRF: the platform co-hosts user-route HTML on the same
origin as /api/v1/admin/*; SameSite=Lax did not block a same-origin POST
from a malicious script at /<route>, riding the admin's picloud_session
cookie. The dashboard already uses Bearer (dashboard/src/lib/auth.ts +
api.ts:495), so cookie auth was dead weight on the admin side and
exploitable.

Cuts the cookie path entirely:
- auth_middleware::extract_token is Bearer-only; cookie branch removed.
- auth_api::login no longer sets Set-Cookie.
- auth_api::logout no longer clears the cookie (the bearer token is
  still revoked by deleting the session row).
- extract_token_for_logout matches.
- SESSION_COOKIE const + PICLOUD_COOKIE_SECURE env var deleted.

Audit ref: security_audit/07_http_cors_csrf_xss.md#c07-01.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-11 20:18:55 +02:00
parent 285a76f2e2
commit b096ea9c4e
3 changed files with 51 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files

94
crates/manager-core/src/auth_api.rs
View File

@@ -1,10 +1,14 @@
//! `/api/v1/admin/auth/*` — login, logout, who-am-I. //! `/api/v1/admin/auth/*` — login, logout, who-am-I.
//! //!
//! Login mints an opaque session token, stores its SHA-256, sets the //! Login mints an opaque session token, stores its SHA-256, and returns
//! `picloud_session` HttpOnly cookie, and also returns the raw token in //! the raw token in the JSON body. Clients must send it as
//! the JSON body for non-browser clients. The same token works as //! `Authorization: Bearer …` on subsequent requests; the same token is
//! `Authorization: Bearer …` afterward; there is no separate "API //! used for the session and admin-API authentication paths (there is no
//! token" concept yet. //! separate "API token" concept yet).
//!
//! Cookie auth was removed in the audit 2026-06-11 C-1 fix — same-origin
//! user-route HTML defeated the previous `SameSite=Lax` cookie. The
//! dashboard already uses Bearer; non-browser clients always have.
//! //!
//! Logout deletes the session row regardless of whether the supplied //! Logout deletes the session row regardless of whether the supplied
//! token matched anything (idempotent). `me` returns the row that the //! token matched anything (idempotent). `me` returns the row that the
@@ -12,7 +16,7 @@
use axum::body::Body; use axum::body::Body;
use axum::extract::{Extension, Request, State}; use axum::extract::{Extension, Request, State};
use axum::http::{header, HeaderMap, HeaderValue, StatusCode}; use axum::http::{header, StatusCode};
use axum::middleware::from_fn_with_state; use axum::middleware::from_fn_with_state;
use axum::response::{IntoResponse, Json, Response}; use axum::response::{IntoResponse, Json, Response};
use axum::routing::{get, post}; use axum::routing::{get, post};
@@ -25,7 +29,7 @@ use serde_json::json;
use picloud_shared::Principal; use picloud_shared::Principal;
use crate::auth::{generate_session_token, hash_token, verify_password}; use crate::auth::{generate_session_token, hash_token, verify_password};
use crate::auth_middleware::{require_authenticated, AuthState, SESSION_COOKIE}; use crate::auth_middleware::{require_authenticated, AuthState};
pub fn auth_router(state: AuthState) -> Router { pub fn auth_router(state: AuthState) -> Router {
// /login + /logout are unguarded (login is how you get in; logout // /login + /logout are unguarded (login is how you get in; logout
@@ -139,19 +143,8 @@ async fn login(State(state): State<AuthState>, Json(input): Json<LoginRequest>)
tracing::warn!(?err, "failed to touch admin last_login_at"); tracing::warn!(?err, "failed to touch admin last_login_at");
} }
let mut headers = HeaderMap::new();
headers.insert(
header::SET_COOKIE,
HeaderValue::from_str(&build_cookie(&token.raw, state.ttl)).unwrap_or_else(|_| {
// Cookie text is ASCII-clean by construction; this branch is
// unreachable in practice but the type signature requires it.
HeaderValue::from_static("")
}),
);
( (
StatusCode::OK, StatusCode::OK,
headers,
Json(LoginResponse { Json(LoginResponse {
user: AdminUserDto { user: AdminUserDto {
id: user_row.id, id: user_row.id,
@@ -167,23 +160,15 @@ async fn login(State(state): State<AuthState>, Json(input): Json<LoginRequest>)
} }
async fn logout(State(state): State<AuthState>, req: Request<Body>) -> Response { async fn logout(State(state): State<AuthState>, req: Request<Body>) -> Response {
// Pull token without requiring a valid session (logout is idempotent // Pull token without requiring a valid session (logout is idempotent).
// and we still want to clear the cookie on the client side).
let token = extract_token_for_logout(&req); let token = extract_token_for_logout(&req);
if let Some(raw) = token { if let Some(raw) = token {
let hash = hash_token(&raw); let hash = hash_token(&raw);
if let Err(err) = state.sessions.delete(&hash).await { if let Err(err) = state.sessions.delete(&hash).await {
tracing::error!(?err, "admin_sessions delete failed"); tracing::error!(?err, "admin_sessions delete failed");
// Still clear the cookie below.
} }
} }
StatusCode::NO_CONTENT.into_response()
let mut headers = HeaderMap::new();
headers.insert(
header::SET_COOKIE,
HeaderValue::from_static("picloud_session=; HttpOnly; Path=/; SameSite=Lax; Max-Age=0"),
);
(StatusCode::NO_CONTENT, headers).into_response()
} }
async fn me( async fn me(
@@ -213,51 +198,18 @@ async fn me(
// Helpers // Helpers
// ---------------------------------------------------------------------------- // ----------------------------------------------------------------------------
fn build_cookie(raw_token: &str, ttl: std::time::Duration) -> String {
// Secure is on by default; flip to off for HTTP-only dev with
// PICLOUD_COOKIE_SECURE=0. The header-injected bearer token works
// either way, so this is purely for browsers that prefer the cookie
// path (e.g., direct API hits without the dashboard's auth.ts).
let secure = std::env::var("PICLOUD_COOKIE_SECURE").ok().is_none_or(|v| {
!matches!(
v.to_ascii_lowercase().as_str(),
"0" | "false" | "no" | "off"
)
});
let secure_attr = if secure { "; Secure" } else { "" };
format!(
"{SESSION_COOKIE}={raw_token}; HttpOnly{secure_attr}; SameSite=Lax; Path=/; Max-Age={}",
ttl.as_secs()
)
}
fn extract_token_for_logout(req: &Request<Body>) -> Option<String> { fn extract_token_for_logout(req: &Request<Body>) -> Option<String> {
// Same precedence as the middleware — Authorization first, cookie // Duplicated from auth_middleware::extract_token because logout runs
// fallback. Duplicated here because logout has to read the request // before the auth middleware (logout is idempotent and must accept
// before any middleware would run. // already-expired tokens). Cookie auth was dropped in the audit
if let Some(value) = req.headers().get(header::AUTHORIZATION) { // 2026-06-11 C-1 fix — Bearer only.
if let Ok(s) = value.to_str() { let value = req.headers().get(header::AUTHORIZATION)?;
if let Some(token) = s.strip_prefix("Bearer ") { let s = value.to_str().ok()?;
let trimmed = token.trim(); let token = s.strip_prefix("Bearer ")?.trim();
if !trimmed.is_empty() { if token.is_empty() {
return Some(trimmed.to_string()); return None;
}
}
}
} }
if let Some(value) = req.headers().get(header::COOKIE) { Some(token.to_string())
if let Ok(s) = value.to_str() {
for chunk in s.split(';') {
let chunk = chunk.trim();
if let Some(rest) = chunk.strip_prefix(&format!("{SESSION_COOKIE}=")) {
if !rest.is_empty() {
return Some(rest.to_string());
}
}
}
}
}
None
} }
fn invalid_credentials() -> Response { fn invalid_credentials() -> Response {

64
crates/manager-core/src/auth_middleware.rs
View File

@@ -1,7 +1,7 @@
//! Authentication middleware — resolves the caller's `Principal` from //! Authentication middleware — resolves the caller's `Principal` from
//! either a session cookie / Bearer session-token OR an API key //! a Bearer session-token OR an API key (`Authorization: Bearer pic_…`).
//! (`Authorization: Bearer pic_…`). Both paths converge on the same //! Both paths converge on the same request extension so downstream
//! request extension so downstream handlers see one shape. //! handlers see one shape.
//! //!
//! Capability checks live in `crate::authz` and are called per-handler //! Capability checks live in `crate::authz` and are called per-handler
//! (after the relevant resource is loaded, so the capability binds to //! (after the relevant resource is loaded, so the capability binds to
@@ -10,8 +10,14 @@
//! //!
//! Token discriminator: the `pic_` prefix on a Bearer value selects //! Token discriminator: the `pic_` prefix on a Bearer value selects
//! the API-key path; anything else (raw 32-byte base64-url-encoded //! the API-key path; anything else (raw 32-byte base64-url-encoded
//! string) takes the session path. The session cookie can only ever //! string) takes the session path.
//! carry a session token (cookies are never API keys). //!
//! Audit 2026-06-11 (C-1): cookie auth removed. The platform co-hosts
//! attacker-controlled user-route HTML on the same origin as the admin
//! API; `SameSite=Lax` did not block same-origin POSTs from a malicious
//! script under `/<route>`. Admin clients must send
//! `Authorization: Bearer <session_token>` (the dashboard already does;
//! see `dashboard/src/lib/api.ts`).
use std::collections::HashMap; use std::collections::HashMap;
use std::sync::{Arc, Mutex}; use std::sync::{Arc, Mutex};
@@ -88,8 +94,6 @@ impl PrincipalCache {
} }
} }
pub const SESSION_COOKIE: &str = "picloud_session";
/// Prefix on the wire that selects the API-key path. The body that /// Prefix on the wire that selects the API-key path. The body that
/// follows is `base32(32 random bytes)`; the first 8 chars of the body /// follows is `base32(32 random bytes)`; the first 8 chars of the body
/// index into `api_keys.prefix` for verification. /// index into `api_keys.prefix` for verification.
@@ -355,34 +359,17 @@ async fn username_for(state: &AuthState, id: AdminUserId) -> Option<String> {
} }
} }
/// Pull the bearer token out of an `Authorization` header (preferred) /// Pull the bearer token out of an `Authorization` header. Cookie auth
/// or the `picloud_session` cookie (fallback for browser clients). /// was removed in the audit 2026-06-11 C-1 fix; same-origin user-route
/// Same shape as Phase 3a; the cookie only ever carries session /// HTML made `SameSite=Lax` insufficient against CSRF.
/// tokens — no `pic_` prefix expected there.
fn extract_token(req: &Request<Body>) -> Option<String> { fn extract_token(req: &Request<Body>) -> Option<String> {
if let Some(value) = req.headers().get(header::AUTHORIZATION) { let value = req.headers().get(header::AUTHORIZATION)?;
if let Ok(s) = value.to_str() { let s = value.to_str().ok()?;
if let Some(token) = s.strip_prefix("Bearer ") { let token = s.strip_prefix("Bearer ")?.trim();
let trimmed = token.trim(); if token.is_empty() {
if !trimmed.is_empty() { return None;
return Some(trimmed.to_string());
}
}
}
} }
if let Some(value) = req.headers().get(header::COOKIE) { Some(token.to_string())
if let Ok(s) = value.to_str() {
for chunk in s.split(';') {
let chunk = chunk.trim();
if let Some(rest) = chunk.strip_prefix(&format!("{SESSION_COOKIE}=")) {
if !rest.is_empty() {
return Some(rest.to_string());
}
}
}
}
}
None
} }
/// Sentinel returned from the resolve functions when a DB error should /// Sentinel returned from the resolve functions when a DB error should
@@ -438,13 +425,16 @@ mod tests {
} }
#[test] #[test]
fn extracts_cookie_token() { fn cookie_alone_is_rejected_post_audit_2026_06_11() {
// C-1 closure: `picloud_session` cookie is no longer accepted as
// an auth fallback; a same-origin malicious script could ride the
// cookie via SameSite=Lax POST. Bearer-only.
let r = req_with_header("cookie", "foo=bar; picloud_session=xyz; baz=qux"); let r = req_with_header("cookie", "foo=bar; picloud_session=xyz; baz=qux");
assert_eq!(extract_token(&r).as_deref(), Some("xyz")); assert_eq!(extract_token(&r), None);
} }
#[test] #[test]
fn bearer_wins_over_cookie() { fn bearer_wins_when_both_present() {
let r = Request::builder() let r = Request::builder()
.header("authorization", "Bearer header-token") .header("authorization", "Bearer header-token")
.header("cookie", "picloud_session=cookie-token") .header("cookie", "picloud_session=cookie-token")
@@ -454,7 +444,7 @@ mod tests {
} }
#[test] #[test]
fn returns_none_when_neither_present() { fn returns_none_when_no_authorization_header() {
let r = Request::builder().body(Body::empty()).unwrap(); let r = Request::builder().body(Body::empty()).unwrap();
assert_eq!(extract_token(&r), None); assert_eq!(extract_token(&r), None);
} }

2
crates/manager-core/src/lib.rs
View File

@@ -133,7 +133,7 @@ pub use auth_bootstrap::{
#[allow(deprecated)] #[allow(deprecated)]
pub use auth_middleware::{ pub use auth_middleware::{
attach_principal_if_present, require_admin, require_authenticated, AuthState, AuthedAdmin, attach_principal_if_present, require_admin, require_authenticated, AuthState, AuthedAdmin,
API_KEY_PREFIX, API_KEY_PREFIX_LEN, SESSION_COOKIE, API_KEY_PREFIX, API_KEY_PREFIX_LEN,
}; };
pub use authz::{can, require, AuthzDenied, AuthzError, AuthzRepo, Capability, Decision}; pub use authz::{can, require, AuthzDenied, AuthzError, AuthzRepo, Capability, Decision};
pub use cron_scheduler::spawn_cron_scheduler; pub use cron_scheduler::spawn_cron_scheduler;