Refactor session ID handling and improve error management
- Introduced `SessionId` newtype for validated session identifiers, ensuring safety against path traversal attacks. - Updated session repository methods to accept `SessionId` instead of raw strings, enhancing type safety. - Removed redundant error handling in repository methods by leveraging the new `Error` type from `zesdex_utils`. - Simplified atomic JSON write operations by eliminating unnecessary error conversions. - Enhanced integer casting with a new `CastOr` trait for safer narrowing conversions. - Removed deprecated error handling code and consolidated error types across the codebase. - Updated HTTP handlers to utilize the new session ID validation, improving overall robustness.
This commit is contained in:
asepharyana
@@ -15,8 +15,9 @@
|
||||
//!
|
||||
//! - `SessionServiceImpl<R, L>` — service over two generic repositories
|
||||
//! - `new` / `create_session` / `list_all` / `archive_session` — lifecycle ops
|
||||
use std::convert::TryInto;
|
||||
use std::path::PathBuf;
|
||||
use zesdex_entities::domain::auth::SessionId;
|
||||
use zesdex_utils::CastOr;
|
||||
use tracing;
|
||||
use uuid::Uuid;
|
||||
|
||||
@@ -49,13 +50,14 @@ impl<R: SessionRepository, L: SessionLockRepository> SessionServiceImpl<R, L> {
|
||||
|
||||
impl<R: SessionRepository, L: SessionLockRepository> SessionService for SessionServiceImpl<R, L> {
|
||||
fn create_session(&self, title: &str) -> Result<Session, ServiceError> {
|
||||
let id = Uuid::new_v4().to_string();
|
||||
let id = SessionId::new(&Uuid::new_v4().to_string())
|
||||
.expect("UUID is always a valid session id");
|
||||
let title_owned = if title.is_empty() {
|
||||
"New Session".to_string()
|
||||
} else {
|
||||
title.to_string()
|
||||
};
|
||||
let session = Session::new(id, title_owned);
|
||||
let session = Session::new(id.into_string(), title_owned);
|
||||
tracing::debug!(session_id = %session.id, title = %session.title, "creating new session");
|
||||
self.session_repo
|
||||
.save_session(&self.base_dir, &session)?; // RepositoryError → ServiceError via From
|
||||
@@ -69,16 +71,16 @@ impl<R: SessionRepository, L: SessionLockRepository> SessionService for SessionS
|
||||
.map_err(ServiceError::Repository)
|
||||
}
|
||||
|
||||
fn archive_session(&self, id: &str) -> Result<(), ServiceError> {
|
||||
fn archive_session(&self, id: SessionId) -> Result<(), ServiceError> {
|
||||
tracing::debug!(session_id = %id, "archiving session");
|
||||
let mut session = self.session_repo
|
||||
.load_session(&self.base_dir, id)?; // RepositoryError → ServiceError
|
||||
.load_session(&self.base_dir, &id)?; // RepositoryError → ServiceError
|
||||
session.archived = true;
|
||||
let millis = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.unwrap_or_default()
|
||||
.as_millis();
|
||||
session.updated_at = millis.try_into().unwrap_or(i64::MAX);
|
||||
session.updated_at = millis.cast_or(i64::MAX);
|
||||
self.session_repo
|
||||
.save_session(&self.base_dir, &session)?; // RepositoryError → ServiceError
|
||||
Ok(())
|
||||
|
||||
@@ -4,8 +4,11 @@
|
||||
//! application services, enabling callers to match on specific error
|
||||
//! variants (e.g. `NotFound` vs `Conflict`) rather than string-checking.
|
||||
//!
|
||||
//! `From` impls tie `std::io::Error` and `serde_json::Error` into
|
||||
//! `RepositoryError`, and `RepositoryError` into `ServiceError`.
|
||||
//! The `RepositoryError` type is re-exported from `zesdex_utils::Error`,
|
||||
//! which has all needed variants: `NotFound`, `Conflict`, `Io`, `Serde`,
|
||||
//! `InvalidId`, `Other`, etc.
|
||||
//!
|
||||
//! `From` impls are generated by `thiserror::Error` derive macros.
|
||||
//! Downstream `anyhow::Result` code uses `?` directly — anyhow's
|
||||
//! blanket `From<E: StdError + Send + Sync + 'static>` covers both
|
||||
//! `RepositoryError` and `ServiceError` automatically.
|
||||
@@ -16,74 +19,12 @@
|
||||
//! - [`ServiceError`] — use-case / orchestration errors (config, state
|
||||
//! mismatch, provider failures)
|
||||
|
||||
use std::fmt;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// RepositoryError
|
||||
// RepositoryError (type alias)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Errors from repository operations in the IAM domain.
|
||||
#[derive(Debug)]
|
||||
pub enum RepositoryError {
|
||||
/// The requested entity does not exist.
|
||||
NotFound(String),
|
||||
/// The operation conflicts with existing state (e.g. duplicate entry).
|
||||
Conflict(String),
|
||||
/// An I/O error occurred during persistence.
|
||||
Io(std::io::Error),
|
||||
/// A serialisation / deserialisation error occurred.
|
||||
Serialization(serde_json::Error),
|
||||
/// The supplied identifier is invalid (e.g. path traversal attempt).
|
||||
InvalidId(String),
|
||||
/// An error that could not be downcast to a specific variant.
|
||||
Other(String),
|
||||
}
|
||||
|
||||
impl RepositoryError {
|
||||
/// Convert an `anyhow::Error` to `RepositoryError` by attempting
|
||||
/// downcast to known inner types.
|
||||
pub fn from_anyhow(e: anyhow::Error) -> Self {
|
||||
if let Some(ioe) = e.downcast_ref::<std::io::Error>() {
|
||||
return RepositoryError::Io(std::io::Error::new(ioe.kind(), ioe.to_string()));
|
||||
}
|
||||
RepositoryError::Other(e.to_string())
|
||||
}
|
||||
}
|
||||
|
||||
impl fmt::Display for RepositoryError {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
match self {
|
||||
RepositoryError::NotFound(msg) => write!(f, "not found: {msg}"),
|
||||
RepositoryError::Conflict(msg) => write!(f, "conflict: {msg}"),
|
||||
RepositoryError::Io(e) => write!(f, "I/O error: {e}"),
|
||||
RepositoryError::Serialization(e) => write!(f, "serialization error: {e}"),
|
||||
RepositoryError::InvalidId(msg) => write!(f, "invalid id: {msg}"),
|
||||
RepositoryError::Other(msg) => write!(f, "{msg}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl std::error::Error for RepositoryError {
|
||||
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
|
||||
match self {
|
||||
RepositoryError::Io(e) => Some(e),
|
||||
RepositoryError::Serialization(e) => Some(e),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl From<std::io::Error> for RepositoryError {
|
||||
fn from(e: std::io::Error) -> Self {
|
||||
RepositoryError::Io(e)
|
||||
}
|
||||
}
|
||||
|
||||
impl From<serde_json::Error> for RepositoryError {
|
||||
fn from(e: serde_json::Error) -> Self {
|
||||
RepositoryError::Serialization(e)
|
||||
}
|
||||
}
|
||||
/// Re-export shared repository error from `zesdex_utils`.
|
||||
pub use zesdex_utils::Error as RepositoryError;
|
||||
|
||||
// `From<RepositoryError> for anyhow::Error` is covered by anyhow's blanket
|
||||
// `impl<E: StdError + Send + Sync + 'static> From<E> for Error` — no
|
||||
@@ -94,47 +35,23 @@ impl From<serde_json::Error> for RepositoryError {
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Errors from service / use-case operations in the IAM domain.
|
||||
#[derive(Debug)]
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum ServiceError {
|
||||
/// A repository operation failed.
|
||||
Repository(RepositoryError),
|
||||
#[error("repository error: {0}")]
|
||||
Repository(#[from] RepositoryError),
|
||||
/// The provided configuration is invalid.
|
||||
#[error("invalid configuration: {0}")]
|
||||
InvalidConfig(String),
|
||||
/// OAuth state mismatch — possible CSRF attack.
|
||||
#[error("OAuth state mismatch — possible CSRF attack")]
|
||||
StateMismatch,
|
||||
/// The OAuth provider returned an error.
|
||||
#[error("OAuth provider error: {0}")]
|
||||
OAuthProvider(String),
|
||||
/// A generic error with a message.
|
||||
#[error("{0}")]
|
||||
Other(String),
|
||||
}
|
||||
|
||||
impl fmt::Display for ServiceError {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
match self {
|
||||
ServiceError::Repository(e) => write!(f, "repository error: {e}"),
|
||||
ServiceError::InvalidConfig(msg) => write!(f, "invalid configuration: {msg}"),
|
||||
ServiceError::StateMismatch => {
|
||||
write!(f, "OAuth state mismatch — possible CSRF attack")
|
||||
}
|
||||
ServiceError::OAuthProvider(msg) => write!(f, "OAuth provider error: {msg}"),
|
||||
ServiceError::Other(msg) => write!(f, "{msg}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl std::error::Error for ServiceError {
|
||||
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
|
||||
match self {
|
||||
ServiceError::Repository(e) => Some(e),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl From<RepositoryError> for ServiceError {
|
||||
fn from(e: RepositoryError) -> Self {
|
||||
ServiceError::Repository(e)
|
||||
}
|
||||
}
|
||||
|
||||
// `From<ServiceError> for anyhow::Error` is covered by anyhow's blanket impl.
|
||||
|
||||
@@ -11,6 +11,8 @@
|
||||
//! - [`OAuthRepository`] — persist/load OAuth tokens
|
||||
use std::path::Path;
|
||||
|
||||
use zesdex_entities::domain::auth::SessionId;
|
||||
|
||||
use crate::domain::error::RepositoryError;
|
||||
use crate::domain::oauth::OAuthToken;
|
||||
use crate::domain::session::Session;
|
||||
@@ -21,13 +23,13 @@ pub trait SessionRepository {
|
||||
fn list_sessions(&self, base_dir: &Path) -> Result<Vec<Session>, RepositoryError>;
|
||||
|
||||
/// Load a single session by id.
|
||||
fn load_session(&self, base_dir: &Path, id: &str) -> Result<Session, RepositoryError>;
|
||||
fn load_session(&self, base_dir: &Path, id: &SessionId) -> Result<Session, RepositoryError>;
|
||||
|
||||
/// Save a session's metadata to disk.
|
||||
fn save_session(&self, base_dir: &Path, session: &Session) -> Result<(), RepositoryError>;
|
||||
|
||||
/// Delete a session directory and all its contents.
|
||||
fn delete_session(&self, base_dir: &Path, id: &str) -> Result<(), RepositoryError>;
|
||||
fn delete_session(&self, base_dir: &Path, id: &SessionId) -> Result<(), RepositoryError>;
|
||||
}
|
||||
|
||||
/// Repository for per-session PID-file advisory locks.
|
||||
|
||||
@@ -8,6 +8,8 @@
|
||||
//!
|
||||
//! - [`SessionService`] — create, list, archive sessions
|
||||
//! - [`OAuthService`] — start PKCE flow, complete code exchange, retrieve token
|
||||
use zesdex_entities::domain::auth::SessionId;
|
||||
|
||||
use crate::domain::error::ServiceError;
|
||||
use crate::domain::oauth::{OAuthConfig, OAuthToken};
|
||||
use crate::domain::session::Session;
|
||||
@@ -21,7 +23,7 @@ pub trait SessionService {
|
||||
fn list_all(&self) -> Result<Vec<Session>, ServiceError>;
|
||||
|
||||
/// Archive a session by id (sets `archived = true`).
|
||||
fn archive_session(&self, id: &str) -> Result<(), ServiceError>;
|
||||
fn archive_session(&self, id: SessionId) -> Result<(), ServiceError>;
|
||||
}
|
||||
|
||||
/// OAuth flow use-case boundary.
|
||||
|
||||
@@ -16,6 +16,8 @@
|
||||
//! - `handle_start_oauth` / `handle_complete_oauth` / `handle_get_token`
|
||||
use tracing::instrument;
|
||||
|
||||
use zesdex_entities::domain::auth::SessionId;
|
||||
|
||||
use crate::domain::service::{OAuthService, SessionService};
|
||||
use crate::infrastructure::http::dto::{
|
||||
CreateSessionRequest, OAuthCompleteRequest, OAuthStartRequest, OAuthStartResponse,
|
||||
@@ -43,7 +45,9 @@ pub fn handle_list_sessions<S: SessionService>(service: &S) -> anyhow::Result<Se
|
||||
/// Handle an archive-session request.
|
||||
#[instrument(skip(service), fields(session_id = %id))]
|
||||
pub fn handle_archive_session<S: SessionService>(service: &S, id: &str) -> anyhow::Result<()> {
|
||||
service.archive_session(id)?;
|
||||
let sid = SessionId::new(id)
|
||||
.map_err(|e| anyhow::anyhow!("invalid session id: {e}"))?;
|
||||
service.archive_session(sid)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -17,8 +17,8 @@
|
||||
//! - `LoopbackServer` — single-use TCP listener for one OAuth callback
|
||||
//! - `wait_for_code` / `read_callback` / `extract_code` / `extract_state`
|
||||
//! - `urlencoding` — minimal percent-decoder for query parameters
|
||||
use std::convert::TryInto;
|
||||
use std::io::{Read, Write};
|
||||
use zesdex_utils::CastOr;
|
||||
use std::net::{TcpListener, TcpStream};
|
||||
use tracing;
|
||||
|
||||
@@ -144,7 +144,7 @@ fn urlencoding(s: &str) -> String {
|
||||
) {
|
||||
(Some(hi), Some(lo)) => {
|
||||
// hi/lo are hex digits (0–15), product is 0–255 — safe.
|
||||
let byte: u8 = (hi * 16 + lo).try_into().unwrap_or(0);
|
||||
let byte: u8 = (hi * 16 + lo).cast_or(0u8);
|
||||
result.push(char::from(byte));
|
||||
}
|
||||
_ => {
|
||||
|
||||
@@ -40,7 +40,7 @@ impl OAuthRepository for FileSystemOAuthRepository {
|
||||
std::fs::create_dir_all(parent)?; // io::Error → RepositoryError via From
|
||||
}
|
||||
tracing::debug!(path = %path.display(), "saving OAuth token");
|
||||
write_json_atomic(path, token, Some(0o600)).map_err(RepositoryError::from_anyhow)?;
|
||||
write_json_atomic(path, token, Some(0o600))?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -98,7 +98,8 @@ impl SessionLockRepository for FileSystemSessionLockRepository {
|
||||
// SAFETY: `libc::kill(pid, 0)` sends no signal; it only probes
|
||||
// whether the process exists and is signalable by us.
|
||||
// PIDs on Linux fit in i32 (default pid_max ≈ 4 million).
|
||||
let pid_signed: i32 = pid.try_into().unwrap_or(0);
|
||||
let pid_signed: i32 = pid.try_into()
|
||||
.expect("PID exceeds i32 range — kernel pid_max > 2^31");
|
||||
if unsafe { libc::kill(pid_signed, 0) != 0 } {
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -7,14 +7,15 @@
|
||||
//!
|
||||
//! - **`list_sessions`** — enumerate `<base_dir>/sessions/` subdirectories,
|
||||
//! attempt `load_session` on each (silently skipping failures).
|
||||
//! - **`load_session`** — validates id (path-traversal check), reads JSON.
|
||||
//! - **`load_session`** — reads and deserialises `session.json`.
|
||||
//! - **`save_session`** — creates session directory, writes JSON atomically.
|
||||
//! - **`delete_session`** — validates id, removes the session directory.
|
||||
//! - **`delete_session`** — removes the session directory.
|
||||
//!
|
||||
//! # Security
|
||||
//!
|
||||
//! All methods that accept a user-supplied `id` string reject ids containing
|
||||
//! `/`, `\\`, or `..` to prevent directory-traversal attacks.
|
||||
//! Session IDs are validated at construction via [`SessionId::new`], so
|
||||
//! directory-traversal attacks are prevented by the type system — no
|
||||
//! per-method checks needed.
|
||||
//!
|
||||
//! # Components
|
||||
//!
|
||||
@@ -22,22 +23,13 @@
|
||||
use std::path::Path;
|
||||
use tracing;
|
||||
|
||||
use zesdex_entities::domain::auth::SessionId;
|
||||
use zesdex_utils::write_json_atomic;
|
||||
|
||||
use crate::domain::error::RepositoryError;
|
||||
use crate::domain::repository::SessionRepository;
|
||||
use crate::domain::session::Session;
|
||||
|
||||
/// Validate a session id, rejecting path-traversal patterns.
|
||||
fn validate_id(id: &str) -> Result<(), RepositoryError> {
|
||||
if id.contains('/') || id.contains('\\') || id.contains("..") {
|
||||
return Err(RepositoryError::InvalidId(format!(
|
||||
"session id '{id}' must not contain path separators"
|
||||
)));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Concrete filesystem session repository.
|
||||
#[derive(Debug, Clone, Default)]
|
||||
pub struct FileSystemSessionRepository;
|
||||
@@ -65,20 +57,25 @@ impl SessionRepository for FileSystemSessionRepository {
|
||||
if !entry.path().is_dir() {
|
||||
continue;
|
||||
}
|
||||
let id = entry.file_name().to_string_lossy().to_string();
|
||||
if let Ok(session) = self.load_session(base_dir, &id) {
|
||||
sessions.push(session);
|
||||
let name = entry.file_name().to_string_lossy().to_string();
|
||||
// Directory names from UUIDs are always valid session IDs.
|
||||
if let Ok(sid) = SessionId::new(&name) {
|
||||
if let Ok(session) = self.load_session(base_dir, &sid) {
|
||||
sessions.push(session);
|
||||
}
|
||||
}
|
||||
}
|
||||
tracing::debug!(count = sessions.len(), "listed sessions");
|
||||
Ok(sessions)
|
||||
}
|
||||
|
||||
fn load_session(&self, base_dir: &Path, id: &str) -> Result<Session, RepositoryError> {
|
||||
validate_id(id)?;
|
||||
let path = base_dir.join("sessions").join(id).join("session.json");
|
||||
fn load_session(&self, base_dir: &Path, id: &SessionId) -> Result<Session, RepositoryError> {
|
||||
let path = base_dir.join("sessions").join(id.as_str()).join("session.json");
|
||||
if !path.exists() {
|
||||
return Err(RepositoryError::NotFound(format!("session not found: {id}")));
|
||||
return Err(RepositoryError::NotFound(format!(
|
||||
"session not found: {}",
|
||||
id.as_str()
|
||||
)));
|
||||
}
|
||||
tracing::debug!(session_id = %id, path = %path.display(), "loading session");
|
||||
let data = std::fs::read_to_string(&path)?; // → RepositoryError
|
||||
@@ -91,13 +88,12 @@ impl SessionRepository for FileSystemSessionRepository {
|
||||
std::fs::create_dir_all(&dir)?; // → RepositoryError
|
||||
let path = dir.join("session.json");
|
||||
tracing::debug!(session_id = %session.id, path = %path.display(), "saving session");
|
||||
write_json_atomic(&path, session, None).map_err(RepositoryError::from_anyhow)?;
|
||||
write_json_atomic(&path, session, None)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn delete_session(&self, base_dir: &Path, id: &str) -> Result<(), RepositoryError> {
|
||||
validate_id(id)?;
|
||||
let dir = base_dir.join("sessions").join(id);
|
||||
fn delete_session(&self, base_dir: &Path, id: &SessionId) -> Result<(), RepositoryError> {
|
||||
let dir = base_dir.join("sessions").join(id.as_str());
|
||||
tracing::debug!(session_id = %id, path = %dir.display(), "deleting session");
|
||||
if dir.exists() {
|
||||
std::fs::remove_dir_all(&dir)?; // → RepositoryError
|
||||
|
||||
Reference in New Issue
Block a user