diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index f5e10d6b..9e152df0 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -107,6 +107,31 @@ pub enum WorkdirTransportErrorCode { Internal, } +impl WorkdirTransportErrorCode { + pub const fn as_str(self) -> &'static str { + match self { + Self::NotFound => "not_found", + Self::Conflict => "conflict", + Self::Unsupported => "unsupported", + Self::InvalidRequest => "invalid_request", + Self::UnknownCommand => "unknown_command", + Self::Unavailable => "unavailable", + Self::Internal => "internal", + } + } + + /// Shared public HTTP classification for Runtime and Workspace Workdir operation boundaries. + pub const fn http_status(self) -> u16 { + match self { + Self::NotFound | Self::UnknownCommand => 404, + Self::Conflict => 409, + Self::Unsupported | Self::InvalidRequest => 400, + Self::Unavailable => 503, + Self::Internal => 500, + } + } +} + #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct WorkdirTransportError { pub code: WorkdirTransportErrorCode, @@ -126,6 +151,9 @@ impl WorkdirTransportError { message: format!("Workdir capability {capability:?} is not available"), }; } + WorkdirError::UnsupportedOperation(_) => { + (Code::Unsupported, "Workdir operation is not supported") + } WorkdirError::UnknownCommand(_) => { (Code::UnknownCommand, "Workdir command was not found") } @@ -161,7 +189,7 @@ impl WorkdirTransportError { match self.code { Code::NotFound => WorkdirError::NotFound("".into()), Code::Conflict => WorkdirError::Conflict(self.message), - Code::Unsupported => WorkdirError::Unavailable(self.message), + Code::Unsupported => WorkdirError::UnsupportedOperation(self.message), Code::UnknownCommand => WorkdirError::UnknownCommand("".to_string()), Code::InvalidRequest => WorkdirError::InvalidArgument(self.message), Code::Unavailable => WorkdirError::Unavailable(self.message), @@ -517,13 +545,13 @@ mod client { .json::() .await .map(WorkdirTransportError::into_workdir_error) - .unwrap_or_else(|error| { - WorkdirError::Unavailable(format!("Runtime HTTP error: {error}")) + .unwrap_or_else(|_| { + WorkdirError::Transport("Runtime Workdir error response was invalid".to_string()) }) } - fn http_unavailable(error: reqwest::Error) -> WorkdirError { - WorkdirError::Unavailable(format!("Runtime Workdir HTTP request failed: {error}")) + fn http_unavailable(_error: reqwest::Error) -> WorkdirError { + WorkdirError::Transport("Runtime Workdir HTTP request failed".to_string()) } pub use self::RemoteWorkdirSession as ClientSession; @@ -536,6 +564,57 @@ pub use client::{ClientSession as RemoteWorkdirSession, WorkdirHttpAuthorization mod tests { use super::*; + #[test] + fn transport_error_round_trip_keeps_public_classification() { + for (code, expected_status, expected_error) in [ + ( + WorkdirTransportErrorCode::InvalidRequest, + 400, + "invalid argument", + ), + (WorkdirTransportErrorCode::NotFound, 404, "file not found"), + ( + WorkdirTransportErrorCode::UnknownCommand, + 404, + "unknown Workdir session command", + ), + ( + WorkdirTransportErrorCode::Conflict, + 409, + "modified externally", + ), + (WorkdirTransportErrorCode::Unsupported, 400, "unsupported"), + (WorkdirTransportErrorCode::Unavailable, 503, "unavailable"), + (WorkdirTransportErrorCode::Internal, 500, "transport failed"), + ] { + let transport = WorkdirTransportError { + code, + message: "safe provider message".to_string(), + }; + assert_eq!(code.http_status(), expected_status); + let workdir_error = transport.clone().into_workdir_error(); + assert!(workdir_error.to_string().contains(expected_error)); + assert_eq!( + WorkdirTransportError::from_workdir_error(&workdir_error).code, + code + ); + } + } + + #[test] + fn local_validation_errors_share_invalid_request_classification() { + for error in [ + WorkdirError::InvalidGlob("[".to_string()), + WorkdirError::InvalidRegex("(".to_string()), + WorkdirError::InvalidArgument("limit must be positive".to_string()), + ] { + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code, WorkdirTransportErrorCode::InvalidRequest); + assert_eq!(transport.code.http_status(), 400); + assert_eq!(transport.message, "Workdir operation request is invalid"); + } + } + #[test] fn transport_failure_remains_distinct_from_session_unavailable() { let transport = WorkdirTransportError::from_workdir_error(&WorkdirError::Transport( diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index 22fb57de..d673b651 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -225,6 +225,9 @@ pub enum WorkdirError { #[error("Workdir session does not support {0:?}")] Unsupported(WorkdirSessionCapability), + #[error("Workdir operation is unsupported: {0}")] + UnsupportedOperation(String), + #[error("invalid Workdir path: {0}")] InvalidPath(String), diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 6bfd2100..610ea770 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -1721,17 +1721,8 @@ impl RuntimeHttpWorkdirError { impl From for RuntimeHttpWorkdirError { fn from(error: workdir::WorkdirError) -> Self { let payload = WorkdirTransportError::from_workdir_error(&error); - let status = match payload.code { - WorkdirTransportErrorCode::NotFound | WorkdirTransportErrorCode::UnknownCommand => { - StatusCode::NOT_FOUND - } - WorkdirTransportErrorCode::Conflict => StatusCode::CONFLICT, - WorkdirTransportErrorCode::Unsupported | WorkdirTransportErrorCode::InvalidRequest => { - StatusCode::BAD_REQUEST - } - WorkdirTransportErrorCode::Unavailable => StatusCode::SERVICE_UNAVAILABLE, - WorkdirTransportErrorCode::Internal => StatusCode::INTERNAL_SERVER_ERROR, - }; + let status = StatusCode::from_u16(payload.code.http_status()) + .expect("Workdir transport error status is valid"); Self { status, payload } } } diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index ffc7cbdc..46397ea0 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -195,11 +195,16 @@ impl WorkspaceAttachedWorkdirSession { .execute(request) .map_err(workspace_workdir_error)?; if !response.is_success() { - return Err(WorkdirError::Transport(format!( - "Workspace Workdir API returned HTTP {}: {}", - response.status, - bounded_error_body(&response.body) - ))); + return Err( + serde_json::from_str::(&response.body) + .map(workdir::http::WorkdirTransportError::into_workdir_error) + .unwrap_or_else(|_| { + WorkdirError::Transport(format!( + "Workspace Workdir operation failed with HTTP {}", + response.status + )) + }), + ); } serde_json::from_str(&response.body).map_err(|error| { WorkdirError::Transport(format!( @@ -795,6 +800,21 @@ mod tests { } } + fn error_response( + status: u16, + code: workdir::http::WorkdirTransportErrorCode, + message: &str, + ) -> WorkspaceResponse { + WorkspaceResponse { + status, + body: serde_json::to_string(&workdir::http::WorkdirTransportError { + code, + message: message.to_string(), + }) + .unwrap(), + } + } + fn workdir_json(id: &str) -> serde_json::Value { json!({ "working_directory_id": id, @@ -1175,6 +1195,48 @@ mod tests { assert_eq!(validation["delegations"].as_array().unwrap().len(), 1); } + #[tokio::test] + async fn attached_session_preserves_typed_provider_validation_error() { + let client = Arc::new(RecordingWorkspaceClient::new(vec![error_response( + 400, + workdir::http::WorkdirTransportErrorCode::InvalidRequest, + "Workdir operation request is invalid", + )])); + let session = WorkspaceAttachedWorkdirSession::handle(client); + + let error = session + .glob(workdir::GlobRequest { + pattern: "[".to_string(), + path: workdir::WorkdirPath::root(), + limit: 10, + }) + .await + .unwrap_err(); + + assert!(matches!(error, WorkdirError::InvalidArgument(_))); + } + + #[tokio::test] + async fn attached_session_does_not_expose_untyped_workspace_error_body() { + let client = Arc::new(RecordingWorkspaceClient::new(vec![WorkspaceResponse { + status: 502, + body: "secret token and /host/private/path".to_string(), + }])); + let session = WorkspaceAttachedWorkdirSession::handle(client); + + let error = session + .stat(StatRequest { + path: workdir::WorkdirPath::root(), + }) + .await + .unwrap_err(); + let message = error.to_string(); + assert!(matches!(error, WorkdirError::Transport(_))); + assert!(!message.contains("secret token")); + assert!(!message.contains("/host/private/path")); + assert!(message.contains("HTTP 502")); + } + #[tokio::test] async fn nested_attached_session_preserves_full_delegation_chain() { let client = Arc::new(RecordingWorkspaceClient::new(vec![ diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index ef2ec0ab..38b198c5 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -44,7 +44,9 @@ use webauthn_rs::prelude::{ PublicKeyCredential, RegisterPublicKeyCredential, RequestChallengeResponse, Webauthn, WebauthnBuilder, }; -use workdir::http::{WorkdirSessionOperation, WorkdirSessionOperationResult}; +use workdir::http::{ + WorkdirSessionOperation, WorkdirSessionOperationResult, WorkdirTransportError, +}; use workdir::workspace::{ MaterializerKind, WorkingDirectoryCleanupTarget, WorkingDirectoryDetailResponse as BrowserWorkingDirectoryDetailResponse, @@ -6932,12 +6934,56 @@ fn validated_current_worker_attachment( Ok(link) } +#[derive(Debug)] +enum WorkdirOperationApiError { + Api(ApiError), + Provider(WorkdirTransportError), +} + +impl From for WorkdirOperationApiError { + fn from(error: ApiError) -> Self { + Self::Api(error) + } +} + +impl From for WorkdirOperationApiError { + fn from(error: Error) -> Self { + Self::Api(error.into()) + } +} + +impl From for WorkdirOperationApiError { + fn from(error: WorkdirTransportError) -> Self { + Self::Provider(error) + } +} + +impl IntoResponse for WorkdirOperationApiError { + fn into_response(self) -> Response { + match self { + Self::Api(error) => error.into_response(), + Self::Provider(error) => { + let status = StatusCode::from_u16(error.code.http_status()) + .expect("Workdir transport error status is valid"); + let log = ApiErrorLog { + kind: format!("workdir_session_operation_{}", error.code.as_str()), + message: error.message.clone(), + diagnostics: Vec::new(), + }; + let mut response = (status, Json(error)).into_response(); + response.extensions_mut().insert(log); + response + } + } + } +} + async fn scoped_execute_current_worker_workdir_operation( State(api): State, AxumPath(path): AxumPath, headers: HeaderMap, Json(request): Json, -) -> ApiResult> { +) -> std::result::Result, WorkdirOperationApiError> { validate_workspace_scope(&api, &path.workspace_id)?; let worker = current_worker_identity(&api, &path.workspace_id, &headers)?; let expected_session_fence = request.expected_session_fence; @@ -7076,7 +7122,8 @@ async fn current_worker_command_session( external_handle: &CommandHandle, delegations: &[workdir::WorkdirDelegationRequest], expected_session_fence: Option<&str>, -) -> ApiResult<(workdir::AppliedWorkdirDelegation, CommandHandle)> { +) -> std::result::Result<(workdir::AppliedWorkdirDelegation, CommandHandle), WorkdirOperationApiError> +{ let _link = validated_current_worker_attachment(api, worker, expected_session_fence)?; let command = api .workdir_sessions @@ -7084,7 +7131,7 @@ async fn current_worker_command_session( .expect("Workdir session registry lock poisoned") .command(worker, external_handle) .ok_or_else(|| { - ApiError::from(current_worker_workdir_operation_error( + WorkdirOperationApiError::Provider(current_worker_workdir_operation_error( worker, workdir::WorkdirError::UnknownCommand(external_handle.0.clone()), )) @@ -7101,14 +7148,10 @@ async fn current_worker_command_session( } fn current_worker_workdir_operation_error( - worker: &RuntimeWorkerRef, + _worker: &RuntimeWorkerRef, error: workdir::WorkdirError, -) -> Error { - Error::RuntimeOperationFailed { - runtime_id: worker.runtime_id.clone(), - code: "workdir_session_operation_failed".to_string(), - message: error.to_string(), - } +) -> WorkdirTransportError { + WorkdirTransportError::from_workdir_error(&error) } async fn execute_workdir_session_operation( @@ -15737,6 +15780,107 @@ mod tests { assert_eq!(conflict.status(), StatusCode::CONFLICT); } + #[tokio::test] + async fn workdir_operation_errors_preserve_typed_public_status_and_code() { + for (code, expected_status) in [ + ( + workdir::http::WorkdirTransportErrorCode::InvalidRequest, + StatusCode::BAD_REQUEST, + ), + ( + workdir::http::WorkdirTransportErrorCode::NotFound, + StatusCode::NOT_FOUND, + ), + ( + workdir::http::WorkdirTransportErrorCode::UnknownCommand, + StatusCode::NOT_FOUND, + ), + ( + workdir::http::WorkdirTransportErrorCode::Conflict, + StatusCode::CONFLICT, + ), + ( + workdir::http::WorkdirTransportErrorCode::Unsupported, + StatusCode::BAD_REQUEST, + ), + ( + workdir::http::WorkdirTransportErrorCode::Unavailable, + StatusCode::SERVICE_UNAVAILABLE, + ), + ( + workdir::http::WorkdirTransportErrorCode::Internal, + StatusCode::INTERNAL_SERVER_ERROR, + ), + ] { + let response = WorkdirOperationApiError::Provider(WorkdirTransportError { + code, + message: "safe provider message".to_string(), + }) + .into_response(); + assert_eq!(response.status(), expected_status); + let log = response.extensions().get::().unwrap(); + assert_eq!( + log.kind, + format!("workdir_session_operation_{}", code.as_str()) + ); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + let decoded: WorkdirTransportError = serde_json::from_slice(&body).unwrap(); + assert_eq!(decoded.code, code); + assert_eq!(decoded.message, "safe provider message"); + } + } + + #[tokio::test] + async fn local_and_remote_validation_errors_share_workspace_classification() { + let worker = RuntimeWorkerRef::new("runtime", "worker"); + let local = current_worker_workdir_operation_error( + &worker, + workdir::WorkdirError::InvalidGlob("[".to_string()), + ); + let remote = current_worker_workdir_operation_error( + &worker, + WorkdirTransportError { + code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, + message: "Workdir operation request is invalid".to_string(), + } + .into_workdir_error(), + ); + assert_eq!(local, remote); + + for public in [local, remote] { + let response = WorkdirOperationApiError::Provider(public).into_response(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + let decoded: WorkdirTransportError = serde_json::from_slice(&body).unwrap(); + assert_eq!( + decoded.code, + workdir::http::WorkdirTransportErrorCode::InvalidRequest + ); + } + } + + #[tokio::test] + async fn workdir_operation_error_response_redacts_provider_internal_details() { + let error = workdir::WorkdirError::Io { + path: PathBuf::from("/host/private/worktree/secret.txt"), + source: std::io::Error::new(std::io::ErrorKind::PermissionDenied, "token=secret"), + }; + let public = current_worker_workdir_operation_error( + &RuntimeWorkerRef::new("runtime", "worker"), + error, + ); + let response = WorkdirOperationApiError::Provider(public).into_response(); + let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + let text = String::from_utf8(body.to_vec()).unwrap(); + assert!(!text.contains("/host/private")); + assert!(!text.contains("token=secret")); + let decoded: WorkdirTransportError = serde_json::from_str(&text).unwrap(); + assert_eq!( + decoded.code, + workdir::http::WorkdirTransportErrorCode::Internal + ); + } + #[test] fn backend_worker_projection_preserves_missing_rows_links_and_redacts_paths() { let worker = WorkerRegistryRecord {