diff --git a/.changesets/fix_task_crust_preacher_toothpaste.md b/.changesets/fix_task_crust_preacher_toothpaste.md index 7c8395b4f7..6de105b49c 100644 --- a/.changesets/fix_task_crust_preacher_toothpaste.md +++ b/.changesets/fix_task_crust_preacher_toothpaste.md @@ -1,4 +1,4 @@ -### Fix Router's validation of ObjectValue variables ([PR #8821](https://github.com/apollographql/router/pull/8821)) +### Fix Router's validation of `ObjectValue` variables ([PR #8821](https://github.com/apollographql/router/pull/8821) and [PR #8884](https://github.com/apollographql/router/pull/8884)) This change addresses an issue in Router whereby invalid additional fields of an input object were able to pass variable validation because the fields of the object were not being properly checked. @@ -34,6 +34,19 @@ query($msg: MessageInput) { ``` This request would pass validation because the variable `msg` from the query was present in the input, however, the fields of `msg` from the input were not being validated against the `MessageInput` type. -[ROUTER-981]: https://apollographql.atlassian.net/browse/ROUTER-981?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ +> [!WARNING] +> If you need to opt out, you must set the `supergraph.strict_variable_validation` config option to `measure` instead. -By [@conwuegb](https://github.com/conwuegb) in https://github.com/apollographql/router/pull/8821 \ No newline at end of file +Enabled: +```yaml +supergraph: + strict_variable_validation: enforce +``` + +Disabled: +```yaml +supergraph: + strict_variable_validation: measure +``` + +By [@conwuegb](https://github.com/conwuegb) in https://github.com/apollographql/router/pull/8821 and https://github.com/apollographql/router/pull/8884 diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index ab126e4d9b..836b64c5de 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -49,6 +49,7 @@ use self::subgraph::SubgraphConfiguration; use crate::ApolloRouterError; use crate::cache::DEFAULT_CACHE_CAPACITY; use crate::configuration::cooperative_cancellation::CooperativeCancellation; +use crate::configuration::mode::Mode; use crate::graphql; use crate::plugin::plugins; use crate::plugins::chaos; @@ -743,6 +744,11 @@ pub(crate) struct Supergraph { /// Log a message if the client closes the connection before the response is sent. /// Default: false. pub(crate) experimental_log_on_broken_pipe: bool, + + /// Determines how to handle queries which include additional fields of an input object. + /// - `enforce` (default): rejects query + /// - `measure`: permits query and the logs unknown fields + pub(crate) strict_variable_validation: Mode, } const fn default_generate_query_fragments() -> bool { @@ -767,6 +773,7 @@ impl Supergraph { early_cancel: Option, experimental_log_on_broken_pipe: Option, insert_result_coercion_errors: Option, + strict_variable_validation: Option, ) -> Self { Self { listen: listen.unwrap_or_else(default_graphql_listen), @@ -781,6 +788,8 @@ impl Supergraph { early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), + strict_variable_validation: strict_variable_validation + .unwrap_or_else(default_strict_variable_validation), } } } @@ -800,6 +809,7 @@ impl Supergraph { early_cancel: Option, experimental_log_on_broken_pipe: Option, insert_result_coercion_errors: Option, + strict_variable_validation: Option, ) -> Self { Self { listen: listen.unwrap_or_else(test_listen), @@ -814,6 +824,8 @@ impl Supergraph { early_cancel: early_cancel.unwrap_or_default(), experimental_log_on_broken_pipe: experimental_log_on_broken_pipe.unwrap_or_default(), enable_result_coercion_errors: insert_result_coercion_errors.unwrap_or_default(), + strict_variable_validation: strict_variable_validation + .unwrap_or_else(default_strict_variable_validation), } } } @@ -1506,6 +1518,10 @@ fn default_connection_shutdown_timeout() -> Duration { Duration::from_secs(60) } +fn default_strict_variable_validation() -> Mode { + Mode::Enforce +} + #[derive(Clone, Debug, Default, Error, Display, Serialize, Deserialize, JsonSchema)] #[serde(deny_unknown_fields, rename_all = "snake_case")] pub(crate) enum BatchingMode { diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 29b0c56665..0bc8e3ade9 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -10935,6 +10935,15 @@ expression: "&schema" "warmed_up_queries": null }, "description": "Query planning options" + }, + "strict_variable_validation": { + "allOf": [ + { + "$ref": "#/definitions/Mode" + } + ], + "default": "enforce", + "description": "Determines how to handle queries which include additional fields of an input object.\n- `enforce` (default): rejects query\n- `measure`: permits query and the logs unknown fields" } }, "type": "object" @@ -12490,7 +12499,8 @@ expression: "&schema" "experimental_plans_limit": null, "experimental_reuse_query_plans": false, "warmed_up_queries": null - } + }, + "strict_variable_validation": "enforce" }, "description": "Configuration for the supergraph" }, diff --git a/apollo-router/src/graphql/response.rs b/apollo-router/src/graphql/response.rs index b4bcdd7868..78c1f58397 100644 --- a/apollo-router/src/graphql/response.rs +++ b/apollo-router/src/graphql/response.rs @@ -254,6 +254,19 @@ impl From for Response { } } +#[cfg(test)] +impl Response { + pub(crate) fn errors_with_code<'a>(&'a self, code: &'a str) -> impl Iterator { + self.errors + .iter() + .filter(move |err| err.extension_code().is_some_and(|c| c == code)) + } + + pub(crate) fn contains_error_code(&self, code: &str) -> bool { + self.errors_with_code(code).next().is_some() + } +} + #[cfg(test)] mod tests { use serde_json::json; diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index d316bf98b5..1eb40ba84d 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -25,6 +25,7 @@ use crate::Context; use crate::batching::BatchQuery; use crate::configuration::Batching; use crate::configuration::PersistedQueriesPrewarmQueryPlanCache; +use crate::configuration::mode::Mode; use crate::error::CacheResolverError; use crate::graphql; use crate::graphql::IntoGraphQLErrors; @@ -79,6 +80,7 @@ pub(crate) struct SupergraphService { query_planner_service: CachingQueryPlanner, execution_service: execution::BoxCloneService, schema: Arc, + strict_variable_validation: Mode, } #[buildstructor::buildstructor] @@ -88,11 +90,13 @@ impl SupergraphService { query_planner_service: CachingQueryPlanner, execution_service: execution::BoxCloneService, schema: Arc, + strict_variable_validation: Mode, ) -> Self { SupergraphService { query_planner_service, execution_service, schema, + strict_variable_validation, } } } @@ -122,22 +126,27 @@ impl Service for SupergraphService { let schema = self.schema.clone(); let context_cloned = req.context.clone(); - let fut = service_call(planning, self.execution_service.clone(), schema, req).or_else( - |error: BoxError| async move { - let errors = vec![ - crate::error::Error::builder() - .message(error.to_string()) - .extension_code("INTERNAL_SERVER_ERROR") - .build(), - ]; - - Ok(SupergraphResponse::infallible_builder() - .errors(errors) - .status_code(StatusCode::INTERNAL_SERVER_ERROR) - .context(context_cloned) - .build()) - }, - ); + let fut = service_call( + planning, + self.execution_service.clone(), + schema, + req, + self.strict_variable_validation, + ) + .or_else(|error: BoxError| async move { + let errors = vec![ + crate::error::Error::builder() + .message(error.to_string()) + .extension_code("INTERNAL_SERVER_ERROR") + .build(), + ]; + + Ok(SupergraphResponse::infallible_builder() + .errors(errors) + .status_code(StatusCode::INTERNAL_SERVER_ERROR) + .context(context_cloned) + .build()) + }); Box::pin(fut) } @@ -148,6 +157,7 @@ async fn service_call( execution_service: execution::BoxCloneService, schema: Arc, req: SupergraphRequest, + strict_variable_validation: Mode, ) -> Result { let context = req.context; let body = req.supergraph_request.body(); @@ -306,7 +316,11 @@ async fn service_call( ); *response.response.status_mut() = StatusCode::NOT_ACCEPTABLE; Ok(response) - } else if let Some(err) = plan.query.validate_variables(body, &schema).err() { + } else if let Some(err) = plan + .query + .validate_variables(body, &schema, strict_variable_validation) + .err() + { let mut res = SupergraphResponse::new_from_graphql_response(err, context); *res.response.status_mut() = StatusCode::BAD_REQUEST; Ok(res) @@ -583,6 +597,7 @@ impl PluggableSupergraphServiceBuilder { .query_planner_service(query_planner_service.clone()) .execution_service(execution_service) .schema(schema.clone()) + .strict_variable_validation(configuration.supergraph.strict_variable_validation) .build(); let supergraph_service = diff --git a/apollo-router/src/spec/field_type.rs b/apollo-router/src/spec/field_type.rs index 792b686c54..1aaa450eb2 100644 --- a/apollo-router/src/spec/field_type.rs +++ b/apollo-router/src/spec/field_type.rs @@ -1,3 +1,5 @@ +use std::iter::once; + use apollo_compiler::Name; use apollo_compiler::schema; use serde::Deserialize; @@ -5,6 +7,7 @@ use serde::Serialize; use serde::de::Error as _; use super::query::parse_hir_value; +use crate::configuration::mode::Mode; use crate::json_ext::Value; use crate::json_ext::ValueExt; use crate::spec::Schema; @@ -123,6 +126,7 @@ fn validate_input_value( value: Option<&Value>, schema: &Schema, path: &JsonValuePath<'_>, + strict_variable_validation: Mode, ) -> Result<(), InvalidInputValue> { let fmt_path = |var_path: &JsonValuePath<'_>| match var_path { JsonValuePath::Variable { .. } => format!("variable `{var_path}`"), @@ -161,12 +165,24 @@ fn validate_input_value( index: i, parent: path, }; - validate_input_value(inner_type, Some(x), schema, &path)? + validate_input_value( + inner_type, + Some(x), + schema, + &path, + strict_variable_validation, + )? } return Ok(()); } else { // For coercion from single value to list - return validate_input_value(inner_type, Some(value), schema, path); + return validate_input_value( + inner_type, + Some(value), + schema, + path, + strict_variable_validation, + ); } } }; @@ -217,17 +233,24 @@ fn validate_input_value( )) }; - let unknown = obj.keys().find_map(|k| { - let k = k.as_str(); - if !def.fields.contains_key(k) { - Some(k) - } else { - None + let mut unknown_input_fields = obj + .keys() + .map(|k| k.as_str()) + .filter(|&k| !def.fields.contains_key(k)); + if let Some(unknown_input_field) = unknown_input_fields.next() { + match strict_variable_validation { + Mode::Enforce => { + return Err(unknown_field(unknown_input_field)); + } + Mode::Measure => { + let unknown_fields: Vec<&str> = once(unknown_input_field) + .chain(unknown_input_fields) + .collect(); + // NB: warning will be attached to the span via trace id, so you can figure out + // operation name from parent span + tracing::warn!(variables = ?unknown_fields, "encountered unexpected variable(s)"); + } } - }); - - if let Some(unknown) = unknown { - return Err(unknown_field(unknown)); } // Validate all fields present on def @@ -242,9 +265,21 @@ fn validate_input_value( .default_value .as_ref() .and_then(|v| parse_hir_value(v)); - validate_input_value(&field.ty, default.as_ref(), schema, &path) + validate_input_value( + &field.ty, + default.as_ref(), + schema, + &path, + strict_variable_validation, + ) } - value => validate_input_value(&field.ty, value, schema, &path), + value => validate_input_value( + &field.ty, + value, + schema, + &path, + strict_variable_validation, + ), } }) } @@ -264,8 +299,9 @@ impl FieldType { value: Option<&Value>, schema: &Schema, path: &JsonValuePath<'_>, + strict_variable_validation: Mode, ) -> Result<(), InvalidInputValue> { - validate_input_value(&self.0, value, schema, path) + validate_input_value(&self.0, value, schema, path, strict_variable_validation) } pub(crate) fn is_non_null(&self) -> bool { diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 6507d769c4..ab19799d2a 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -24,6 +24,7 @@ use self::subselections::SubSelectionValue; use super::Fragment; use super::QueryHash; use crate::Configuration; +use crate::configuration::mode::Mode; use crate::error::FetchError; use crate::graphql::Error; use crate::graphql::Request; @@ -1042,6 +1043,7 @@ impl Query { &self, request: &Request, schema: &Schema, + strict_variable_validation: Mode, ) -> Result<(), Response> { if LevelFilter::current() >= LevelFilter::DEBUG { let known_variables = self @@ -1085,7 +1087,7 @@ impl Query { let path = super::JsonValuePath::Variable { name: name.as_str(), }; - ty.validate_input_value(value, schema, &path) + ty.validate_input_value(value, schema, &path, strict_variable_validation) .err() .map(|message| { FetchError::ValidationInvalidTypeVariable { diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 9ec2c6d2c1..9d328ab9d5 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -2515,55 +2515,57 @@ fn reformat_response_unknown_typename() { .test(); } -macro_rules! run_validation { - ($schema:expr, $query:expr, $variables:expr $(,)?) => {{ - let variables = match $variables { - Value::Object(object) => object, - _ => unreachable!("variables must be an object"), - }; - let schema = Schema::parse(&$schema, &Default::default()).expect("could not parse schema"); - let request = Request::builder() - .variables(variables) - .query($query.to_string()) - .build(); - let query = Query::parse( - request - .query - .as_ref() - .expect("query has been added right above; qed"), - None, - &schema, - &Default::default(), - ) - .expect("could not parse query"); - query.validate_variables(&request, &schema) - }}; +#[allow(clippy::result_large_err)] +fn run_validation( + schema: String, + query: &str, + variables: serde_json_bytes::Value, + mode: Mode, +) -> Result<(), Response> { + let variables = match variables { + Value::Object(object) => object, + _ => unreachable!("variables must be an object"), + }; + let schema = Schema::parse(&schema, &Default::default()).expect("could not parse schema"); + let request = Request::builder() + .variables(variables) + .query(query.to_string()) + .build(); + let query = Query::parse( + request + .query + .as_ref() + .expect("query has been added right above; qed"), + None, + &schema, + &Default::default(), + ) + .expect("could not parse query"); + query.validate_variables(&request, &schema, mode) } -macro_rules! assert_validation { - ($schema:expr, $query:expr, $variables:expr $(,)?) => {{ - let res = run_validation!( - with_supergraph_boilerplate($schema, "Query"), - $query, - $variables - ); - assert!(res.is_ok(), "validation should have succeeded: {:?}", res); - }}; +fn assert_validation(schema: &str, query: &str, variables: serde_json_bytes::Value) { + let res = run_validation( + with_supergraph_boilerplate(schema, "Query"), + query, + variables, + Mode::Enforce, + ); + assert!(res.is_ok(), "validation should have succeeded: {:?}", res); } -macro_rules! assert_validation_error { - ($schema:expr, $query:expr, $variables:expr $(,)?) => {{ - let res = run_validation!( - with_supergraph_boilerplate($schema, "Query"), - $query, - $variables - ); - assert!(res.is_err(), "validation should have failed"); - }}; +fn assert_validation_error(schema: &str, query: &str, variables: serde_json_bytes::Value) { + let res = run_validation( + with_supergraph_boilerplate(schema, "Query"), + query, + variables, + Mode::Enforce, + ); + assert!(res.is_err(), "validation should have failed"); } #[test] -fn variable_validation() { +fn variable_validation_enforce_mode() { let schema = r#" type Query { int(a: Int): String @@ -2577,304 +2579,304 @@ fn variable_validation() { } "#; // https://spec.graphql.org/June2018/#sec-Int - assert_validation!(schema, "query($foo:Int){int(a:$foo)}", json!({})); - assert_validation_error!(schema, "query($foo:Int!){int(a:$foo)}", json!({})); - assert_validation!(schema, "query($foo:Int=1){int(a:$foo)}", json!({})); - assert_validation!(schema, "query($foo:Int!=1){int(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Int){int(a:$foo)}", json!({})); + assert_validation_error(schema, "query($foo:Int!){int(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Int=1){int(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Int!=1){int(a:$foo)}", json!({})); // When expected as an input type, only integer input values are accepted. - assert_validation!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":2})); - assert_validation!( + assert_validation(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":2})); + assert_validation( schema, "query($foo:Int){int(a:$foo)}", - json!({ "foo": i32::MAX }) + json!({ "foo": i32::MAX }), ); - assert_validation!( + assert_validation( schema, "query($foo:Int){int(a:$foo)}", - json!({ "foo": i32::MIN }) + json!({ "foo": i32::MIN }), ); // All other input values, including strings with numeric content, must raise a query error indicating an incorrect type. - assert_validation_error!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":"2"})); - assert_validation_error!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":2.0})); - assert_validation_error!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":"str"})); - assert_validation_error!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":true})); - assert_validation_error!(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":{}})); + assert_validation_error(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":"2"})); + assert_validation_error(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":2.0})); + assert_validation_error(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":"str"})); + assert_validation_error(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":true})); + assert_validation_error(schema, "query($foo:Int){int(a:$foo)}", json!({"foo":{}})); // If the integer input value represents a value less than -231 or greater than or equal to 231, a query error should be raised. - assert_validation_error!( + assert_validation_error( schema, "query($foo:Int){int(a:$foo)}", - json!({ "foo": i32::MAX as i64 + 1 }) + json!({ "foo": i32::MAX as i64 + 1 }), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:Int){int(a:$foo)}", - json!({ "foo": i32::MIN as i64 - 1 }) + json!({ "foo": i32::MIN as i64 - 1 }), ); // https://spec.graphql.org/draft/#sec-Float.Input-Coercion - assert_validation!(schema, "query($foo:Float){float(a:$foo)}", json!({})); - assert_validation_error!(schema, "query($foo:Float!){float(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Float){float(a:$foo)}", json!({})); + assert_validation_error(schema, "query($foo:Float!){float(a:$foo)}", json!({})); // When expected as an input type, both integer and float input values are accepted. - assert_validation!(schema, "query($foo:Float){float(a:$foo)}", json!({"foo":2})); - assert_validation!( + assert_validation(schema, "query($foo:Float){float(a:$foo)}", json!({"foo":2})); + assert_validation( schema, "query($foo:Float){float(a:$foo)}", - json!({"foo":2.0}) + json!({"foo":2.0}), ); // double precision floats are valid - assert_validation!( + assert_validation( schema, "query($foo:Float){float(a:$foo)}", - json!({"foo":1600341978193i64}) + json!({"foo":1600341978193i64}), ); - assert_validation!( + assert_validation( schema, "query($foo:Float){float(a:$foo)}", - json!({"foo":1600341978193f64}) + json!({"foo":1600341978193f64}), ); // All other input values, including strings with numeric content, // must raise a request error indicating an incorrect type. - assert_validation_error!( + assert_validation_error( schema, "query($foo:Float){float(a:$foo)}", - json!({"foo":"2.0"}) + json!({"foo":"2.0"}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:Float){float(a:$foo)}", - json!({"foo":"2"}) + json!({"foo":"2"}), ); // https://spec.graphql.org/June2018/#sec-String - assert_validation!(schema, "query($foo:String){str(a:$foo)}", json!({})); - assert_validation_error!(schema, "query($foo:String!){str(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:String){str(a:$foo)}", json!({})); + assert_validation_error(schema, "query($foo:String!){str(a:$foo)}", json!({})); // When expected as an input type, only valid UTF‐8 string input values are accepted. - assert_validation!( + assert_validation( schema, "query($foo:String){str(a:$foo)}", - json!({"foo": "str"}) + json!({"foo": "str"}), ); // All other input values must raise a query error indicating an incorrect type. - assert_validation_error!( + assert_validation_error( schema, "query($foo:String){str(a:$foo)}", - json!({"foo":true}) + json!({"foo":true}), ); - assert_validation_error!(schema, "query($foo:String){str(a:$foo)}", json!({"foo": 0})); - assert_validation_error!( + assert_validation_error(schema, "query($foo:String){str(a:$foo)}", json!({"foo": 0})); + assert_validation_error( schema, "query($foo:String){str(a:$foo)}", - json!({"foo": 42.0}) + json!({"foo": 42.0}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:String){str(a:$foo)}", - json!({"foo": {}}) + json!({"foo": {}}), ); // https://spec.graphql.org/June2018/#sec-Boolean - assert_validation!(schema, "query($foo:Boolean){bool(a:$foo)}", json!({})); - assert_validation_error!(schema, "query($foo:Boolean!){bool(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Boolean){bool(a:$foo)}", json!({})); + assert_validation_error(schema, "query($foo:Boolean!){bool(a:$foo)}", json!({})); // When expected as an input type, only boolean input values are accepted. // All other input values must raise a query error indicating an incorrect type. - assert_validation!( + assert_validation( schema, "query($foo:Boolean!){bool(a:$foo)}", - json!({"foo":true}) + json!({"foo":true}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:Boolean!){bool(a:$foo)}", - json!({"foo":"true"}) + json!({"foo":"true"}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:Boolean!){bool(a:$foo)}", - json!({"foo": 0}) + json!({"foo": 0}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:Boolean!){bool(a:$foo)}", - json!({"foo": "no"}) + json!({"foo": "no"}), ); - assert_validation!(schema, "query($foo:Boolean=true){bool(a:$foo)}", json!({})); - assert_validation!(schema, "query($foo:Boolean!=true){bool(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Boolean=true){bool(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:Boolean!=true){bool(a:$foo)}", json!({})); // https://spec.graphql.org/June2018/#sec-ID - assert_validation!(schema, "query($foo:ID){id(a:$foo)}", json!({})); - assert_validation_error!(schema, "query($foo:ID!){id(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:ID){id(a:$foo)}", json!({})); + assert_validation_error(schema, "query($foo:ID!){id(a:$foo)}", json!({})); // When expected as an input type, any string (such as "4") or integer (such as 4) // input value should be coerced to ID as appropriate for the ID formats a given GraphQL server expects. - assert_validation!(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": 4})); - assert_validation!(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": "4"})); - assert_validation!( + assert_validation(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": 4})); + assert_validation(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": "4"})); + assert_validation( schema, "query($foo:String){str(a:$foo)}", - json!({"foo": "str"}) + json!({"foo": "str"}), ); - assert_validation!( + assert_validation( schema, "query($foo:String){str(a:$foo)}", - json!({"foo": "4.0"}) + json!({"foo": "4.0"}), ); // Any other input value, including float input values (such as 4.0), must raise a query error indicating an incorrect type. - assert_validation_error!(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": 4.0})); - assert_validation_error!(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": true})); - assert_validation_error!(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": {}})); + assert_validation_error(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": 4.0})); + assert_validation_error(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": true})); + assert_validation_error(schema, "query($foo:ID){id(a:$foo)}", json!({"foo": {}})); // https://spec.graphql.org/June2018/#sec-Type-System.List - assert_validation!(schema, "query($foo:[Int]){intList(a:$foo)}", json!({})); - assert_validation!(schema, "query($foo:[Int!]){intList(a:$foo)}", json!({})); - assert_validation!( + assert_validation(schema, "query($foo:[Int]){intList(a:$foo)}", json!({})); + assert_validation(schema, "query($foo:[Int!]){intList(a:$foo)}", json!({})); + assert_validation( schema, "query($foo:[Int!]){intList(a:$foo)}", - json!({ "foo": null }) + json!({ "foo": null }), ); - assert_validation!( + assert_validation( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":1}) + json!({"foo":1}), ); - assert_validation!( + assert_validation( schema, "query($foo:[String]){strList(a:$foo)}", - json!({"foo":"bar"}) + json!({"foo":"bar"}), ); - assert_validation!( + assert_validation( schema, "query($foo:[[Int]]){intListList(a:$foo)}", - json!({"foo":1}) + json!({"foo":1}), ); - assert_validation!( + assert_validation( schema, "query($foo:[[Int]]){intListList(a:$foo)}", - json!({"foo":[[1], [2, 3]]}) + json!({"foo":[[1], [2, 3]]}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":"str"}) + json!({"foo":"str"}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":{}}) + json!({"foo":{}}), ); - assert_validation_error!(schema, "query($foo:[Int]!){intList(a:$foo)}", json!({})); - assert_validation_error!( + assert_validation_error(schema, "query($foo:[Int]!){intList(a:$foo)}", json!({})); + assert_validation_error( schema, "query($foo:[Int!]){intList(a:$foo)}", - json!({"foo":[1, null]}) + json!({"foo":[1, null]}), ); - assert_validation!( + assert_validation( schema, "query($foo:[Int]!){intList(a:$foo)}", - json!({"foo":[]}) + json!({"foo":[]}), ); - assert_validation!( + assert_validation( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":[1,2,3]}) + json!({"foo":[1,2,3]}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":["f","o","o"]}) + json!({"foo":["f","o","o"]}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":["1","2","3"]}) + json!({"foo":["1","2","3"]}), ); - assert_validation!( + assert_validation( schema, "query($foo:[String]){strList(a:$foo)}", - json!({"foo":["1","2","3"]}) + json!({"foo":["1","2","3"]}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[String]){strList(a:$foo)}", - json!({"foo":[1,2,3]}) + json!({"foo":[1,2,3]}), ); - assert_validation!( + assert_validation( schema, "query($foo:[Int!]){intList(a:$foo)}", - json!({"foo":[1,2,3]}) + json!({"foo":[1,2,3]}), ); - assert_validation_error!( + assert_validation_error( schema, "query($foo:[Int!]){intList(a:$foo)}", - json!({"foo":[1,null,3]}) + json!({"foo":[1,null,3]}), ); - assert_validation!( + assert_validation( schema, "query($foo:[Int]){intList(a:$foo)}", - json!({"foo":[1,null,3]}) + json!({"foo":[1,null,3]}), ); // https://spec.graphql.org/June2018/#sec-Input-Objects - assert_validation!( + assert_validation( "input Foo{ y: String } type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({}) + json!({}), ); - assert_validation!( + assert_validation( "input Foo{ y: String } type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":{}}) + json!({"foo":{}}), ); - assert_validation_error!( + assert_validation_error( "input Foo{ y: String } type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":1}) + json!({"foo":1}), ); - assert_validation_error!( + assert_validation_error( "input Foo{ y: String } type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":"str"}) + json!({"foo":"str"}), ); - assert_validation_error!( + assert_validation_error( "input Foo{x:Int!} type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":{}}) + json!({"foo":{}}), ); - assert_validation!( + assert_validation( "input Foo{x:Int!} type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":{"x":1}}) + json!({"foo":{"x":1}}), ); - assert_validation!( + assert_validation( "scalar Foo type Query { x(foo: Foo): String }", "query($foo:Foo!){x(foo: $foo)}", - json!({"foo":{}}) + json!({"foo":{}}), ); - assert_validation!( + assert_validation( "scalar Foo type Query { x(foo: Foo): String }", "query($foo:Foo!){x(foo: $foo)}", - json!({"foo":1}) + json!({"foo":1}), ); - assert_validation_error!( + assert_validation_error( "scalar Foo type Query { x(foo: Foo): String }", "query($foo:Foo!){x(foo: $foo)}", - json!({}) + json!({}), ); - assert_validation!( + assert_validation( "input Foo{bar:Bar!} input Bar{x:Int!} type Query { x(foo: Foo): String }", "query($foo:Foo){x(foo: $foo)}", - json!({"foo":{"bar":{"x":1}}}) + json!({"foo":{"bar":{"x":1}}}), ); - assert_validation!( + assert_validation( "enum Availability{AVAILABLE} type Product{availability:Availability! name:String} type Query{products(availability: Availability!): [Product]!}", "query GetProductsByAvailability($availability: Availability!){products(availability: $availability) {name}}", - json!({"availability": "AVAILABLE"}) + json!({"availability": "AVAILABLE"}), ); - assert_validation!( + assert_validation( "input MessageInput { content: String author: String @@ -2891,10 +2893,10 @@ fn variable_validation() { }) { id }}", - json!({"availability": "AVAILABLE"}) + json!({"availability": "AVAILABLE"}), ); - assert_validation!( + assert_validation( "input MessageInput { content: String author: String @@ -2911,10 +2913,10 @@ fn variable_validation() { json!({"msg": { "content": "Hello", "author": "Me" - }}) + }}), ); - assert_validation_error!( + assert_validation_error( "input MessageInput { content: String author: String @@ -2932,11 +2934,11 @@ fn variable_validation() { "content": "Hello", "author": "Me", "unknownField": "unknown", - }}) + }}), ); // Tests if nested inputs are correctly validated - assert_validation_error!( + assert_validation_error( "input MessageInput { content: String author: String @@ -2962,7 +2964,7 @@ fn variable_validation() { {"input": 4}, {"input": 5, "unknownField": "unknown"} ], - }}) + }}), ); let schema = r#" @@ -3009,17 +3011,77 @@ fn variable_validation() { } "#; - let res = run_validation!( - schema, + let res = run_validation( + schema.to_string(), "mutation foo($input: FooInput!) { foo (input: $input) { __typename }}", - json!({"input":{}}) + json!({"input":{}}), + Mode::Enforce, ); assert!(res.is_ok(), "validation should have succeeded: {res:?}"); } +#[test] +#[rstest::rstest] +#[case::top_level_unexpected_field( + json!({"content": "Hello", "canvas": [], "unknownField": "unknown"}), + Ok(()) +)] +#[case::nested_unexpected_field( + json!({"canvas": [{"input": 3}, {"input": 5, "unknownField": "unknown"}]}), + Ok(()) +)] +#[case::top_level_missing_field( + json!({}), + Err("VALIDATION_INVALID_TYPE_VARIABLE") +)] +#[case::nested_missing_field( + json!({"canvas": [{"unknownField": 3}, {"input": 4}]}), + Err("VALIDATION_INVALID_TYPE_VARIABLE") +)] +fn variable_validation_measure_mode( + #[case] msg_variables: Value, + #[case] expected_result: Result<(), &str>, +) { + let schema = " + input MessageInput { + content: String + canvas: [CanvasInput]! + } + input CanvasInput { + input: Int! + } + type Query { + send(message: MessageInput): ID + }"; + + // Tests validation of variable fields + let result = run_validation( + with_supergraph_boilerplate(schema, "Query"), + "query($msg: MessageInput) { send(message: $msg) }", + json!({"msg": msg_variables}), + Mode::Measure, + ); + + match (result, expected_result) { + (Ok(()), Ok(())) => {} + (Err(response), Err(expected_code)) => { + assert!( + response.contains_error_code(expected_code), + "response = {response:?}" + ); + } + (Err(response), Ok(())) => { + panic!("expected validation to pass: response = {response:?}"); + } + (Ok(()), Err(code)) => { + panic!("expected validation to fail with code {code}"); + } + } +} + #[test] fn filter_root_errors() { let schema = "type Query { diff --git a/apollo-router/tests/fixtures/supergraph_input_variables.graphql b/apollo-router/tests/fixtures/supergraph_input_variables.graphql new file mode 100644 index 0000000000..0f588af94f --- /dev/null +++ b/apollo-router/tests/fixtures/supergraph_input_variables.graphql @@ -0,0 +1,47 @@ +schema +@link(url: "https://specs.apollo.dev/link/v1.0") +@link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) +@link(url: "https://specs.apollo.dev/join/v0.2", for: EXECUTION) +{ + query: Query +} + +directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION +directive @join__graph(name: String!, url: String!) on ENUM_VALUE +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR +directive @join__implements( + graph: join__Graph! + interface: String! +) repeatable on OBJECT | INTERFACE + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA +directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION + +scalar join__FieldSet +scalar link__Import +enum link__Purpose { + SECURITY + EXECUTION +} + +enum join__Graph { + TEST @join__graph(name: "test", url: "http://localhost:4001/graphql") +} + + +input MessageInput @join__type(graph: TEST) { + content: String + author: String + canvas: [CanvasInput] +} +input CanvasInput @join__type(graph: TEST) { + input: Int! +} +type Receipt @join__type(graph: TEST) { + id: ID! +} +type Query @join__type(graph: TEST) { + send(message: MessageInput): Receipt +} + + diff --git a/apollo-router/tests/integration/operation_limits.rs b/apollo-router/tests/integration/operation_limits.rs index 818c80f8bf..1a494fffdd 100644 --- a/apollo-router/tests/integration/operation_limits.rs +++ b/apollo-router/tests/integration/operation_limits.rs @@ -7,7 +7,6 @@ use apollo_router::TestHarness; use apollo_router::graphql; use apollo_router::services::execution; use apollo_router::services::supergraph; -use serde_json::Value; use serde_json::json; use tower::BoxError; use tower::ServiceExt; @@ -304,7 +303,7 @@ limits: .build(); let (_, response) = router.execute_query(request.clone()).await; - let body: Value = response.json().await.unwrap(); + let body: serde_json::Value = response.json().await.unwrap(); assert!( body.get("errors").is_none(), "expected no errors with warn_only, got: {body:?}" @@ -315,7 +314,7 @@ limits: router.assert_reloaded().await; let (_, response) = router.execute_query(request).await; - let body: Value = response.json().await.unwrap(); + let body: serde_json::Value = response.json().await.unwrap(); let errors = body .get("errors") diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@enforce.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@enforce.snap new file mode 100644 index 0000000000..ce6e07d195 --- /dev/null +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@enforce.snap @@ -0,0 +1,15 @@ +--- +source: apollo-router/tests/integration/validation.rs +expression: response_body +--- +{ + "errors": [ + { + "extensions": { + "code": "VALIDATION_INVALID_TYPE_VARIABLE", + "name": "msg" + }, + "message": "unknown field input value at `$msg.canvas[0].innput` found for GraphQL type `input CanvasInput @join__type(graph: TEST) {\n input: Int!\n}\n`" + } + ] +} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@measure.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@measure.snap new file mode 100644 index 0000000000..d4790044f4 --- /dev/null +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@measure.snap @@ -0,0 +1,13 @@ +--- +source: apollo-router/tests/integration/validation.rs +expression: response_body +--- +{ + "data": null, + "errors": [ + { + "message": "Subgraph errors redacted", + "path": [] + } + ] +} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@missing.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@missing.snap new file mode 100644 index 0000000000..ce6e07d195 --- /dev/null +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__validation__variable_validation_mode_propagates_fully@missing.snap @@ -0,0 +1,15 @@ +--- +source: apollo-router/tests/integration/validation.rs +expression: response_body +--- +{ + "errors": [ + { + "extensions": { + "code": "VALIDATION_INVALID_TYPE_VARIABLE", + "name": "msg" + }, + "message": "unknown field input value at `$msg.canvas[0].innput` found for GraphQL type `input CanvasInput @join__type(graph: TEST) {\n input: Int!\n}\n`" + } + ] +} diff --git a/apollo-router/tests/integration/validation.rs b/apollo-router/tests/integration/validation.rs index fa5ed5cdcf..9c58510540 100644 --- a/apollo-router/tests/integration/validation.rs +++ b/apollo-router/tests/integration/validation.rs @@ -1,6 +1,12 @@ +use std::path::PathBuf; + use apollo_router::_private::create_test_service_factory_from_yaml; +use serde_json::json; use tower::ServiceExt; +use crate::integration::IntegrationTest; +use crate::integration::common::Query; + #[tokio::test] async fn test_supergraph_validation_errors_are_passed_on() { create_test_service_factory_from_yaml( @@ -205,3 +211,71 @@ async fn test_lots_of_validation_errors() { ); assert!(errors.len() <= 100, "should return limited error count"); } + +#[rstest::rstest] +#[case::enforce(Some("enforce"), true, false)] +#[case::measure(Some("measure"), false, true)] +#[case::missing(None, true, false)] +#[tokio::test(flavor = "multi_thread")] +async fn variable_validation_mode_propagates_fully( + #[case] strict_variable_validation: Option<&str>, + #[case] response_should_be_error: bool, + #[case] logs_should_contain_warning: bool, +) { + let mut settings = insta::Settings::clone_current(); + settings.set_snapshot_suffix(strict_variable_validation.unwrap_or("missing")); + settings.set_sort_maps(true); + let _guard = settings.bind_to_scope(); + + let mut config = json!({"supergraph": {}}); + if let Some(strict_variable_validation) = strict_variable_validation { + config["supergraph"] = json!({ "strict_variable_validation": strict_variable_validation }); + } + + let mut router = IntegrationTest::builder() + .config(serde_yaml::to_string(&config).unwrap()) + .supergraph(PathBuf::from( + "tests/fixtures/supergraph_input_variables.graphql", + )) + .build() + .await; + + router.start().await; + router.assert_started().await; + + // Execute a query to trigger all the callbacks + let (_trace_id, response) = router + .execute_query( + Query::builder() + .body(json!({ + "query": "query($msg: MessageInput) { send(message: $msg) { id } }", + "variables": { + "msg": { + "content": "Hello", + "author": "Me", + "canvas": [{"input": 4, "innput": 4}], + } + } + })) + .build(), + ) + .await; + + assert_eq!( + response.status().is_client_error(), + response_should_be_error + ); + let response_body: serde_json::Value = + serde_json::from_slice(response.text().await.unwrap().as_bytes()).unwrap(); + insta::assert_json_snapshot!(response_body); + + router.read_logs(); + const VALIDATION_MESSAGE: &str = "encountered unexpected variable(s)"; + if logs_should_contain_warning { + router.assert_log_contained(VALIDATION_MESSAGE); + } else { + router.assert_log_not_contained(VALIDATION_MESSAGE); + } + + router.graceful_shutdown().await; +} diff --git a/docs/shared/config/supergraph.mdx b/docs/shared/config/supergraph.mdx index 289f8660cc..b54fd21408 100644 --- a/docs/shared/config/supergraph.mdx +++ b/docs/shared/config/supergraph.mdx @@ -37,6 +37,7 @@ supergraph: experimental_plans_limit: null experimental_reuse_query_plans: false warmed_up_queries: null + strict_variable_validation: enforce ``` diff --git a/docs/shared/router-yaml-complete.mdx b/docs/shared/router-yaml-complete.mdx index 73147e74e5..f0b8285808 100644 --- a/docs/shared/router-yaml-complete.mdx +++ b/docs/shared/router-yaml-complete.mdx @@ -357,6 +357,7 @@ supergraph: experimental_plans_limit: null experimental_reuse_query_plans: false warmed_up_queries: null + strict_variable_validation: enforce telemetry: apollo: batch_processor: diff --git a/docs/source/routing/configuration/yaml.mdx b/docs/source/routing/configuration/yaml.mdx index 6182b956c7..7947a9e923 100644 --- a/docs/source/routing/configuration/yaml.mdx +++ b/docs/source/routing/configuration/yaml.mdx @@ -476,13 +476,25 @@ traffic_shaping: By default, the router compresses subgraph requests by generating fragment definitions based on the shape of the subgraph operation. In many cases this significantly reduces the size of the query sent to subgraphs. -You can explicitly opt-out of this behavior by specifying `supergraph.generate_query_fragments`: +Opt out of this behavior by specifying `supergraph.generate_query_fragments`: ```yaml supergraph: generate_query_fragments: false ``` +#### Variable validation modes + +By default, the router validates input variables strictly. It validates each input object value against its type definition, and any unknown fields result in a request error. + +```yaml +supergraph: + strict_variable_validation: enforce +``` + +If your implementation requires unknown fields on a defined type, you can opt out of stricter validation by specifying `strict_variable_validation: measure`. +In this case, the router will not error when encountering unknown fields, but will log the field for reference. + ---