From da08927fea2272117931c37f1c2240d58fd92fb2 Mon Sep 17 00:00:00 2001 From: MicrosoftWindows96 Date: Sat, 16 May 2026 17:31:54 +0100 Subject: [PATCH] fix(identity): address auth review findings Signed-off-by: MicrosoftWindows96 --- crates/zagrosi-identity/src/error.rs | 12 ++ crates/zagrosi-identity/src/http/auth.rs | 150 +++++++++++++++--- crates/zagrosi-identity/src/http/mod.rs | 2 +- crates/zagrosi-identity/src/service/signup.rs | 21 +-- crates/zagrosi-identity/src/session/cookie.rs | 34 ++++ crates/zagrosi-identity/src/session/mod.rs | 5 +- documentation/api/identity.openapi.yaml | 40 +++-- 7 files changed, 218 insertions(+), 46 deletions(-) diff --git a/crates/zagrosi-identity/src/error.rs b/crates/zagrosi-identity/src/error.rs index 359bd85..3168a4b 100644 --- a/crates/zagrosi-identity/src/error.rs +++ b/crates/zagrosi-identity/src/error.rs @@ -73,6 +73,17 @@ pub enum IdentityError { #[error("database error: {0}")] Database(#[source] Box), + /// HTTP response construction failed because a generated header + /// value contained bytes that cannot be represented in an HTTP + /// header. This is an internal invariant breach, never a caller + /// validation failure. + #[error("response header malformed: {reason}")] + ResponseHeaderMalformed { + /// Human-readable description of the malformed generated + /// header. + reason: String, + }, + /// Raw token string failed prefix / body validation. Domain-layer /// `domain::token_format::parse_raw` is the single chokepoint; this /// variant is returned rather than the gateway-facing @@ -677,6 +688,7 @@ impl axum::response::IntoResponse for IdentityError { | Self::MalformedEnvelope(_) | Self::UnknownKeyId(_) | Self::Database(_) + | Self::ResponseHeaderMalformed { .. } | Self::Argon2ProfileTooSlow { .. } | Self::Argon2Internal(_) | Self::MalformedRateLimit { .. } diff --git a/crates/zagrosi-identity/src/http/auth.rs b/crates/zagrosi-identity/src/http/auth.rs index 5a7c0fc..c32875b 100644 --- a/crates/zagrosi-identity/src/http/auth.rs +++ b/crates/zagrosi-identity/src/http/auth.rs @@ -3,18 +3,22 @@ #![allow(clippy::doc_markdown, clippy::too_long_first_doc_paragraph)] //! Sign-up / sign-in / sign-out HTTP handlers. -use std::net::IpAddr; - -use axum::Json; use axum::extract::{ConnectInfo, State}; -use axum::http::StatusCode; -use axum::response::IntoResponse; +use axum::http::{HeaderMap, HeaderValue, StatusCode, header}; +use axum::response::{IntoResponse, Response}; +use axum::{Extension, Json}; +use chrono::{DateTime, Utc}; use uuid::Uuid; +use zagrosi_core::AuthContext; -use crate::error::Result; +use crate::error::{IdentityError, Result}; use crate::http::IdentityState; use crate::service::signin::SignInRequest; use crate::service::signup::{SignUpRequest, SignUpResponse}; +use crate::session::{ + IssuedSession, SessionAttachment, build_clear_csrf_cookie, build_clear_session_cookie, + generate_csrf_value, +}; /// JSON body for `POST /v1/auth/sign-up`. #[derive(Debug, serde::Deserialize)] @@ -63,6 +67,38 @@ pub struct SignInResponse { /// Issued session id (for client-side telemetry; cookie is what /// authorises subsequent requests). pub session_id: Uuid, + /// Issued session metadata. + pub session: SignInSession, + /// Raw `sid_*` credential for non-browser clients that use bearer + /// authentication instead of the emitted session cookie. + pub session_token: String, + /// CSRF double-submit value emitted both in the response body and + /// the readable CSRF cookie. + pub csrf_token: String, +} + +/// Session metadata returned after password sign-in. +#[derive(Debug, serde::Serialize)] +pub struct SignInSession { + /// Session row identifier. + pub id: Uuid, + /// Owning user. + pub user_id: Uuid, + /// Active organisation selected at issue time, when known. + pub org_id: Option, + /// Hard expiry timestamp for the issued session. + pub expires_at: DateTime, +} + +impl From<&IssuedSession> for SignInSession { + fn from(session: &IssuedSession) -> Self { + Self { + id: session.id, + user_id: session.user_id, + org_id: session.org_id, + expires_at: session.expires_at, + } + } } /// `POST /v1/auth/sign-in` handler. @@ -70,7 +106,7 @@ pub async fn sign_in( State(state): State, ConnectInfo(addr): ConnectInfo, Json(body): Json, -) -> Result> { +) -> Result { let session = state .service .sign_in(SignInRequest { @@ -80,32 +116,96 @@ pub async fn sign_in( correlation_id: Uuid::now_v7(), }) .await?; - Ok(Json(SignInResponse { - status: "ok", - session_id: session.id, - })) -} - -/// JSON body for `POST /v1/auth/sign-out`. -#[derive(Debug, serde::Deserialize)] -pub struct SignOutBody { - /// Session id to revoke. - pub session_id: Uuid, + let attachment = SessionAttachment::new(session.raw_token.clone(), generate_csrf_value()); + let mut headers = HeaderMap::new(); + append_set_cookie(&mut headers, &attachment.session_set_cookie())?; + append_set_cookie(&mut headers, &attachment.csrf_set_cookie())?; + Ok(( + StatusCode::OK, + headers, + Json(SignInResponse { + status: "ok", + session_id: session.id, + session: SignInSession::from(&session), + session_token: attachment.raw_session_token, + csrf_token: attachment.csrf_value, + }), + ) + .into_response()) } /// `POST /v1/auth/sign-out` handler. pub async fn sign_out( State(state): State, + ctx: Option>, ConnectInfo(addr): ConnectInfo, - Json(body): Json, -) -> Result { - let _ip: IpAddr = addr.ip(); +) -> Result { + let Some(Extension(ctx)) = ctx else { + return Err(IdentityError::InvalidCredentials); + }; state .service - .sign_out(body.session_id, None, Some(addr.ip()), Uuid::now_v7()) + .sign_out( + ctx.session_id(), + Some(ctx.subject_id()), + Some(addr.ip()), + Uuid::now_v7(), + ) .await?; - Ok(StatusCode::NO_CONTENT) + let mut headers = HeaderMap::new(); + append_set_cookie(&mut headers, &build_clear_session_cookie())?; + append_set_cookie(&mut headers, &build_clear_csrf_cookie())?; + Ok((StatusCode::NO_CONTENT, headers).into_response()) +} + +fn append_set_cookie(headers: &mut HeaderMap, value: &str) -> Result<()> { + let header_value = + HeaderValue::from_str(value).map_err(|_| IdentityError::ResponseHeaderMalformed { + reason: "set-cookie header value contains illegal byte".into(), + })?; + headers.append(header::SET_COOKIE, header_value); + Ok(()) } -#[allow(dead_code)] -fn dummy_into_response_use(_: T) {} +#[cfg(test)] +mod tests { + #![allow(clippy::expect_used)] + + use super::*; + + #[test] + fn sign_in_response_surfaces_session_credential_material() { + let issued = IssuedSession { + id: Uuid::now_v7(), + user_id: Uuid::now_v7(), + org_id: Some(Uuid::now_v7()), + expires_at: Utc::now(), + raw_token: "sid_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), + }; + let attachment = SessionAttachment::new(issued.raw_token.clone(), "c".repeat(43)); + let response = SignInResponse { + status: "ok", + session_id: issued.id, + session: SignInSession::from(&issued), + session_token: attachment.raw_session_token, + csrf_token: attachment.csrf_value, + }; + + assert_eq!(response.session_id, issued.id); + assert_eq!( + response.session_token, + "sid_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ); + assert_eq!(response.csrf_token, "c".repeat(43)); + assert_eq!(response.session.user_id, issued.user_id); + } + + #[test] + fn appends_multiple_set_cookie_headers() { + let mut headers = HeaderMap::new(); + append_set_cookie(&mut headers, "a=b").expect("first cookie is valid"); + append_set_cookie(&mut headers, "c=d").expect("second cookie is valid"); + + assert_eq!(headers.get_all(header::SET_COOKIE).iter().count(), 2); + } +} diff --git a/crates/zagrosi-identity/src/http/mod.rs b/crates/zagrosi-identity/src/http/mod.rs index c940433..6895d85 100644 --- a/crates/zagrosi-identity/src/http/mod.rs +++ b/crates/zagrosi-identity/src/http/mod.rs @@ -15,7 +15,7 @@ //! - [`router`] returns the public surface only: //! - `POST /v1/auth/sign-up` //! - `POST /v1/auth/sign-in` -//! - `POST /v1/auth/sign-out` +//! - `POST /v1/auth/sign-out` (requires an attached `AuthContext`) //! - `POST /v1/auth/password-reset/request` //! - `POST /v1/auth/password-reset/confirm` //! - `GET /v1/auth/password-reset/landing` diff --git a/crates/zagrosi-identity/src/service/signup.rs b/crates/zagrosi-identity/src/service/signup.rs index 7540d94..2a4d50e 100644 --- a/crates/zagrosi-identity/src/service/signup.rs +++ b/crates/zagrosi-identity/src/service/signup.rs @@ -144,15 +144,18 @@ impl IdentityService { let mut tx = self.pool.begin().await?; let user = self .user_repo - .create(NewUser { - id: user_id, - email: &req.email, - display_name: &req.display_name, - password_hash: Some(&phc), - password_updated_at: Some(now), - password_hash_version: 1, - external_id: None, - }) + .create_in_tx( + &mut tx, + NewUser { + id: user_id, + email: &req.email, + display_name: &req.display_name, + password_hash: Some(&phc), + password_updated_at: Some(now), + password_hash_version: 1, + external_id: None, + }, + ) .await?; self.email_verification_repo .insert( diff --git a/crates/zagrosi-identity/src/session/cookie.rs b/crates/zagrosi-identity/src/session/cookie.rs index ec4295a..075ab9d 100644 --- a/crates/zagrosi-identity/src/session/cookie.rs +++ b/crates/zagrosi-identity/src/session/cookie.rs @@ -35,6 +35,18 @@ pub const CSRF_COOKIE_NAME: &str = "__Host-zagrosi_csrf"; /// [`crate::http::csrf::csrf_middleware`]. pub const CSRF_HEADER_NAME: &str = "x-zagrosi-csrf"; +/// Render a `Set-Cookie` value that clears the browser session cookie. +#[must_use] +pub fn build_clear_session_cookie() -> String { + format!("{SESSION_COOKIE_NAME}=; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=0") +} + +/// Render a `Set-Cookie` value that clears the CSRF cookie. +#[must_use] +pub fn build_clear_csrf_cookie() -> String { + format!("{CSRF_COOKIE_NAME}=; Path=/; Secure; SameSite=Lax; Max-Age=0") +} + /// Issued cookie pair the auth handler attaches to its response. /// /// `session` carries the raw `sid_*` token; `csrf` carries the @@ -146,4 +158,26 @@ mod tests { assert_eq!(attach.raw_session_token, raw); assert_eq!(attach.csrf_value, "c".repeat(43)); } + + #[test] + fn clear_session_cookie_expires_host_cookie() { + let rendered = build_clear_session_cookie(); + assert!(rendered.contains("__Host-zagrosi_sid=")); + assert!(rendered.contains("Path=/")); + assert!(rendered.contains("Secure")); + assert!(rendered.contains("HttpOnly")); + assert!(rendered.contains("SameSite=Lax")); + assert!(rendered.contains("Max-Age=0")); + } + + #[test] + fn clear_csrf_cookie_expires_host_cookie_without_httponly() { + let rendered = build_clear_csrf_cookie(); + assert!(rendered.contains("__Host-zagrosi_csrf=")); + assert!(rendered.contains("Path=/")); + assert!(rendered.contains("Secure")); + assert!(rendered.contains("SameSite=Lax")); + assert!(rendered.contains("Max-Age=0")); + assert!(!rendered.contains("HttpOnly")); + } } diff --git a/crates/zagrosi-identity/src/session/mod.rs b/crates/zagrosi-identity/src/session/mod.rs index 19b7bcd..eac8873 100644 --- a/crates/zagrosi-identity/src/session/mod.rs +++ b/crates/zagrosi-identity/src/session/mod.rs @@ -44,7 +44,10 @@ pub mod write_behind; pub use cache::{CachedSession, SessionCache}; pub use continuation::SessionView; -pub use cookie::{CSRF_COOKIE_NAME, CSRF_HEADER_NAME, SESSION_COOKIE_NAME, SessionAttachment}; +pub use cookie::{ + CSRF_COOKIE_NAME, CSRF_HEADER_NAME, SESSION_COOKIE_NAME, SessionAttachment, + build_clear_csrf_cookie, build_clear_session_cookie, +}; pub use events::{BusError, SessionEventBus}; pub use introspector::IdentitySessionIntrospector; pub use issuer::{IdentitySessionIssuer, generate_csrf_value}; diff --git a/documentation/api/identity.openapi.yaml b/documentation/api/identity.openapi.yaml index f14e26d..b91ff6e 100644 --- a/documentation/api/identity.openapi.yaml +++ b/documentation/api/identity.openapi.yaml @@ -61,7 +61,7 @@ paths: post: tags: [Auth] operationId: signIn - summary: Verify email and password and issue browser session cookies. + summary: Verify email and password and issue browser session cookies plus bearer fallback tokens. requestBody: required: true content: @@ -91,13 +91,6 @@ paths: summary: Revoke the current browser session. security: - SessionCookie: [] - requestBody: - required: false - content: - application/json: - schema: - type: object - additionalProperties: false responses: "204": description: Signed out. @@ -1155,10 +1148,37 @@ components: type: string SignInResponse: type: object - required: [session] + required: [status, session_id, session, session_token, csrf_token] properties: + status: + type: string + enum: [ok] + session_id: + type: string + format: uuid session: - $ref: "#/components/schemas/Session" + type: object + required: [id, user_id, expires_at] + properties: + id: + type: string + format: uuid + user_id: + type: string + format: uuid + org_id: + type: string + format: uuid + nullable: true + expires_at: + type: string + format: date-time + session_token: + $ref: "#/components/schemas/PrefixedToken" + csrf_token: + type: string + minLength: 43 + maxLength: 43 EmailRequest: type: object required: [email]