From b06aca19a3ac194d0970d3381d2233385c283dbf Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Thu, 22 Jan 2026 21:58:02 +0530 Subject: [PATCH 1/2] Standardize error handling with error-stack Unified error reporting across storage and signing modules allows for better debugging and traceability. Bare error enums previously lacked context on where failures originated. Using meaningful reports with attached context ensures that logs provide actionable information for operations teams without requiring changes to the core error variants. Resolves: #191 --- CONTRIBUTING.md | 16 +++ crates/common/src/fastly_storage.rs | 116 ++++++++++-------- crates/common/src/request_signing/jwks.rs | 16 ++- crates/common/src/request_signing/rotation.rs | 67 +++++----- crates/common/src/request_signing/signing.rs | 110 +++++++++-------- 5 files changed, 191 insertions(+), 134 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 46536a73..580f73e9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -118,6 +118,22 @@ Consistency is the most important. Following the existing Rust style, formatting Style and format will be enforced with a linter when the PR is created. +## :warning: Error Handling + +We use [error-stack](https://docs.rs/error-stack/latest/error_stack/) for error handling to provide rich context and traceability. + +### Guidelines + +1. **Use `Report`**: Public functions should generally return `Result>`. +2. **Context**: Use `.change_context(TrustedServerError::Variant)` to wrap errors and provide semantic meaning. + ```rust + // Good + file.read_to_string(&mut content) + .change_context(TrustedServerError::Configuration { message: "Failed to read config".into() })?; + ``` +3. **Attachments**: Use `.attach_printable("additional info")` to add debugging context without changing the error variant. +4. **Consistency**: Avoid returning bare `TrustedServerError` unless absolutely necessary (e.g. implementing traits). Wrap them in `Report::new()`. + ## :pray: Credits - https://github.com/jessesquires/.github/blob/main/CONTRIBUTING.md diff --git a/crates/common/src/fastly_storage.rs b/crates/common/src/fastly_storage.rs index c4a8241f..9b9f9923 100644 --- a/crates/common/src/fastly_storage.rs +++ b/crates/common/src/fastly_storage.rs @@ -1,5 +1,6 @@ use std::io::Read; +use error_stack::{Report, ResultExt}; use fastly::{ConfigStore, Request, Response, SecretStore}; use http::StatusCode; @@ -17,17 +18,17 @@ impl FastlyConfigStore { } } - pub fn get(&self, key: &str) -> Result { + pub fn get(&self, key: &str) -> Result> { // TODO use try_open and return the error let store = ConfigStore::open(&self.store_name); - store - .get(key) - .ok_or_else(|| TrustedServerError::Configuration { + store.get(key).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { message: format!( "Key '{}' not found in config store '{}'", key, self.store_name ), }) + }) } } @@ -42,33 +43,36 @@ impl FastlySecretStore { } } - pub fn get(&self, key: &str) -> Result, TrustedServerError> { - let store = - SecretStore::open(&self.store_name).map_err(|_| TrustedServerError::Configuration { + pub fn get(&self, key: &str) -> Result, Report> { + let store = SecretStore::open(&self.store_name).map_err(|_| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to open SecretStore '{}'", self.store_name), - })?; + }) + })?; - let secret = store - .get(key) - .ok_or_else(|| TrustedServerError::Configuration { + let secret = store.get(key).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { message: format!( "Secret '{}' not found in secret store '{}'", key, self.store_name ), - })?; + }) + })?; secret .try_plaintext() - .map_err(|_| TrustedServerError::Configuration { - message: "Failed to get secret plaintext".into(), + .map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Failed to get secret plaintext".into(), + }) }) .map(|bytes| bytes.into_iter().collect()) } - pub fn get_string(&self, key: &str) -> Result { + pub fn get_string(&self, key: &str) -> Result> { let bytes = self.get(key)?; - String::from_utf8(bytes).map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode secret as UTF-8: {}", e), + String::from_utf8(bytes).change_context(TrustedServerError::Configuration { + message: format!("Failed to decode secret as UTF-8: {}", key), }) } } @@ -79,16 +83,19 @@ pub struct FastlyApiClient { } impl FastlyApiClient { - pub fn new() -> Result { + pub fn new() -> Result> { Self::from_secret_store("api-keys", "api_key") } - pub fn from_secret_store(store_name: &str, key_name: &str) -> Result { - ensure_backend_from_url("https://api.fastly.com").map_err(|e| { + pub fn from_secret_store( + store_name: &str, + key_name: &str, + ) -> Result> { + ensure_backend_from_url("https://api.fastly.com").change_context( TrustedServerError::Configuration { - message: format!("Failed to ensure API backend: {}", e), - } - })?; + message: "Failed to ensure API backend".to_string(), + }, + )?; let secret_store = FastlySecretStore::new(store_name); let api_key = secret_store.get(key_name)?; @@ -105,7 +112,7 @@ impl FastlyApiClient { path: &str, body: Option, content_type: &str, - ) -> Result { + ) -> Result> { let url = format!("{}{}", self.base_url, path); let api_key_str = String::from_utf8_lossy(&self.api_key).to_string(); @@ -116,9 +123,9 @@ impl FastlyApiClient { "PUT" => Request::put(&url), "DELETE" => Request::delete(&url), _ => { - return Err(TrustedServerError::Configuration { + return Err(Report::new(TrustedServerError::Configuration { message: format!("Unsupported HTTP method: {}", method), - }) + })) } }; @@ -133,9 +140,9 @@ impl FastlyApiClient { } request.send("backend_https_api_fastly_com").map_err(|e| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: format!("Failed to send API request: {}", e), - } + }) }) } @@ -144,7 +151,7 @@ impl FastlyApiClient { store_id: &str, key: &str, value: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!("/resources/stores/config/{}/item/{}", store_id, key); let payload = format!("item_value={}", value); @@ -159,20 +166,22 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to update config item: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } @@ -181,7 +190,7 @@ impl FastlyApiClient { store_id: &str, secret_name: &str, secret_value: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!("/resources/stores/secret/{}/secrets", store_id); let payload = serde_json::json!({ @@ -196,24 +205,30 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to create secret: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } - pub fn delete_config_item(&self, store_id: &str, key: &str) -> Result<(), TrustedServerError> { + pub fn delete_config_item( + &self, + store_id: &str, + key: &str, + ) -> Result<(), Report> { let path = format!("/resources/stores/config/{}/item/{}", store_id, key); let mut response = self.make_request("DELETE", &path, None, "application/json")?; @@ -222,8 +237,10 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK @@ -231,13 +248,13 @@ impl FastlyApiClient { { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to delete config item: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } @@ -245,7 +262,7 @@ impl FastlyApiClient { &self, store_id: &str, secret_name: &str, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let path = format!( "/resources/stores/secret/{}/secrets/{}", store_id, secret_name @@ -257,8 +274,10 @@ impl FastlyApiClient { response .get_body_mut() .read_to_string(&mut buf) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to read API response: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to read API response: {}", e), + }) })?; if response.get_status() == StatusCode::OK @@ -266,13 +285,13 @@ impl FastlyApiClient { { Ok(()) } else { - Err(TrustedServerError::Configuration { + Err(Report::new(TrustedServerError::Configuration { message: format!( "Failed to delete secret: HTTP {} - {}", response.get_status(), buf ), - }) + })) } } } @@ -329,6 +348,7 @@ mod tests { } } + // Other tests logic is preserved, prints error which is now a Report #[test] fn test_update_config_item() { let result = FastlyApiClient::new(); diff --git a/crates/common/src/request_signing/jwks.rs b/crates/common/src/request_signing/jwks.rs index abbf20cd..1c260f78 100644 --- a/crates/common/src/request_signing/jwks.rs +++ b/crates/common/src/request_signing/jwks.rs @@ -4,6 +4,7 @@ //! Ed25519 keypairs in JWK format for request signing. use ed25519_dalek::{SigningKey, VerifyingKey}; +use error_stack::{Report, ResultExt}; use jose_jwk::{ jose_jwa::{Algorithm, Signing}, Jwk, Key, Okp, OkpCurves, Parameters, @@ -51,9 +52,14 @@ impl Keypair { } } -pub fn get_active_jwks() -> Result { +pub fn get_active_jwks() -> Result> { let store = FastlyConfigStore::new("jwks_store"); - let active_kids_str = store.get("active-kids")?; + let active_kids_str = + store + .get("active-kids") + .change_context(TrustedServerError::Configuration { + message: "Failed to get active-kids".into(), + })?; let active_kids: Vec<&str> = active_kids_str .split(',') @@ -63,7 +69,11 @@ pub fn get_active_jwks() -> Result { let mut jwks = Vec::new(); for kid in active_kids { - let jwk = store.get(kid)?; + let jwk = store + .get(kid) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get JWK for kid: {}", kid), + })?; jwks.push(jwk); } diff --git a/crates/common/src/request_signing/rotation.rs b/crates/common/src/request_signing/rotation.rs index fd77dcaf..0e20a61b 100644 --- a/crates/common/src/request_signing/rotation.rs +++ b/crates/common/src/request_signing/rotation.rs @@ -5,6 +5,7 @@ use base64::{engine::general_purpose, Engine}; use ed25519_dalek::SigningKey; +use error_stack::{Report, ResultExt}; use jose_jwk::Jwk; use crate::error::TrustedServerError; @@ -31,7 +32,7 @@ impl KeyRotationManager { pub fn new( config_store_id: impl Into, secret_store_id: impl Into, - ) -> Result { + ) -> Result> { let config_store_id = config_store_id.into(); let secret_store_id = secret_store_id.into(); @@ -46,7 +47,10 @@ impl KeyRotationManager { }) } - pub fn rotate_key(&self, kid: Option) -> Result { + pub fn rotate_key( + &self, + kid: Option, + ) -> Result> { let new_kid = kid.unwrap_or_else(generate_date_based_kid); let keypair = Keypair::generate(); @@ -76,49 +80,50 @@ impl KeyRotationManager { &self, kid: &str, signing_key: &SigningKey, - ) -> Result<(), TrustedServerError> { + ) -> Result<(), Report> { let key_bytes = signing_key.as_bytes(); let key_b64 = general_purpose::STANDARD.encode(key_bytes); self.api_client .create_secret(&self.secret_store_id, kid, &key_b64) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to store private key '{}': {}", kid, e), + .change_context(TrustedServerError::Configuration { + message: format!("Failed to store private key '{}'", kid), }) } - fn store_public_jwk(&self, kid: &str, jwk: &Jwk) -> Result<(), TrustedServerError> { - let jwk_json = - serde_json::to_string(jwk).map_err(|e| TrustedServerError::Configuration { + fn store_public_jwk(&self, kid: &str, jwk: &Jwk) -> Result<(), Report> { + let jwk_json = serde_json::to_string(jwk).map_err(|e| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to serialize JWK: {}", e), - })?; + }) + })?; self.api_client .update_config_item(&self.config_store_id, kid, &jwk_json) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to store public JWK '{}': {}", kid, e), + .change_context(TrustedServerError::Configuration { + message: format!("Failed to store public JWK '{}'", kid), }) } - fn update_current_kid(&self, kid: &str) -> Result<(), TrustedServerError> { + fn update_current_kid(&self, kid: &str) -> Result<(), Report> { self.api_client .update_config_item(&self.config_store_id, "current-kid", kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to update current-kid: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to update current-kid".into(), }) } - fn update_active_kids(&self, active_kids: &[String]) -> Result<(), TrustedServerError> { + fn update_active_kids(&self, active_kids: &[String]) -> Result<(), Report> { let active_kids_str = active_kids.join(","); self.api_client .update_config_item(&self.config_store_id, "active-kids", &active_kids_str) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to update active-kids: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to update active-kids".into(), }) } - pub fn list_active_keys(&self) -> Result, TrustedServerError> { + pub fn list_active_keys(&self) -> Result, Report> { let active_kids_str = self.config_store.get("active-kids")?; let active_kids: Vec = active_kids_str @@ -130,33 +135,33 @@ impl KeyRotationManager { Ok(active_kids) } - pub fn deactivate_key(&self, kid: &str) -> Result<(), TrustedServerError> { + pub fn deactivate_key(&self, kid: &str) -> Result<(), Report> { let mut active_kids = self.list_active_keys()?; active_kids.retain(|k| k != kid); if active_kids.is_empty() { - return Err(TrustedServerError::Configuration { + return Err(Report::new(TrustedServerError::Configuration { message: "Cannot deactivate the last active key".into(), - }); + })); } self.update_active_kids(&active_kids) } - pub fn delete_key(&self, kid: &str) -> Result<(), TrustedServerError> { + pub fn delete_key(&self, kid: &str) -> Result<(), Report> { self.deactivate_key(kid)?; self.api_client .delete_config_item(&self.config_store_id, kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to delete JWK from ConfigStore: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to delete JWK from ConfigStore".into(), })?; self.api_client .delete_secret(&self.secret_store_id, kid) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to delete secret from SecretStore: {}", e), + .change_context(TrustedServerError::Configuration { + message: "Failed to delete secret from SecretStore".into(), })?; Ok(()) @@ -177,15 +182,11 @@ mod tests { #[test] fn test_generate_date_based_kid() { let kid = generate_date_based_kid(); - println!("Generated KID: {}", kid); - // Verify format: ts-YYYY-MM-DD assert!(kid.starts_with("ts-")); - assert!(kid.len() >= 13); // "ts-" + "YYYY-MM-DD" = 13 chars minimum - - // Verify it contains only valid characters + assert!(kid.len() >= 13); let parts: Vec<&str> = kid.split('-').collect(); - assert_eq!(parts.len(), 4); // ["ts", "YYYY", "MM", "DD"] + assert_eq!(parts.len(), 4); assert_eq!(parts[0], "ts"); } @@ -196,7 +197,6 @@ mod tests { Ok(manager) => { assert_eq!(manager.config_store_id, "jwks_store"); assert_eq!(manager.secret_store_id, "signing_keys"); - println!("✓ KeyRotationManager created successfully"); } Err(e) => { println!("Expected error in test environment: {}", e); @@ -210,7 +210,6 @@ mod tests { if let Ok(manager) = result { match manager.list_active_keys() { Ok(keys) => { - println!("Active keys: {:?}", keys); assert!(!keys.is_empty(), "Should have at least one active key"); } Err(e) => println!("Expected error in test environment: {}", e), diff --git a/crates/common/src/request_signing/signing.rs b/crates/common/src/request_signing/signing.rs index 6420a120..e12d0007 100644 --- a/crates/common/src/request_signing/signing.rs +++ b/crates/common/src/request_signing/signing.rs @@ -5,31 +5,32 @@ use base64::{engine::general_purpose, Engine}; use ed25519_dalek::{Signature, Signer as Ed25519Signer, SigningKey, Verifier, VerifyingKey}; +use error_stack::{Report, ResultExt}; use crate::error::TrustedServerError; use crate::fastly_storage::{FastlyConfigStore, FastlySecretStore}; -pub fn get_current_key_id() -> Result { +pub fn get_current_key_id() -> Result> { let store = FastlyConfigStore::new("jwks_store"); store.get("current-kid") } -fn parse_ed25519_signing_key(key_bytes: Vec) -> Result { +fn parse_ed25519_signing_key(key_bytes: Vec) -> Result> { let bytes = if key_bytes.len() > 32 { general_purpose::STANDARD.decode(&key_bytes).map_err(|_| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: "Failed to decode base64 key".into(), - } + }) })? } else { key_bytes }; - let key_array: [u8; 32] = bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { + let key_array: [u8; 32] = bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { message: "Invalid key length (expected 32 bytes for Ed25519)".into(), - })?; + }) + })?; Ok(SigningKey::from_bytes(&key_array)) } @@ -40,12 +41,22 @@ pub struct RequestSigner { } impl RequestSigner { - pub fn from_config() -> Result { + pub fn from_config() -> Result> { let config_store = FastlyConfigStore::new("jwks_store"); - let key_id = config_store.get("current-kid")?; + let key_id = + config_store + .get("current-kid") + .change_context(TrustedServerError::Configuration { + message: "Failed to get current-kid".into(), + })?; let secret_store = FastlySecretStore::new("signing_keys"); - let key_bytes = secret_store.get(&key_id)?; + let key_bytes = + secret_store + .get(&key_id) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get signing key for kid: {}", key_id), + })?; let signing_key = parse_ed25519_signing_key(key_bytes)?; Ok(Self { @@ -54,7 +65,7 @@ impl RequestSigner { }) } - pub fn sign(&self, payload: &[u8]) -> Result { + pub fn sign(&self, payload: &[u8]) -> Result> { let signature_bytes = self.key.sign(payload).to_bytes(); Ok(general_purpose::URL_SAFE_NO_PAD.encode(signature_bytes)) @@ -65,54 +76,60 @@ pub fn verify_signature( payload: &[u8], signature_b64: &str, kid: &str, -) -> Result { +) -> Result> { let store = FastlyConfigStore::new("jwks_store"); - let jwk_json = store.get(kid)?; + let jwk_json = store + .get(kid) + .change_context(TrustedServerError::Configuration { + message: format!("Failed to get JWK for kid: {}", kid), + })?; - let jwk: serde_json::Value = - serde_json::from_str(&jwk_json).map_err(|e| TrustedServerError::Configuration { + let jwk: serde_json::Value = serde_json::from_str(&jwk_json).map_err(|e| { + Report::new(TrustedServerError::Configuration { message: format!("Failed to parse JWK: {}", e), - })?; + }) + })?; - let x_b64 = - jwk.get("x") - .and_then(|v| v.as_str()) - .ok_or_else(|| TrustedServerError::Configuration { - message: "JWK missing 'x' parameter".into(), - })?; + let x_b64 = jwk.get("x").and_then(|v| v.as_str()).ok_or_else(|| { + Report::new(TrustedServerError::Configuration { + message: "JWK missing 'x' parameter".into(), + }) + })?; let public_key_bytes = general_purpose::URL_SAFE_NO_PAD .decode(x_b64) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode public key: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to decode public key: {}", e), + }) })?; - let verifying_key_bytes: [u8; 32] = - public_key_bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { - message: "Public key must be 32 bytes".into(), - })?; + let verifying_key_bytes: [u8; 32] = public_key_bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Public key must be 32 bytes".into(), + }) + })?; let verifying_key = VerifyingKey::from_bytes(&verifying_key_bytes).map_err(|e| { - TrustedServerError::Configuration { + Report::new(TrustedServerError::Configuration { message: format!("Failed to create verifying key: {}", e), - } + }) })?; let signature_bytes = general_purpose::URL_SAFE_NO_PAD .decode(signature_b64) .or_else(|_| general_purpose::STANDARD.decode(signature_b64)) - .map_err(|e| TrustedServerError::Configuration { - message: format!("Failed to decode signature: {}", e), + .map_err(|e| { + Report::new(TrustedServerError::Configuration { + message: format!("Failed to decode signature: {}", e), + }) })?; - let signature_array: [u8; 64] = - signature_bytes - .try_into() - .map_err(|_| TrustedServerError::Configuration { - message: "Signature must be 64 bytes".into(), - })?; + let signature_array: [u8; 64] = signature_bytes.try_into().map_err(|_| { + Report::new(TrustedServerError::Configuration { + message: "Signature must be 64 bytes".into(), + }) + })?; let signature = Signature::from_bytes(&signature_array); @@ -125,18 +142,19 @@ mod tests { #[test] fn test_request_signer_sign() { + // We propagate errors in tests too, or unwrap. + // Note: unwrapping a Report prints it nicely if test fails. let signer = RequestSigner::from_config().unwrap(); let signature = signer .sign(b"these pretzels are making me thirsty") .unwrap(); assert!(!signature.is_empty()); - assert!(signature.len() > 32); // Ed25519 signatures are 64 bytes, base64 encoded should be longer + assert!(signature.len() > 32); } #[test] fn test_request_signer_from_config() { let signer = RequestSigner::from_config().unwrap(); - // Verify that we can successfully load the signing key from Fastly config assert!(!signer.kid.is_empty()); } @@ -146,7 +164,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let signature = signer.sign(payload).unwrap(); - // Verify the signature let result = verify_signature(payload, &signature, &signer.kid).unwrap(); assert!(result, "Signature should be valid"); } @@ -156,10 +173,8 @@ mod tests { let payload = b"test payload"; let signer = RequestSigner::from_config().unwrap(); - // Create a valid Ed25519 signature (64 bytes) but for a different payload let wrong_signature = signer.sign(b"different payload").unwrap(); - // Should return false for signature of different payload let result = verify_signature(payload, &wrong_signature, &signer.kid).unwrap(); assert!(!result, "Invalid signature should not verify"); } @@ -170,7 +185,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let signature = signer.sign(original_payload).unwrap(); - // Try to verify with different payload let wrong_payload = b"wrong payload"; let result = verify_signature(wrong_payload, &signature, &signer.kid).unwrap(); assert!(!result, "Signature should not verify with wrong payload"); @@ -183,7 +197,6 @@ mod tests { let signature = signer.sign(payload).unwrap(); let nonexistent_kid = "nonexistent-key-id"; - // Should return an error for missing key let result = verify_signature(payload, &signature, nonexistent_kid); assert!(result.is_err(), "Should error for missing key"); } @@ -194,7 +207,6 @@ mod tests { let signer = RequestSigner::from_config().unwrap(); let malformed_signature = "not-valid-base64!!!"; - // Should return an error for malformed base64 let result = verify_signature(payload, malformed_signature, &signer.kid); assert!(result.is_err(), "Should error for malformed signature"); } From cb3f36a7287a49127b3f48f75e356a136d574382 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 27 Jan 2026 11:43:01 +0530 Subject: [PATCH 2/2] Refactor error handling to avoid double wrapping Using on errors that are already of the correct type causes redundant nesting in the error report. This switches to to add context without changing the error type, producing cleaner error chains. Resolves: #191 --- .agents/rules/rust-error-handling.mdc | 2 +- CONTRIBUTING.md | 2 +- crates/common/src/fastly_storage.rs | 8 ++------ crates/common/src/request_signing/jwks.rs | 13 ++++--------- crates/common/src/request_signing/signing.rs | 11 ++++------- 5 files changed, 12 insertions(+), 24 deletions(-) diff --git a/.agents/rules/rust-error-handling.mdc b/.agents/rules/rust-error-handling.mdc index e1e2a913..007ba6e5 100644 --- a/.agents/rules/rust-error-handling.mdc +++ b/.agents/rules/rust-error-handling.mdc @@ -19,7 +19,7 @@ alwaysApply: false - Use `change_context()` to map error types consistently - Ensure that errors include sufficient context for debugging -- Add `attach_printable()` or `attach_printable_lazy()` to include relevant debug information +- Add `attach()` or `attach_with()` to include relevant debug information - When reporting errors to users, provide actionable guidance where possible ## Error Definition diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 580f73e9..4888c74e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -116,7 +116,7 @@ See also: #456, #789 Consistency is the most important. Following the existing Rust style, formatting, and naming conventions of the file you are modifying and of the overall project. Failure to do so will result in a prolonged review process that has to focus on updating the superficial aspects of your code, rather than improving its functionality and performance. -Style and format will be enforced with a linter when the PR is created. +Style and format will be enforced with a linter when PR is created. ## :warning: Error Handling diff --git a/crates/common/src/fastly_storage.rs b/crates/common/src/fastly_storage.rs index 9b9f9923..9f756151 100644 --- a/crates/common/src/fastly_storage.rs +++ b/crates/common/src/fastly_storage.rs @@ -72,7 +72,7 @@ impl FastlySecretStore { pub fn get_string(&self, key: &str) -> Result> { let bytes = self.get(key)?; String::from_utf8(bytes).change_context(TrustedServerError::Configuration { - message: format!("Failed to decode secret as UTF-8: {}", key), + message: "Failed to decode secret as UTF-8".to_string(), }) } } @@ -91,11 +91,7 @@ impl FastlyApiClient { store_name: &str, key_name: &str, ) -> Result> { - ensure_backend_from_url("https://api.fastly.com").change_context( - TrustedServerError::Configuration { - message: "Failed to ensure API backend".to_string(), - }, - )?; + ensure_backend_from_url("https://api.fastly.com")?; let secret_store = FastlySecretStore::new(store_name); let api_key = secret_store.get(key_name)?; diff --git a/crates/common/src/request_signing/jwks.rs b/crates/common/src/request_signing/jwks.rs index 1c260f78..7678a41a 100644 --- a/crates/common/src/request_signing/jwks.rs +++ b/crates/common/src/request_signing/jwks.rs @@ -54,12 +54,9 @@ impl Keypair { pub fn get_active_jwks() -> Result> { let store = FastlyConfigStore::new("jwks_store"); - let active_kids_str = - store - .get("active-kids") - .change_context(TrustedServerError::Configuration { - message: "Failed to get active-kids".into(), - })?; + let active_kids_str = store + .get("active-kids") + .attach("while fetching active kids list")?; let active_kids: Vec<&str> = active_kids_str .split(',') @@ -71,9 +68,7 @@ pub fn get_active_jwks() -> Result> { for kid in active_kids { let jwk = store .get(kid) - .change_context(TrustedServerError::Configuration { - message: format!("Failed to get JWK for kid: {}", kid), - })?; + .attach(format!("Failed to get JWK for kid: {}", kid))?; jwks.push(jwk); } diff --git a/crates/common/src/request_signing/signing.rs b/crates/common/src/request_signing/signing.rs index e12d0007..eaf85818 100644 --- a/crates/common/src/request_signing/signing.rs +++ b/crates/common/src/request_signing/signing.rs @@ -51,12 +51,9 @@ impl RequestSigner { })?; let secret_store = FastlySecretStore::new("signing_keys"); - let key_bytes = - secret_store - .get(&key_id) - .change_context(TrustedServerError::Configuration { - message: format!("Failed to get signing key for kid: {}", key_id), - })?; + let key_bytes = secret_store + .get(&key_id) + .attach(format!("Failed to get signing key for kid: {}", key_id))?; let signing_key = parse_ed25519_signing_key(key_bytes)?; Ok(Self { @@ -142,7 +139,7 @@ mod tests { #[test] fn test_request_signer_sign() { - // We propagate errors in tests too, or unwrap. + // Report unwraps print full error chain on test failure // Note: unwrapping a Report prints it nicely if test fails. let signer = RequestSigner::from_config().unwrap(); let signature = signer