From eeef5c6fc76a80622d0f1bbe3bbbd8818675227a Mon Sep 17 00:00:00 2001 From: bryn Date: Thu, 30 Jul 2026 14:15:56 +0100 Subject: [PATCH 01/38] test: characterize authorization whole-query rejection behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `filter_query` empties the document, or `reject_unauthorized` is set and any path was removed, the query planner returns `QueryPlannerContent::Response` and the supergraph service forwards it verbatim. We are about to move that rejection out of the query planner into a supergraph layer, and three properties of the current behaviour had no test at all: - Execution is never reached. The existing reject tests register subgraph mocks but never assert they went unused, so a regression that let execution run would pass them as long as the response body matched. - The response carries HTTP 200. Directive-based authorization strips selections from the document, so a rejection has field-error semantics: value completion nulls the stripped selections and, once everything is stripped, propagates to `data: null`. 200 with errors is the correct response for that. Moving the rejection ahead of the planner makes it look like a request-level rejection, where 400 becomes the instinctive choice, so this needs pinning. - Usage reporting is not recorded, because `CachingQueryPlanner` only inserts it for the `Plan` variant. The operation is still metered — telemetry falls back to counting it as one licensed operation — but it arrives with no operation signature, referenced fields, or per-type stats, so Studio cannot attribute the rejection to an operation. Every subgraph is replaced by a `tower_test` mock with no canned responses, so reaching one fails the test. `assert_no_subgraph_calls` additionally fails when no subgraph service was built at all, which would otherwise make the assertion vacuous. Verified all three tests discriminate: with `reject_unauthorized` flipped to false the operation becomes a partial filter that does reach execution, and all three fail. --- .../src/plugins/authorization/tests.rs | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 8fd62bfbbc..1cd1bd4b4a 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -12,6 +12,7 @@ use tower::ServiceExt; use crate::Context; use crate::MockedSubgraphs; use crate::TestHarness; +use crate::apollo_studio_interop::UsageReporting; use crate::graphql; use crate::plugin::test::MockSubgraph; use crate::plugins::authorization::APOLLO_AUTHENTICATION_JWT_CLAIMS; @@ -452,6 +453,137 @@ async fn authenticated_directive_reject_unauthorized() { assert_logs_contain_entire_request_authorization_error(); } +mod whole_query_rejection { + use super::*; + + /// `Organization.id` and `User.phone` are both `@authenticated`, so filtering removes + /// paths from an unauthenticated request and `reject_unauthorized` turns that into a + /// whole-query rejection. + const REJECTED_QUERY: &str = "query { orga(id: 1) { id creatorUser { id name phone } } }"; + + type SubgraphHandles = + Arc>>>; + + /// Builds a router that rejects `REJECTED_QUERY`, replacing every subgraph with a + /// `tower_test` mock. The mocks hold no canned responses, so reaching one fails. + async fn build_router_rejecting_whole_query() -> (router::BoxCloneService, SubgraphHandles) { + let handles: SubgraphHandles = Arc::new(Mutex::new(Vec::new())); + let handles_clone = handles.clone(); + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { + "directives": { + "enabled": true, + "reject_unauthorized": true + } + } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(move |_name, _service| { + let (mock, handle) = + tower_test::mock::pair::(); + handles_clone.lock().unwrap().push(handle); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + (service, handles) + } + + /// Fails if any subgraph mock received a request, or if the router built no subgraph + /// service at all, which would make the check vacuous. + async fn assert_no_subgraph_calls(handles: SubgraphHandles) { + let handles: Vec<_> = handles.lock().unwrap().drain(..).collect(); + assert!(!handles.is_empty(), "no subgraph services were created"); + for handle in handles { + crate::plugin::test::assert_no_mock_calls(handle).await; + } + } + + fn rejected_request(context: Context) -> router::Request { + let req = graphql::Request { + query: Some(REJECTED_QUERY.to_string()), + ..Default::default() + }; + router::Request { + context, + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + } + } + + /// Sends `REJECTED_QUERY`, asserts the router rejected it on authorization grounds, + /// and returns the HTTP status. + async fn send_rejected_request( + service: router::BoxCloneService, + context: Context, + ) -> http::StatusCode { + let response = service.oneshot(rejected_request(context)).await.unwrap(); + let status = response.response.status(); + + let body = response + .into_graphql_response_stream() + .await + .next() + .await + .unwrap() + .unwrap(); + assert_eq!( + body.errors.first().map(|e| e.message.as_str()), + Some("Unauthorized field or type"), + "the operation was not rejected on authorization grounds" + ); + + status + } + + #[tokio::test] + async fn does_not_reach_execution() { + let (service, handles) = build_router_rejecting_whole_query().await; + + send_rejected_request(service, Context::new()).await; + + assert_no_subgraph_calls(handles).await; + } + + /// Directive-based authorization strips selections from the document, so a rejection + /// carries field-error semantics: value completion nulls the stripped selections and + /// propagates to `data: null`. 200 with errors is the correct response. + #[tokio::test] + async fn returns_http_200() { + let (service, _handles) = build_router_rejecting_whole_query().await; + + let status = send_rejected_request(service, Context::new()).await; + + assert_eq!(status, http::StatusCode::OK); + } + + /// `CachingQueryPlanner` records usage reporting only for the `Plan` variant. + /// Telemetry still meters the rejection as one licensed operation but sends no + /// signature, referenced fields, or per-type stats, so Studio cannot attribute it. + #[tokio::test] + async fn does_not_record_usage_reporting() { + let (service, _handles) = build_router_rejecting_whole_query().await; + let context = Context::new(); + + send_rejected_request(service, context.clone()).await; + + assert!( + !context + .extensions() + .with_lock(|lock| lock.contains_key::>()) + ); + } +} + #[tokio::test] async fn authenticated_directive_dry_run() { let _guard = tracing_test::dispatcher_guard(); From 0d9252e73155fcc0f3e6e9fc94968eb4eb5aaea4 Mon Sep 17 00:00:00 2001 From: bryn Date: Fri, 31 Jul 2026 10:47:57 +0100 Subject: [PATCH 02/38] test: assert plan cache segments by authorization state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, which is what stops an unauthenticated request from being served a plan built for an authenticated one. Until now that invariant was only covered incidentally, by two supergraph snapshot tests (`authorization::tests::{authenticated_directive, scopes_directive}`) that send the same query twice against a shared cache and compare bodies. Nothing named the invariant, so it was easy to weaken by accident while editing either test for unrelated reasons. `plan_cache_is_segmented_by_authorization_metadata` asserts it directly at the caching planner: the same query under two different `CacheKeyMetadata` values reaches the inner planner twice, and repeating the first value is served from cache. Counting inner invocations rather than comparing response bodies means the test states the cache behaviour instead of inferring it. `rejection_response_is_cached` pins the fact that a `QueryPlannerContent::Response` is cached like any other output, since `entry.insert` does not discriminate on variant. Characterization only — moving the rejection ahead of the cache would change this, and it should be a deliberate decision. Verified the segmentation assertion discriminates: making both metadata values identical drops the inner planner to a single invocation and fails the test, so the extra plan is genuinely caused by the metadata difference and the third call genuinely hits the cache. --- .../query_planner/caching_query_planner.rs | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 118c8eec69..cc4c1aaad3 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1847,6 +1847,155 @@ mod tests { crate::plugin::test::await_mock_driver(driver).await; } + /// Drives the planner mock, counting requests and answering each with `content`. + fn spawn_counting_planner( + mut handle: tower_test::mock::Handle, + content: QueryPlannerContent, + ) -> (tokio::task::JoinHandle<()>, Arc) { + let calls = Arc::new(AtomicUsize::new(0)); + let calls_clone = calls.clone(); + let driver = tokio::task::spawn(async move { + while let Some((_request, responder)) = handle.next_request().await { + calls_clone.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + responder.send_response( + QueryPlannerResponse::builder() + .content(content.clone()) + .build(), + ); + } + }); + (driver, calls) + } + + async fn caching_planner_for_test( + mock: tower_test::mock::Mock, + schema: &Arc, + configuration: &Configuration, + ) -> impl Service { + CachingQueryPlanner::for_test( + mock.map_err(|err| panic!("tower-test errored: {err}")), + schema.clone(), + Default::default(), + configuration, + ) + .await + .unwrap() + } + + fn caching_request_with_metadata( + query: &str, + doc: &ParsedDocument, + metadata: CacheKeyMetadata, + ) -> query_planner::CachingRequest { + let context = Context::new(); + context.extensions().with_lock(|lock| { + lock.insert::(doc.clone()); + lock.insert(metadata); + }); + query_planner::CachingRequest::new(query.to_string(), None, context) + } + + /// `CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, so the same query + /// under different authorization state reaches the inner planner again. That keeps an + /// unauthenticated request from receiving a plan built for an authenticated one. + #[test(tokio::test)] + async fn plan_cache_is_segmented_by_authorization_metadata() { + let (mock, handle) = tower_test::mock::pair::(); + let (driver, planner_calls) = spawn_counting_planner( + handle, + QueryPlannerContent::Plan { + plan: Arc::new(QueryPlan::fake_new(None, None)), + }, + ); + + let configuration: Configuration = Default::default(); + let schema = include_str!("../testdata/starstuff@current.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let mut service = caching_planner_for_test(mock, &schema, &configuration).await; + + let query = "query ExampleQuery { me { name } }"; + let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); + + let unauthenticated = CacheKeyMetadata::default(); + let authenticated = CacheKeyMetadata { + is_authenticated: true, + ..Default::default() + }; + + for metadata in [ + unauthenticated.clone(), + authenticated, + // Repeats the first key, which must now hit the cache. + unauthenticated, + ] { + service + .ready() + .await + .unwrap() + .call(caching_request_with_metadata(query, &doc, metadata)) + .await + .unwrap(); + } + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 2, + "each distinct authorization state must be planned separately, \ + and a repeated state must be served from cache" + ); + + drop(service); + crate::plugin::test::await_mock_driver(driver).await; + } + + /// `entry.insert` does not discriminate on the `QueryPlannerContent` variant, so the + /// cache stores an authorization rejection like any other planner output. + #[test(tokio::test)] + async fn rejection_response_is_cached() { + let (mock, handle) = tower_test::mock::pair::(); + let (driver, planner_calls) = spawn_counting_planner( + handle, + QueryPlannerContent::Response { + response: Box::new( + crate::graphql::Response::builder() + .data(crate::json_ext::Value::Null) + .build(), + ), + }, + ); + + let configuration: Configuration = Default::default(); + let schema = include_str!("../testdata/starstuff@current.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let mut service = caching_planner_for_test(mock, &schema, &configuration).await; + + let query = "query ExampleQuery { me { name } }"; + let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); + + for _ in 0..2 { + service + .ready() + .await + .unwrap() + .call(caching_request_with_metadata( + query, + &doc, + CacheKeyMetadata::default(), + )) + .await + .unwrap(); + } + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 1, + "the second identical request must be served from cache" + ); + + drop(service); + crate::plugin::test::await_mock_driver(driver).await; + } + #[test(tokio::test)] async fn test_temporary_errors_arent_cached() { let (mock, mut handle) = From bd912d74487302d4adac538195033fe8a1f8b72c Mon Sep 17 00:00:00 2001 From: bryn Date: Fri, 31 Jul 2026 10:55:48 +0100 Subject: [PATCH 03/38] test: pin the invariant that keeps warm-up from caching unfiltered plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Query plan warm-up bypasses the router and supergraph pipelines entirely — `WarmupParseQueryLayer` wraps the `CachingQueryPlanner` directly — and plans with `CacheKeyMetadata::default()`, which is byte-for-byte the metadata a genuinely unauthenticated request produces. Warm-up is therefore only safe because authorization filtering sits inside the query planner, below the plan cache, where every caller hits it regardless of how it got there. Nothing tested that. No warm-up test involves authorization directives at all, and the caching planner tests all use default metadata, so the property held by construction rather than by assertion. This asserts it at the planner: with directives enabled and unauthenticated metadata, planning an operation whose root field requires a scope returns a rejection, not a plan. Moving authorization out of the planner has to keep that true by some other means — otherwise warm-up populates the default-metadata cache key with an unfiltered plan and any unauthenticated request is served it. The failure message spells out that consequence, because a bare "expected Response, got Plan" would not tell whoever trips it why it matters. Verified the assertion discriminates: disabling the directives makes the planner return a plan and the test fails with the leak message. --- .../query_planner/query_planner_service.rs | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 7cf7ac7671..bcd79f8567 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -1089,6 +1089,59 @@ mod tests { } } + /// Warm-up skips the router and supergraph pipelines and plans with + /// `CacheKeyMetadata::default()`, the same metadata an unauthenticated request + /// produces. The planner filters below the plan cache, so it rejects here rather than + /// caching an unfiltered plan that a later unauthenticated request could hit. + #[test(tokio::test)] + async fn planning_unauthenticated_rejects_rather_than_returning_unfiltered_plan() { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap(); + let configuration = Arc::new(configuration); + + // `Query.me` requires the `profile` scope, so filtering an unauthenticated + // request empties the document. + let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let planner = QueryPlannerService::for_test(schema.clone(), configuration.clone()).unwrap(); + + let query = "query { me { name } }"; + let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); + + let content = planner + .get( + QueryKey { + original_query: query.to_string(), + filtered_query: query.to_string(), + operation_name: None, + metadata: CacheKeyMetadata::default(), + plan_options: PlanOptions::default(), + }, + doc, + ComputeJobType::QueryPlanningWarmup, + ) + .await + .unwrap(); + + match content { + QueryPlannerContent::Response { response } => { + assert_eq!( + response.errors.first().map(|e| e.message.as_str()), + Some("Unauthorized field or type") + ); + } + QueryPlannerContent::Plan { .. } => { + panic!( + "planner returned a query plan for an unauthenticated request; \ + an unfiltered plan cached under default metadata is reachable by \ + any unauthenticated request" + ) + } + } + } + #[tokio::test] async fn test_rust_mode_subgraph_operation_serialization() { let subgraph_queries = Arc::new(tokio::sync::Mutex::new(String::new())); From 2c69fd32199aaf58c4f761c9a2e3ec58fb46d92e Mon Sep 17 00:00:00 2001 From: bryn Date: Fri, 31 Jul 2026 11:03:25 +0100 Subject: [PATCH 04/38] test: cover __typename retention in authorization-filtered queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When authorization filters an operation, response formatting runs twice: once for the filtered query, then once for the original. The filtered pass has to copy `__typename` through, because the original pass needs it to decide whether a type condition applies. Without it, every field behind an inline fragment or fragment spread is dropped from the response. Both branches implementing this (`is_original == false` in `apply_selection_set`) were unreachable from the test suite. `is_original: false` appears exactly once, in `filtered_defer_fragment`, whose filtered query is `{ a { b } }` — no fragments, so neither branch can fire. The new test filters a field out of `... on Foo` and asserts `__typename` survives the filtered pass, then that `foo` survives the original pass, which it can only do if `__typename` did. Asserting the intermediate state as well as the final one means a failure says which of the two passes broke. `query_for_test` extracts the `Query` construction so the new test does not repeat the twenty lines `filtered_defer_fragment` uses to build one by hand. Verified the test reaches the branch: setting `is_original: true` on the filtered query drops `__typename` and fails the first assertion. --- apollo-router/src/spec/query/tests.rs | 137 ++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 05e46101b2..4db9c0f741 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -7620,6 +7620,143 @@ fn test_query_not_named_query() { ); } +/// Builds a [`Query`] from a query string, as the query planner does for the original +/// and the authorization-filtered operation. +fn query_for_test(schema: &Schema, query: &str, is_original: bool) -> Query { + let ast = Parser::new().parse_ast(query, "query.graphql").unwrap(); + let doc = ast.to_executable(schema.supergraph_schema()).unwrap(); + let (fragments, operation, defer_stats, schema_aware_hash) = + Query::extract_query_information(schema, query, &doc, None).unwrap(); + let subselections = crate::spec::query::subselections::collect_subselections( + &Configuration::default(), + &operation, + &fragments.map, + &defer_stats, + ) + .unwrap(); + + Query { + string: query.to_string(), + fragments, + operation, + filtered_query: None, + subselections, + defer_stats, + is_original, + unauthorized: UnauthorizedPaths::default(), + schema_aware_hash, + } +} + +/// Response formatting runs twice for a filtered operation: once for the filtered query, +/// then once for the original. The filtered pass copies `__typename` through so the +/// original pass can resolve type conditions. Without it, every field behind an inline +/// fragment or fragment spread disappears from the response. +#[test] +fn filtered_query_keeps_typename_for_type_conditions() { + let schema = Schema::parse( + r#" + schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) + { + query: Query + } + directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + 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, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + + scalar join__FieldSet + scalar link__Import + + enum link__Purpose { + SECURITY + EXECUTION + } + enum join__Graph { + TEST @join__graph(name: "test", url: "http://localhost:4001/graphql") + } + + type Query @join__type(graph: TEST) { + thing: Thing + } + + interface Thing @join__type(graph: TEST) { + id: ID + } + + type Foo implements Thing + @join__type(graph: TEST) + @join__implements(graph: TEST, interface: "Thing") { + id: ID + foo: String + secret: String + } + "#, + &Default::default(), + ) + .unwrap(); + + // `secret` is the field authorization removed, so the filtered operation is the + // original minus that one selection. + let original = "{ thing { id ... on Foo { foo secret } } }"; + let filtered = "{ thing { id ... on Foo { foo } } }"; + + let mut query = query_for_test(&schema, original, true); + query.filtered_query = Some(Arc::new(query_for_test(&schema, filtered, false))); + + let mut response = crate::graphql::Response::builder() + .data(json! {{ + "thing": { + "__typename": "Foo", + "id": "1", + "foo": "foo", + } + }}) + .build(); + + query.filtered_query.as_ref().unwrap().format_response( + &mut response, + Object::new(), + schema.api_schema(), + BooleanValues { bits: 0 }, + true, + ); + + assert_eq!( + response + .data + .as_ref() + .unwrap() + .get("thing") + .unwrap() + .get(TYPENAME), + Some(&json!("Foo")), + "the filtered pass must carry __typename through for the original pass to use" + ); + + query.format_response( + &mut response, + Object::new(), + schema.api_schema(), + BooleanValues { bits: 0 }, + true, + ); + + // `foo` sits behind `... on Foo`, so it survives only if __typename did. + assert_eq!( + response + .data + .as_ref() + .unwrap() + .get("thing") + .unwrap() + .get("foo"), + Some(&json!("foo")) + ); +} + #[test] fn filtered_defer_fragment() { let config = Configuration::default(); From 15bf899b44dc6d1cb85037ae8bdcc73359855e41 Mon Sep 17 00:00:00 2001 From: bryn Date: Fri, 31 Jul 2026 11:03:33 +0100 Subject: [PATCH 05/38] chore: remove orphaned response cache authorization snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These three snapshots have no test function anywhere in the repo. They were added with the experimental response cache plugin and the test that produced them was removed later without them, so `cargo insta` has been carrying them as unreferenced files ever since. The path they used to cover — `CacheKeyMetadata` folded into the entity cache key by `response_cache::cache_key::hash_additional_data` — has no test now. Deleting the snapshots does not lose coverage, it stops the files from implying coverage that does not exist. --- ...cache__response_cache_authorization-2.snap | 50 ---------------- ...cache__response_cache_authorization-3.snap | 54 ------------------ ...e_cache__response_cache_authorization.snap | 57 ------------------- 3 files changed, 161 deletions(-) delete mode 100644 apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap delete mode 100644 apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap delete mode 100644 apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap deleted file mode 100644 index b6437c2c7e..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-2.snap +++ /dev/null @@ -1,50 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": null, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - }, - { - "body": "I can eat on it", - "author": { - "username": "charles" - } - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap deleted file mode 100644 index b7e02eae2f..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization-3.snap +++ /dev/null @@ -1,54 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": { - "id": "1", - "name": null - }, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": { - "username": "ada" - } - }, - { - "body": "I can eat on it", - "author": { - "username": "charles" - } - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me", - "name" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap deleted file mode 100644 index c8804ce0f4..0000000000 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__response_cache__response_cache_authorization.snap +++ /dev/null @@ -1,57 +0,0 @@ ---- -source: apollo-router/tests/integration/response_cache.rs -expression: response ---- -{ - "data": { - "me": null, - "topProducts": [ - { - "name": "chair", - "reviews": [ - { - "body": "I can sit on it", - "author": null - } - ] - }, - { - "name": "table", - "reviews": [ - { - "body": "I can sit on it", - "author": null - }, - { - "body": "I can eat on it", - "author": null - } - ] - } - ] - }, - "errors": [ - { - "message": "Unauthorized field or type", - "path": [ - "me" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - }, - { - "message": "Unauthorized field or type", - "path": [ - "topProducts", - "@", - "reviews", - "@", - "author" - ], - "extensions": { - "code": "UNAUTHORIZED_FIELD_OR_TYPE" - } - } - ] -} From 2f08e5e3914004aef4760ba045f1c0fbcd6d712b Mon Sep 17 00:00:00 2001 From: rohan-b99 <43239788+rohan-b99@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:04:18 +0100 Subject: [PATCH 06/38] test: drive authorization cache-key tests through the production metadata path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plan_cache_is_segmented_by_authorization_metadata` inserted `CacheKeyMetadata` into the request context directly. Nothing in the router does that: `CachingQueryPlanner::plan` calls `AuthorizationPlugin::update_cache_key` first, which unconditionally overwrites the context's metadata with one derived from the request's JWT claims. The test only survived that overwrite because `Configuration::default()` plus `starstuff@current.graphql` (no authorization spec linked) left `enable_authorization_directives` false, so `update_cache_key` never ran — meaning the test pinned the derived `Hash`/`Eq` on `CachingQueryKey` and not the segmentation it claimed. Both cache tests now use a schema and configuration for which directives are enabled, and carry authorization state as JWT claims so `update_cache_key` produces the metadata. Removing that call now fails the test. `rejection_response_is_cached` fed a `QueryPlannerContent::Response` with no errors and used default metadata on both calls, so it observed neither a rejection nor authorization-dependent keying. It now uses the content shape `QueryPlannerService::get` returns for a whole-query rejection and adds a call in a different authorization state, pinning that a cached rejection is not served across authorization states. Two comments claimed more than their tests checked. `planning_unauthenticated_rejects_rather_than_returning_unfiltered_plan` stated an invariant about the plan cache while asserting on the planner service one layer below it, and passed `ComputeJobType::QueryPlanningWarmup` as though that selected a warm-up path — it only sets the compute-pool priority and the metric label, and `get` filters before reading it. `returns_http_200` credited the status to value completion nulling stripped selections, but the rejection short-circuits in the planner and `new_from_graphql_response` wraps it with `http::Response::new`, which is 200 regardless. Both now describe the code that actually runs. Co-Authored-By: Claude Opus 5 --- .../src/plugins/authorization/tests.rs | 11 ++- .../query_planner/caching_query_planner.rs | 91 +++++++++++++------ .../query_planner/query_planner_service.rs | 20 ++-- 3 files changed, 87 insertions(+), 35 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 1cd1bd4b4a..eb48e5e62b 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -554,9 +554,14 @@ mod whole_query_rejection { assert_no_subgraph_calls(handles).await; } - /// Directive-based authorization strips selections from the document, so a rejection - /// carries field-error semantics: value completion nulls the stripped selections and - /// propagates to `data: null`. 200 with errors is the correct response. + /// The status comes from the short-circuit in the query planner, not from response + /// formatting: `filter_query` returns `Err(Unauthorized)`, `QueryPlannerService::get` + /// builds a `graphql::Response` with `data: null` directly, and + /// `SupergraphResponse::new_from_graphql_response` wraps it with `http::Response::new`, + /// which is 200 regardless of errors or data shape. Execution and value completion never + /// run — see `does_not_reach_execution`. 200 with errors is the right answer for a + /// rejection with field-error semantics, so this pins that the short-circuit does not + /// pick up an error status along the way. #[tokio::test] async fn returns_http_200() { let (service, _handles) = build_router_rejecting_whole_query().await; diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index cc4c1aaad3..17115c95a1 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -726,6 +726,7 @@ mod tests { use crate::apollo_studio_interop::UsageReporting; use crate::configuration::QueryPlanning; use crate::configuration::Supergraph; + use crate::plugins::authentication::APOLLO_AUTHENTICATION_JWT_CLAIMS; use crate::query_planner::QueryPlan; use crate::spec::Query; use crate::spec::Schema; @@ -1882,15 +1883,38 @@ mod tests { .unwrap() } - fn caching_request_with_metadata( + /// A configuration and schema pair for which `AuthorizationPlugin::enable_directives` is + /// true. Without that, `plan` never calls `update_cache_key` and every request is keyed + /// under `CacheKeyMetadata::default()`, so no segmentation can be observed. + fn authorization_enabled_config_and_schema() -> (Configuration, Arc) { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap(); + // Links `requiresScopes`; a schema with no authorization spec keeps + // `enable_directives` false whatever the configuration says. + let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); + let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + (configuration, schema) + } + + /// Builds a request the way the router does: authorization state travels in the context + /// as JWT claims, and `plan` derives `CacheKeyMetadata` from them via + /// `AuthorizationPlugin::update_cache_key`. Inserting `CacheKeyMetadata` into the + /// context directly would not survive — `update_cache_key` overwrites it. + fn caching_request( query: &str, doc: &ParsedDocument, - metadata: CacheKeyMetadata, + authenticated: bool, ) -> query_planner::CachingRequest { let context = Context::new(); + if authenticated { + context + .insert(APOLLO_AUTHENTICATION_JWT_CLAIMS, "placeholder".to_string()) + .unwrap(); + } context.extensions().with_lock(|lock| { lock.insert::(doc.clone()); - lock.insert(metadata); }); query_planner::CachingRequest::new(query.to_string(), None, context) } @@ -1898,6 +1922,10 @@ mod tests { /// `CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, so the same query /// under different authorization state reaches the inner planner again. That keeps an /// unauthenticated request from receiving a plan built for an authenticated one. + /// + /// Drives it through the producer that runs in production — `update_cache_key`, reading + /// the request's JWT claims — because that call overwrites the context's + /// `CacheKeyMetadata` before the cache key is built. #[test(tokio::test)] async fn plan_cache_is_segmented_by_authorization_metadata() { let (mock, handle) = tower_test::mock::pair::(); @@ -1908,31 +1936,23 @@ mod tests { }, ); - let configuration: Configuration = Default::default(); - let schema = include_str!("../testdata/starstuff@current.graphql"); - let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let (configuration, schema) = authorization_enabled_config_and_schema(); let mut service = caching_planner_for_test(mock, &schema, &configuration).await; let query = "query ExampleQuery { me { name } }"; let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); - let unauthenticated = CacheKeyMetadata::default(); - let authenticated = CacheKeyMetadata { - is_authenticated: true, - ..Default::default() - }; - - for metadata in [ - unauthenticated.clone(), - authenticated, + for authenticated in [ + false, + true, // Repeats the first key, which must now hit the cache. - unauthenticated, + false, ] { service .ready() .await .unwrap() - .call(caching_request_with_metadata(query, &doc, metadata)) + .call(caching_request(query, &doc, authenticated)) .await .unwrap(); } @@ -1949,24 +1969,32 @@ mod tests { } /// `entry.insert` does not discriminate on the `QueryPlannerContent` variant, so the - /// cache stores an authorization rejection like any other planner output. + /// cache stores an authorization rejection like any other planner output — and, like any + /// other planner output, it stays keyed by authorization state, so a rejection cached for + /// an unauthenticated request is never served to an authenticated one. #[test(tokio::test)] async fn rejection_response_is_cached() { let (mock, handle) = tower_test::mock::pair::(); let (driver, planner_calls) = spawn_counting_planner( handle, + // The content `QueryPlannerService::get` returns for a whole-query rejection: + // null data carrying the unauthorized-path errors. QueryPlannerContent::Response { response: Box::new( crate::graphql::Response::builder() .data(crate::json_ext::Value::Null) + .error( + crate::graphql::Error::builder() + .message("Unauthorized field or type") + .extension_code("UNAUTHORIZED_FIELD_OR_TYPE") + .build(), + ) .build(), ), }, ); - let configuration: Configuration = Default::default(); - let schema = include_str!("../testdata/starstuff@current.graphql"); - let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); + let (configuration, schema) = authorization_enabled_config_and_schema(); let mut service = caching_planner_for_test(mock, &schema, &configuration).await; let query = "query ExampleQuery { me { name } }"; @@ -1977,11 +2005,7 @@ mod tests { .ready() .await .unwrap() - .call(caching_request_with_metadata( - query, - &doc, - CacheKeyMetadata::default(), - )) + .call(caching_request(query, &doc, false)) .await .unwrap(); } @@ -1992,6 +2016,21 @@ mod tests { "the second identical request must be served from cache" ); + service + .ready() + .await + .unwrap() + .call(caching_request(query, &doc, true)) + .await + .unwrap(); + + assert_eq!( + planner_calls.load(std::sync::atomic::Ordering::SeqCst), + 2, + "a cached rejection must not be served to a request in a different \ + authorization state" + ); + drop(service); crate::plugin::test::await_mock_driver(driver).await; } diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index bcd79f8567..f677af7949 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -1089,10 +1089,18 @@ mod tests { } } - /// Warm-up skips the router and supergraph pipelines and plans with - /// `CacheKeyMetadata::default()`, the same metadata an unauthenticated request - /// produces. The planner filters below the plan cache, so it rejects here rather than - /// caching an unfiltered plan that a later unauthenticated request could hit. + /// Warm-up reaches this service with `CacheKeyMetadata::default()`, the same metadata an + /// unauthenticated request produces: `queries_to_warm_up` supplies no metadata for + /// persisted queries, and for re-warmed cache entries `update_cache_key` derives default + /// metadata from warm-up's claimless context. This asserts what the service does with + /// those inputs — filtering rejects the operation instead of handing back a plan. + /// + /// Scope: this is not a warm-up-specific code path. `compute_job_type` only selects the + /// compute-pool priority and the metric label, and `get` filters before reading it, so + /// `QueryPlanningWarmup` behaves exactly like `QueryPlanning` here. Nor does this observe + /// the plan cache, which sits above this service; that a rejection is what gets cached, + /// and that it stays keyed by authorization state, is covered by + /// `caching_query_planner::tests::rejection_response_is_cached`. #[test(tokio::test)] async fn planning_unauthenticated_rejects_rather_than_returning_unfiltered_plan() { let configuration: Configuration = serde_json::from_value(serde_json::json!({ @@ -1135,8 +1143,8 @@ mod tests { QueryPlannerContent::Plan { .. } => { panic!( "planner returned a query plan for an unauthenticated request; \ - an unfiltered plan cached under default metadata is reachable by \ - any unauthenticated request" + filtering must reject instead, or an unfiltered plan would be handed \ + back for caching under default metadata" ) } } From 043547f9b575db8785508dcc08049814afc8b1ab Mon Sep 17 00:00:00 2001 From: rohan-b99 <43239788+rohan-b99@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:36:09 +0100 Subject: [PATCH 07/38] chore: lint --- apollo-router/src/query_planner/caching_query_planner.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 17115c95a1..873dd81990 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1943,9 +1943,7 @@ mod tests { let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); for authenticated in [ - false, - true, - // Repeats the first key, which must now hit the cache. + false, true, // Repeats the first key, which must now hit the cache. false, ] { service From 5d944849bc50e8217acddffbcd11b652a9e48e9b Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 10:28:14 +0100 Subject: [PATCH 08/38] test: derive the __typename retention fixture from the query planner `filtered_query_keeps_typename_for_type_conditions` hand-wrote both the original and the filtered query and hand-built the `Query` pair around them, so it pinned a shape that nothing guaranteed filtering produces. It now plans an unauthenticated request through `QueryPlannerService` and takes the pair from `plan.query`, which is where `filter_query` produces the filtered document and the planner sets `is_original = false`. The schema gains the `authenticated` spec and `Foo.secret` gains `@authenticated` so filtering has something to remove. Split into one test per fragment form. The single test covered an inline fragment and a fragment spread in one query, which pinned neither branch: `apply_selection_set` copies `__typename` in both the `InlineFragment` and `FragmentSpread` arms, so either copy alone kept both fields alive. Disabling one arm left the test passing. With one form per test, disabling either arm fails exactly the test that covers it, verified in both directions. `Thing` stays an interface because the copy only matters for abstract types: `apply_selection_set` derives `current_type` from the response `__typename` for interfaces and unions, and from the schema otherwise, so a concrete field type would resolve type conditions with or without the copy. --- apollo-router/src/spec/query/tests.rs | 244 ++++++++++++++------------ 1 file changed, 136 insertions(+), 108 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 4db9c0f741..ee3a4aa612 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -2,9 +2,16 @@ use apollo_compiler::parser::Parser; use insta::assert_json_snapshot; use serde_json_bytes::json; use test_log::test; +use tower::ServiceExt; use super::*; +use crate::compute_job::ComputeJobType; use crate::json_ext::ValueExt; +use crate::plugins::authorization::CacheKeyMetadata; +use crate::query_planner::query_planner_service::QueryPlannerService; +use crate::services::QueryPlannerContent; +use crate::services::QueryPlannerRequest; +use crate::services::query_planner::PlanOptions; macro_rules! assert_eq_and_ordered { ($a:expr, $b:expr $(,)?) => { @@ -7620,101 +7627,97 @@ fn test_query_not_named_query() { ); } -/// Builds a [`Query`] from a query string, as the query planner does for the original -/// and the authorization-filtered operation. -fn query_for_test(schema: &Schema, query: &str, is_original: bool) -> Query { - let ast = Parser::new().parse_ast(query, "query.graphql").unwrap(); - let doc = ast.to_executable(schema.supergraph_schema()).unwrap(); - let (fragments, operation, defer_stats, schema_aware_hash) = - Query::extract_query_information(schema, query, &doc, None).unwrap(); - let subselections = crate::spec::query::subselections::collect_subselections( - &Configuration::default(), - &operation, - &fragments.map, - &defer_stats, - ) - .unwrap(); - - Query { - string: query.to_string(), - fragments, - operation, - filtered_query: None, - subselections, - defer_stats, - is_original, - unauthorized: UnauthorizedPaths::default(), - schema_aware_hash, +const AUTHENTICATED_INTERFACE_SCHEMA: &str = r#" + schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) + @link(url: "https://specs.apollo.dev/authenticated/v0.1", for: SECURITY) + { + query: Query } -} - -/// Response formatting runs twice for a filtered operation: once for the filtered query, -/// then once for the original. The filtered pass copies `__typename` through so the -/// original pass can resolve type conditions. Without it, every field behind an inline -/// fragment or fragment spread disappears from the response. -#[test] -fn filtered_query_keeps_typename_for_type_conditions() { - let schema = Schema::parse( - r#" - schema - @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) - { - query: Query - } - directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA - 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, isInterfaceObject: Boolean! = false) 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 @join__graph(name: String!, url: String!) on ENUM_VALUE + directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + 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__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + directive @authenticated on OBJECT | FIELD_DEFINITION | INTERFACE | SCALAR | ENUM - scalar join__FieldSet - scalar link__Import + scalar join__FieldSet + scalar link__Import - enum link__Purpose { - SECURITY - EXECUTION - } - enum join__Graph { - TEST @join__graph(name: "test", url: "http://localhost:4001/graphql") - } + enum link__Purpose { + SECURITY + EXECUTION + } + enum join__Graph { + TEST @join__graph(name: "test", url: "http://localhost:4001/graphql") + } - type Query @join__type(graph: TEST) { - thing: Thing - } + type Query @join__type(graph: TEST) { + thing: Thing + } - interface Thing @join__type(graph: TEST) { - id: ID - } + interface Thing @join__type(graph: TEST) { + id: ID + } - type Foo implements Thing - @join__type(graph: TEST) - @join__implements(graph: TEST, interface: "Thing") { - id: ID - foo: String - secret: String - } - "#, - &Default::default(), - ) + type Foo implements Thing + @join__type(graph: TEST) + @join__implements(graph: TEST, interface: "Thing") { + id: ID + inline: String + spread: String + secret: String @authenticated + } +"#; + +/// Plans `query_str` as an unauthenticated request and returns the original `Query` with +/// `filtered_query` populated, alongside the schema. +/// +/// `QueryPlannerService` builds this pair in production: `filter_query` produces the +/// filtered document and the planner marks it `is_original = false`. Hand-writing the +/// filtered query risks pinning a shape filtering never produces. +async fn authorization_filtered_query(query_str: &str) -> (Arc, Arc) { + let configuration: Configuration = serde_json::from_value(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) .unwrap(); + let configuration = Arc::new(configuration); + let schema = Arc::new(Schema::parse(AUTHENTICATED_INTERFACE_SCHEMA, &configuration).unwrap()); + let doc = Query::parse_document(query_str, None, &schema, &configuration).unwrap(); + + let planner = QueryPlannerService::for_test(schema.clone(), configuration.clone()).unwrap(); + let response = planner + .oneshot(QueryPlannerRequest { + query: query_str.to_string(), + operation_name: None, + document: doc, + metadata: CacheKeyMetadata::default(), + plan_options: PlanOptions::default(), + compute_job_type: ComputeJobType::QueryPlanning, + }) + .await + .unwrap(); - // `secret` is the field authorization removed, so the filtered operation is the - // original minus that one selection. - let original = "{ thing { id ... on Foo { foo secret } } }"; - let filtered = "{ thing { id ... on Foo { foo } } }"; + let query = match response.content { + Some(QueryPlannerContent::Plan { plan }) => plan.query.clone(), + _ => panic!("filtering removed only `secret`, so the planner must return a plan"), + }; + assert!( + query.filtered_query.is_some(), + "filtering must have produced a second Query, otherwise the two-pass formatting \ + under test never runs" + ); - let mut query = query_for_test(&schema, original, true); - query.filtered_query = Some(Arc::new(query_for_test(&schema, filtered, false))); + (query, schema) +} - let mut response = crate::graphql::Response::builder() - .data(json! {{ - "thing": { - "__typename": "Foo", - "id": "1", - "foo": "foo", - } - }}) - .build(); +/// Runs the two passes `ExecutionService` runs over a filtered operation and returns the +/// resulting `thing` object. +fn format_filtered_then_original(query: &Query, schema: &Schema, data: Value) -> Value { + let mut response = crate::graphql::Response::builder().data(data).build(); query.filtered_query.as_ref().unwrap().format_response( &mut response, @@ -7723,19 +7726,6 @@ fn filtered_query_keeps_typename_for_type_conditions() { BooleanValues { bits: 0 }, true, ); - - assert_eq!( - response - .data - .as_ref() - .unwrap() - .get("thing") - .unwrap() - .get(TYPENAME), - Some(&json!("Foo")), - "the filtered pass must carry __typename through for the original pass to use" - ); - query.format_response( &mut response, Object::new(), @@ -7744,17 +7734,55 @@ fn filtered_query_keeps_typename_for_type_conditions() { true, ); - // `foo` sits behind `... on Foo`, so it survives only if __typename did. - assert_eq!( - response - .data - .as_ref() - .unwrap() - .get("thing") - .unwrap() - .get("foo"), - Some(&json!("foo")) + response + .data + .as_ref() + .unwrap() + .get("thing") + .unwrap() + .clone() +} + +/// `Thing` is an interface, so `apply_selection_set` takes the concrete type from the +/// response `__typename` rather than from the schema. The filtered pass has to copy +/// `__typename` into its output for the original pass to resolve the type condition on +/// `... on Foo`, so `inline` survives only if the copy happened. +/// +/// A query carrying an inline fragment and a fragment spread together would not pin this: +/// each form copies `__typename` independently, so either one alone keeps both fields +/// alive. Hence one query form per test. +#[tokio::test] +async fn filtered_query_keeps_typename_for_inline_fragment() { + // `secret` is `@authenticated`, so filtering drops it and leaves `inline`. + let (query, schema) = + authorization_filtered_query("{ thing { id ... on Foo { inline secret } } }").await; + + // What the subgraph returns for the filtered plan: `secret` was never requested. + let thing = format_filtered_then_original( + &query, + &schema, + json! {{ "thing": { "__typename": "Foo", "id": "1", "inline": "inline" } }}, + ); + + assert_eq!(thing.get("inline"), Some(&json!("inline"))); +} + +/// The fragment-spread counterpart of `filtered_query_keeps_typename_for_inline_fragment`, +/// covering the second `!is_original` branch in `apply_selection_set`. +#[tokio::test] +async fn filtered_query_keeps_typename_for_fragment_spread() { + let (query, schema) = authorization_filtered_query( + "{ thing { id ...Spread } } fragment Spread on Foo { spread secret }", + ) + .await; + + let thing = format_filtered_then_original( + &query, + &schema, + json! {{ "thing": { "__typename": "Foo", "id": "1", "spread": "spread" } }}, ); + + assert_eq!(thing.get("spread"), Some(&json!("spread"))); } #[test] From f0e6c61245a2687fe88ada4b30499eb423783c51 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 11:14:45 +0100 Subject: [PATCH 09/38] test: pin that the filtered response pass strips overfetched data `ExecutionService` formats a filtered operation twice: the filtered query projects the subgraph data onto the authorized shape, then the original query expands it to the shape the client requested. Both properties matter and neither was named by a test. Disabling the filtered pass entirely left 92 of the 93 authorization tests passing. The only failure was a snapshot in `cache_key_metadata`, whose name points at cache keys rather than at response filtering, and whose diff for the leak is a single line. A refactor of the response path could drop that pass and return protected data with the suite still green. This test has a subgraph return `phone` even though filtering removed it from the operation, so the data reaching formatting is wider than the query. Asserting `Some(Value::Null)` covers both halves in one key: present means the original query restored the requested shape, null rather than "1234" means the filtered query stripped the value. Verified against both mutations. Removing the filtered pass yields `Some(String("1234"))`, a leak of an `@authenticated` field. Running the passes in the other order yields `None`, dropping a requested field from the response and orphaning the authorization error already recorded for its path. --- .../src/plugins/authorization/tests.rs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index eb48e5e62b..da1a1f8755 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -453,6 +453,79 @@ async fn authenticated_directive_reject_unauthorized() { assert_logs_contain_entire_request_authorization_error(); } +/// A subgraph can return more than the operation selected, so the data reaching response +/// formatting is not bounded by what authorization left in the query. `ExecutionService` +/// handles that by formatting twice: the filtered query projects the data onto the +/// authorized shape, then the original query expands it to the shape the client asked +/// for. +/// +/// `User.phone` is `@authenticated`, so filtering removes it from an unauthenticated +/// operation while this subgraph returns it anyway. `Some(Value::Null)` pins both halves +/// of that arrangement in one assertion: `phone` is present, so the original query +/// restored the requested shape, and it is null rather than `"1234"`, so the filtered +/// query stripped the value the client may not see. +/// +/// Removing either property changes this key: drop the filtered pass and it holds +/// `"1234"`, run the passes in the other order and it disappears from the response. +#[tokio::test] +async fn overfetched_unauthorized_field_is_not_returned() { + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(|_name, _service| { + let (mock, mut handle) = + tower_test::mock::pair::(); + tokio::spawn(async move { + while let Some((req, responder)) = handle.next_request().await { + // `phone` is not in the filtered operation this subgraph was sent. + responder.send_response( + subgraph::Response::fake_builder() + .context(req.context) + .data(serde_json::json! {{ + "currentUser": { "name": "Ada", "phone": "1234" } + }}) + .build(), + ); + } + }); + mock.boxed_clone() + }) + .build_supergraph() + .await + .unwrap(); + + let request = supergraph::Request::fake_builder() + .query("query { currentUser { name phone } }") + .context(Context::new()) + .build() + .unwrap(); + let response = service + .oneshot(request) + .await + .unwrap() + .next_response() + .await + .unwrap(); + + let current_user = response + .data + .as_ref() + .expect("the operation kept `name`, so it must not reject outright") + .get("currentUser") + .expect("`currentUser` must survive; only `phone` is @authenticated"); + + assert_eq!( + current_user.get("phone"), + Some(&serde_json_bytes::Value::Null), + "the subgraph returned `phone` outside the filtered operation, so it must reach \ + the client as null rather than as its value" + ); + assert_eq!(current_user.get("name"), Some(&json!("Ada"))); +} + mod whole_query_rejection { use super::*; From 08bf779f9cca90ea799f34ce0f5da359d32945c7 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 11:54:53 +0100 Subject: [PATCH 10/38] test: pin the rejection response contract before moving it out of the planner The refactor moves whole-query rejection out of the query planner into a layer on the execution service, and the response it produces has to stay byte-identical. Four properties of that response had no test. `data: null` is sent, rather than `data` being left out. GraphQL distinguishes the two: an absent `data` marks a request error, a null one marks a field error that propagated to the root. The existing snapshots cannot see this, because `into_graphql_response_stream` deserializes into `graphql::Response` first, where `data: Option` turns JSON `null` into `None` and then skips it on re-serialization. These tests read the wire bytes instead. Building the rejection without `data` leaves all three pre-existing rejection tests passing. `errors.response: disabled` and `errors.response: extensions` were only covered for partial filtering. `errors_in_extensions` sets no `reject_unauthorized`, and the `config_parsing` cases check that the options deserialize, not what they do. Both are now covered for a whole rejection: `disabled` leaves `data: null` as the client's only signal, `extensions` moves the errors to `extensions.authorizationErrors` and leaves `errors` out. `dry_run` combined with `reject_unauthorized` still rejects. `dry_run` reports the paths without modifying the operation, so a rejection there can only come from the config, never from an emptied document. Any change that keys rejection off the document being empty has to keep the config check independent, or `dry_run` silently stops enforcing. Each test was verified against a mutation that isolates it: omitting `data` fails all four, ignoring `ErrorLocation::Disabled` fails only the disabled test, and skipping the `reject_unauthorized` check under `dry_run` fails only the dry-run test. --- .../src/plugins/authorization/tests.rs | 121 ++++++++++++++++-- 1 file changed, 112 insertions(+), 9 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index da1a1f8755..a4ce9dd0be 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -537,20 +537,18 @@ mod whole_query_rejection { type SubgraphHandles = Arc>>>; - /// Builds a router that rejects `REJECTED_QUERY`, replacing every subgraph with a - /// `tower_test` mock. The mocks hold no canned responses, so reaching one fails. - async fn build_router_rejecting_whole_query() -> (router::BoxCloneService, SubgraphHandles) { + /// Builds a router that rejects `REJECTED_QUERY` under the given `directives` config, + /// replacing every subgraph with a `tower_test` mock. The mocks hold no canned + /// responses, so reaching one fails. + async fn build_rejecting_router( + directives: serde_json::Value, + ) -> (router::BoxCloneService, SubgraphHandles) { let handles: SubgraphHandles = Arc::new(Mutex::new(Vec::new())); let handles_clone = handles.clone(); let service = TestHarness::builder() .configuration_json(serde_json::json!({ - "authorization": { - "directives": { - "enabled": true, - "reject_unauthorized": true - } - } + "authorization": { "directives": directives } })) .unwrap() .schema(AUTHENTICATED_SCHEMA) @@ -567,6 +565,31 @@ mod whole_query_rejection { (service, handles) } + async fn build_router_rejecting_whole_query() -> (router::BoxCloneService, SubgraphHandles) { + build_rejecting_router(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true + })) + .await + } + + /// Sends `REJECTED_QUERY` and parses the response body as it goes on the wire. + /// + /// `into_graphql_response_stream` deserializes into `graphql::Response`, where + /// `data: Option` turns JSON `null` into `None` and then skips it on + /// re-serialization. Anything asserting on whether `data` is present has to read the + /// bytes instead. + async fn rejected_response_body(service: router::BoxCloneService) -> serde_json::Value { + let response = service + .oneshot(rejected_request(Context::new())) + .await + .unwrap(); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + serde_json::from_slice(&bytes).unwrap() + } + /// Fails if any subgraph mock received a request, or if the router built no subgraph /// service at all, which would make the check vacuous. async fn assert_no_subgraph_calls(handles: SubgraphHandles) { @@ -660,6 +683,86 @@ mod whole_query_rejection { .with_lock(|lock| lock.contains_key::>()) ); } + + /// The rejection sends `data: null`, not an absent `data`. GraphQL gives those two + /// different meanings — an absent `data` marks a request error, a null one marks a + /// field error that propagated to the root — so the presence of the key is part of the + /// response contract. The snapshot tests cannot cover this, because they assert on a + /// `graphql::Response` that has already lost the distinction. + #[tokio::test] + async fn rejection_sends_null_data() { + let (service, _handles) = build_router_rejecting_whole_query().await; + + let body = rejected_response_body(service).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + } + + /// `errors.response: disabled` suppresses the authorization errors, so `data: null` is + /// the only thing left telling the client the operation produced nothing. + #[tokio::test] + async fn rejection_with_errors_disabled_sends_null_data_and_no_errors() { + let (service, _handles) = build_rejecting_router(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "disabled" } + })) + .await; + + let body = rejected_response_body(service).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); + } + + /// `errors.response: extensions` moves the authorization errors under + /// `extensions.authorizationErrors` and leaves `errors` out of the response. + #[tokio::test] + async fn rejection_with_errors_in_extensions() { + let (service, _handles) = build_rejecting_router(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "extensions" } + })) + .await; + + let body = rejected_response_body(service).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); + let authorization_errors = body + .pointer("/extensions/authorizationErrors") + .and_then(|value| value.as_array()) + .expect("the errors must move under extensions.authorizationErrors"); + assert_eq!( + authorization_errors.len(), + 2, + "one error per unauthorized path: `orga.id` and `orga.creatorUser.phone`" + ); + } + + /// `dry_run` and `reject_unauthorized` combine rather than cancelling out: `dry_run` + /// reports the paths without modifying the operation, and `reject_unauthorized` then + /// refuses it anyway. + /// + /// This matters for any change that treats an emptied document as the trigger for + /// rejection. `dry_run` never empties the document, so a rejection here can only come + /// from the config, and getting that wrong turns `dry_run` into a mode that silently + /// stops enforcing. + #[tokio::test] + async fn dry_run_with_reject_unauthorized_still_rejects() { + let (service, handles) = build_rejecting_router(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "dry_run": true + })) + .await; + + let body = rejected_response_body(service).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_no_subgraph_calls(handles).await; + } } #[tokio::test] From 0912ce271ace729f17a50ff1528b5daec31c60fb Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 12:55:03 +0100 Subject: [PATCH 11/38] test: pin the licensed operation count for an unreported operation Moving whole-query rejection out of the query planner turns the planner's output for a rejected operation from `QueryPlannerContent::Response` into a plan, and `QueryPlan::usage_reporting` is not optional, so that plan has to carry a `UsageReporting`. Which variant it carries decides what the operation costs: fn licensed_operation_count(usage_reporting: &UsageReporting) -> u64 { match usage_reporting { UsageReporting::Error(_) => 0, _ => 1, } } `UsageReporting::Error` is the natural-looking choice for an operation that produced no plan, and it bills nothing. A rejected operation is billed as one licensed operation today, because telemetry falls back to a count of 1 when the context holds no `UsageReporting` at all, so choosing `Error` would drop rejected operations off the bill. No test covered any of this: every snapshot asserting `licensed_operation_count: 1` covers a successful operation. These drive `update_apollo_metrics` over a context directly and assert the count for all three cases: absent reporting bills 1, `Error` bills 0, and `Operation` bills 1. The middle one exists to make the trap visible rather than to protect current behaviour. `apollo_reports.rs` was the obvious home, but its fixture schema has no authorization directives, and the file documents itself as flaky with process-wide collectors. The unit-level metrics tests could not host these either, since `get_metrics_for_request` fixes the schema and configuration. Driving the reporting function directly needs neither. Each test body runs under `FutureMetricsExt::with_metrics`, which gives it a task-local meter provider. Without it these emit through the global meter and intermittently break `plugins::authorization::authenticated::tests` when both modules run together; with it that combination passed five consecutive runs. Verified each test still discriminates afterwards: changing the fallback to 0, `Error` to 1, or the catch-all to 0 fails exactly one test each. --- apollo-router/src/plugins/telemetry/mod.rs | 100 +++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 832e1ab293..9fb461894f 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -3545,3 +3545,103 @@ mod tests { .await; } } + +#[cfg(test)] +mod licensed_operation_count_tests { + use std::collections::HashMap; + use std::sync::Arc; + use std::time::Duration; + + use crate::Context; + use crate::apollo_studio_interop::UsageReporting; + use crate::apollo_studio_interop::UsageReportingOperationDetails; + use crate::metrics::FutureMetricsExt as _; + use crate::plugins::telemetry::EnabledFeatures; + use crate::plugins::telemetry::Telemetry; + use crate::plugins::telemetry::apollo::SingleReport; + use crate::plugins::telemetry::apollo_exporter::Sender; + use crate::query_planner::OperationKind; + + /// Drives `update_apollo_metrics` over a context and returns the licensed operation + /// count Studio would be billed for. + async fn licensed_operation_count_for(context: Context) -> u64 { + let (tx, mut rx) = tokio::sync::mpsc::channel(10); + Telemetry::update_apollo_metrics( + &context, + 0.0, + Sender::Apollo(tx), + false, + Duration::from_millis(1), + OperationKind::Query, + None, + HashMap::new(), + EnabledFeatures { + distributed_apq_cache: false, + response_cache: false, + }, + ); + + let report = rx + .recv() + .await + .expect("update_apollo_metrics must send a stats report"); + match report { + SingleReport::Stats(stats) => stats + .licensed_operation_count_by_type + .map(|by_type| by_type.licensed_operation_count) + .unwrap_or(0), + SingleReport::Traces(_) => panic!("expected a stats report"), + } + } + + /// An operation the query planner rejected on authorization grounds records no + /// `UsageReporting`, and is still billed as one licensed operation. Billing does not + /// depend on the operation reaching execution. + /// + /// Anything that changes what a rejected operation puts in the context has to keep this + /// at 1. See `usage_reporting_error_is_not_billed` for the way that goes wrong. + #[tokio::test] + async fn missing_usage_reporting_is_billed_as_one_operation() { + async { + assert_eq!(licensed_operation_count_for(Context::new()).await, 1); + } + .with_metrics() + .await; + } + + /// `UsageReporting::Error` bills nothing. It is the tempting variant to reach for when + /// an operation produced no plan, and choosing it drops that operation off the bill. + #[tokio::test] + async fn usage_reporting_error_is_not_billed() { + async { + let context = Context::new(); + context.extensions().with_lock(|lock| { + lock.insert::>(Arc::new(UsageReporting::Error( + "some error key".to_string(), + ))) + }); + + assert_eq!(licensed_operation_count_for(context).await, 0); + } + .with_metrics() + .await; + } + + /// An operation that carries real reporting details is billed the same as one carrying + /// none, so attributing a rejected operation does not change what it costs. + #[tokio::test] + async fn operation_details_are_billed_as_one_operation() { + async { + let context = Context::new(); + context.extensions().with_lock(|lock| { + lock.insert::>(Arc::new(UsageReporting::Operation( + UsageReportingOperationDetails::default(), + ))) + }); + + assert_eq!(licensed_operation_count_for(context).await, 1); + } + .with_metrics() + .await; + } +} From b8241ee3c0a203a44227dd13eb0c7840210f480d Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 13:31:02 +0100 Subject: [PATCH 12/38] docs: changeset for reporting authorization-rejected operations to Studio Describes the one user-visible effect of moving authorization out of the query planner: an operation refused outright now reaches Studio with a signature, referenced fields, and per-type stats, where before it contributed only to the operation count. States the licensed operation count explicitly, since a change to what Studio receives invites the question of what it costs. --- .../fix_bryn_router_1973_report_rejected_operations.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changesets/fix_bryn_router_1973_report_rejected_operations.md diff --git a/.changesets/fix_bryn_router_1973_report_rejected_operations.md b/.changesets/fix_bryn_router_1973_report_rejected_operations.md new file mode 100644 index 0000000000..c4b7890dae --- /dev/null +++ b/.changesets/fix_bryn_router_1973_report_rejected_operations.md @@ -0,0 +1,9 @@ +### Report operations rejected by authorization to Apollo Studio ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +When authorization refuses an operation outright, Apollo Studio now receives it as an operation, identified by the signature of the query the client sent and carrying the client name, version, and request count. Studio previously received the operation count alone and had nothing to attribute it to. + +A refused operation counts as one licensed operation. + +The `Authorization error` log event for a refused operation now appears in the `execution` span instead of the `query_planning` span. Update log or trace filters that match this event by span name. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 From fd7e1e96a9e5aa1d10e4a15ca13918f7ba0fe893 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 13:46:46 +0100 Subject: [PATCH 13/38] refactor: return authorization refusal as data rather than an error `filter_query` signalled a refused operation with `Err(QueryPlannerError::Unauthorized)`, so the query planner had to catch an error variant that meant success in order to build a 200 response from it. An authorization refusal is an outcome of filtering, not a failure of it. `filter_query` now returns `FilterResult`, which names its three outcomes: the operation is `Unchanged`, it is `Filtered` and carries the removed paths alongside the new document, or it is `Refused`. `Err` is left for the spec errors the filtering visitors raise. The query planner matches the three arms, which also drops the tuple that `clippy::type_complexity` needed a type alias to quieten. `QueryPlannerError::Unauthorized` had no handler beyond that one arm, so it goes with it. The response is untouched: `Refused` runs the same `UnauthorizedPaths` calls in the same order and returns the same `QueryPlannerContent::Response`. --- apollo-router/src/error.rs | 4 - .../src/plugins/authorization/mod.rs | 40 +++++++-- .../query_planner/query_planner_service.rs | 89 +++++++++---------- 3 files changed, 76 insertions(+), 57 deletions(-) diff --git a/apollo-router/src/error.rs b/apollo-router/src/error.rs index 9916a413dc..970afbb9a9 100644 --- a/apollo-router/src/error.rs +++ b/apollo-router/src/error.rs @@ -279,10 +279,6 @@ pub(crate) enum QueryPlannerError { /// spec error: {0} SpecError(SpecError), - // Safe to cache because user scopes and policies are included in the cache key. - /// Unauthorized field or type - Unauthorized(Vec), - /// Federation error: {0} FederationError(FederationErrorBridge), diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index 94aafcb868..4516fdc9d2 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -34,7 +34,6 @@ use crate::layers::ServiceBuilderExt; use crate::plugin::Plugin; use crate::plugin::PluginInit; use crate::plugins::authentication::APOLLO_AUTHENTICATION_JWT_CLAIMS; -use crate::query_planner::FilteredQuery; use crate::query_planner::QueryKey; use crate::services::execution; use crate::services::supergraph; @@ -138,6 +137,20 @@ pub(crate) struct UnauthorizedPaths { pub(crate) errors: ErrorConfig, } +/// What [`AuthorizationPlugin::filter_query`] did to an operation. +pub(crate) enum FilterResult { + /// The operation asks for nothing the request lacks authorization for. + Unchanged, + /// `document` is the operation with `paths` removed. + Filtered { + paths: Vec, + document: ast::Document, + }, + /// The whole operation is refused: filtering emptied the document, or + /// `reject_unauthorized` is set and the operation lost at least one path. + Refused { paths: Vec }, +} + impl UnauthorizedPaths { pub(crate) fn log_unauthorized_paths(&self) { // nothing to do if we have no paths or we're not supposed to log @@ -346,7 +359,7 @@ impl AuthorizationPlugin { configuration: &Conf, key: &QueryKey, schema: &Schema, - ) -> Result, QueryPlannerError> { + ) -> Result { let reject_unauthorized = configuration.directives.reject_unauthorized; let dry_run = configuration.directives.dry_run; @@ -375,7 +388,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Refused { + paths: unauthorized_paths, + }); } is_filtered = true; @@ -393,7 +408,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Refused { + paths: unauthorized_paths, + }); } is_filtered = true; @@ -411,7 +428,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Refused { + paths: unauthorized_paths, + }); } is_filtered = true; @@ -421,13 +440,18 @@ impl AuthorizationPlugin { }; if reject_unauthorized && !unauthorized_paths.is_empty() { - return Err(QueryPlannerError::Unauthorized(unauthorized_paths)); + return Ok(FilterResult::Refused { + paths: unauthorized_paths, + }); } if is_filtered { - Ok(Some((unauthorized_paths, doc))) + Ok(FilterResult::Filtered { + paths: unauthorized_paths, + document: doc, + }) } else { - Ok(None) + Ok(FilterResult::Unchanged) } } diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index f677af7949..46631b7e9c 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -7,7 +7,6 @@ use std::task::Poll; use std::time::Instant; use apollo_compiler::Name; -use apollo_compiler::ast; use apollo_federation::error::FederationError; use apollo_federation::error::SingleFederationError; use apollo_federation::query_plan::query_planner::QueryPlanOptions; @@ -31,11 +30,11 @@ use crate::error::QueryPlannerError; use crate::error::ServiceBuildError; use crate::error::ValidationErrors; use crate::graphql; -use crate::json_ext::Path; use crate::metrics::meter_provider; use crate::plugins::authorization; use crate::plugins::authorization::AuthorizationPlugin; use crate::plugins::authorization::CacheKeyMetadata; +use crate::plugins::authorization::FilterResult; use crate::plugins::authorization::UnauthorizedPaths; use crate::plugins::telemetry::config::ApolloSignatureNormalizationAlgorithm; use crate::plugins::telemetry::config::Conf as TelemetryConfig; @@ -439,9 +438,6 @@ impl Service for QueryPlannerService { } } -// Appease clippy::type_complexity -pub(crate) type FilteredQuery = (Vec, ast::Document); - impl QueryPlannerService { async fn get( &self, @@ -459,49 +455,52 @@ impl QueryPlannerService { // TODO(@goto-bus-stop): this is not a query planning concern let filter_res = if self.enable_authorization_directives { - match AuthorizationPlugin::filter_query(&self.authorization_config, &key, &self.schema) - { - Err(QueryPlannerError::Unauthorized(paths)) => { - let mut response = graphql::Response::builder().data(Value::Null).build(); - - if !paths.is_empty() { - let unauthorized = UnauthorizedPaths { - paths, - errors: self.authorization_config.error_config(), - }; - unauthorized.log_unauthorized_paths(); - unauthorized.update_response_with_unauthorized_path_errors(&mut response); - } - - return Ok(QueryPlannerContent::Response { - response: Box::new(response), - }); - } - other => other?, - } + AuthorizationPlugin::filter_query(&self.authorization_config, &key, &self.schema)? } else { - None + FilterResult::Unchanged }; - if let Some((unauthorized_paths, new_doc)) = filter_res { - let new_query = new_doc.to_string(); - let new_hash = self - .schema - .schema_id - .operation_hash(&new_query, key.operation_name.as_deref()); - - key.filtered_query = new_query; - let executable_document = new_doc - .to_executable_validate(self.schema.api_schema()) - .map_err(|e| QueryPlannerError::from(SpecError::ValidationError(e.into())))?; - doc = ParsedDocumentInner::new( - new_doc, - Arc::new(executable_document), - key.operation_name.as_deref(), - Arc::new(new_hash), - ) - .map_err(QueryPlannerError::from)?; - selections.unauthorized.paths = unauthorized_paths; + match filter_res { + FilterResult::Unchanged => {} + FilterResult::Refused { paths } => { + let mut response = graphql::Response::builder().data(Value::Null).build(); + + if !paths.is_empty() { + let unauthorized = UnauthorizedPaths { + paths, + errors: self.authorization_config.error_config(), + }; + unauthorized.log_unauthorized_paths(); + unauthorized.update_response_with_unauthorized_path_errors(&mut response); + } + + return Ok(QueryPlannerContent::Response { + response: Box::new(response), + }); + } + FilterResult::Filtered { + paths, + document: new_doc, + } => { + let new_query = new_doc.to_string(); + let new_hash = self + .schema + .schema_id + .operation_hash(&new_query, key.operation_name.as_deref()); + + key.filtered_query = new_query; + let executable_document = new_doc + .to_executable_validate(self.schema.api_schema()) + .map_err(|e| QueryPlannerError::from(SpecError::ValidationError(e.into())))?; + doc = ParsedDocumentInner::new( + new_doc, + Arc::new(executable_document), + key.operation_name.as_deref(), + Arc::new(new_hash), + ) + .map_err(QueryPlannerError::from)?; + selections.unauthorized.paths = paths; + } } if key.filtered_query != key.original_query { From 75bef51ea39a0ce9932c09d9a28dedcb3e5db436 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 14:12:18 +0100 Subject: [PATCH 14/38] test: pin a filtered operation that leaves no executable work Filtering removes `currentUser.phone`, emptying that selection, and `@skip(if: true)` removes the only other root field. The operation still has a definition, so filtering reports it as filtered rather than refused, and the plan comes back with no root node. That plan shape is indistinguishable from a refused operation: no root node, and unauthorized paths present. The responses differ though. This one carries a shaped `data` of `{"currentUser": null}` alongside the authorization error, where a refused operation carries `data: null`. Deciding refusal from the plan shape would answer this operation as a refusal and change its `data`. The test asserts no subgraph was called, which is what establishes that the plan held no executable work; without it the snapshot alone would not show that this is the colliding case. --- ...ial_filter_leaving_no_executable_work.snap | 21 +++++++ .../src/plugins/authorization/tests.rs | 62 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap diff --git a/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap new file mode 100644 index 0000000000..6cc5deb9ed --- /dev/null +++ b/apollo-router/src/plugins/authorization/snapshots/apollo_router__plugins__authorization__tests__partial_filter_leaving_no_executable_work.snap @@ -0,0 +1,21 @@ +--- +source: apollo-router/src/plugins/authorization/tests.rs +expression: body +--- +{ + "data": { + "currentUser": null + }, + "errors": [ + { + "message": "Unauthorized field or type", + "path": [ + "currentUser", + "phone" + ], + "extensions": { + "code": "UNAUTHORIZED_FIELD_OR_TYPE" + } + } + ] +} diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index a4ce9dd0be..b6bde68917 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -765,6 +765,68 @@ mod whole_query_rejection { } } +/// A partial filter can leave an operation with nothing to execute. Filtering removes +/// `phone`, emptying `currentUser`, and `@skip(if: true)` removes the only other root +/// field, so the plan comes back with no root node while the operation was filtered rather +/// than refused. +/// +/// The plan shape here matches what a refused operation produces, so anything deciding +/// refusal from the absence of a root node together with the presence of unauthorized +/// paths answers this operation as though the router had refused it. +#[tokio::test] +async fn partial_filter_leaving_no_executable_work() { + let handles: Arc>>> = + Arc::new(Mutex::new(Vec::new())); + let handles_clone = handles.clone(); + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(move |_name, _service| { + let (mock, handle) = tower_test::mock::pair::(); + handles_clone.lock().unwrap().push(handle); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + let req = graphql::Request { + query: Some( + "query { currentUser { phone } orga(id: 1) @skip(if: true) { name } }".to_string(), + ), + ..Default::default() + }; + let response = service + .oneshot(router::Request { + context: Context::new(), + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + }) + .await + .unwrap(); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + + // No subgraph answered, so the plan carried no executable work. + let handles: Vec<_> = handles.lock().unwrap().drain(..).collect(); + assert!(!handles.is_empty(), "no subgraph services were created"); + for handle in handles { + crate::plugin::test::assert_no_mock_calls(handle).await; + } + + insta::assert_json_snapshot!(body); +} + #[tokio::test] async fn authenticated_directive_dry_run() { let _guard = tracing_test::dispatcher_guard(); From 29867ed8b3087eca6d0be072bc128782d16467c7 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 15:39:19 +0100 Subject: [PATCH 15/38] test: pin the span and cardinality of the authorization error event The `Authorization error` event for a refused operation fires exactly once, inside the `query_planning` span, carrying the unauthorized paths. The unit tests cannot cover the span: it takes the telemetry plugin, which only joins the pipeline when OpenTelemetry is initialised for the process, so this spawns a real router and parses the JSON log's span list. Exactly-once is the assertion with teeth. The refusal is decided at a single place, so a second event for the same request means two code paths both believe they own the log. --- .../authorization_error_span.router.yaml | 8 +++ .../tests/integration/telemetry/logging.rs | 62 +++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml diff --git a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml new file mode 100644 index 0000000000..5173c12d2d --- /dev/null +++ b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml @@ -0,0 +1,8 @@ +telemetry: + exporters: + logging: + stdout: + format: json +authorization: + directives: + enabled: true diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index 91ba0b4685..ca6853735b 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -241,3 +241,65 @@ async fn test_text_sampler_off() -> Result<(), BoxError> { router.graceful_shutdown().await; Ok(()) } + +/// The `Authorization error` event for a refused operation belongs to the +/// `query_planning` span, where the query planner decides the refusal. The unit tests +/// around authorization cannot pin the span: it takes the telemetry plugin, which only +/// joins the pipeline when OpenTelemetry is initialised for the process, so a spawned +/// router is the smallest thing that has the full span hierarchy. +#[tokio::test(flavor = "multi_thread")] +async fn test_authorization_error_event_in_query_planning_span() -> Result<(), BoxError> { + let mut router = IntegrationTest::builder() + .config(include_str!( + "fixtures/authorization_error_span.router.yaml" + )) + .supergraph("tests/fixtures/supergraph-auth.graphql") + .build() + .await; + + router.start().await; + router.assert_started().await; + + // `Query.me` requires the `profile` scope, so an unauthenticated request loses its + // only root field and authorization refuses the operation. + router + .execute_query( + Query::builder() + .body(serde_json::json!({ "query": "{ me { name } }" })) + .build(), + ) + .await; + router.wait_for_log_message("Authorization error").await; + + let events: Vec = router + .logs() + .iter() + .filter(|line| line.contains("Authorization error")) + .map(|line| serde_json::from_str(line).expect("log line is JSON")) + .collect(); + + // Exactly once: a refusal is decided at a single place, so a second event for the + // same request means two code paths both believe they own the log. + assert_eq!(events.len(), 1, "events: {events:?}"); + + let event = &events[0]; + // The event records the paths with `?`, so they arrive debug-formatted as one string. + assert_eq!( + event.pointer("/unauthorized_query_paths"), + Some(&serde_json::json!(r#"["/me"]"#)) + ); + let span_names: Vec<&str> = event + .pointer("/spans") + .and_then(|spans| spans.as_array()) + .expect("json logs carry a span list") + .iter() + .filter_map(|span| span.get("name").and_then(|name| name.as_str())) + .collect(); + assert!( + span_names.contains(&"query_planning"), + "expected the event inside the query_planning span, got spans: {span_names:?}" + ); + + router.graceful_shutdown().await; + Ok(()) +} From 168ad57ccbbd8cb45436a6971ac419f47135db66 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 15:22:46 +0100 Subject: [PATCH 16/38] refactor: answer authorization refusals on the execution service The query planner answered a refused operation itself, returning `QueryPlannerContent::Response` instead of a plan. The planner now plans or errors, and the authorization plugin's execution-service layer answers refusals. `filter_query` loses the `reject_unauthorized` check entirely: that decision belongs to the layer, which receives the flag at plugin construction. `FilterResult::Refused` becomes `Emptied`, meaning only that filtering removed every selection. For an emptied operation the planner returns a plan with no root node, usage reporting generated from the original query with empty references (nothing resolves, so nothing is referenced), and `operation_emptied` set on `UnauthorizedPaths`, which rides the `Query` through the plan cache. The flag is not derivable downstream: a plan with no root node and non-empty paths also describes a partially filtered operation whose surviving selections are all statically skipped, pinned by `partial_filter_leaving_no_executable_work` as returning shaped data rather than a refusal. The layer is a `checkpoint_async` ahead of the existing authorization counter, so a refusal breaks before the counter fires and rejected operations stay uncounted, as they were when they never reached execution at all. It answers when the operation was emptied or when it holds `reject_unauthorized` and paths are present, with the same `data: null` response and `ErrorLocation` handling as before, byte-identical on the wire. Two observable changes, both in the changeset: - A refused operation reaches `CachingQueryPlanner` as a plan, so its usage reporting now lands in the context and Studio receives the operation signature. The licensed operation count stays 1: the reporting is `UsageReporting::Operation`, and telemetry previously billed the missing-reporting fallback at 1. - The `Authorization error` event moves from the `query_planning` span to `execution`, visible here as the assertion flip in the integration test. The unit harness cannot see the span (without process-wide OpenTelemetry init the telemetry plugin is absent and no `execution` span exists), so the unit helper now asserts the event fires exactly once and leaves the span to the integration test. The exactly-once assertion is load-bearing: the first version of this change logged in both the planner and the layer for an emptied operation, and nothing failed until that assertion existed. Its negative control (reintroducing the planner's log call) fails showing one event in `query_planning` and one in `execution`. --- .../src/plugins/authorization/mod.rs | 48 ++++++++---- .../src/plugins/authorization/tests.rs | 44 +++++++++-- .../query_planner/query_planner_service.rs | 75 +++++++++++-------- .../tests/integration/telemetry/logging.rs | 15 ++-- 4 files changed, 123 insertions(+), 59 deletions(-) diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index 4516fdc9d2..8a152be2b1 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -135,6 +135,13 @@ pub(crate) enum ErrorLocation { pub(crate) struct UnauthorizedPaths { pub(crate) paths: Vec, pub(crate) errors: ErrorConfig, + /// Whether filtering removed every selection, leaving the operation with nothing to + /// execute. + /// + /// A plan with no root node does not imply this on its own: an operation whose + /// surviving selections are all statically `@skip`ped also plans to no work. + #[serde(default)] + pub(crate) operation_emptied: bool, } /// What [`AuthorizationPlugin::filter_query`] did to an operation. @@ -146,9 +153,8 @@ pub(crate) enum FilterResult { paths: Vec, document: ast::Document, }, - /// The whole operation is refused: filtering emptied the document, or - /// `reject_unauthorized` is set and the operation lost at least one path. - Refused { paths: Vec }, + /// Filtering removed every selection, so nothing is left to plan. + Emptied { paths: Vec }, } impl UnauthorizedPaths { @@ -207,6 +213,7 @@ fn default_enable_directives() -> bool { pub(crate) struct AuthorizationPlugin { require_authentication: bool, + reject_unauthorized: bool, } impl AuthorizationPlugin { @@ -360,7 +367,6 @@ impl AuthorizationPlugin { key: &QueryKey, schema: &Schema, ) -> Result { - let reject_unauthorized = configuration.directives.reject_unauthorized; let dry_run = configuration.directives.dry_run; // The filtered query will then be used @@ -388,7 +394,7 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Refused { + return Ok(FilterResult::Emptied { paths: unauthorized_paths, }); } @@ -408,7 +414,7 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Refused { + return Ok(FilterResult::Emptied { paths: unauthorized_paths, }); } @@ -428,7 +434,7 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Refused { + return Ok(FilterResult::Emptied { paths: unauthorized_paths, }); } @@ -439,12 +445,6 @@ impl AuthorizationPlugin { } }; - if reject_unauthorized && !unauthorized_paths.is_empty() { - return Ok(FilterResult::Refused { - paths: unauthorized_paths, - }); - } - if is_filtered { Ok(FilterResult::Filtered { paths: unauthorized_paths, @@ -575,6 +575,7 @@ impl Plugin for AuthorizationPlugin { async fn new(init: PluginInit) -> Result { Ok(AuthorizationPlugin { require_authentication: init.config.require_authentication, + reject_unauthorized: init.config.directives.reject_unauthorized, }) } @@ -615,7 +616,28 @@ impl Plugin for AuthorizationPlugin { } fn execution_service(&self, service: execution::BoxCloneService) -> execution::BoxCloneService { + let reject_unauthorized = self.reject_unauthorized; + ServiceBuilder::new() + // Ahead of the counter below, so a refused operation stays uncounted. + .checkpoint_async(move |request: execution::Request| async move { + let unauthorized = request.query_plan.query.unauthorized.clone(); + + if unauthorized.operation_emptied + || (reject_unauthorized && !unauthorized.paths.is_empty()) + { + unauthorized.log_unauthorized_paths(); + + let mut response = graphql::Response::builder().data(Value::Null).build(); + unauthorized.update_response_with_unauthorized_path_errors(&mut response); + + return Ok(ControlFlow::Break( + execution::Response::new_from_graphql_response(response, request.context), + )); + } + + Ok(ControlFlow::Continue(request)) + }) .map_request(|request: execution::Request| { let filtered = !request.query_plan.query.unauthorized.paths.is_empty(); let needs_authenticated = request.context.contains_key(AUTHENTICATION_REQUIRED_KEY); diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index b6bde68917..1676c4a92d 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -43,8 +43,32 @@ fn assert_span_contains_authorization_error_event(span: &str) { assert!(contains_err_event_in_span.is_ok()); } +/// Asserts the `Authorization error` event for a refused operation was logged exactly +/// once. One place decides a refusal, so a second event means two code paths both +/// believe they own the log. +/// +/// The span the event belongs to is asserted by +/// `integration::telemetry::logging::test_authorization_error_event_in_execution_span`. +/// This harness runs without the telemetry plugin, so the `execution` span does not +/// exist here and the event lands directly in the `router` span. fn assert_logs_contain_entire_request_authorization_error() { - assert_span_contains_authorization_error_event("query_planning"); + let event_regex = + Regex::new(r"ERROR .*Authorization error unauthorized_query_paths=\[.*]$").unwrap(); + + let exactly_one = tracing_test::logs_assert(|lines| { + match lines + .iter() + .filter(|line| event_regex.captures(line).is_some()) + .count() + { + 1 => Ok(()), + n => Err(format!( + "expected exactly one authorization error event, found {n}:\n{}", + lines.join("\n") + )), + } + }); + assert!(exactly_one.is_ok(), "{exactly_one:?}"); } fn assert_logs_contain_partial_authorization_error() { @@ -667,20 +691,24 @@ mod whole_query_rejection { assert_eq!(status, http::StatusCode::OK); } - /// `CachingQueryPlanner` records usage reporting only for the `Plan` variant. - /// Telemetry still meters the rejection as one licensed operation but sends no - /// signature, referenced fields, or per-type stats, so Studio cannot attribute it. + /// A refused operation reaches `CachingQueryPlanner` as a plan, so its usage + /// reporting lands in the context like any other operation's and Studio can + /// attribute the refusal to an operation signature. + /// `licensed_operation_count_tests` pins what the report bills. #[tokio::test] - async fn does_not_record_usage_reporting() { + async fn records_usage_reporting() { let (service, _handles) = build_router_rejecting_whole_query().await; let context = Context::new(); send_rejected_request(service, context.clone()).await; + let usage_reporting = context + .extensions() + .with_lock(|lock| lock.get::>().cloned()) + .expect("a refused operation records usage reporting"); assert!( - !context - .extensions() - .with_lock(|lock| lock.contains_key::>()) + matches!(*usage_reporting, UsageReporting::Operation(_)), + "the report must carry operation details, not an error key: {usage_reporting:?}" ); } diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 46631b7e9c..b1f51366c5 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -6,6 +6,7 @@ use std::sync::OnceLock; use std::task::Poll; use std::time::Instant; +use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; use apollo_federation::error::FederationError; use apollo_federation::error::SingleFederationError; @@ -15,7 +16,6 @@ use futures::future::BoxFuture; use opentelemetry::KeyValue; use opentelemetry::metrics::MeterProvider as _; use opentelemetry::metrics::ObservableGauge; -use serde_json_bytes::Value; use tower::Service; use super::PlanNode; @@ -29,7 +29,6 @@ use crate::error::FederationErrorBridge; use crate::error::QueryPlannerError; use crate::error::ServiceBuildError; use crate::error::ValidationErrors; -use crate::graphql; use crate::metrics::meter_provider; use crate::plugins::authorization; use crate::plugins::authorization::AuthorizationPlugin; @@ -263,6 +262,7 @@ impl QueryPlannerService { unauthorized: UnauthorizedPaths { paths: vec![], errors: self.authorization_config.error_config(), + operation_emptied: false, }, subselections, defer_stats, @@ -462,20 +462,27 @@ impl QueryPlannerService { match filter_res { FilterResult::Unchanged => {} - FilterResult::Refused { paths } => { - let mut response = graphql::Response::builder().data(Value::Null).build(); - - if !paths.is_empty() { - let unauthorized = UnauthorizedPaths { - paths, - errors: self.authorization_config.error_config(), - }; - unauthorized.log_unauthorized_paths(); - unauthorized.update_response_with_unauthorized_path_errors(&mut response); - } + FilterResult::Emptied { paths } => { + selections.unauthorized.paths = paths; + selections.unauthorized.operation_emptied = true; + + // References come from the operation that ran, and nothing did. + let usage_reporting = generate_usage_reporting( + &doc.executable, + &ExecutableDocument::new(), + &key.operation_name, + self.schema.supergraph_schema(), + &self.signature_normalization_algorithm, + ); - return Ok(QueryPlannerContent::Response { - response: Box::new(response), + return Ok(QueryPlannerContent::Plan { + plan: Arc::new(super::QueryPlan { + usage_reporting: Arc::new(usage_reporting), + root: None, + formatted_query_plan: None, + query: Arc::new(selections), + estimated_size: Default::default(), + }), }); } FilterResult::Filtered { @@ -1132,21 +1139,29 @@ mod tests { .await .unwrap(); - match content { - QueryPlannerContent::Response { response } => { - assert_eq!( - response.errors.first().map(|e| e.message.as_str()), - Some("Unauthorized field or type") - ); - } - QueryPlannerContent::Plan { .. } => { - panic!( - "planner returned a query plan for an unauthenticated request; \ - filtering must reject instead, or an unfiltered plan would be handed \ - back for caching under default metadata" - ) - } - } + let QueryPlannerContent::Plan { plan } = content else { + panic!( + "a refusal must arrive as a plan, so the caching layer records its usage reporting" + ) + }; + assert!( + plan.root.is_none(), + "an unauthenticated request must not plan any work; a plan with fetches \ + cached under default metadata is reachable by any unauthenticated request" + ); + assert!( + plan.query.unauthorized.operation_emptied, + "the plan must mark the operation as emptied, or the execution layer \ + would run the two-pass formatting instead of refusing" + ); + assert_eq!( + plan.query + .unauthorized + .paths + .first() + .map(ToString::to_string), + Some("/me".to_string()) + ); } #[tokio::test] diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index ca6853735b..8fef98cfa9 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -242,13 +242,12 @@ async fn test_text_sampler_off() -> Result<(), BoxError> { Ok(()) } -/// The `Authorization error` event for a refused operation belongs to the -/// `query_planning` span, where the query planner decides the refusal. The unit tests -/// around authorization cannot pin the span: it takes the telemetry plugin, which only -/// joins the pipeline when OpenTelemetry is initialised for the process, so a spawned -/// router is the smallest thing that has the full span hierarchy. +/// The `Authorization error` event for a refused operation belongs to the `execution` +/// span. The unit tests around authorization cannot see this: the `execution` span comes +/// from the telemetry plugin, which only joins the pipeline when OpenTelemetry is +/// initialised for the process, so a spawned router is the smallest thing that has it. #[tokio::test(flavor = "multi_thread")] -async fn test_authorization_error_event_in_query_planning_span() -> Result<(), BoxError> { +async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxError> { let mut router = IntegrationTest::builder() .config(include_str!( "fixtures/authorization_error_span.router.yaml" @@ -296,8 +295,8 @@ async fn test_authorization_error_event_in_query_planning_span() -> Result<(), B .filter_map(|span| span.get("name").and_then(|name| name.as_str())) .collect(); assert!( - span_names.contains(&"query_planning"), - "expected the event inside the query_planning span, got spans: {span_names:?}" + span_names.contains(&"execution"), + "expected the event inside the execution span, got spans: {span_names:?}" ); router.graceful_shutdown().await; From 7abe084e07938af2f8546c8e5a7ad8685ffc9f5d Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 15:34:51 +0100 Subject: [PATCH 17/38] refactor: remove QueryPlannerContent::Response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The authorization layer on the execution service answers refused operations, so no producer of `QueryPlannerContent::Response` remains. The supergraph service's forwarding arm, the cache's size estimation arm, and the variant itself go together. `QueryPlannerContent` keeps its single `Plan` variant rather than collapsing into `QueryPlan`: flattening the type ripples through every construction and match in the crate, and is mechanical enough to review separately. `rejection_response_is_cached` becomes `refused_operation_plan_is_cached`, feeding the shape `QueryPlannerService::get` now returns for a refusal: a plan with no root node and the query marked emptied. What it pins is unchanged — a refusal is cached, and stays keyed by authorization state. The remaining matches on the enum simplify to irrefutable bindings. --- .../src/batching/query_plan_analysis_layer.rs | 36 +++++------ .../cost_calculator/static_cost.rs | 5 +- .../query_planner/caching_query_planner.rs | 46 ++++++------- .../query_planner/query_planner_service.rs | 64 +++++++------------ apollo-router/src/services/query_planner.rs | 1 - .../src/services/supergraph/service.rs | 3 - 6 files changed, 61 insertions(+), 94 deletions(-) diff --git a/apollo-router/src/batching/query_plan_analysis_layer.rs b/apollo-router/src/batching/query_plan_analysis_layer.rs index d473b02bd5..9ea42e880a 100644 --- a/apollo-router/src/batching/query_plan_analysis_layer.rs +++ b/apollo-router/src/batching/query_plan_analysis_layer.rs @@ -137,26 +137,22 @@ mod tests { ) -> Arc { let document = Query::parse_document(query, None, &schema, &configuration).unwrap(); - let QueryPlannerContent::Plan { - plan: query_plan, .. - } = QueryPlannerService::for_test(schema, configuration) - .unwrap() - .oneshot( - QueryPlannerRequest::builder() - .query(query) - .document(document) - .metadata(crate::plugins::authorization::CacheKeyMetadata::default()) - .plan_options(crate::services::PlanOptions::default()) - .compute_job_type(ComputeJobType::QueryPlanning) - .build(), - ) - .await - .unwrap() - .content - .unwrap() - else { - panic!("unexpected query planner output"); - }; + let QueryPlannerContent::Plan { plan: query_plan } = + QueryPlannerService::for_test(schema, configuration) + .unwrap() + .oneshot( + QueryPlannerRequest::builder() + .query(query) + .document(document) + .metadata(crate::plugins::authorization::CacheKeyMetadata::default()) + .plan_options(crate::services::PlanOptions::default()) + .compute_job_type(ComputeJobType::QueryPlanning) + .build(), + ) + .await + .unwrap() + .content + .unwrap(); query_plan } diff --git a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs index 89d88c24fb..3f79ac7145 100644 --- a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs +++ b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs @@ -877,10 +877,7 @@ mod tests { )) .await .unwrap(); - let query_plan = match planner_res.content.unwrap() { - QueryPlannerContent::Plan { plan } => plan, - _ => panic!("Query planner returned unexpected non-plan content"), - }; + let QueryPlannerContent::Plan { plan: query_plan } = planner_res.content.unwrap(); let schema = DemandControlledSchema::new(Arc::new(supergraph_schema)).unwrap(); let mut demand_controlled_subgraph_schemas = HashMap::new(); diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 873dd81990..629c3aba55 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -605,11 +605,10 @@ where match res { Ok(content) => { - if let QueryPlannerContent::Plan { plan, .. } = &content { - context.extensions().with_lock(|lock| { - lock.insert::>(plan.usage_reporting.clone()) - }); - } + let QueryPlannerContent::Plan { plan } = &content; + context.extensions().with_lock(|lock| { + lock.insert::>(plan.usage_reporting.clone()) + }); Ok(QueryPlannerResponse::builder().content(content).build()) } @@ -696,7 +695,6 @@ impl ValueType for Result> { fn estimated_size(&self) -> Option { match self { Ok(QueryPlannerContent::Plan { plan }) => Some(plan.estimated_size()), - Ok(QueryPlannerContent::Response { response }) => Some(estimate_size(response)), Err(e) => Some(estimate_size(e)), } } @@ -1966,29 +1964,27 @@ mod tests { crate::plugin::test::await_mock_driver(driver).await; } - /// `entry.insert` does not discriminate on the `QueryPlannerContent` variant, so the - /// cache stores an authorization rejection like any other planner output — and, like any - /// other planner output, it stays keyed by authorization state, so a rejection cached for - /// an unauthenticated request is never served to an authenticated one. + /// The cache stores a refused operation's plan like any other — and, like any other, + /// it stays keyed by authorization state, so a refusal cached for an unauthenticated + /// request is never served to an authenticated one. #[test(tokio::test)] - async fn rejection_response_is_cached() { + async fn refused_operation_plan_is_cached() { let (mock, handle) = tower_test::mock::pair::(); + // The plan `QueryPlannerService::get` returns for a refused operation: no root + // node, and the query marked as emptied. + let mut refused_query = Query::empty_for_tests(); + refused_query.unauthorized.operation_emptied = true; + let refusal_plan = QueryPlan { + usage_reporting: Arc::new(UsageReporting::Operation(Default::default())), + root: None, + formatted_query_plan: None, + query: Arc::new(refused_query), + estimated_size: Default::default(), + }; let (driver, planner_calls) = spawn_counting_planner( handle, - // The content `QueryPlannerService::get` returns for a whole-query rejection: - // null data carrying the unauthorized-path errors. - QueryPlannerContent::Response { - response: Box::new( - crate::graphql::Response::builder() - .data(crate::json_ext::Value::Null) - .error( - crate::graphql::Error::builder() - .message("Unauthorized field or type") - .extension_code("UNAUTHORIZED_FIELD_OR_TYPE") - .build(), - ) - .build(), - ), + QueryPlannerContent::Plan { + plan: Arc::new(refusal_plan), }, ); diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index b1f51366c5..217a62edb4 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -661,19 +661,11 @@ mod tests { .await .unwrap(); - if let QueryPlannerContent::Plan { plan, .. } = - response.content.expect("successful response") - { - insta::with_settings!({sort_maps => true}, { - insta::assert_json_snapshot!("plan_usage_reporting", plan.usage_reporting); - }); - insta::assert_debug_snapshot!( - "plan_root", - plan.root.as_deref().expect("non-empty plan") - ); - } else { - panic!("unexpected query planner content") - } + let QueryPlannerContent::Plan { plan } = response.content.expect("successful response"); + insta::with_settings!({sort_maps => true}, { + insta::assert_json_snapshot!("plan_usage_reporting", plan.usage_reporting); + }); + insta::assert_debug_snapshot!("plan_root", plan.root.as_deref().expect("non-empty plan")); } #[test(tokio::test)] @@ -736,10 +728,7 @@ mod tests { let content = response.content.expect("expected a successful response"); - let plan = match content { - QueryPlannerContent::Plan { plan, .. } => plan, - _ => panic!("expected a Plan response, received {content:?}"), - }; + let QueryPlannerContent::Plan { plan } = content; assert_eq!(plan.root, None, "expected an empty plan"); } @@ -1072,27 +1061,24 @@ mod tests { .await .unwrap(); - if let QueryPlannerContent::Plan { plan, .. } = result { - check_query_plan_coverage( - plan.root.as_ref().expect("non-empty query plan"), - None, - &plan.query.subselections, - ); + let QueryPlannerContent::Plan { plan } = result; + check_query_plan_coverage( + plan.root.as_ref().expect("non-empty query plan"), + None, + &plan.query.subselections, + ); - let mut keys: Vec = Vec::new(); - for (key, value) in plan.query.subselections.iter() { - let mut serialized = String::from("query"); - serialize_selection_set(&value.selection_set, &mut serialized); - keys.push(format!( - "{:?} {} {}", - key.defer_label, key.defer_conditions.bits, serialized - )) - } - keys.sort(); - keys.join("\n") - } else { - panic!() + let mut keys: Vec = Vec::new(); + for (key, value) in plan.query.subselections.iter() { + let mut serialized = String::from("query"); + serialize_selection_set(&value.selection_set, &mut serialized); + keys.push(format!( + "{:?} {} {}", + key.defer_label, key.defer_conditions.bits, serialized + )) } + keys.sort(); + keys.join("\n") } /// Warm-up reaches this service with `CacheKeyMetadata::default()`, the same metadata an @@ -1139,11 +1125,7 @@ mod tests { .await .unwrap(); - let QueryPlannerContent::Plan { plan } = content else { - panic!( - "a refusal must arrive as a plan, so the caching layer records its usage reporting" - ) - }; + let QueryPlannerContent::Plan { plan } = content; assert!( plan.root.is_none(), "an unauthenticated request must not plan any work; a plan with fetches \ diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index dd9099527e..0aaec84fbe 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -102,7 +102,6 @@ pub(crate) struct Response { #[derive(Debug, Clone, Serialize, Deserialize)] pub(crate) enum QueryPlannerContent { Plan { plan: Arc }, - Response { response: Box }, } #[buildstructor::buildstructor] diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index c662d8ef40..716abd97e7 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -271,9 +271,6 @@ async fn service_call( } match content { - Some(QueryPlannerContent::Response { response }) => Ok( - SupergraphResponse::new_from_graphql_response(*response, context), - ), Some(QueryPlannerContent::Plan { plan }) => { let is_deferred = plan.is_deferred(&variables); let is_subscription = plan.is_subscription(); From b7fed03def1a047ef587b7db6dd64d465de5b255 Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 16:27:13 +0100 Subject: [PATCH 18/38] test: address review feedback on authorization test helpers Assert the rejection by its error code rather than its message: the code is the contract, the wording is not. Reword the HTTP 200 doc along the lines review suggested: field-error treatment of a whole-operation refusal is historical, the spec arguably calls for a request error with a 4xx status and no `data` key, and `rejection_sends_null_data` pins the null-versus-absent distinction on the wire until spec compliance changes deliberately. The previous wording also described the pre-refactor mechanism, with the planner building the response. In the caching tests, derive the parsed document inside the request helper instead of taking it as a second argument that must agree with the query, name the helper for the authorization tests it serves, inline the one-argument planner wrapper, and cut the config helper's doc to one line. --- .../src/plugins/authorization/tests.rs | 21 +++-- .../query_planner/caching_query_planner.rs | 78 +++++++++++-------- 2 files changed, 53 insertions(+), 46 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 1676c4a92d..16b7e9ce6e 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -656,10 +656,9 @@ mod whole_query_rejection { .await .unwrap() .unwrap(); - assert_eq!( - body.errors.first().map(|e| e.message.as_str()), - Some("Unauthorized field or type"), - "the operation was not rejected on authorization grounds" + assert!( + body.contains_error_code("UNAUTHORIZED_FIELD_OR_TYPE"), + "the operation was not rejected on authorization grounds: {body:?}" ); status @@ -674,14 +673,12 @@ mod whole_query_rejection { assert_no_subgraph_calls(handles).await; } - /// The status comes from the short-circuit in the query planner, not from response - /// formatting: `filter_query` returns `Err(Unauthorized)`, `QueryPlannerService::get` - /// builds a `graphql::Response` with `data: null` directly, and - /// `SupergraphResponse::new_from_graphql_response` wraps it with `http::Response::new`, - /// which is 200 regardless of errors or data shape. Execution and value completion never - /// run — see `does_not_reach_execution`. 200 with errors is the right answer for a - /// rejection with field-error semantics, so this pins that the short-circuit does not - /// pick up an error status along the way. + /// We historically treat authorization errors as field errors, even when the whole + /// operation is refused: null data, auth errors, and status code 200. The GraphQL + /// spec arguably calls for a request error here, with a 4xx status and `data` left + /// out entirely; `rejection_sends_null_data` pins the null-versus-absent side of + /// that on the wire. Until spec compliance changes deliberately, these pin what the + /// router sends. #[tokio::test] async fn returns_http_200() { let (service, _handles) = build_router_rejecting_whole_query().await; diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 629c3aba55..92df0a4116 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1866,45 +1866,28 @@ mod tests { (driver, calls) } - async fn caching_planner_for_test( - mock: tower_test::mock::Mock, - schema: &Arc, - configuration: &Configuration, - ) -> impl Service { - CachingQueryPlanner::for_test( - mock.map_err(|err| panic!("tower-test errored: {err}")), - schema.clone(), - Default::default(), - configuration, - ) - .await - .unwrap() - } - - /// A configuration and schema pair for which `AuthorizationPlugin::enable_directives` is - /// true. Without that, `plan` never calls `update_cache_key` and every request is keyed - /// under `CacheKeyMetadata::default()`, so no segmentation can be observed. + /// A configuration and schema pair that enables auth directives to work. fn authorization_enabled_config_and_schema() -> (Configuration, Arc) { let configuration: Configuration = serde_json::from_value(serde_json::json!({ "authorization": { "directives": { "enabled": true } } })) .unwrap(); - // Links `requiresScopes`; a schema with no authorization spec keeps - // `enable_directives` false whatever the configuration says. let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); (configuration, schema) } - /// Builds a request the way the router does: authorization state travels in the context - /// as JWT claims, and `plan` derives `CacheKeyMetadata` from them via - /// `AuthorizationPlugin::update_cache_key`. Inserting `CacheKeyMetadata` into the - /// context directly would not survive — `update_cache_key` overwrites it. - fn caching_request( + /// Builds a request the way the router does: authorization state travels in the + /// context as JWT claims, and `plan` derives `CacheKeyMetadata` from them via + /// `AuthorizationPlugin::update_cache_key`, overwriting any metadata inserted into + /// the context directly. + fn authorization_caching_request( query: &str, - doc: &ParsedDocument, + schema: &Schema, + configuration: &Configuration, authenticated: bool, ) -> query_planner::CachingRequest { + let doc = Query::parse_document(query, None, schema, configuration).unwrap(); let context = Context::new(); if authenticated { context @@ -1912,7 +1895,7 @@ mod tests { .unwrap(); } context.extensions().with_lock(|lock| { - lock.insert::(doc.clone()); + lock.insert::(doc); }); query_planner::CachingRequest::new(query.to_string(), None, context) } @@ -1935,10 +1918,16 @@ mod tests { ); let (configuration, schema) = authorization_enabled_config_and_schema(); - let mut service = caching_planner_for_test(mock, &schema, &configuration).await; + let mut service = CachingQueryPlanner::for_test( + mock.map_err(|err| panic!("tower-test errored: {err}")), + schema.clone(), + Default::default(), + &configuration, + ) + .await + .unwrap(); let query = "query ExampleQuery { me { name } }"; - let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); for authenticated in [ false, true, // Repeats the first key, which must now hit the cache. @@ -1948,7 +1937,12 @@ mod tests { .ready() .await .unwrap() - .call(caching_request(query, &doc, authenticated)) + .call(authorization_caching_request( + query, + &schema, + &configuration, + authenticated, + )) .await .unwrap(); } @@ -1989,17 +1983,28 @@ mod tests { ); let (configuration, schema) = authorization_enabled_config_and_schema(); - let mut service = caching_planner_for_test(mock, &schema, &configuration).await; + let mut service = CachingQueryPlanner::for_test( + mock.map_err(|err| panic!("tower-test errored: {err}")), + schema.clone(), + Default::default(), + &configuration, + ) + .await + .unwrap(); let query = "query ExampleQuery { me { name } }"; - let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); for _ in 0..2 { service .ready() .await .unwrap() - .call(caching_request(query, &doc, false)) + .call(authorization_caching_request( + query, + &schema, + &configuration, + false, + )) .await .unwrap(); } @@ -2014,7 +2019,12 @@ mod tests { .ready() .await .unwrap() - .call(caching_request(query, &doc, true)) + .call(authorization_caching_request( + query, + &schema, + &configuration, + true, + )) .await .unwrap(); From db487c70bae8315f223bb3f9efb162ccf693a93c Mon Sep 17 00:00:00 2001 From: bryn Date: Mon, 17 Aug 2026 22:58:52 +0100 Subject: [PATCH 19/38] test: pin the multi-operation gap in document emptiness, rename the field to match `filter_query` reports `Emptied` from a document-wide `definitions.is_empty()` check, so fully filtering the executed operation while a sibling operation shares the document reports `Filtered` instead. Planning then fails to find the executed operation, and the client receives 400 `GRAPHQL_UNKNOWN_OPERATION_NAME` with no mention of authorization, where the single-operation case receives the refusal response. The `FIXME`s in `filter_query` point at exactly this check; the new test pins the outcome so resolving them changes it deliberately. `operation_emptied` becomes `document_emptied`: the field records what the check measures, and the old name claimed the stronger per-operation meaning. The field doc gains the two counterexamples that make it non-derivable from the plan and the multi-operation case that bounds what it means. --- .../src/plugins/authorization/mod.rs | 20 +++++--- .../src/plugins/authorization/tests.rs | 50 +++++++++++++++++++ .../query_planner/caching_query_planner.rs | 2 +- .../query_planner/query_planner_service.rs | 8 +-- 4 files changed, 68 insertions(+), 12 deletions(-) diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index 8a152be2b1..b4e41b2db4 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -135,13 +135,17 @@ pub(crate) enum ErrorLocation { pub(crate) struct UnauthorizedPaths { pub(crate) paths: Vec, pub(crate) errors: ErrorConfig, - /// Whether filtering removed every selection, leaving the operation with nothing to - /// execute. + /// Whether filtering removed every definition from the document. /// - /// A plan with no root node does not imply this on its own: an operation whose - /// surviving selections are all statically `@skip`ped also plans to no work. + /// The plan's shape does not imply this on its own. A plan with no root node also + /// describes a partially filtered operation whose surviving selections are all + /// statically `@skip`ped, and adding `filtered_query.is_none()` still matches a + /// `dry_run` over an all-skipped operation, which modifies nothing. + /// + /// Document, not operation: when another operation shares the document, fully + /// filtering the executed one leaves the document non-empty and this stays false. #[serde(default)] - pub(crate) operation_emptied: bool, + pub(crate) document_emptied: bool, } /// What [`AuthorizationPlugin::filter_query`] did to an operation. @@ -153,7 +157,9 @@ pub(crate) enum FilterResult { paths: Vec, document: ast::Document, }, - /// Filtering removed every selection, so nothing is left to plan. + /// Filtering removed every definition from the document, so nothing is left to + /// plan. A fully filtered operation in a document that other operations keep + /// non-empty is reported as `Filtered`. Emptied { paths: Vec }, } @@ -623,7 +629,7 @@ impl Plugin for AuthorizationPlugin { .checkpoint_async(move |request: execution::Request| async move { let unauthorized = request.query_plan.query.unauthorized.clone(); - if unauthorized.operation_emptied + if unauthorized.document_emptied || (reject_unauthorized && !unauthorized.paths.is_empty()) { unauthorized.log_unauthorized_paths(); diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 16b7e9ce6e..2875a4075d 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -664,6 +664,56 @@ mod whole_query_rejection { status } + /// Fully filtering the executed operation while a sibling operation shares the + /// document reports `Filtered`, not `Emptied`: the emptiness check is document-wide. + /// Planning then fails to find the executed operation, so the client is told the + /// operation is unknown rather than refused. + /// + /// This pins the mismatch the `FIXME`s in `filter_query` point at. Resolving them + /// changes this response deliberately; the single-operation refusal contract is + /// pinned by the rest of this module. + #[tokio::test] + async fn fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation() { + let (service, handles) = build_rejecting_router(serde_json::json!({ + "enabled": true + })) + .await; + + let req = graphql::Request { + query: Some( + "query A { orga(id: 1) { id } } query B { currentUser { name } }".to_string(), + ), + operation_name: Some("A".to_string()), + ..Default::default() + }; + let response = service + .oneshot(router::Request { + context: Context::new(), + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + }) + .await + .unwrap(); + + assert_eq!(response.response.status(), http::StatusCode::BAD_REQUEST); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("GRAPHQL_UNKNOWN_OPERATION_NAME")), + "body: {body}" + ); + assert_eq!(body.get("data"), None); + + assert_no_subgraph_calls(handles).await; + } + #[tokio::test] async fn does_not_reach_execution() { let (service, handles) = build_router_rejecting_whole_query().await; diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 92df0a4116..3e13f7f6f9 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1967,7 +1967,7 @@ mod tests { // The plan `QueryPlannerService::get` returns for a refused operation: no root // node, and the query marked as emptied. let mut refused_query = Query::empty_for_tests(); - refused_query.unauthorized.operation_emptied = true; + refused_query.unauthorized.document_emptied = true; let refusal_plan = QueryPlan { usage_reporting: Arc::new(UsageReporting::Operation(Default::default())), root: None, diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 217a62edb4..821b696dea 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -262,7 +262,7 @@ impl QueryPlannerService { unauthorized: UnauthorizedPaths { paths: vec![], errors: self.authorization_config.error_config(), - operation_emptied: false, + document_emptied: false, }, subselections, defer_stats, @@ -464,7 +464,7 @@ impl QueryPlannerService { FilterResult::Unchanged => {} FilterResult::Emptied { paths } => { selections.unauthorized.paths = paths; - selections.unauthorized.operation_emptied = true; + selections.unauthorized.document_emptied = true; // References come from the operation that ran, and nothing did. let usage_reporting = generate_usage_reporting( @@ -1132,8 +1132,8 @@ mod tests { cached under default metadata is reachable by any unauthenticated request" ); assert!( - plan.query.unauthorized.operation_emptied, - "the plan must mark the operation as emptied, or the execution layer \ + plan.query.unauthorized.document_emptied, + "the plan must mark the document as emptied, or the execution layer \ would run the two-pass formatting instead of refusing" ); assert_eq!( From b9d5647cd9c752a0b52a45334518dcb115a3d2c8 Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 09:50:25 +0100 Subject: [PATCH 20/38] docs: authorization and query planning before and after ROUTER-1973 Sequence diagrams of both states, what dropping `document_emptied` would change (measured wire outputs, security analysis, cost of keeping it), and the multi-operation gap where a fully filtered operation beside a surviving sibling reports GRAPHQL_UNKNOWN_OPERATION_NAME instead of a refusal. Written for whoever picks up the spec-compliance follow-up. --- dev-docs/authorization-query-planning.md | 217 +++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 dev-docs/authorization-query-planning.md diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md new file mode 100644 index 0000000000..316f345d1b --- /dev/null +++ b/dev-docs/authorization-query-planning.md @@ -0,0 +1,217 @@ +# Authorization and query planning + +Scratch notes for ROUTER-1973 (separate authorization from query planning). +The before section describes `dev-v3.x` at `639c65a88`; the after section describes the +branch as implemented. File references are `file:line` on the branch. + +## Before + +```mermaid +sequenceDiagram + autonumber + participant C as Client + participant PQ as ParseQueryLayer + participant EX as ExtractAuthChecksLayer + participant PL as Auth / policy plugins + participant SG as SupergraphService + participant CQP as CachingQueryPlanner + participant QPS as QueryPlannerService + participant ES as ExecutionService + participant SUB as Subgraphs + + C->>PQ: POST /graphql + PQ->>EX: parse, ParsedDocument in context + Note over EX: generate_cache_metadata
= what the document REQUIRES + EX->>PL: request + Note over PL: authentication sets JWT claims
policies resolve to true or false + PL->>SG: supergraph Request + SG->>CQP: CachingRequest + Note over CQP: update_cache_key
= what the request was GRANTED + CQP->>CQP: key on ORIGINAL text + metadata + + alt cache miss + CQP->>QPS: QueryPlannerRequest + QPS->>QPS: filter_query + alt document emptied, or reject_unauthorized + QPS-->>CQP: QueryPlannerContent::Response + Note right of QPS: the planner ANSWERS the request + else filtering succeeded + QPS-->>CQP: QueryPlannerContent::Plan + end + CQP->>CQP: cache either variant + end + + alt Response variant + SG-->>C: 200, data null and errors + else Plan variant + SG->>ES: execution Request + ES->>SUB: fetches + Note over ES: errors, then format twice:
FILTERED shape, ORIGINAL shape + ES-->>C: 200, nulls and authorization errors + end +``` + +## After + +The planner plans or errors. The decision to refuse lives on the execution service. + +```mermaid +sequenceDiagram + autonumber + participant C as Client + participant SG as SupergraphService + participant CQP as CachingQueryPlanner + participant QPS as QueryPlannerService + participant AL as AuthorizationLayer (execution) + participant ES as ExecutionService + participant SUB as Subgraphs + + C->>SG: request (parse and auth extraction as before) + SG->>CQP: CachingRequest + CQP->>CQP: key on ORIGINAL text + metadata + + alt cache miss + CQP->>QPS: QueryPlannerRequest + QPS->>QPS: filter_query, no reject_unauthorized input + alt FilterResult::Emptied + QPS-->>CQP: Plan with root None,
document_emptied set,
usage reporting from the ORIGINAL query + else Filtered or Unchanged + QPS-->>CQP: Plan + end + CQP->>CQP: cache the plan + end + Note over CQP: usage reporting inserted for EVERY plan,
so refusals reach Studio attributed + + CQP-->>SG: Plan + SG->>AL: execution Request + Note over AL: refuse when document_emptied,
or reject_unauthorized (held from config)
and paths are present + alt refused + AL-->>C: 200, data null and errors,
logs Authorization error once,
inside the EXECUTION span + else continue + AL->>ES: request (authorization counter fires here) + ES->>SUB: fetches from the filtered plan + Note over ES: errors, then format twice:
FILTERED shape, ORIGINAL shape + ES-->>C: 200, nulls and authorization errors + end +``` + +Key references, on the branch: + +| What | Where | +| --- | --- | +| `FilterResult` (Unchanged / Filtered / Emptied) | `plugins/authorization/mod.rs:152` | +| `document_emptied` on `UnauthorizedPaths` | `plugins/authorization/mod.rs:148` | +| `filter_query`, no config-driven refusal | `plugins/authorization/mod.rs:371` | +| The layer: `checkpoint_async` ahead of the counter | `plugins/authorization/mod.rs:629` | +| Planner's `Emptied` arm, empty plan | `query_planner/query_planner_service.rs:465` | +| Single-variant `QueryPlannerContent` | `services/query_planner.rs:103` | +| Two-pass formatting (unchanged) | `services/execution/service.rs:291` | + +Behaviour intentionally identical to before, byte-level, in every `ErrorLocation` mode. +Two observable changes, both in the changeset: refusals reach Studio attributed, and the +`Authorization error` event moves from the `query_planning` span to `execution`. + +## Known gap, pinned but unresolved: multi-operation documents + +`Emptied` means the *document* emptied, not the executed operation. `filter_query` +checks `filtered_doc.definitions.is_empty()` after each directive stage, and +definitions include every operation and fragment in the document, executed or not. + +The consequence, measured on the branch (`orga.id` is `@authenticated`, request is +unauthenticated, directives enabled): + +```graphql +# operationName: "A" +query A { orga(id: 1) { id } } +query B { currentUser { name } } +``` + +Filtering empties `A` and removes it from the document. `B` keeps the document +non-empty, so `filter_query` reports `Filtered`, not `Emptied`. The planner then looks +up operation `A` in a document that no longer contains it: + +``` +400 {"errors":[{"message":"Unknown operation named \"A\"", + "extensions":{"code":"GRAPHQL_UNKNOWN_OPERATION_NAME"}}]} +``` + +Send `query A` alone and the same refusal produces +`200 {"data":null,"errors":[{...,"code":"UNAUTHORIZED_FIELD_OR_TYPE"}]}`. The same +authorization outcome yields two different statuses, two different error codes, and in +the multi-operation case tells the client their operation does not exist rather than +that it was refused. + +The three `FIXME`s in `filter_query` mark the check +(`consider only filtered_doc.operations.get(key.operation_name)?`). Resolving them — +checking emptiness of the executed operation instead of the document — would fold this +case into the ordinary refusal path. That is a behaviour change: the 400 becomes the +refusal response, so it belongs to the spec-compliance follow-up, which is deciding +what the refusal response is anyway. + +Pinned by `fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation` +in `plugins/authorization/tests.rs`, which asserts the 400, the error code, the absent +`data` key, and that no subgraph was contacted. + +## Dropping `document_emptied` + +Measured by deleting the layer's `document_emptied` clause and sending an emptied +operation (`{ orga(id: 1) { id } }`, `orga.id` `@authenticated`, unauthenticated, +directives enabled, no `reject_unauthorized`): + +``` +with the flag: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} +without the flag: 200 {"data":{"orga":null}, "errors":[{...,"path":["orga","id"]}]} +``` + +Same status, same errors, same paths. The flag's entire effect is `data: null` versus a +shaped `data` with null roots. + +What actually gets deleted: the field, its serialization, and the layer's first clause. +The `FilterResult::Emptied` variant stays — an empty document fails executable +validation, so the planner cannot treat it as ordinary `Filtered`; it still returns the +empty plan. `reject_unauthorized` is untouched: the layer holds it from config and keeps +answering `data: null` for those. + +The shaped response is the more spec-aligned of the two: a field error on a nullable +root field nulls that field; `data: null` belongs to non-null propagation reaching the +root. Neither matches the other candidate reading, a request error (4xx, no `data` key). +Deciding between those two is the spec-compliance follow-up; today's `data: null` at 200 +is a third shape that satisfies neither and survives as preserved history. + +### Security consequences + +None found. The load-bearing properties do not involve the flag: + +- Subgraphs are unreachable either way. The refusal's plan has `root: None`, so + execution has no fetch nodes; the flag only decides who formats the empty result. + The `@authenticated` field's value never leaves a subgraph because no subgraph is + asked. +- Over-fetch stripping is untouched. The filtered-then-original formatting passes and + their ordering (`overfetched_unauthorized_field_is_not_returned`) sit below the + layer and do not read the flag. +- Cache poisoning is unchanged. The refusal plan is cached keyed by + `CacheKeyMetadata`, flag or no flag; an unauthenticated request can only ever hit an + entry planned for unauthenticated metadata. +- The error paths disclose the same information in both shapes: the paths in the + errors already name every refused field, so `{"orga": null}` reveals nothing that + `data: null` conceals. + +The one behavioural wrinkle: with `errors.response: disabled`, an emptied operation +returns shaped nulls with no errors instead of `data: null` with no errors — a response +indistinguishable from every root field genuinely being null. `data: null` today is +almost as ambiguous. Operators choosing `disabled` have opted out of the signal either +way; noted for the changeset if the flag goes. + +### Cost of keeping it + +One serialized bool, the layer clause, a nine-line field doc carrying two +counterexamples (`@skip`-emptied partial filters, `dry_run` over all-skipped +operations), and the recurring explanation of why the flag cannot be derived from the +plan. The flag is scaffolding for one byte-level compatibility: delete it the moment +the spec follow-up decides the refusal shape. + +## Still open elsewhere + +Keying the plan cache on the filtered query text (making `CacheKeyMetadata` redundant +in the key, deduplicating plans across grant sets that filter identically) remains +unimplemented and belongs with the cache-key work, not this ticket. From f40c739065bb470a72116cb7d1b3d232d32f4667 Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 10:44:01 +0100 Subject: [PATCH 21/38] feat!: run emptied operations through the normal response pipeline An operation that authorization filtering empties no longer short-circuits to `data: null`. The planner still returns a plan with no root node, execution fetches nothing, and response formatting against the original operation shapes the result: each requested root field null, one error per removed path, matching what a partial filter produces. `reject_unauthorized` keeps answering `data: null` from the execution-service layer, which holds the flag from configuration. This deletes `document_emptied` and the layer's clause reading it. The field carried one bit through the plan cache to preserve `data: null` for exactly this case, could not be derived from the plan (a rootless plan equally describes an all-skipped partial filter; `filtered_query` absence equally describes `dry_run`), and its serialized form was the only thing distinguishing two configurations whose responses now match. The shaped response is also the field-error reading of the GraphQL spec; `data: null` belonged to neither that nor the request-error reading. The `Authorization error` event for an emptied operation now comes from the execution service alongside error attachment, inside `format_response` under `execution`. The integration span test holds: it asserts containment in `execution`, which covers both the layer's event for config refusals and the execution service's for emptied operations, still exactly once. The breaking changeset carries the client-visible shape change; the reporting changeset's span wording widens from "in the execution span" to "under" it. Written test-first: `emptied_operation_without_reject_returns_shaped_data` asserted the shaped response and failed with `data: null` before the code changed. --- ...ter_1973_emptied_operations_shaped_data.md | 11 ++++ ..._router_1973_report_rejected_operations.md | 2 +- .../src/plugins/authorization/mod.rs | 23 ++------ .../src/plugins/authorization/tests.rs | 55 +++++++++++++++++++ .../query_planner/caching_query_planner.rs | 21 ++++--- .../query_planner/query_planner_service.rs | 7 --- 6 files changed, 82 insertions(+), 37 deletions(-) create mode 100644 .changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md diff --git a/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md new file mode 100644 index 0000000000..4b7963fd27 --- /dev/null +++ b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md @@ -0,0 +1,11 @@ +### Fully unauthorized operations return the same response shape as partially unauthorized ones ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +When authorization directives remove every field from an operation, the response now carries each requested root field as `null` alongside the authorization errors, matching what clients already receive when only some fields are removed: + +```json +{"data": {"orga": null}, "errors": [{"message": "Unauthorized field or type", "path": ["orga", "id"], "extensions": {"code": "UNAUTHORIZED_FIELD_OR_TYPE"}}]} +``` + +Such responses previously carried `"data": null`. Clients that detect a fully refused operation by checking `data` for `null` should check for errors with the `UNAUTHORIZED_FIELD_OR_TYPE` code instead, which covers partial refusals as well. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/.changesets/fix_bryn_router_1973_report_rejected_operations.md b/.changesets/fix_bryn_router_1973_report_rejected_operations.md index c4b7890dae..663012c9dd 100644 --- a/.changesets/fix_bryn_router_1973_report_rejected_operations.md +++ b/.changesets/fix_bryn_router_1973_report_rejected_operations.md @@ -4,6 +4,6 @@ When authorization refuses an operation outright, Apollo Studio now receives it A refused operation counts as one licensed operation. -The `Authorization error` log event for a refused operation now appears in the `execution` span instead of the `query_planning` span. Update log or trace filters that match this event by span name. +The `Authorization error` log event for a refused operation now appears under the `execution` span instead of inside `query_planning`. Update log or trace filters that match this event by span name. By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index b4e41b2db4..2c708bf30a 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -135,17 +135,6 @@ pub(crate) enum ErrorLocation { pub(crate) struct UnauthorizedPaths { pub(crate) paths: Vec, pub(crate) errors: ErrorConfig, - /// Whether filtering removed every definition from the document. - /// - /// The plan's shape does not imply this on its own. A plan with no root node also - /// describes a partially filtered operation whose surviving selections are all - /// statically `@skip`ped, and adding `filtered_query.is_none()` still matches a - /// `dry_run` over an all-skipped operation, which modifies nothing. - /// - /// Document, not operation: when another operation shares the document, fully - /// filtering the executed one leaves the document non-empty and this stays false. - #[serde(default)] - pub(crate) document_emptied: bool, } /// What [`AuthorizationPlugin::filter_query`] did to an operation. @@ -158,8 +147,9 @@ pub(crate) enum FilterResult { document: ast::Document, }, /// Filtering removed every definition from the document, so nothing is left to - /// plan. A fully filtered operation in a document that other operations keep - /// non-empty is reported as `Filtered`. + /// plan and the operation proceeds as a plan with no work. A fully filtered + /// operation in a document that other operations keep non-empty is reported as + /// `Filtered`. Emptied { paths: Vec }, } @@ -627,11 +617,8 @@ impl Plugin for AuthorizationPlugin { ServiceBuilder::new() // Ahead of the counter below, so a refused operation stays uncounted. .checkpoint_async(move |request: execution::Request| async move { - let unauthorized = request.query_plan.query.unauthorized.clone(); - - if unauthorized.document_emptied - || (reject_unauthorized && !unauthorized.paths.is_empty()) - { + if reject_unauthorized && !request.query_plan.query.unauthorized.paths.is_empty() { + let unauthorized = request.query_plan.query.unauthorized.clone(); unauthorized.log_unauthorized_paths(); let mut response = graphql::Response::builder().data(Value::Null).build(); diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 2875a4075d..2ee5de676c 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -714,6 +714,61 @@ mod whole_query_rejection { assert_no_subgraph_calls(handles).await; } + /// Without `reject_unauthorized`, an operation that filtering empties runs through + /// the same pipeline as a partial filter: an empty plan executes nothing, and + /// response formatting against the original operation shapes the result. The client + /// receives each requested root field as null alongside the path errors, exactly as + /// it would if some fields had survived. + #[tokio::test] + async fn emptied_operation_without_reject_returns_shaped_data() { + let (service, handles) = build_rejecting_router(serde_json::json!({ + "enabled": true + })) + .await; + + let req = graphql::Request { + // `orga.id` is `@authenticated` and the only selection, so filtering + // removes everything. + query: Some("query { orga(id: 1) { id } }".to_string()), + ..Default::default() + }; + let response = service + .oneshot(router::Request { + context: Context::new(), + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + }) + .await + .unwrap(); + + assert_eq!(response.response.status(), http::StatusCode::OK); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!( + body.get("data"), + Some(&serde_json::json!({ "orga": null })), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/path"), + Some(&serde_json::json!(["orga", "id"])), + "body: {body}" + ); + + assert_no_subgraph_calls(handles).await; + } + #[tokio::test] async fn does_not_reach_execution() { let (service, handles) = build_router_rejecting_whole_query().await; diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 3e13f7f6f9..ddf856ed4d 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1958,27 +1958,26 @@ mod tests { crate::plugin::test::await_mock_driver(driver).await; } - /// The cache stores a refused operation's plan like any other — and, like any other, - /// it stays keyed by authorization state, so a refusal cached for an unauthenticated - /// request is never served to an authenticated one. + /// The cache stores an emptied operation's plan like any other — and, like any + /// other, it stays keyed by authorization state, so a plan cached for an + /// unauthenticated request is never served to an authenticated one. #[test(tokio::test)] - async fn refused_operation_plan_is_cached() { + async fn emptied_operation_plan_is_cached() { let (mock, handle) = tower_test::mock::pair::(); - // The plan `QueryPlannerService::get` returns for a refused operation: no root - // node, and the query marked as emptied. - let mut refused_query = Query::empty_for_tests(); - refused_query.unauthorized.document_emptied = true; - let refusal_plan = QueryPlan { + // The plan `QueryPlannerService::get` returns for an emptied operation: no + // root node. + let emptied_query = Query::empty_for_tests(); + let emptied_plan = QueryPlan { usage_reporting: Arc::new(UsageReporting::Operation(Default::default())), root: None, formatted_query_plan: None, - query: Arc::new(refused_query), + query: Arc::new(emptied_query), estimated_size: Default::default(), }; let (driver, planner_calls) = spawn_counting_planner( handle, QueryPlannerContent::Plan { - plan: Arc::new(refusal_plan), + plan: Arc::new(emptied_plan), }, ); diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 821b696dea..13f61663ea 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -262,7 +262,6 @@ impl QueryPlannerService { unauthorized: UnauthorizedPaths { paths: vec![], errors: self.authorization_config.error_config(), - document_emptied: false, }, subselections, defer_stats, @@ -464,7 +463,6 @@ impl QueryPlannerService { FilterResult::Unchanged => {} FilterResult::Emptied { paths } => { selections.unauthorized.paths = paths; - selections.unauthorized.document_emptied = true; // References come from the operation that ran, and nothing did. let usage_reporting = generate_usage_reporting( @@ -1131,11 +1129,6 @@ mod tests { "an unauthenticated request must not plan any work; a plan with fetches \ cached under default metadata is reachable by any unauthenticated request" ); - assert!( - plan.query.unauthorized.document_emptied, - "the plan must mark the document as emptied, or the execution layer \ - would run the two-pass formatting instead of refusing" - ); assert_eq!( plan.query .unauthorized From ecd91ed3089c39baa6d0d901139e106a3d0ece8a Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 10:44:01 +0100 Subject: [PATCH 22/38] docs: emptied operations run the normal pipeline, marker considered and rejected --- dev-docs/authorization-query-planning.md | 90 ++++++++---------------- 1 file changed, 31 insertions(+), 59 deletions(-) diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md index 316f345d1b..1408756fb6 100644 --- a/dev-docs/authorization-query-planning.md +++ b/dev-docs/authorization-query-planning.md @@ -74,7 +74,7 @@ sequenceDiagram CQP->>QPS: QueryPlannerRequest QPS->>QPS: filter_query, no reject_unauthorized input alt FilterResult::Emptied - QPS-->>CQP: Plan with root None,
document_emptied set,
usage reporting from the ORIGINAL query + QPS-->>CQP: Plan with root None,
usage reporting from the ORIGINAL query else Filtered or Unchanged QPS-->>CQP: Plan end @@ -84,12 +84,12 @@ sequenceDiagram CQP-->>SG: Plan SG->>AL: execution Request - Note over AL: refuse when document_emptied,
or reject_unauthorized (held from config)
and paths are present + Note over AL: refuse when reject_unauthorized
(held from config) and paths are present alt refused AL-->>C: 200, data null and errors,
logs Authorization error once,
inside the EXECUTION span else continue AL->>ES: request (authorization counter fires here) - ES->>SUB: fetches from the filtered plan + ES->>SUB: fetches from the filtered plan, none for an empty one Note over ES: errors, then format twice:
FILTERED shape, ORIGINAL shape ES-->>C: 200, nulls and authorization errors end @@ -100,16 +100,16 @@ Key references, on the branch: | What | Where | | --- | --- | | `FilterResult` (Unchanged / Filtered / Emptied) | `plugins/authorization/mod.rs:152` | -| `document_emptied` on `UnauthorizedPaths` | `plugins/authorization/mod.rs:148` | | `filter_query`, no config-driven refusal | `plugins/authorization/mod.rs:371` | | The layer: `checkpoint_async` ahead of the counter | `plugins/authorization/mod.rs:629` | | Planner's `Emptied` arm, empty plan | `query_planner/query_planner_service.rs:465` | | Single-variant `QueryPlannerContent` | `services/query_planner.rs:103` | | Two-pass formatting (unchanged) | `services/execution/service.rs:291` | -Behaviour intentionally identical to before, byte-level, in every `ErrorLocation` mode. -Two observable changes, both in the changeset: refusals reach Studio attributed, and the -`Authorization error` event moves from the `query_planning` span to `execution`. +Observable changes, all in the changesets: refusals reach Studio attributed, the +`Authorization error` event moves from `query_planning` to under `execution`, and an +emptied operation without `reject_unauthorized` returns shaped data with null roots +instead of `data: null` (the breaking changeset). ## Known gap, pinned but unresolved: multi-operation documents @@ -152,63 +152,35 @@ Pinned by `fully_filtered_operation_beside_surviving_sibling_reports_unknown_ope in `plugins/authorization/tests.rs`, which asserts the 400, the error code, the absent `data` key, and that no subgraph was contacted. -## Dropping `document_emptied` +## Emptied operations run the normal pipeline -Measured by deleting the layer's `document_emptied` clause and sending an emptied -operation (`{ orga(id: 1) { id } }`, `orga.id` `@authenticated`, unauthenticated, -directives enabled, no `reject_unauthorized`): +An emptied operation carries no marker. The planner returns a plan with `root: None`, +execution fetches nothing, and response formatting against the original operation +shapes the result, exactly as for a partial filter whose surviving selections are all +statically `@skip`ped: ``` -with the flag: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} -without the flag: 200 {"data":{"orga":null}, "errors":[{...,"path":["orga","id"]}]} +without reject_unauthorized: 200 {"data":{"orga":null}, "errors":[{...,"path":["orga","id"]}]} +with reject_unauthorized: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} ``` -Same status, same errors, same paths. The flag's entire effect is `data: null` versus a -shaped `data` with null roots. - -What actually gets deleted: the field, its serialization, and the layer's first clause. -The `FilterResult::Emptied` variant stays — an empty document fails executable -validation, so the planner cannot treat it as ordinary `Filtered`; it still returns the -empty plan. `reject_unauthorized` is untouched: the layer holds it from config and keeps -answering `data: null` for those. - -The shaped response is the more spec-aligned of the two: a field error on a nullable -root field nulls that field; `data: null` belongs to non-null propagation reaching the -root. Neither matches the other candidate reading, a request error (4xx, no `data` key). -Deciding between those two is the spec-compliance follow-up; today's `data: null` at 200 -is a third shape that satisfies neither and survives as preserved history. - -### Security consequences - -None found. The load-bearing properties do not involve the flag: - -- Subgraphs are unreachable either way. The refusal's plan has `root: None`, so - execution has no fetch nodes; the flag only decides who formats the empty result. - The `@authenticated` field's value never leaves a subgraph because no subgraph is - asked. -- Over-fetch stripping is untouched. The filtered-then-original formatting passes and - their ordering (`overfetched_unauthorized_field_is_not_returned`) sit below the - layer and do not read the flag. -- Cache poisoning is unchanged. The refusal plan is cached keyed by - `CacheKeyMetadata`, flag or no flag; an unauthenticated request can only ever hit an - entry planned for unauthenticated metadata. -- The error paths disclose the same information in both shapes: the paths in the - errors already name every refused field, so `{"orga": null}` reveals nothing that - `data: null` conceals. - -The one behavioural wrinkle: with `errors.response: disabled`, an emptied operation -returns shaped nulls with no errors instead of `data: null` with no errors — a response -indistinguishable from every root field genuinely being null. `data: null` today is -almost as ambiguous. Operators choosing `disabled` have opted out of the signal either -way; noted for the changeset if the flag goes. - -### Cost of keeping it - -One serialized bool, the layer clause, a nine-line field doc carrying two -counterexamples (`@skip`-emptied partial filters, `dry_run` over all-skipped -operations), and the recurring explanation of why the flag cannot be derived from the -plan. The flag is scaffolding for one byte-level compatibility: delete it the moment -the spec follow-up decides the refusal shape. +The `reject_unauthorized` response comes from the execution-service layer, which holds +the flag from configuration and answers before execution. + +A refusal marker on the plan was considered and rejected. It would preserve +`data: null` for the emptied-without-reject case, and that is its entire effect: the +shaped response discloses nothing the errors do not already name, subgraphs are +unreachable either way (`root: None` has no fetch nodes), and the cache keys plans by +authorization state with or without a marker. The marker also cannot be derived from +the plan (a rootless plan equally describes an all-`@skip`ped partial filter, and +`filtered_query` absence equally describes `dry_run`), so it would have to be carried +as a serialized field for one byte-level difference. The shaped response is also the +field-error reading of the GraphQL spec; the remaining open question, field error +versus request error (4xx, no `data` key), belongs to the spec-compliance follow-up. + +The one wrinkle: with `errors.response: disabled`, an emptied operation returns shaped +nulls with no errors, indistinguishable from every root field genuinely being null. +Operators choosing `disabled` have opted out of the signal. ## Still open elsewhere From 8382510284f7243832a4e4ca7e91b4e1feb6fc44 Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 11:13:55 +0100 Subject: [PATCH 23/38] test: pin cross-operation authorization effects in multi-operation documents Unauthorized paths accumulate document-wide, so a sibling operation affects the executed one. With `reject_unauthorized`, a fully authorized operation is refused because a sibling asked for unauthorized fields, and the error cites a path from an operation nobody ran. Without it, the authorized operation executes and returns its data, and the sibling's error survives with its path truncated away, since the path matches nothing in the executed operation's shape. Neither leaks: filtering removes unauthorized fields from the document before planning, so no plan contains a fetch for them whichever operation executes. The refusal case is fail-closed, and that is the property most at risk from resolving the per-operation `FIXME`s in `filter_query`: scoping the collected paths to the executed operation without scoping the execution layer's `reject_unauthorized` check turns the refusal into an execution. Disabling that check fails exactly the refusal test and leaves the execution test passing. The dev-doc's multi-operation section carries the measured outputs and the warning. --- .../src/plugins/authorization/tests.rs | 116 ++++++++++++++++++ dev-docs/authorization-query-planning.md | 29 +++++ 2 files changed, 145 insertions(+) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 2ee5de676c..0be9183f5f 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -769,6 +769,122 @@ mod whole_query_rejection { assert_no_subgraph_calls(handles).await; } + fn multi_op_request() -> router::Request { + let req = graphql::Request { + // `A` asks for nothing unauthorized; `B` asks for `orga.id`, which is + // `@authenticated`. + query: Some( + "query A { currentUser { name } } query B { orga(id: 1) { id } }".to_string(), + ), + operation_name: Some("A".to_string()), + ..Default::default() + }; + router::Request { + context: Context::new(), + router_request: http::Request::builder() + .method("POST") + .header(CONTENT_TYPE, "application/json") + .header(ACCEPT, "application/json") + .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) + .unwrap(), + } + } + + /// `filter_query` collects unauthorized paths from the whole document, and the + /// execution layer's `reject_unauthorized` check reads those paths without knowing + /// which operation they came from. A fully authorized operation is therefore refused + /// when a sibling operation in the same document asks for unauthorized fields, and + /// the error cites a path that does not exist in the executed operation. + /// + /// Fail-closed: scoping the paths to the executed operation without scoping this + /// check changes the refusal into an execution, which some operators may rely on + /// not happening. + #[tokio::test] + async fn authorized_operation_beside_unauthorized_sibling_is_refused() { + let (service, handles) = build_rejecting_router(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true + })) + .await; + + let response = service.oneshot(multi_op_request()).await.unwrap(); + + assert_eq!(response.response.status(), http::StatusCode::OK); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!( + body.get("data"), + Some(&serde_json::Value::Null), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + // The path belongs to `B`, which nobody executed. + assert_eq!( + body.pointer("/errors/0/path"), + Some(&serde_json::json!(["orga", "id"])), + "body: {body}" + ); + + assert_no_subgraph_calls(handles).await; + } + + /// Without `reject_unauthorized`, the authorized operation executes and returns its + /// data, and the sibling's filtering leaves an error whose path was truncated away: + /// `B`'s path matches nothing in `A`'s shape, so the error arrives with a code and + /// no path on a successful response. + #[tokio::test] + async fn authorized_operation_beside_unauthorized_sibling_executes() { + let service = TestHarness::builder() + .configuration_json(serde_json::json!({ + "authorization": { "directives": { "enabled": true } } + })) + .unwrap() + .schema(AUTHENTICATED_SCHEMA) + .subgraph_hook(|_name, _service| { + let (mock, mut handle) = + tower_test::mock::pair::(); + tokio::spawn(async move { + while let Some((req, responder)) = handle.next_request().await { + responder.send_response( + subgraph::Response::fake_builder() + .context(req.context) + .data(serde_json::json! {{ "currentUser": { "name": "Ada" } }}) + .build(), + ); + } + }); + mock.boxed_clone() + }) + .build_router() + .await + .unwrap(); + + let response = service.oneshot(multi_op_request()).await.unwrap(); + + assert_eq!(response.response.status(), http::StatusCode::OK); + let bytes = body::into_bytes(response.response.into_body()) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!( + body.pointer("/data/currentUser/name"), + Some(&serde_json::json!("Ada")), + "body: {body}" + ); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "body: {body}" + ); + assert_eq!(body.pointer("/errors/0/path"), None, "body: {body}"); + } + #[tokio::test] async fn does_not_reach_execution() { let (service, handles) = build_router_rejecting_whole_query().await; diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md index 1408756fb6..b33e3927f0 100644 --- a/dev-docs/authorization-query-planning.md +++ b/dev-docs/authorization-query-planning.md @@ -152,6 +152,35 @@ Pinned by `fully_filtered_operation_beside_surviving_sibling_reports_unknown_ope in `plugins/authorization/tests.rs`, which asserts the 400, the error code, the absent `data` key, and that no subgraph was contacted. +### Cross-operation effects, measured + +Unauthorized paths accumulate document-wide, so a sibling operation affects the +executed one. With `A` fully authorized and executed, and `B` asking for an +`@authenticated` field: + +``` +reject_unauthorized on: 200 {"data":null, "errors":[{...,"path":["orga","id"]}]} +reject_unauthorized off: 200 {"data":{"currentUser":{"name":"Ada"}}, + "errors":[{..., no path}]} +``` + +With `reject_unauthorized`, the router refuses the fully authorized operation because +the sibling's paths are indistinguishable from the executed operation's, and the error +cites a path from an operation nobody ran. Fail-closed, so it denies rather than +leaks. Without it, `A` executes and returns its data, and `B`'s error survives with +its path truncated away: the path matches nothing in `A`'s shape. + +No shape of the multi-operation case leaks data. Filtering removes unauthorized +fields from the document before planning, so no plan ever contains a fetch for them, +whichever operation executes; the plan cache keys on document text, operation name, +and authorization metadata, so entries never cross operations or grant sets. + +For whoever scopes the emptiness check per-operation: scoping `paths` to the executed +operation without scoping the execution layer's `reject_unauthorized` check turns the +fail-closed refusal above into an execution. Both behaviours are pinned by +`authorized_operation_beside_unauthorized_sibling_is_refused` and +`..._executes`. + ## Emptied operations run the normal pipeline An emptied operation carries no marker. The planner returns a plan with `root: None`, From 694a91c61c68054dcb4c5866caeb7d6370298a33 Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 11:32:29 +0100 Subject: [PATCH 24/38] test: polish the authorization tests added on this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename `whole_query_rejection` to `whole_operation_authorization`: the module covers refusals, emptied operations that execute, and multi-operation documents, and every test in it now asserts on the bytes the router sends. Two helpers replace the repetition: `graphql_post` builds every request, and `wire_response` reads status and body from the wire, where deserializing into `graphql::Response` would collapse a null `data` into an absent key. `send_rejected_request` composes them and guards that the refusal took the authorization path. Doc comments across the branch's tests now describe only the code as it stands. The telemetry billing docs claimed a refused operation records no usage reporting, which `records_usage_reporting` disproves; they now state what each `UsageReporting` state bills. The warm-up test is `planning_unauthenticated_returns_empty_plan_not_unfiltered_plan`, matching what it asserts, and its doc points at `emptied_operation_plan_is_cached` rather than a test that does not exist. The dev-doc gains the `data: null` versus `data` absent analysis: the request-error and over-HTTP status obligations quoted from both specifications, and the three coupled decisions resolving them requires — status by negotiated content type, error paths on a response with no result, and `errors.response: disabled` producing an invalid response when `data` is absent. --- .../src/plugins/authorization/tests.rs | 499 ++++++++---------- apollo-router/src/plugins/telemetry/mod.rs | 17 +- .../query_planner/caching_query_planner.rs | 4 - .../query_planner/query_planner_service.rs | 23 +- apollo-router/src/spec/query/tests.rs | 10 +- .../tests/integration/telemetry/logging.rs | 12 +- dev-docs/authorization-query-planning.md | 38 ++ 7 files changed, 285 insertions(+), 318 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 0be9183f5f..5f05a55bb1 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -550,21 +550,28 @@ async fn overfetched_unauthorized_field_is_not_returned() { assert_eq!(current_user.get("name"), Some(&json!("Ada"))); } -mod whole_query_rejection { +mod whole_operation_authorization { + //! Outcomes for operations that authorization affects as a whole, asserted on the + //! bytes the router sends. + use super::*; - /// `Organization.id` and `User.phone` are both `@authenticated`, so filtering removes - /// paths from an unauthenticated request and `reject_unauthorized` turns that into a - /// whole-query rejection. + /// `Organization.id` and `User.phone` are both `@authenticated`, so filtering + /// removes paths from an unauthenticated request, and `reject_unauthorized` turns + /// that into a whole-operation refusal. const REJECTED_QUERY: &str = "query { orga(id: 1) { id creatorUser { id name phone } } }"; + /// `A` asks for nothing unauthorized; `B` asks for `orga.id`, which is + /// `@authenticated`. Tests execute `A`. + const MULTI_OP_QUERY: &str = "query A { currentUser { name } } query B { orga(id: 1) { id } }"; + type SubgraphHandles = Arc>>>; - /// Builds a router that rejects `REJECTED_QUERY` under the given `directives` config, - /// replacing every subgraph with a `tower_test` mock. The mocks hold no canned - /// responses, so reaching one fails. - async fn build_rejecting_router( + /// Builds a router with the given `directives` config, replacing every subgraph + /// with a `tower_test` mock. The mocks hold no canned responses, so reaching one + /// fails the test. + async fn router_with_unresponsive_subgraphs( directives: serde_json::Value, ) -> (router::BoxCloneService, SubgraphHandles) { let handles: SubgraphHandles = Arc::new(Mutex::new(Vec::new())); @@ -589,33 +596,17 @@ mod whole_query_rejection { (service, handles) } - async fn build_router_rejecting_whole_query() -> (router::BoxCloneService, SubgraphHandles) { - build_rejecting_router(serde_json::json!({ + /// A router configured to refuse `REJECTED_QUERY`. + async fn rejecting_router() -> (router::BoxCloneService, SubgraphHandles) { + router_with_unresponsive_subgraphs(serde_json::json!({ "enabled": true, "reject_unauthorized": true })) .await } - /// Sends `REJECTED_QUERY` and parses the response body as it goes on the wire. - /// - /// `into_graphql_response_stream` deserializes into `graphql::Response`, where - /// `data: Option` turns JSON `null` into `None` and then skips it on - /// re-serialization. Anything asserting on whether `data` is present has to read the - /// bytes instead. - async fn rejected_response_body(service: router::BoxCloneService) -> serde_json::Value { - let response = service - .oneshot(rejected_request(Context::new())) - .await - .unwrap(); - let bytes = body::into_bytes(response.response.into_body()) - .await - .unwrap(); - serde_json::from_slice(&bytes).unwrap() - } - - /// Fails if any subgraph mock received a request, or if the router built no subgraph - /// service at all, which would make the check vacuous. + /// Fails if any subgraph mock received a request, or if the router built no + /// subgraph service at all, which would make the check vacuous. async fn assert_no_subgraph_calls(handles: SubgraphHandles) { let handles: Vec<_> = handles.lock().unwrap().drain(..).collect(); assert!(!handles.is_empty(), "no subgraph services were created"); @@ -624,9 +615,14 @@ mod whole_query_rejection { } } - fn rejected_request(context: Context) -> router::Request { + fn graphql_post( + query: &str, + operation_name: Option<&str>, + context: Context, + ) -> router::Request { let req = graphql::Request { - query: Some(REJECTED_QUERY.to_string()), + query: Some(query.to_string()), + operation_name: operation_name.map(str::to_string), ..Default::default() }; router::Request { @@ -640,116 +636,177 @@ mod whole_query_rejection { } } - /// Sends `REJECTED_QUERY`, asserts the router rejected it on authorization grounds, - /// and returns the HTTP status. - async fn send_rejected_request( + /// Sends the request and parses the body as it goes on the wire. + /// + /// Deserializing into [`graphql::Response`] collapses a JSON `null` under `data` + /// into an absent key, so assertions on whether `data` is present read the bytes. + async fn wire_response( service: router::BoxCloneService, - context: Context, - ) -> http::StatusCode { - let response = service.oneshot(rejected_request(context)).await.unwrap(); + request: router::Request, + ) -> (http::StatusCode, serde_json::Value) { + let response = service.oneshot(request).await.unwrap(); let status = response.response.status(); - - let body = response - .into_graphql_response_stream() - .await - .next() + let bytes = body::into_bytes(response.response.into_body()) .await - .unwrap() .unwrap(); + (status, serde_json::from_slice(&bytes).unwrap()) + } + + /// Sends `REJECTED_QUERY`, asserts the router refused it on authorization grounds, + /// and returns the HTTP status and wire body. + async fn send_rejected_request( + service: router::BoxCloneService, + context: Context, + ) -> (http::StatusCode, serde_json::Value) { + let (status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, context)).await; + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("UNAUTHORIZED_FIELD_OR_TYPE")), + "the operation was not refused on authorization grounds: {body}" + ); + (status, body) + } + + #[tokio::test] + async fn does_not_reach_execution() { + let (service, handles) = rejecting_router().await; + + send_rejected_request(service, Context::new()).await; + + assert_no_subgraph_calls(handles).await; + } + + /// A refusal answers as a field error: HTTP 200 with `data: null` and the + /// authorization errors, not as a GraphQL request error, which carries a 4xx status + /// and no `data` entry. `rejection_sends_null_data` holds the data side of that + /// line; this holds the status. + #[tokio::test] + async fn returns_http_200() { + let (service, _handles) = rejecting_router().await; + + let (status, _body) = send_rejected_request(service, Context::new()).await; + + assert_eq!(status, http::StatusCode::OK); + } + + /// `data` is `null` and present. An absent `data` marks a request error; a null one + /// marks a field error that propagated to the root. The key's presence is part of + /// the response contract. + #[tokio::test] + async fn rejection_sends_null_data() { + let (service, _handles) = rejecting_router().await; + + let (_status, body) = send_rejected_request(service, Context::new()).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + } + + /// A refused operation reaches `CachingQueryPlanner` as a plan, so its usage + /// reporting lands in the context like any other operation's and Studio can + /// attribute the refusal to an operation signature. + /// `licensed_operation_count_tests` holds what the report bills. + #[tokio::test] + async fn records_usage_reporting() { + let (service, _handles) = rejecting_router().await; + let context = Context::new(); + + send_rejected_request(service, context.clone()).await; + + let usage_reporting = context + .extensions() + .with_lock(|lock| lock.get::>().cloned()) + .expect("a refused operation records usage reporting"); assert!( - body.contains_error_code("UNAUTHORIZED_FIELD_OR_TYPE"), - "the operation was not rejected on authorization grounds: {body:?}" + matches!(*usage_reporting, UsageReporting::Operation(_)), + "the report must carry operation details, not an error key: {usage_reporting:?}" ); + } - status + /// `errors.response: disabled` suppresses the authorization errors, so `data: null` + /// is the only thing telling the client the operation produced nothing. + #[tokio::test] + async fn rejection_with_errors_disabled_sends_null_data_and_no_errors() { + let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "disabled" } + })) + .await; + + let (_status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, Context::new())).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); } - /// Fully filtering the executed operation while a sibling operation shares the - /// document reports `Filtered`, not `Emptied`: the emptiness check is document-wide. - /// Planning then fails to find the executed operation, so the client is told the - /// operation is unknown rather than refused. - /// - /// This pins the mismatch the `FIXME`s in `filter_query` point at. Resolving them - /// changes this response deliberately; the single-operation refusal contract is - /// pinned by the rest of this module. + /// `errors.response: extensions` moves the authorization errors under + /// `extensions.authorizationErrors` and leaves `errors` out of the response. #[tokio::test] - async fn fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation() { - let (service, handles) = build_rejecting_router(serde_json::json!({ - "enabled": true + async fn rejection_with_errors_in_extensions() { + let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "errors": { "response": "extensions" } })) .await; - let req = graphql::Request { - query: Some( - "query A { orga(id: 1) { id } } query B { currentUser { name } }".to_string(), - ), - operation_name: Some("A".to_string()), - ..Default::default() - }; - let response = service - .oneshot(router::Request { - context: Context::new(), - router_request: http::Request::builder() - .method("POST") - .header(CONTENT_TYPE, "application/json") - .header(ACCEPT, "application/json") - .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) - .unwrap(), - }) - .await - .unwrap(); + let (_status, body) = + wire_response(service, graphql_post(REJECTED_QUERY, None, Context::new())).await; - assert_eq!(response.response.status(), http::StatusCode::BAD_REQUEST); - let bytes = body::into_bytes(response.response.into_body()) - .await - .unwrap(); - let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); + assert_eq!(body.get("errors"), None); + let authorization_errors = body + .pointer("/extensions/authorizationErrors") + .and_then(|value| value.as_array()) + .expect("the errors must move under extensions.authorizationErrors"); assert_eq!( - body.pointer("/errors/0/extensions/code"), - Some(&serde_json::json!("GRAPHQL_UNKNOWN_OPERATION_NAME")), - "body: {body}" + authorization_errors.len(), + 2, + "one error per unauthorized path: `orga.id` and `orga.creatorUser.phone`" ); - assert_eq!(body.get("data"), None); + } + /// `dry_run` and `reject_unauthorized` combine rather than cancelling out: `dry_run` + /// reports the paths without modifying the operation, and `reject_unauthorized` + /// refuses on the reported paths. A refusal here can only come from configuration, + /// since `dry_run` never empties the document. + #[tokio::test] + async fn dry_run_with_reject_unauthorized_still_rejects() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true, + "reject_unauthorized": true, + "dry_run": true + })) + .await; + + let (_status, body) = send_rejected_request(service, Context::new()).await; + + assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); assert_no_subgraph_calls(handles).await; } - /// Without `reject_unauthorized`, an operation that filtering empties runs through - /// the same pipeline as a partial filter: an empty plan executes nothing, and - /// response formatting against the original operation shapes the result. The client - /// receives each requested root field as null alongside the path errors, exactly as - /// it would if some fields had survived. + /// Without `reject_unauthorized`, an emptied operation runs the same pipeline as a + /// partial filter: an empty plan executes nothing, and response formatting against + /// the original operation shapes the result. Each requested root field arrives as + /// null alongside the path errors. #[tokio::test] async fn emptied_operation_without_reject_returns_shaped_data() { - let (service, handles) = build_rejecting_router(serde_json::json!({ + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ "enabled": true })) .await; - let req = graphql::Request { - // `orga.id` is `@authenticated` and the only selection, so filtering - // removes everything. - query: Some("query { orga(id: 1) { id } }".to_string()), - ..Default::default() - }; - let response = service - .oneshot(router::Request { - context: Context::new(), - router_request: http::Request::builder() - .method("POST") - .header(CONTENT_TYPE, "application/json") - .header(ACCEPT, "application/json") - .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) - .unwrap(), - }) - .await - .unwrap(); + // `orga.id` is `@authenticated` and the only selection, so filtering removes + // everything. + let (status, body) = wire_response( + service, + graphql_post("query { orga(id: 1) { id } }", None, Context::new()), + ) + .await; - assert_eq!(response.response.status(), http::StatusCode::OK); - let bytes = body::into_bytes(response.response.into_body()) - .await - .unwrap(); - let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(status, http::StatusCode::OK); assert_eq!( body.get("data"), Some(&serde_json::json!({ "orga": null })), @@ -769,51 +826,58 @@ mod whole_query_rejection { assert_no_subgraph_calls(handles).await; } - fn multi_op_request() -> router::Request { - let req = graphql::Request { - // `A` asks for nothing unauthorized; `B` asks for `orga.id`, which is - // `@authenticated`. - query: Some( - "query A { currentUser { name } } query B { orga(id: 1) { id } }".to_string(), + /// The emptiness check in `filter_query` is document-wide, marked by its `FIXME`s: + /// fully filtering the executed operation while a sibling keeps the document + /// non-empty reports `Filtered`, planning fails to find the executed operation, and + /// the client is told the operation is unknown rather than refused. + #[tokio::test] + async fn fully_filtered_operation_beside_surviving_sibling_reports_unknown_operation() { + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ + "enabled": true + })) + .await; + + let (status, body) = wire_response( + service, + graphql_post( + "query A { orga(id: 1) { id } } query B { currentUser { name } }", + Some("A"), + Context::new(), ), - operation_name: Some("A".to_string()), - ..Default::default() - }; - router::Request { - context: Context::new(), - router_request: http::Request::builder() - .method("POST") - .header(CONTENT_TYPE, "application/json") - .header(ACCEPT, "application/json") - .body(body::from_bytes(serde_json::to_vec(&req).unwrap())) - .unwrap(), - } + ) + .await; + + assert_eq!(status, http::StatusCode::BAD_REQUEST); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("GRAPHQL_UNKNOWN_OPERATION_NAME")), + "body: {body}" + ); + assert_eq!(body.get("data"), None); + + assert_no_subgraph_calls(handles).await; } /// `filter_query` collects unauthorized paths from the whole document, and the - /// execution layer's `reject_unauthorized` check reads those paths without knowing - /// which operation they came from. A fully authorized operation is therefore refused - /// when a sibling operation in the same document asks for unauthorized fields, and - /// the error cites a path that does not exist in the executed operation. - /// - /// Fail-closed: scoping the paths to the executed operation without scoping this - /// check changes the refusal into an execution, which some operators may rely on - /// not happening. + /// execution layer's `reject_unauthorized` check reads them without knowing which + /// operation they came from. A fully authorized operation is refused when a sibling + /// operation asks for unauthorized fields, and the error cites a path from the + /// operation nobody ran. #[tokio::test] async fn authorized_operation_beside_unauthorized_sibling_is_refused() { - let (service, handles) = build_rejecting_router(serde_json::json!({ + let (service, handles) = router_with_unresponsive_subgraphs(serde_json::json!({ "enabled": true, "reject_unauthorized": true })) .await; - let response = service.oneshot(multi_op_request()).await.unwrap(); + let (status, body) = wire_response( + service, + graphql_post(MULTI_OP_QUERY, Some("A"), Context::new()), + ) + .await; - assert_eq!(response.response.status(), http::StatusCode::OK); - let bytes = body::into_bytes(response.response.into_body()) - .await - .unwrap(); - let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(status, http::StatusCode::OK); assert_eq!( body.get("data"), Some(&serde_json::Value::Null), @@ -835,8 +899,8 @@ mod whole_query_rejection { } /// Without `reject_unauthorized`, the authorized operation executes and returns its - /// data, and the sibling's filtering leaves an error whose path was truncated away: - /// `B`'s path matches nothing in `A`'s shape, so the error arrives with a code and + /// data. Error-path reconciliation drops the sibling's path, since it matches + /// nothing in the executed operation's shape, so its error arrives with a code and /// no path on a successful response. #[tokio::test] async fn authorized_operation_beside_unauthorized_sibling_executes() { @@ -865,13 +929,13 @@ mod whole_query_rejection { .await .unwrap(); - let response = service.oneshot(multi_op_request()).await.unwrap(); + let (status, body) = wire_response( + service, + graphql_post(MULTI_OP_QUERY, Some("A"), Context::new()), + ) + .await; - assert_eq!(response.response.status(), http::StatusCode::OK); - let bytes = body::into_bytes(response.response.into_body()) - .await - .unwrap(); - let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(status, http::StatusCode::OK); assert_eq!( body.pointer("/data/currentUser/name"), Some(&serde_json::json!("Ada")), @@ -884,131 +948,6 @@ mod whole_query_rejection { ); assert_eq!(body.pointer("/errors/0/path"), None, "body: {body}"); } - - #[tokio::test] - async fn does_not_reach_execution() { - let (service, handles) = build_router_rejecting_whole_query().await; - - send_rejected_request(service, Context::new()).await; - - assert_no_subgraph_calls(handles).await; - } - - /// We historically treat authorization errors as field errors, even when the whole - /// operation is refused: null data, auth errors, and status code 200. The GraphQL - /// spec arguably calls for a request error here, with a 4xx status and `data` left - /// out entirely; `rejection_sends_null_data` pins the null-versus-absent side of - /// that on the wire. Until spec compliance changes deliberately, these pin what the - /// router sends. - #[tokio::test] - async fn returns_http_200() { - let (service, _handles) = build_router_rejecting_whole_query().await; - - let status = send_rejected_request(service, Context::new()).await; - - assert_eq!(status, http::StatusCode::OK); - } - - /// A refused operation reaches `CachingQueryPlanner` as a plan, so its usage - /// reporting lands in the context like any other operation's and Studio can - /// attribute the refusal to an operation signature. - /// `licensed_operation_count_tests` pins what the report bills. - #[tokio::test] - async fn records_usage_reporting() { - let (service, _handles) = build_router_rejecting_whole_query().await; - let context = Context::new(); - - send_rejected_request(service, context.clone()).await; - - let usage_reporting = context - .extensions() - .with_lock(|lock| lock.get::>().cloned()) - .expect("a refused operation records usage reporting"); - assert!( - matches!(*usage_reporting, UsageReporting::Operation(_)), - "the report must carry operation details, not an error key: {usage_reporting:?}" - ); - } - - /// The rejection sends `data: null`, not an absent `data`. GraphQL gives those two - /// different meanings — an absent `data` marks a request error, a null one marks a - /// field error that propagated to the root — so the presence of the key is part of the - /// response contract. The snapshot tests cannot cover this, because they assert on a - /// `graphql::Response` that has already lost the distinction. - #[tokio::test] - async fn rejection_sends_null_data() { - let (service, _handles) = build_router_rejecting_whole_query().await; - - let body = rejected_response_body(service).await; - - assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); - } - - /// `errors.response: disabled` suppresses the authorization errors, so `data: null` is - /// the only thing left telling the client the operation produced nothing. - #[tokio::test] - async fn rejection_with_errors_disabled_sends_null_data_and_no_errors() { - let (service, _handles) = build_rejecting_router(serde_json::json!({ - "enabled": true, - "reject_unauthorized": true, - "errors": { "response": "disabled" } - })) - .await; - - let body = rejected_response_body(service).await; - - assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); - assert_eq!(body.get("errors"), None); - } - - /// `errors.response: extensions` moves the authorization errors under - /// `extensions.authorizationErrors` and leaves `errors` out of the response. - #[tokio::test] - async fn rejection_with_errors_in_extensions() { - let (service, _handles) = build_rejecting_router(serde_json::json!({ - "enabled": true, - "reject_unauthorized": true, - "errors": { "response": "extensions" } - })) - .await; - - let body = rejected_response_body(service).await; - - assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); - assert_eq!(body.get("errors"), None); - let authorization_errors = body - .pointer("/extensions/authorizationErrors") - .and_then(|value| value.as_array()) - .expect("the errors must move under extensions.authorizationErrors"); - assert_eq!( - authorization_errors.len(), - 2, - "one error per unauthorized path: `orga.id` and `orga.creatorUser.phone`" - ); - } - - /// `dry_run` and `reject_unauthorized` combine rather than cancelling out: `dry_run` - /// reports the paths without modifying the operation, and `reject_unauthorized` then - /// refuses it anyway. - /// - /// This matters for any change that treats an emptied document as the trigger for - /// rejection. `dry_run` never empties the document, so a rejection here can only come - /// from the config, and getting that wrong turns `dry_run` into a mode that silently - /// stops enforcing. - #[tokio::test] - async fn dry_run_with_reject_unauthorized_still_rejects() { - let (service, handles) = build_rejecting_router(serde_json::json!({ - "enabled": true, - "reject_unauthorized": true, - "dry_run": true - })) - .await; - - let body = rejected_response_body(service).await; - - assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); - assert_no_subgraph_calls(handles).await; - } } /// A partial filter can leave an operation with nothing to execute. Filtering removes diff --git a/apollo-router/src/plugins/telemetry/mod.rs b/apollo-router/src/plugins/telemetry/mod.rs index 9fb461894f..aeb144a52d 100644 --- a/apollo-router/src/plugins/telemetry/mod.rs +++ b/apollo-router/src/plugins/telemetry/mod.rs @@ -3594,12 +3594,9 @@ mod licensed_operation_count_tests { } } - /// An operation the query planner rejected on authorization grounds records no - /// `UsageReporting`, and is still billed as one licensed operation. Billing does not - /// depend on the operation reaching execution. - /// - /// Anything that changes what a rejected operation puts in the context has to keep this - /// at 1. See `usage_reporting_error_is_not_billed` for the way that goes wrong. + /// A context holding no `UsageReporting` bills one licensed operation. Billing does + /// not depend on the operation reaching execution or reporting anything about + /// itself. #[tokio::test] async fn missing_usage_reporting_is_billed_as_one_operation() { async { @@ -3609,8 +3606,8 @@ mod licensed_operation_count_tests { .await; } - /// `UsageReporting::Error` bills nothing. It is the tempting variant to reach for when - /// an operation produced no plan, and choosing it drops that operation off the bill. + /// `UsageReporting::Error` bills nothing: it is the one variant that zeroes the + /// licensed operation count, so an operation reported with it drops off the bill. #[tokio::test] async fn usage_reporting_error_is_not_billed() { async { @@ -3627,8 +3624,8 @@ mod licensed_operation_count_tests { .await; } - /// An operation that carries real reporting details is billed the same as one carrying - /// none, so attributing a rejected operation does not change what it costs. + /// `UsageReporting::Operation` bills one licensed operation, the same as a context + /// holding no reporting at all: attribution does not change what an operation costs. #[tokio::test] async fn operation_details_are_billed_as_one_operation() { async { diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index ddf856ed4d..5e8cd31560 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -1903,10 +1903,6 @@ mod tests { /// `CacheKeyMetadata` is part of `CachingQueryKey`'s `Hash`/`Eq`, so the same query /// under different authorization state reaches the inner planner again. That keeps an /// unauthenticated request from receiving a plan built for an authenticated one. - /// - /// Drives it through the producer that runs in production — `update_cache_key`, reading - /// the request's JWT claims — because that call overwrites the context's - /// `CacheKeyMetadata` before the cache key is built. #[test(tokio::test)] async fn plan_cache_is_segmented_by_authorization_metadata() { let (mock, handle) = tower_test::mock::pair::(); diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 13f61663ea..c5250adf9d 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -1079,20 +1079,19 @@ mod tests { keys.join("\n") } - /// Warm-up reaches this service with `CacheKeyMetadata::default()`, the same metadata an - /// unauthenticated request produces: `queries_to_warm_up` supplies no metadata for - /// persisted queries, and for re-warmed cache entries `update_cache_key` derives default - /// metadata from warm-up's claimless context. This asserts what the service does with - /// those inputs — filtering rejects the operation instead of handing back a plan. + /// Warm-up reaches this service with `CacheKeyMetadata::default()`, the same metadata + /// an unauthenticated request produces: `queries_to_warm_up` supplies no metadata for + /// persisted queries, and for re-warmed cache entries `update_cache_key` derives + /// default metadata from warm-up's claimless context. Filtering empties the operation + /// under those inputs, so the plan carries no work rather than the unfiltered fetches. /// - /// Scope: this is not a warm-up-specific code path. `compute_job_type` only selects the - /// compute-pool priority and the metric label, and `get` filters before reading it, so - /// `QueryPlanningWarmup` behaves exactly like `QueryPlanning` here. Nor does this observe - /// the plan cache, which sits above this service; that a rejection is what gets cached, - /// and that it stays keyed by authorization state, is covered by - /// `caching_query_planner::tests::rejection_response_is_cached`. + /// Scope: `compute_job_type` only selects the compute-pool priority and the metric + /// label, and `get` filters before reading it, so `QueryPlanningWarmup` behaves + /// exactly like `QueryPlanning` here. The plan cache sits above this service; that + /// the empty plan is cached keyed by authorization state is covered by + /// `caching_query_planner::tests::emptied_operation_plan_is_cached`. #[test(tokio::test)] - async fn planning_unauthenticated_rejects_rather_than_returning_unfiltered_plan() { + async fn planning_unauthenticated_returns_empty_plan_not_unfiltered_plan() { let configuration: Configuration = serde_json::from_value(serde_json::json!({ "authorization": { "directives": { "enabled": true } } })) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index ee3a4aa612..c73d50c794 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -7676,9 +7676,8 @@ const AUTHENTICATED_INTERFACE_SCHEMA: &str = r#" /// Plans `query_str` as an unauthenticated request and returns the original `Query` with /// `filtered_query` populated, alongside the schema. /// -/// `QueryPlannerService` builds this pair in production: `filter_query` produces the -/// filtered document and the planner marks it `is_original = false`. Hand-writing the -/// filtered query risks pinning a shape filtering never produces. +/// The pair comes from `QueryPlannerService`, so the filtered `Query` is the one +/// `filter_query` produces, marked `is_original = false` by the planner. async fn authorization_filtered_query(query_str: &str) -> (Arc, Arc) { let configuration: Configuration = serde_json::from_value(serde_json::json!({ "authorization": { "directives": { "enabled": true } } @@ -7748,9 +7747,8 @@ fn format_filtered_then_original(query: &Query, schema: &Schema, data: Value) -> /// `__typename` into its output for the original pass to resolve the type condition on /// `... on Foo`, so `inline` survives only if the copy happened. /// -/// A query carrying an inline fragment and a fragment spread together would not pin this: -/// each form copies `__typename` independently, so either one alone keeps both fields -/// alive. Hence one query form per test. +/// One fragment form per test: each form copies `__typename` independently, so a query +/// carrying both keeps its fields alive when either copy runs. #[tokio::test] async fn filtered_query_keeps_typename_for_inline_fragment() { // `secret` is `@authenticated`, so filtering drops it and leaves `inline`. diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index 8fef98cfa9..20f2f111ca 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -242,10 +242,10 @@ async fn test_text_sampler_off() -> Result<(), BoxError> { Ok(()) } -/// The `Authorization error` event for a refused operation belongs to the `execution` -/// span. The unit tests around authorization cannot see this: the `execution` span comes -/// from the telemetry plugin, which only joins the pipeline when OpenTelemetry is -/// initialised for the process, so a spawned router is the smallest thing that has it. +/// The `Authorization error` event for an unauthorized operation lands under the +/// `execution` span, exactly once per request. The `execution` span comes from the +/// telemetry plugin, which only joins the pipeline when OpenTelemetry is initialised for +/// the process, so a spawned router is the smallest thing that has it. #[tokio::test(flavor = "multi_thread")] async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxError> { let mut router = IntegrationTest::builder() @@ -277,8 +277,8 @@ async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxErr .map(|line| serde_json::from_str(line).expect("log line is JSON")) .collect(); - // Exactly once: a refusal is decided at a single place, so a second event for the - // same request means two code paths both believe they own the log. + // A second event for the same request means two code paths both believe they own + // the log. assert_eq!(events.len(), 1, "events: {events:?}"); let event = &events[0]; diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md index b33e3927f0..aefd6bf240 100644 --- a/dev-docs/authorization-query-planning.md +++ b/dev-docs/authorization-query-planning.md @@ -211,6 +211,44 @@ The one wrinkle: with `errors.response: disabled`, an emptied operation returns nulls with no errors, indistinguishable from every root field genuinely being null. Operators choosing `disabled` have opted out of the signal. +## Outstanding: `data: null` versus `data` absent + +The router answers a `reject_unauthorized` refusal with HTTP 200 and `"data": null`. +The GraphQL specification defines two error classes, and the refusal fits neither +shape the router sends: + +> "A *request error* is an error raised during a *request* which results in no response +> data." ... "If a request error is raised, the *response* must be a *request error +> result*. The `data` entry in this map must not be present." + +A refusal happens before execution and produces no response data, so it is a request +error: `data` absent, not `data: null`. The GraphQL-over-HTTP specification then binds +the status to the body for `application/graphql-response+json`: + +> "If the GraphQL response contains the data entry and it is not null, then the server +> MUST reply with a `2xx` status code." ... "If the GraphQL response does not contain +> the data entry then the server MUST reply with an appropriate `4xx` or `5xx` status +> code." + +Resolving this carries three coupled decisions: + +1. Status by content type. Data-absent with 200 violates a MUST for + `application/graphql-response+json`; the legacy `application/json` media type keeps + 200 regardless. The response shape therefore depends on the negotiated content + type, which `ClientRequestAccepts` already tracks. +2. Error shape. The spec attaches `path` to errors "associated to a particular field + in the GraphQL result"; a request error has no result, so the per-path + `UNAUTHORIZED_FIELD_OR_TYPE` errors need restructuring or lose their paths. +3. `errors.response: disabled`. The spec requires non-empty `errors` when `data` is + absent, so a request-error refusal with suppressed errors is an invalid response; + the option has to override the shape or be rejected for this combination. + +`apollo-errors` models per-error HTTP status and per-format rendering, which fits +decision 1; it does not yet model multi-error responses with paths or the +absent-versus-null `data` distinction, which live on the response rather than the +error. `returns_http_200` and `rejection_sends_null_data` hold the current shape and +flip when this resolves. + ## Still open elsewhere Keying the plan cache on the filtered query text (making `CacheKeyMetadata` redundant From f3c1796962827da9c1e181597ef9737eb269d93c Mon Sep 17 00:00:00 2001 From: bryn Date: Tue, 18 Aug 2026 13:59:41 +0100 Subject: [PATCH 25/38] refactor: collapse single-variant QueryPlannerContent into a type alias `QueryPlannerContent` held one variant wrapping `Arc`, so every consumer paid a destructure for no information. It is now a type alias for `Arc`: signatures keep naming the planner's output as a single seam, so changing the payload touches one definition, while constructions and uses handle the plan directly. 41 lines of pattern matching collapse into direct use. The distributed plan cache serializes the cache value, so its wire shape loses the enum tag. Cache keys carry the crate version, and 2.17.0 is unreleased, so no released router shares the namespace this shape lands in. --- .../src/batching/query_plan_analysis_layer.rs | 34 +++++++-------- .../cost_calculator/static_cost.rs | 3 +- .../query_planner/caching_query_planner.rs | 37 +++++------------ .../query_planner/query_planner_service.rs | 41 +++++++++---------- apollo-router/src/query_planner/warmup.rs | 4 +- apollo-router/src/services/query_planner.rs | 15 +++---- .../src/services/supergraph/service.rs | 3 +- .../src/services/supergraph/tests.rs | 5 +-- apollo-router/src/spec/query/tests.rs | 3 +- 9 files changed, 55 insertions(+), 90 deletions(-) diff --git a/apollo-router/src/batching/query_plan_analysis_layer.rs b/apollo-router/src/batching/query_plan_analysis_layer.rs index 75248726c1..bc1898c3a2 100644 --- a/apollo-router/src/batching/query_plan_analysis_layer.rs +++ b/apollo-router/src/batching/query_plan_analysis_layer.rs @@ -215,7 +215,6 @@ mod tests { use crate::graphql; use crate::query_planner::QueryPlan; use crate::query_planner::QueryPlannerService; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::execution; use crate::spec::Query; @@ -228,24 +227,21 @@ mod tests { ) -> Arc { let document = Query::parse_document(query, None, &schema, &configuration).unwrap(); - let QueryPlannerContent::Plan { plan: query_plan } = - QueryPlannerService::for_test(schema, configuration) - .unwrap() - .oneshot( - QueryPlannerRequest::builder() - .query(query) - .document(document) - .metadata(crate::plugins::authorization::CacheKeyMetadata::default()) - .plan_options(crate::services::PlanOptions::default()) - .compute_job_type(ComputeJobType::QueryPlanning) - .build(), - ) - .await - .unwrap() - .content - .unwrap(); - - query_plan + QueryPlannerService::for_test(schema, configuration) + .unwrap() + .oneshot( + QueryPlannerRequest::builder() + .query(query) + .document(document) + .metadata(crate::plugins::authorization::CacheKeyMetadata::default()) + .plan_options(crate::services::PlanOptions::default()) + .compute_job_type(ComputeJobType::QueryPlanning) + .build(), + ) + .await + .unwrap() + .content + .unwrap() } #[tokio::test] diff --git a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs index 1b451b0e95..d34db64ccd 100644 --- a/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs +++ b/apollo-router/src/plugins/demand_control/cost_calculator/static_cost.rs @@ -788,7 +788,6 @@ mod tests { use crate::compute_job::ComputeJobType; use crate::plugins::authorization::CacheKeyMetadata; use crate::query_planner::QueryPlannerService; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::query_parsing::ParsedDocument; use crate::services::query_planner::PlanOptions; @@ -909,7 +908,7 @@ mod tests { )) .await .unwrap(); - let QueryPlannerContent::Plan { plan: query_plan } = planner_res.content.unwrap(); + let query_plan = planner_res.content.unwrap(); let schema = DemandControlledSchema::new(Arc::new(supergraph_schema)).unwrap(); let mut demand_controlled_subgraph_schemas = HashMap::new(); diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index cb9a5859b7..564865ada9 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -142,7 +142,7 @@ fn init_query_plan_from_redis( subgraph_schemas: &SubgraphSchemas, cache_entry: &mut Result>, ) -> Result<(), String> { - if let Ok(QueryPlannerContent::Plan { plan }) = cache_entry { + if let Ok(plan) = cache_entry { // Arc freshly deserialized from Redis should be unique, so this doesn't clone: let plan = Arc::make_mut(plan); if let Some(root) = plan.root.as_mut() { @@ -401,7 +401,7 @@ where } // This will be overridden by the Rust usage reporting implementation - if let Some(QueryPlannerContent::Plan { plan, .. }) = &content { + if let Some(plan) = &content { context.extensions().with_lock(|lock| { lock.insert::>(plan.usage_reporting.clone()) }); @@ -607,7 +607,7 @@ where match res { Ok(content) => { - let QueryPlannerContent::Plan { plan } = &content; + let plan = &content; context.extensions().with_lock(|lock| { lock.insert::>(plan.usage_reporting.clone()) }); @@ -696,7 +696,7 @@ impl Hasher for StructHasher { impl ValueType for Result> { fn estimated_size(&self) -> Option { match self { - Ok(QueryPlannerContent::Plan { plan }) => Some(plan.estimated_size()), + Ok(plan) => Some(plan.estimated_size()), Err(e) => Some(estimate_size(e)), } } @@ -810,9 +810,7 @@ mod tests { } else { // In measurement mode, this should complete successfully even after timeout let plan = Arc::new(QueryPlan::fake_new(None, None)); - Ok(QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build()) + Ok(QueryPlannerResponse::builder().content(plan).build()) } }) } @@ -848,9 +846,7 @@ mod tests { } else { // In measurement mode, this should complete successfully even after exceeding the memory limit let plan = Arc::new(QueryPlan::fake_new(None, None)); - Ok(QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build()) + Ok(QueryPlannerResponse::builder().content(plan).build()) } }) } @@ -1729,7 +1725,7 @@ mod tests { let plan = Arc::new(query_plan); while let Some((_request, responder)) = handle.next_request().await { - let qp_content = QueryPlannerContent::Plan { plan: plan.clone() }; + let qp_content = plan.clone(); responder .send_response(QueryPlannerResponse::builder().content(qp_content).build()); } @@ -1793,9 +1789,7 @@ mod tests { .await .expect("should receive one request"); - let content = QueryPlannerContent::Plan { - plan: Arc::new(QueryPlan::fake_new(None, None)), - }; + let content = Arc::new(QueryPlan::fake_new(None, None)); responder.send_response(QueryPlannerResponse::builder().content(content).build()); }); @@ -1908,12 +1902,8 @@ mod tests { #[test(tokio::test)] async fn plan_cache_is_segmented_by_authorization_metadata() { let (mock, handle) = tower_test::mock::pair::(); - let (driver, planner_calls) = spawn_counting_planner( - handle, - QueryPlannerContent::Plan { - plan: Arc::new(QueryPlan::fake_new(None, None)), - }, - ); + let (driver, planner_calls) = + spawn_counting_planner(handle, Arc::new(QueryPlan::fake_new(None, None))); let (configuration, schema) = authorization_enabled_config_and_schema(); let mut service = CachingQueryPlanner::for_test( @@ -1972,12 +1962,7 @@ mod tests { query: Arc::new(emptied_query), estimated_size: Default::default(), }; - let (driver, planner_calls) = spawn_counting_planner( - handle, - QueryPlannerContent::Plan { - plan: Arc::new(emptied_plan), - }, - ); + let (driver, planner_calls) = spawn_counting_planner(handle, Arc::new(emptied_plan)); let (configuration, schema) = authorization_enabled_config_and_schema(); let mut service = CachingQueryPlanner::for_test( diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 621205699d..4393260bef 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -20,6 +20,7 @@ use tower::Service; use super::PlanNode; use super::QueryKey; +use super::QueryPlan; use crate::Configuration; use crate::apollo_studio_interop::generate_usage_reporting; use crate::compute_job; @@ -337,15 +338,13 @@ impl QueryPlannerService { evaluated_plan_paths ); - Ok(QueryPlannerContent::Plan { - plan: Arc::new(super::QueryPlan { - usage_reporting: Arc::new(usage_reporting), - root: query_plan_root_node, - formatted_query_plan, - query: Arc::new(selections), - estimated_size: Default::default(), - }), - }) + Ok(Arc::new(QueryPlan { + usage_reporting: Arc::new(usage_reporting), + root: query_plan_root_node, + formatted_query_plan, + query: Arc::new(selections), + estimated_size: Default::default(), + })) } } @@ -475,15 +474,13 @@ impl QueryPlannerService { &self.signature_normalization_algorithm, ); - return Ok(QueryPlannerContent::Plan { - plan: Arc::new(super::QueryPlan { - usage_reporting: Arc::new(usage_reporting), - root: None, - formatted_query_plan: None, - query: Arc::new(selections), - estimated_size: Default::default(), - }), - }); + return Ok(Arc::new(QueryPlan { + usage_reporting: Arc::new(usage_reporting), + root: None, + formatted_query_plan: None, + query: Arc::new(selections), + estimated_size: Default::default(), + })); } FilterResult::Filtered { paths, @@ -661,7 +658,7 @@ mod tests { .await .unwrap(); - let QueryPlannerContent::Plan { plan } = response.content.expect("successful response"); + let plan = response.content.expect("successful response"); insta::with_settings!({sort_maps => true}, { insta::assert_json_snapshot!("plan_usage_reporting", plan.usage_reporting); }); @@ -728,7 +725,7 @@ mod tests { let content = response.content.expect("expected a successful response"); - let QueryPlannerContent::Plan { plan } = content; + let plan = content; assert_eq!(plan.root, None, "expected an empty plan"); } @@ -1061,7 +1058,7 @@ mod tests { .await .unwrap(); - let QueryPlannerContent::Plan { plan } = result; + let plan = result; check_query_plan_coverage( plan.root.as_ref().expect("non-empty query plan"), None, @@ -1124,7 +1121,7 @@ mod tests { .await .unwrap(); - let QueryPlannerContent::Plan { plan } = content; + let plan = content; assert!( plan.root.is_none(), "an unauthenticated request must not plan any work; a plan with fetches \ diff --git a/apollo-router/src/query_planner/warmup.rs b/apollo-router/src/query_planner/warmup.rs index ea83387ceb..28111179f9 100644 --- a/apollo-router/src/query_planner/warmup.rs +++ b/apollo-router/src/query_planner/warmup.rs @@ -370,9 +370,7 @@ mod tests { let schema_hash = SchemaHash::new(""); fn empty_query_plan() -> Result> { - Ok(QueryPlannerContent::Plan { - plan: Arc::new(QueryPlan::fake_new(None, None)), - }) + Ok(Arc::new(QueryPlan::fake_new(None, None))) } { diff --git a/apollo-router/src/services/query_planner.rs b/apollo-router/src/services/query_planner.rs index 0aaec84fbe..c87db77a43 100644 --- a/apollo-router/src/services/query_planner.rs +++ b/apollo-router/src/services/query_planner.rs @@ -91,6 +91,10 @@ impl CachingRequest { } assert_impl_all!(Response: Send); +/// What query planning produces on success. An alias so the payload can change in one +/// place. +pub(crate) type QueryPlannerContent = Arc; + /// [`Context`] and [`QueryPlan`] for the response. pub(crate) struct Response { /// Optional in case of error @@ -98,22 +102,13 @@ pub(crate) struct Response { pub(crate) errors: Vec, } -/// Query, QueryPlan and Introspection data. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub(crate) enum QueryPlannerContent { - Plan { plan: Arc }, -} - #[buildstructor::buildstructor] impl Response { /// This is the constructor (or builder) to use when constructing a real QueryPlannerResponse. /// /// Required parameters are required in non-testing code to create a QueryPlannerResponse. #[builder] - pub(crate) fn new( - content: Option, - errors: Vec, - ) -> Response { + pub(crate) fn new(content: Option>, errors: Vec) -> Response { Self { content, errors } } } diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index 6a619e815d..684cf3fb0a 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -50,7 +50,6 @@ use crate::query_planner::SubgraphSchemas; use crate::query_planner::warmup; use crate::services::ExecutionRequest; use crate::services::ExecutionResponse; -use crate::services::QueryPlannerContent; use crate::services::QueryPlannerResponse; use crate::services::SubgraphServiceFactory; use crate::services::SupergraphRequest; @@ -269,7 +268,7 @@ async fn service_call( } match content { - Some(QueryPlannerContent::Plan { plan }) => { + Some(plan) => { let is_deferred = plan.is_deferred(&variables); let is_subscription = plan.is_subscription(); diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index 4bbe50e6cb..3908e0f22b 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -3839,7 +3839,6 @@ async fn test_cache_warmup() { use crate::query_planner::QueryPlan; use crate::services::PluggableSupergraphServiceBuilder; - use crate::services::QueryPlannerContent; use crate::services::QueryPlannerResponse; use crate::services::layers::persisted_queries::PersistedQueryExpander; use crate::services::query_planner; @@ -3865,9 +3864,7 @@ async fn test_cache_warmup() { /// Return an empty plan that doesn't require any subgraph requests to fulfill. fn empty_query_plan() -> QueryPlannerResponse { let plan = Arc::new(QueryPlan::fake_new(None, None)); - QueryPlannerResponse::builder() - .content(QueryPlannerContent::Plan { plan }) - .build() + QueryPlannerResponse::builder().content(plan).build() } /// Execute a constant mock query against the given supergraph service. diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index c73d50c794..09d570bb07 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -9,7 +9,6 @@ use crate::compute_job::ComputeJobType; use crate::json_ext::ValueExt; use crate::plugins::authorization::CacheKeyMetadata; use crate::query_planner::query_planner_service::QueryPlannerService; -use crate::services::QueryPlannerContent; use crate::services::QueryPlannerRequest; use crate::services::query_planner::PlanOptions; @@ -7701,7 +7700,7 @@ async fn authorization_filtered_query(query_str: &str) -> (Arc, Arc plan.query.clone(), + Some(plan) => plan.query.clone(), _ => panic!("filtering removed only `secret`, so the planner must return a plan"), }; assert!( From e2eb4f072da10c42580312a89f35b9c7592f9728 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 10:59:47 +0100 Subject: [PATCH 26/38] refactor: collapse FilterResult::Emptied into Filtered Filtering that removes every definition returns `Filtered` with the empty document, and the planner's empty-document arm matches on `document.definitions.is_empty()`. The variant duplicated a fact the document already carries. --- apollo-router/src/plugins/authorization/mod.rs | 17 ++++++++--------- .../src/query_planner/query_planner_service.rs | 4 +++- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index caa6b1b7e6..f09ebd3f71 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -141,16 +141,12 @@ pub(crate) struct UnauthorizedPaths { pub(crate) enum FilterResult { /// The operation asks for nothing the request lacks authorization for. Unchanged, - /// `document` is the operation with `paths` removed. + /// `document` is the operation with `paths` removed. Filtering can empty the + /// document entirely, leaving no definitions. Filtered { paths: Vec, document: ast::Document, }, - /// Filtering removed every definition from the document, so nothing is left to - /// plan and the operation proceeds as a plan with no work. A fully filtered - /// operation in a document that other operations keep non-empty is reported as - /// `Filtered`. - Emptied { paths: Vec }, } impl UnauthorizedPaths { @@ -381,8 +377,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Emptied { + return Ok(FilterResult::Filtered { paths: unauthorized_paths, + document: filtered_doc, }); } @@ -401,8 +398,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Emptied { + return Ok(FilterResult::Filtered { paths: unauthorized_paths, + document: filtered_doc, }); } @@ -421,8 +419,9 @@ impl AuthorizationPlugin { // FIXME: consider only `filtered_doc.operations.get(key.operation_name)`? if filtered_doc.definitions.is_empty() { - return Ok(FilterResult::Emptied { + return Ok(FilterResult::Filtered { paths: unauthorized_paths, + document: filtered_doc, }); } diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 4393260bef..2bd0ad96c8 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -462,7 +462,9 @@ impl QueryPlannerService { match filter_res { FilterResult::Unchanged => {} - FilterResult::Emptied { paths } => { + // Filtering can empty the document; the federation planner cannot plan a + // document with no definitions, so answer with a plan that carries no work. + FilterResult::Filtered { paths, document } if document.definitions.is_empty() => { selections.unauthorized.paths = paths; // References come from the operation that ran, and nothing did. From 70cbf5efe88e686769ef2feb64c9a531f8c0fa9a Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:00:12 +0100 Subject: [PATCH 27/38] docs: mark the refusal response as knowingly non-compliant Review-suggested wording. Execution was prevented, so the spec calls for a request error with no data; the layer answers with execution errors and `data: null` for backwards compatibility. ROUTER-2063 tracks the compliance work. --- apollo-router/src/plugins/authorization/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apollo-router/src/plugins/authorization/mod.rs b/apollo-router/src/plugins/authorization/mod.rs index f09ebd3f71..0d58da7698 100644 --- a/apollo-router/src/plugins/authorization/mod.rs +++ b/apollo-router/src/plugins/authorization/mod.rs @@ -609,6 +609,11 @@ impl Plugin for AuthorizationPlugin { let unauthorized = request.query_plan.query.unauthorized.clone(); unauthorized.log_unauthorized_paths(); + // We knowingly build an invalid response here. Execution was prevented, + // so we should respond with a request error and no data. Instead, we're + // responding with execution errors and a fake/incorrect `data: null`. We + // maintain backwards compatibility for the time being. + // Tracked in ROUTER-2063. let mut response = graphql::Response::builder().data(Value::Null).build(); unauthorized.update_response_with_unauthorized_path_errors(&mut response); From 5f9f5c912a97b7e841d10c9894c1e3e4b257db3b Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:01:46 +0100 Subject: [PATCH 28/38] test: assert the execution error that nullifies an overfetched field The error is already emitted, path `/currentUser/phone` with `UNAUTHORIZED_FIELD_OR_TYPE`, so the assertion pins existing behaviour: the field is null because it errored, not merely stripped. The test doc shrinks to the observable contract, and the two-pass ordering explanation moves to the formatting site in the execution service, where the ordering is enforced. --- .../src/plugins/authorization/tests.rs | 29 ++++++++++--------- .../src/services/execution/service.rs | 7 +++++ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 7b9edb6d62..8fb1521572 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -477,20 +477,8 @@ async fn authenticated_directive_reject_unauthorized() { assert_logs_contain_entire_request_authorization_error(); } -/// A subgraph can return more than the operation selected, so the data reaching response -/// formatting is not bounded by what authorization left in the query. `ExecutionService` -/// handles that by formatting twice: the filtered query projects the data onto the -/// authorized shape, then the original query expands it to the shape the client asked -/// for. -/// -/// `User.phone` is `@authenticated`, so filtering removes it from an unauthenticated -/// operation while this subgraph returns it anyway. `Some(Value::Null)` pins both halves -/// of that arrangement in one assertion: `phone` is present, so the original query -/// restored the requested shape, and it is null rather than `"1234"`, so the filtered -/// query stripped the value the client may not see. -/// -/// Removing either property changes this key: drop the filtered pass and it holds -/// `"1234"`, run the passes in the other order and it disappears from the response. +/// When a subgraph response includes authenticated fields, because of @requires or because +/// the subgraph is misbehaving, the authenticated field should not be propagated to the client. #[tokio::test] async fn overfetched_unauthorized_field_is_not_returned() { let service = TestHarness::builder() @@ -548,6 +536,19 @@ async fn overfetched_unauthorized_field_is_not_returned() { the client as null rather than as its value" ); assert_eq!(current_user.get("name"), Some(&json!("Ada"))); + + // `phone` is null because it errored, and the error names it. + let phone_error = response + .errors + .iter() + .find(|error| { + error.path.as_ref().map(ToString::to_string).as_deref() == Some("/currentUser/phone") + }) + .expect("an execution error nullified `phone`"); + assert_eq!( + phone_error.extensions.get("code"), + Some(&json!("UNAUTHORIZED_FIELD_OR_TYPE")) + ); } mod whole_operation_authorization { diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 07b633e8a1..c8d9416a0f 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -297,6 +297,13 @@ impl ExecutionService { .update_response_with_unauthorized_path_errors(&mut response); } + // Two passes, in this order. The filtered query projects the data onto the + // authorized shape, discarding fields the subgraphs returned beyond it + // (via @requires, or a misbehaving subgraph). The original query then + // expands the result to the shape the client requested, nulling what the + // filtered pass removed. One pass cannot do both: formatting with the + // original alone returns unauthorized values, formatting with the filtered + // alone drops requested fields from the response. if let Some(filtered_query) = query.filtered_query.as_ref() { paths = filtered_query.format_response( &mut response, From 6e975af5b70bba2b18b4c8b5878eb15d125c14d5 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:03:19 +0100 Subject: [PATCH 29/38] test: apply review feedback to the refusal test docs The status and null-data assertions merge into `returns_http_200_with_null_data`, the refusal, disabled, and extensions docs note that the behaviour is asserted for backwards compatibility rather than spec compliance, and the exactly-once helper's doc stops discussing spans it does not assert. --- .../src/plugins/authorization/tests.rs | 30 +++++-------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index 8fb1521572..fad19e795a 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -46,11 +46,6 @@ fn assert_span_contains_authorization_error_event(span: &str) { /// Asserts the `Authorization error` event for a refused operation was logged exactly /// once. One place decides a refusal, so a second event means two code paths both /// believe they own the log. -/// -/// The span the event belongs to is asserted by -/// `integration::telemetry::logging::test_authorization_error_event_in_execution_span`. -/// This harness runs without the telemetry plugin, so the `execution` span does not -/// exist here and the event lands directly in the `router` span. fn assert_logs_contain_entire_request_authorization_error() { let event_regex = Regex::new(r"ERROR .*Authorization error unauthorized_query_paths=\[.*]$").unwrap(); @@ -678,28 +673,17 @@ mod whole_operation_authorization { assert_no_subgraph_calls(handles).await; } - /// A refusal answers as a field error: HTTP 200 with `data: null` and the - /// authorization errors, not as a GraphQL request error, which carries a 4xx status - /// and no `data` entry. `rejection_sends_null_data` holds the data side of that - /// line; this holds the status. + /// A refusal answers as a field error: HTTP 200 with a present-but-`null` `data` and + /// the authorization errors, not as a GraphQL request error, which carries a 4xx + /// status and no `data` entry. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. #[tokio::test] - async fn returns_http_200() { + async fn returns_http_200_with_null_data() { let (service, _handles) = rejecting_router().await; - let (status, _body) = send_rejected_request(service, Context::new()).await; + let (status, body) = send_rejected_request(service, Context::new()).await; assert_eq!(status, http::StatusCode::OK); - } - - /// `data` is `null` and present. An absent `data` marks a request error; a null one - /// marks a field error that propagated to the root. The key's presence is part of - /// the response contract. - #[tokio::test] - async fn rejection_sends_null_data() { - let (service, _handles) = rejecting_router().await; - - let (_status, body) = send_rejected_request(service, Context::new()).await; - assert_eq!(body.get("data"), Some(&serde_json::Value::Null)); } @@ -726,6 +710,7 @@ mod whole_operation_authorization { /// `errors.response: disabled` suppresses the authorization errors, so `data: null` /// is the only thing telling the client the operation produced nothing. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. #[tokio::test] async fn rejection_with_errors_disabled_sends_null_data_and_no_errors() { let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ @@ -744,6 +729,7 @@ mod whole_operation_authorization { /// `errors.response: extensions` moves the authorization errors under /// `extensions.authorizationErrors` and leaves `errors` out of the response. + /// This is not a spec-compliant behaviour, but is asserted for backwards compatibility. #[tokio::test] async fn rejection_with_errors_in_extensions() { let (service, _handles) = router_with_unresponsive_subgraphs(serde_json::json!({ From b8d9c8a5c630264d783dabc799b040234bb2c94d Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:04:48 +0100 Subject: [PATCH 30/38] test: pin validation gates answering before the authorization refusal Variable validation and the subscription and @defer accept-header checks run in the supergraph service, before the execution layer refuses, so an operation failing both receives the validation error alone. Measured for the variable case: 400 VALIDATION_INVALID_TYPE_VARIABLE with no data key and no authorization error, where the refusal previously answered. A breaking changeset describes the ordering. The subscription and @defer cases carry no pin: they need a schema with subscriptions. --- ...breaking_bryn_router_1973_gate_ordering.md | 5 ++++ .../src/plugins/authorization/tests.rs | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100644 .changesets/breaking_bryn_router_1973_gate_ordering.md diff --git a/.changesets/breaking_bryn_router_1973_gate_ordering.md b/.changesets/breaking_bryn_router_1973_gate_ordering.md new file mode 100644 index 0000000000..d0cfef55cc --- /dev/null +++ b/.changesets/breaking_bryn_router_1973_gate_ordering.md @@ -0,0 +1,5 @@ +### Request validation errors answer before authorization errors ([PR #9911](https://github.com/apollographql/router/pull/9911)) + +Authorization enforcement runs at execution, after the router validates the request. An operation that fails both checks now receives the validation error alone: a missing or invalid variable returns the 400 validation response, and a subscription or `@defer` operation sent without the matching `Accept` header returns the 406, where these previously received the authorization errors. Fixing the request then surfaces the authorization errors. + +By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/apollo-router/src/plugins/authorization/tests.rs b/apollo-router/src/plugins/authorization/tests.rs index fad19e795a..72bc0c698c 100644 --- a/apollo-router/src/plugins/authorization/tests.rs +++ b/apollo-router/src/plugins/authorization/tests.rs @@ -664,6 +664,34 @@ mod whole_operation_authorization { (status, body) } + /// Supergraph-level gates run before the execution service, so variable validation + /// answers an unauthorized operation ahead of the authorization refusal. The client + /// fixes the variable and then receives the refusal. The same ordering applies to + /// the accept-header checks for subscriptions and `@defer`. + #[tokio::test] + async fn variable_validation_answers_before_the_refusal() { + let (service, _handles) = rejecting_router().await; + + let (status, body) = wire_response( + service, + graphql_post( + // `orga.id` is `@authenticated`, and `$id` is required but not provided. + "query($id: ID!) { orga(id: $id) { id } }", + None, + Context::new(), + ), + ) + .await; + + assert_eq!(status, http::StatusCode::BAD_REQUEST); + assert_eq!( + body.pointer("/errors/0/extensions/code"), + Some(&serde_json::json!("VALIDATION_INVALID_TYPE_VARIABLE")), + "body: {body}" + ); + assert_eq!(body.pointer("/data"), None, "body: {body}"); + } + #[tokio::test] async fn does_not_reach_execution() { let (service, handles) = rejecting_router().await; From 20aab33dd02c4681a5a87e63314e2d333cefabf7 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:05:52 +0100 Subject: [PATCH 31/38] test: exercise both authorization error emitters in the span test The fixture enabled directives without reject_unauthorized, so the operation ran its empty plan and response formatting emitted the event; the layer's emitter went unexercised, and deleting its log call left the test green. The fixture now sets reject_unauthorized so the layer answers and logs, and a second fixture without it covers the response-formatting emitter. Both cases assert the event lands under the execution span exactly once. Deleting the layer's log call fails exactly the reject case. --- .../authorization_error_span.router.yaml | 1 + ...orization_error_span_no_reject.router.yaml | 8 +++++ .../tests/integration/telemetry/logging.rs | 31 +++++++++++++++---- 3 files changed, 34 insertions(+), 6 deletions(-) create mode 100644 apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml diff --git a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml index 5173c12d2d..5929b15d48 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span.router.yaml @@ -6,3 +6,4 @@ telemetry: authorization: directives: enabled: true + reject_unauthorized: true diff --git a/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml new file mode 100644 index 0000000000..5173c12d2d --- /dev/null +++ b/apollo-router/tests/integration/telemetry/fixtures/authorization_error_span_no_reject.router.yaml @@ -0,0 +1,8 @@ +telemetry: + exporters: + logging: + stdout: + format: json +authorization: + directives: + enabled: true diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index 20f2f111ca..000176f3d9 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -246,12 +246,15 @@ async fn test_text_sampler_off() -> Result<(), BoxError> { /// `execution` span, exactly once per request. The `execution` span comes from the /// telemetry plugin, which only joins the pipeline when OpenTelemetry is initialised for /// the process, so a spawned router is the smallest thing that has it. -#[tokio::test(flavor = "multi_thread")] -async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxError> { +/// +/// With `reject_unauthorized`, the authorization layer on the execution service emits +/// the event; without it, response formatting does. Each configuration exercises its +/// own emitter. +async fn assert_authorization_error_event_in_execution_span( + config: &'static str, +) -> Result<(), BoxError> { let mut router = IntegrationTest::builder() - .config(include_str!( - "fixtures/authorization_error_span.router.yaml" - )) + .config(config) .supergraph("tests/fixtures/supergraph-auth.graphql") .build() .await; @@ -260,7 +263,7 @@ async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxErr router.assert_started().await; // `Query.me` requires the `profile` scope, so an unauthenticated request loses its - // only root field and authorization refuses the operation. + // only root field. router .execute_query( Query::builder() @@ -302,3 +305,19 @@ async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxErr router.graceful_shutdown().await; Ok(()) } + +#[tokio::test(flavor = "multi_thread")] +async fn test_authorization_error_event_in_execution_span() -> Result<(), BoxError> { + assert_authorization_error_event_in_execution_span(include_str!( + "fixtures/authorization_error_span.router.yaml" + )) + .await +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_authorization_error_event_in_execution_span_without_reject() -> Result<(), BoxError> { + assert_authorization_error_event_in_execution_span(include_str!( + "fixtures/authorization_error_span_no_reject.router.yaml" + )) + .await +} From 6eb728252fe0c08a4a2645fdf90f471d2da0dc45 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:06:09 +0100 Subject: [PATCH 32/38] docs: reword the authorization changesets per review Refusal is reserved for reject_unauthorized and named wherever it comes up; the directive case reads as authorization erroring every field rather than fields being removed, since removal is how the router works around the planner, not what the client experiences; and the shaped-data change is framed as the spec fix it is. --- ...ing_bryn_router_1973_emptied_operations_shaped_data.md | 8 +++++--- .../fix_bryn_router_1973_report_rejected_operations.md | 8 ++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md index 4b7963fd27..420060c18b 100644 --- a/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md +++ b/.changesets/breaking_bryn_router_1973_emptied_operations_shaped_data.md @@ -1,11 +1,13 @@ -### Fully unauthorized operations return the same response shape as partially unauthorized ones ([PR #9911](https://github.com/apollographql/router/pull/9911)) +### Operations with only authorization errors return spec-compliant data ([PR #9911](https://github.com/apollographql/router/pull/9911)) -When authorization directives remove every field from an operation, the response now carries each requested root field as `null` alongside the authorization errors, matching what clients already receive when only some fields are removed: +When every field in an operation fails authorization, the response now carries each requested root field as `null` alongside an error for each field, the same shape clients receive when some fields fail: ```json {"data": {"orga": null}, "errors": [{"message": "Unauthorized field or type", "path": ["orga", "id"], "extensions": {"code": "UNAUTHORIZED_FIELD_OR_TYPE"}}]} ``` -Such responses previously carried `"data": null`. Clients that detect a fully refused operation by checking `data` for `null` should check for errors with the `UNAUTHORIZED_FIELD_OR_TYPE` code instead, which covers partial refusals as well. +The router previously returned `"data": null` here, incorrectly reporting that execution never produced a result. Clients that detect this case by checking `data` for `null` should check for errors with the `UNAUTHORIZED_FIELD_OR_TYPE` code instead, which covers partial failures as well. + +`authorization.directives.reject_unauthorized` keeps returning `"data": null`; spec compliance for refused operations is tracked separately. By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 diff --git a/.changesets/fix_bryn_router_1973_report_rejected_operations.md b/.changesets/fix_bryn_router_1973_report_rejected_operations.md index 663012c9dd..88cdd4b9fa 100644 --- a/.changesets/fix_bryn_router_1973_report_rejected_operations.md +++ b/.changesets/fix_bryn_router_1973_report_rejected_operations.md @@ -1,9 +1,9 @@ -### Report operations rejected by authorization to Apollo Studio ([PR #9911](https://github.com/apollographql/router/pull/9911)) +### Report operations whose fields all fail authorization to Apollo Studio ([PR #9911](https://github.com/apollographql/router/pull/9911)) -When authorization refuses an operation outright, Apollo Studio now receives it as an operation, identified by the signature of the query the client sent and carrying the client name, version, and request count. Studio previously received the operation count alone and had nothing to attribute it to. +When authorization raises errors for every field in an operation, or `authorization.directives.reject_unauthorized` refuses the operation outright, Apollo Studio now receives it as an operation, identified by the signature of the query the client sent and carrying the client name, version, and request count. Studio previously received the operation count alone and had nothing to attribute it to. -A refused operation counts as one licensed operation. +Such an operation counts as one licensed operation. -The `Authorization error` log event for a refused operation now appears under the `execution` span instead of inside `query_planning`. Update log or trace filters that match this event by span name. +The `Authorization error` log event for these operations now appears under the `execution` span instead of inside `query_planning`. Update log or trace filters that match this event by span name. By [@BrynCooke](https://github.com/BrynCooke) in https://github.com/apollographql/router/pull/9911 From c86788694e9fc0cd8fc3956c61a6184a6a7621a3 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 11:06:28 +0100 Subject: [PATCH 33/38] docs: record review-deferred issues in the authorization dev doc Gate ordering, the graphql::Response null-collapse and its subgraph-response implication, the partial-filter parent-nulling question, and the ROUTER-2063 scope, alongside the FilterResult references catching up with the Emptied collapse. --- dev-docs/authorization-query-planning.md | 34 ++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/dev-docs/authorization-query-planning.md b/dev-docs/authorization-query-planning.md index aefd6bf240..7d4833dad4 100644 --- a/dev-docs/authorization-query-planning.md +++ b/dev-docs/authorization-query-planning.md @@ -73,7 +73,7 @@ sequenceDiagram alt cache miss CQP->>QPS: QueryPlannerRequest QPS->>QPS: filter_query, no reject_unauthorized input - alt FilterResult::Emptied + alt FilterResult::Filtered with an empty document QPS-->>CQP: Plan with root None,
usage reporting from the ORIGINAL query else Filtered or Unchanged QPS-->>CQP: Plan @@ -99,10 +99,10 @@ Key references, on the branch: | What | Where | | --- | --- | -| `FilterResult` (Unchanged / Filtered / Emptied) | `plugins/authorization/mod.rs:152` | +| `FilterResult` (Unchanged / Filtered) | `plugins/authorization/mod.rs` | | `filter_query`, no config-driven refusal | `plugins/authorization/mod.rs:371` | | The layer: `checkpoint_async` ahead of the counter | `plugins/authorization/mod.rs:629` | -| Planner's `Emptied` arm, empty plan | `query_planner/query_planner_service.rs:465` | +| Planner's empty-document arm, empty plan | `query_planner/query_planner_service.rs` | | Single-variant `QueryPlannerContent` | `services/query_planner.rs:103` | | Two-pass formatting (unchanged) | `services/execution/service.rs:291` | @@ -113,7 +113,7 @@ instead of `data: null` (the breaking changeset). ## Known gap, pinned but unresolved: multi-operation documents -`Emptied` means the *document* emptied, not the executed operation. `filter_query` +The emptiness check covers the *document*, not the executed operation. `filter_query` checks `filtered_doc.definitions.is_empty()` after each directive stage, and definitions include every operation and fragment in the document, executed or not. @@ -127,7 +127,7 @@ query B { currentUser { name } } ``` Filtering empties `A` and removes it from the document. `B` keeps the document -non-empty, so `filter_query` reports `Filtered`, not `Emptied`. The planner then looks +non-empty, so the document is filtered rather than empty. The planner then looks up operation `A` in a document that no longer contains it: ``` @@ -249,6 +249,30 @@ absent-versus-null `data` distinction, which live on the response rather than th error. `returns_http_200` and `rejection_sends_null_data` hold the current shape and flip when this resolves. +## Outstanding: raised in review, deferred + +Validation gates outrank authorization. Variable validation and the subscription and +`@defer` accept-header checks run in the supergraph service, before the execution +layer answers a refusal, so an operation failing both receives the validation error +alone. Changeset `breaking_bryn_router_1973_gate_ordering` describes it; +`variable_validation_answers_before_the_refusal` pins the variable case. The +subscription and `@defer` cases have no pin: they need a schema with subscriptions. + +`graphql::Response` cannot represent `data: null`. Deserializing collapses a JSON +`null` under `data` into an absent key, so anything downstream of deserialization +loses the distinction — including responses parsed *from subgraphs*, which may +misreport a subgraph's `data: null` as no data at all. Wire-level tests read bytes to +route around it. + +Partial filtering nulls the parent, not the field. Filtering `currentUser.phone` out +of an operation yields `{"currentUser": null}` where field-error semantics call for +`{"currentUser": {"phone": null}}` when `phone` is nullable. Whether filtering should +preserve the unfiltered parent shape is unresolved. + +Spec compliance for refused operations is ROUTER-2063: request error, `data` absent, +status by negotiated content type, and the `errors.response` modes that conflict with +an absent `data`. + ## Still open elsewhere Keying the plan cache on the filtered query text (making `CacheKeyMetadata` redundant From 2305e3585c5d9168afd221d7766103e9de9d214c Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 12:56:25 +0100 Subject: [PATCH 34/38] refactor: share the filtered-then-original formatting between execution and tests `Query::format_response_filtered_then_original` runs the filtered pass, when one exists, then the original, and returns the nullified paths from both. The execution service and the spec tests call it instead of each sequencing the two `format_response` calls, so the ordering the passes depend on lives in one place. --- .../src/services/execution/service.rs | 22 +---------- apollo-router/src/spec/query.rs | 37 +++++++++++++++++++ apollo-router/src/spec/query/tests.rs | 13 ++----- 3 files changed, 42 insertions(+), 30 deletions(-) diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index c8d9416a0f..d6085f9da5 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -289,7 +289,6 @@ impl ExecutionService { let variables_set = query.defer_variables_set(variables); tracing::debug_span!("format_response").in_scope(|| { - let mut paths = Vec::new(); if !query.unauthorized.paths.is_empty() { query.unauthorized.log_unauthorized_paths(); query @@ -297,30 +296,13 @@ impl ExecutionService { .update_response_with_unauthorized_path_errors(&mut response); } - // Two passes, in this order. The filtered query projects the data onto the - // authorized shape, discarding fields the subgraphs returned beyond it - // (via @requires, or a misbehaving subgraph). The original query then - // expands the result to the shape the client requested, nulling what the - // filtered pass removed. One pass cannot do both: formatting with the - // original alone returns unauthorized values, formatting with the filtered - // alone drops requested fields from the response. - if let Some(filtered_query) = query.filtered_query.as_ref() { - paths = filtered_query.format_response( - &mut response, - variables.clone(), - schema.api_schema(), - variables_set, - insert_result_coercion_errors, - ); - } - - paths.extend(query.format_response( + let paths = query.format_response_filtered_then_original( &mut response, variables.clone(), schema.api_schema(), variables_set, insert_result_coercion_errors, - )); + ); for error in response.errors.iter_mut() { if let Some(path) = &mut error.path { diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index eb009a23c5..350630f431 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -127,6 +127,43 @@ impl Query { /// This will discard unrequested fields and re-order the output to match the order of the /// query. #[tracing::instrument(skip_all, level = "trace")] + /// Formats a response for an operation that authorization may have filtered, + /// returning the nullified paths from every pass. + /// + /// The filtered query formats first, projecting the data onto the authorized shape + /// and discarding fields the subgraphs returned beyond it (via `@requires`, or a + /// misbehaving subgraph). The original query formats second, expanding the result to + /// the shape the client requested and nulling what filtering removed. One pass cannot + /// do both: the original alone returns unauthorized values, the filtered alone drops + /// requested fields from the response. + pub(crate) fn format_response_filtered_then_original( + &self, + response: &mut Response, + variables: Object, + schema: &ApiSchema, + defer_conditions: BooleanValues, + include_coercion_errors: bool, + ) -> Vec { + let mut paths = Vec::new(); + if let Some(filtered_query) = self.filtered_query.as_ref() { + paths = filtered_query.format_response( + response, + variables.clone(), + schema, + defer_conditions, + include_coercion_errors, + ); + } + paths.extend(self.format_response( + response, + variables, + schema, + defer_conditions, + include_coercion_errors, + )); + paths + } + pub(crate) fn format_response( &self, response: &mut Response, diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 1f977c0f6b..f2dbdaffa3 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -7712,19 +7712,12 @@ async fn authorization_filtered_query(query_str: &str) -> (Arc, Arc Value { let mut response = crate::graphql::Response::builder().data(data).build(); - query.filtered_query.as_ref().unwrap().format_response( - &mut response, - Object::new(), - schema.api_schema(), - BooleanValues { bits: 0 }, - true, - ); - query.format_response( + query.format_response_filtered_then_original( &mut response, Object::new(), schema.api_schema(), From 0ec06b87706cd9d50348d2a84a5cdd4804af7069 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 12:58:48 +0100 Subject: [PATCH 35/38] test: drive the empty-plan test through the planner's public service API The test asserts one thing: an unauthenticated client requesting only fields that require authentication gets a plan with nothing to execute and every requested field in `unauthorized.paths`. It now says so, calls the planner as a tower service instead of the private `get`, and drops the warm-up and caching framing, which were incidental to that claim. Renamed `fully_unauthorized_operation_plans_no_work`. --- .../query_planner/query_planner_service.rs | 45 +++++++------------ 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/apollo-router/src/query_planner/query_planner_service.rs b/apollo-router/src/query_planner/query_planner_service.rs index 2bd0ad96c8..d27dc0745b 100644 --- a/apollo-router/src/query_planner/query_planner_service.rs +++ b/apollo-router/src/query_planner/query_planner_service.rs @@ -1080,27 +1080,18 @@ mod tests { keys.join("\n") } - /// Warm-up reaches this service with `CacheKeyMetadata::default()`, the same metadata - /// an unauthenticated request produces: `queries_to_warm_up` supplies no metadata for - /// persisted queries, and for re-warmed cache entries `update_cache_key` derives - /// default metadata from warm-up's claimless context. Filtering empties the operation - /// under those inputs, so the plan carries no work rather than the unfiltered fetches. - /// - /// Scope: `compute_job_type` only selects the compute-pool priority and the metric - /// label, and `get` filters before reading it, so `QueryPlanningWarmup` behaves - /// exactly like `QueryPlanning` here. The plan cache sits above this service; that - /// the empty plan is cached keyed by authorization state is covered by - /// `caching_query_planner::tests::emptied_operation_plan_is_cached`. + /// An unauthenticated client requesting only fields that require authentication gets + /// an empty query plan: nothing to execute, and every requested field in + /// `unauthorized.paths`. #[test(tokio::test)] - async fn planning_unauthenticated_returns_empty_plan_not_unfiltered_plan() { + async fn fully_unauthorized_operation_plans_no_work() { let configuration: Configuration = serde_json::from_value(serde_json::json!({ "authorization": { "directives": { "enabled": true } } })) .unwrap(); let configuration = Arc::new(configuration); - // `Query.me` requires the `profile` scope, so filtering an unauthenticated - // request empties the document. + // `Query.me` requires the `profile` scope. let schema = include_str!("../../tests/fixtures/supergraph-auth.graphql"); let schema = Arc::new(Schema::parse(schema, &configuration).unwrap()); let planner = QueryPlannerService::for_test(schema.clone(), configuration.clone()).unwrap(); @@ -1108,26 +1099,22 @@ mod tests { let query = "query { me { name } }"; let doc = Query::parse_document(query, None, &schema, &configuration).unwrap(); - let content = planner - .get( - QueryKey { - original_query: query.to_string(), - filtered_query: query.to_string(), - operation_name: None, - metadata: CacheKeyMetadata::default(), - plan_options: PlanOptions::default(), - }, - doc, - ComputeJobType::QueryPlanningWarmup, - ) + let response = planner + .oneshot(QueryPlannerRequest { + query: query.to_string(), + operation_name: None, + document: doc, + metadata: CacheKeyMetadata::default(), + plan_options: PlanOptions::default(), + compute_job_type: ComputeJobType::QueryPlanning, + }) .await .unwrap(); - let plan = content; + let plan = response.content.expect("planning succeeded"); assert!( plan.root.is_none(), - "an unauthenticated request must not plan any work; a plan with fetches \ - cached under default metadata is reachable by any unauthenticated request" + "the plan must carry no fetches when every requested field failed authorization" ); assert_eq!( plan.query From 95980bf626f614295553329d150463aa65fb54e4 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 13:00:13 +0100 Subject: [PATCH 36/38] test: name the abstract-type formatting tests by what they verify Review-suggested doc and name: the tests verify that double-applied response formatting resolves fragment spreads on abstract types correctly, and the internal `__typename` hand-off is the mechanism rather than the point. Renamed `filtered_inline_fragment_on_abstract_type` and `filtered_fragment_spread_on_abstract_type`. --- apollo-router/src/spec/query/tests.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index f2dbdaffa3..32a2a192b5 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -7734,15 +7734,16 @@ fn format_filtered_then_original(query: &Query, schema: &Schema, data: Value) -> .clone() } -/// `Thing` is an interface, so `apply_selection_set` takes the concrete type from the -/// response `__typename` rather than from the schema. The filtered pass has to copy -/// `__typename` into its output for the original pass to resolve the type condition on -/// `... on Foo`, so `inline` survives only if the copy happened. +/// Double-applying response formatting on an abstract type should apply the +/// correct fragment spread. +/// `__typename` is not selected by the input query, but internally we have to +/// pass it along to the final formatting pass, else it wouldn't know which concrete +/// type it's working on. /// -/// One fragment form per test: each form copies `__typename` independently, so a query -/// carrying both keeps its fields alive when either copy runs. +/// One fragment form per test: each form passes `__typename` along independently, so a +/// query carrying both keeps its fields alive when either does. #[tokio::test] -async fn filtered_query_keeps_typename_for_inline_fragment() { +async fn filtered_inline_fragment_on_abstract_type() { // `secret` is `@authenticated`, so filtering drops it and leaves `inline`. let (query, schema) = authorization_filtered_query("{ thing { id ... on Foo { inline secret } } }").await; @@ -7757,10 +7758,10 @@ async fn filtered_query_keeps_typename_for_inline_fragment() { assert_eq!(thing.get("inline"), Some(&json!("inline"))); } -/// The fragment-spread counterpart of `filtered_query_keeps_typename_for_inline_fragment`, +/// The fragment-spread counterpart of `filtered_inline_fragment_on_abstract_type`, /// covering the second `!is_original` branch in `apply_selection_set`. #[tokio::test] -async fn filtered_query_keeps_typename_for_fragment_spread() { +async fn filtered_fragment_spread_on_abstract_type() { let (query, schema) = authorization_filtered_query( "{ thing { id ...Spread } } fragment Spread on Foo { spread secret }", ) From 33a892f0b3cd638d0a793b7edd260a759303911a Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 16:08:16 +0100 Subject: [PATCH 37/38] test: policy directive tests assert the shaped response An operation whose every field fails authorization now returns each requested root field as null rather than `data: null`, so these assert `data.secure` and `data.private` are null instead of asserting on `data` itself. Each test's error placement assertions are unchanged, and the docs name the field rather than the whole response. --- .../tests/integration/directives/policy.rs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/apollo-router/tests/integration/directives/policy.rs b/apollo-router/tests/integration/directives/policy.rs index 5d4f5bdd35..074ebb2ba3 100644 --- a/apollo-router/tests/integration/directives/policy.rs +++ b/apollo-router/tests/integration/directives/policy.rs @@ -226,7 +226,7 @@ async fn policy_directive_should_not_pass_if_coproc_disallowed() -> Result<(), B .unwrap(); // THEN - // * we get NO data back for the private field! + // * the private field is null let data = supergraph_harness .oneshot(request) .await @@ -236,7 +236,7 @@ async fn policy_directive_should_not_pass_if_coproc_disallowed() -> Result<(), B .unwrap() .data .unwrap(); - assert!(data.is_null()); + assert_eq!(data.get("private"), Some(&serde_json_bytes::Value::Null)); Ok(()) } @@ -460,7 +460,7 @@ async fn interface_with_different_implementation_policies_should_require_auth() let data = response.data.unwrap(); let error = response.errors.first().unwrap(); - assert!(data.is_null()); + assert_eq!(data.get("secure"), Some(&serde_json_bytes::Value::Null)); assert_eq!( error.extension_code().unwrap(), "UNAUTHORIZED_FIELD_OR_TYPE".to_string() @@ -553,7 +553,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to put errors into extensions rather than the errors array /// Then: - /// * data is null + /// * `secure` is null /// * errors array is empty /// * the authorization error appears in extensions["authorizationErrors"] #[tokio::test(flavor = "multi_thread")] @@ -565,7 +565,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(response.errors.is_empty()); assert!(!response.extensions.is_empty()); @@ -578,7 +581,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to put errors into the errors array /// Then: - /// * data is null + /// * `secure` is null /// * the authorization error appears in errors /// * extensions has no `authorizationErrors` #[tokio::test(flavor = "multi_thread")] @@ -590,7 +593,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(!response.errors.is_empty()); assert!(!response.extensions.contains_key("authorizationErrors")); @@ -603,7 +609,7 @@ mod all_unauthorized_paths { /// * the router configuration described in `send_request` /// * authorization configured to suppress errors entirely /// Then: - /// * data is null + /// * `secure` is null /// * errors array is empty /// * extensions has no `authorizationErrors` #[tokio::test(flavor = "multi_thread")] @@ -615,7 +621,10 @@ mod all_unauthorized_paths { }); let response = send_request(authorization_conf).await.unwrap(); - assert!(response.data.unwrap().is_null()); + assert_eq!( + response.data.unwrap().get("secure"), + Some(&serde_json_bytes::Value::Null) + ); assert!(response.errors.is_empty()); assert!(!response.extensions.contains_key("authorizationErrors")); } From b661e3c40b1e4ec2f0a61cbfcf20a53381783666 Mon Sep 17 00:00:00 2001 From: bryn Date: Wed, 19 Aug 2026 16:08:16 +0100 Subject: [PATCH 38/38] test: read the query plan cache entry without the removed enum tag The cached value serializes as `{"Ok": }` now that `QueryPlannerContent` is a type alias, so the `Plan` and `plan` levels are gone from the JSON path. The header's sample value is elided rather than restated: the instructions above it are about finding the key, and a full value transcript goes stale on every schema or field change. --- apollo-router/tests/integration/redis.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/apollo-router/tests/integration/redis.rs b/apollo-router/tests/integration/redis.rs index 93c5ffd956..df2c385776 100644 --- a/apollo-router/tests/integration/redis.rs +++ b/apollo-router/tests/integration/redis.rs @@ -10,15 +10,7 @@ // ```bash // 1724831727.472732 [0 127.0.0.1:56720] "SET" // "plan:0:v2.8.5:70f115ebba5991355c17f4f56ba25bb093c519c4db49a30f3b10de279a4e3fa4:3973e022e93220f9212c18d0d0c543ae7c309e46640da93a4a0314de999f5112:4f9f0183101b2f249a364b98adadfda6e5e2001d1f2465c988428cf1ac0b545f" -// "{\"Ok\":{\"Plan\":{\"plan\":{\"usage_reporting\":{\"statsReportKey\":\"# -// -\\n{topProducts{name -// name}}\",\"referencedFieldsByType\":{\"Product\":{\"fieldNames\":[\"name\"],\"isInterface\":false},\"Query\":{\"fieldNames\":[\"topProducts\"],\"isInterface\":false}}},\"root\":{\"kind\":\"Fetch\",\"serviceName\":\"products\",\"variableUsages\":[],\"operation\":\"{topProducts{name -// name2:name}}\",\"operationName\":null,\"operationKind\":\"query\",\"id\":null,\"inputRewrites\":null,\"outputRewrites\":null,\"contextRewrites\":null,\"schemaAwareHash\":\"121b9859eba2d8fa6dde0a54b6e3781274cf69f7ffb0af912e92c01c6bfff6ca\",\"authorization\":{\"is_authenticated\":false,\"scopes\":[],\"policies\":[]}},\"formatted_query_plan\":\"QueryPlan -// {\\n Fetch(service: \\\"products\\\") {\\n {\\n topProducts {\\n -// name\\n name2: name\\n }\\n }\\n -// n },\\n}\",\"query\":{\"string\":\"{\\n topProducts {\\n name\\n -// name2: name\\n -// }\\n}\\n\",\"fragments\":{\"map\":{}},\"operations\":[{\"name\":null,\"kind\":\"query\",\"type_name\":\"Query\",\"selection_set\":[{\"Field\":{\"name\":\"topProducts\",\"alias\":null,\"selection_set\":[{\"Field\":{\"name\":\"name\",\"alias\":null,\"selection_set\":null,\"field_type\":{\"Named\":\"String\"},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}},{\"Field\":{\"name\":\"name\",\"alias\":\"name2\",\"selection_set\":null,\"field_type\":{\"Named\":\"String\"},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}}],\"field_type\":{\"List\":{\"Named\":\"Product\"}},\"include_skip\":{\"include\":\"Yes\",\"skip\":\"No\"}}}],\"variables\":{}}],\"subselections\":{},\"unauthorized\":{\"paths\":[],\"errors\":{\"log\":true,\"response\":\"errors\"}},\"filtered_query\":null,\"defer_stats\":{\"has_defer\":false,\"has_unconditional_defer\":false,\"conditional_defer_variable_names\":[]},\"is_original\":true,\"schema_aware_hash\":[20,152,93,92,189,0,240,140,9,65,84,255,4,76,202,231,69,183,58,121,37,240,0,109,198,125,1,82,12,42,179,189]},\"query_metrics\":{\"depth\":2,\"height\":3,\"root_fields\":1,\"aliases\":1},\"estimated_size\":0}}}}" +// "{\"Ok\":{\"usage_reporting\":{...},\"root\":{...},\"query\":{...},...}}" // "EX" "10" // ``` @@ -166,10 +158,6 @@ async fn query_planner_cache() -> Result<(), BoxError> { .unwrap() .get("Ok") .unwrap() - .get("Plan") - .unwrap() - .get("plan") - .unwrap() .get("root"); insta::assert_json_snapshot!(query_plan);