fix(router): better handling of internal and pipeline errors (#1387)

Author: dotansimha

.changeset/align_variable_coercion_errors_with_graphql_js.md

@@ -0,0 +1,10 @@
+---
+hive-router-plan-executor: patch
+hive-router: patch
+---
+
+# Improve variable coercion error messages
+
+Variable coercion errors (invalid scalar/enum/object values, missing required fields, non-null violations) reports clear and informative error messages.
+
+This only changes error text - error codes and HTTP status codes are unchanged.

.changeset/mask_internal_pipeline_errors.md

@@ -0,0 +1,11 @@
+---
+hive-router: patch
+---
+
+# Mask internal error details from client responses
+
+Improve router's error handling by masking internal error details from client responses.
+
+Client-caused errors still return their real message, since it only ever reflects the client's own request. Internal errors now always return a generic `"Internal server error"` message and never the underlying error message, which previously leaked details such as subgraph URLs, storage/network errors, and other backend internals. The real error is still logged for debugging purposes.
+
+Error codes are unchanged. HTTP status codes are unchanged, except for GraphQL operation normalization and minification failures, which are now correctly treated as router-side bugs and always return `500` (previously `400`, or `200` based on `Accept` header) instead of being treated as a client mistake.

bin/router/src/pipeline/authorization/mod.rs

@@ -12,7 +12,7 @@ pub mod metadata;
 
 use std::sync::Arc;
 
-use crate::pipeline::error::PipelineError;
+use crate::pipeline::error::{ClientPipelineError, PipelineError};
 use crate::pipeline::normalize::GraphQLNormalizationPayload;
 use crate::pipeline::nullify::rebuilder::{
     rebuild_nulled_operation, rebuild_nulled_projection_plan,
@@ -195,7 +195,7 @@ pub fn enforce_operation_authorization(
             errors,
         ),
         AuthorizationDecision::Reject { errors } => {
-            return Err(PipelineError::AuthorizationFailed(errors));
+            return Err(ClientPipelineError::AuthorizationFailed(errors).into());
         }
     })
 }

bin/router/src/pipeline/coerce_variables.rs

@@ -8,7 +8,7 @@ use hive_router_plan_executor::variables::collect_variables;
 use sonic_rs::Value;
 use tracing::{debug, warn};
 
-use crate::pipeline::error::PipelineError;
+use crate::pipeline::error::{ClientPipelineError, PipelineError};
 use crate::pipeline::normalize::GraphQLNormalizationPayload;
 
 #[inline]
@@ -35,13 +35,13 @@ pub fn coerce_request_variables(
                 variables_map: values,
             })
         }
-        Err(err_msg) => {
+        Err(err) => {
             warn!(
                 target: targets::COERCE_VARIABLES,
-                error = ?err_msg,
+                error = %err,
                 "failed to collect variables from incoming request",
             );
-            Err(PipelineError::VariablesCoercionError(err_msg))
+            Err(ClientPipelineError::VariablesCoercionError(err).into())
         }
     }
 }

bin/router/src/pipeline/csrf_prevention.rs

@@ -3,7 +3,7 @@ use hive_router_internal::telemetry::logging::targets;
 use ntex::web::HttpRequest;
 use tracing::warn;
 
-use crate::pipeline::error::PipelineError;
+use crate::pipeline::error::{ClientPipelineError, PipelineError};
 
 // NON_PREFLIGHTED_CONTENT_TYPES are content types that do not require a preflight
 // OPTIONS request. These are content types that are considered "simple" by the CORS
@@ -43,7 +43,7 @@ pub fn perform_csrf_prevention(
     } else {
         warn!(target: targets::HTTP_SERVER, "csrf failed: missing required header");
 
-        Err(PipelineError::CsrfPreventionFailed)
+        Err(ClientPipelineError::CsrfPreventionFailed.into())
     }
 }
 

bin/router/src/pipeline/demand_control/formula.rs

@@ -21,7 +21,7 @@ use tracing::warn;
 
 use std::{collections::BTreeMap, fmt, sync::Arc};
 
-use crate::pipeline::error::PipelineError;
+use crate::pipeline::error::{ClientPipelineError, PipelineError};
 
 /// Size overrides threaded from a parent @listSize(sizedFields:[…]) down to children.
 /// Each entry is (remaining path from the current selection, size expression).
@@ -305,10 +305,11 @@ fn eval_cost_expr(
                         "rejecting operation: expected exactly one slicing argument for @listSize"
                     );
 
-                    Err(PipelineError::CostInvalidSlicingArguments {
+                    Err(ClientPipelineError::CostInvalidSlicingArguments {
                         field_name: field_name.clone(),
                         found: resolved_count,
-                    })
+                    }
+                    .into())
                 }
             } else {
                 let mut max_value: Option<u64> = None;

bin/router/src/pipeline/demand_control/runtime.rs

@@ -25,7 +25,7 @@ use http::{HeaderName, HeaderValue};
 use moka::future::Cache;
 use tracing::{debug, info, warn};
 
-use crate::pipeline::error::PipelineError;
+use crate::pipeline::error::{ClientPipelineError, PipelineError};
 
 use super::formula::{
     compile_cost_expr_for_operation, evaluate_formula_plan, DemandControlFormulaPlan,
@@ -163,9 +163,10 @@ impl DemandControlRuntime {
                             .push((header_name.get_header_ref().to_owned(), max_cost.into()));
                     }
 
-                    return Err(PipelineError::CostEstimatedTooExpensive {
+                    return Err(ClientPipelineError::CostEstimatedTooExpensive {
                         response_headers: err_extra_headers,
-                    });
+                    }
+                    .into());
                 }
                 DemandControlMode::Measure => {
                     warn!(