refactor(executor): handle header propagation outside of the plan executor#652
refactor(executor): handle header propagation outside of the plan executor#652
Conversation
Summary of ChangesHello @ardatan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the GraphQL plan executor to directly produce and return Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the plan executor to return a complete ntex::http::Response instead of raw bytes and headers, simplifying the pipeline in bin/router/src/pipeline/mod.rs. However, a high-severity vulnerability was identified in the response header propagation logic: it currently uses a denylist for hop-by-hop headers, which is insufficient. A strict allow-list is required to prevent malicious subgraphs from injecting dangerous headers like Set-Cookie or Content-Security-Policy, which could lead to session fixation or clickjacking. Additionally, performance concerns were noted due to allocations on the hot path during header type conversions, which should be addressed as per the repository's style guide.
✅
|
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-3e87a278-2b2a-4ebc-87dd-3ddfe5d883ad/builder-3e87a278-2b2a-4ebc-87dd-3ddfe5d883ad0/v0bzl10dd4xq3mzzpbrf5e4sz",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:930fc4f492a3781f7ac76e345a2fae6cde94299c7db7d0fced600396725ab795",
"size": 1609
},
"containerimage.digest": "sha256:930fc4f492a3781f7ac76e345a2fae6cde94299c7db7d0fced600396725ab795",
"image.name": "ghcr.io/graphql-hive/router:pr-652,ghcr.io/graphql-hive/router:sha-5e4be2d"
} |
ardatan
force-pushed
the
return-response-from-executor
branch
from
234544c to
47655cb
Compare
ardatan
force-pushed
the
return-response-from-executor
branch
2 times, most recently
from
8a61cea to
406299f
Compare
lib/executor/src/execution/plan.rs
Outdated
| .content_type(ctx.response_content_type) | ||
| .body(body); | ||
|
|
||
| modify_client_response_headers(exec_ctx.response_headers_aggregator, response.headers_mut()) |
There was a problem hiding this comment.
It gives users the power to modify the content-type header, we're okay with it?
There was a problem hiding this comment.
I can prevent that inside modify_client_response_headers ?
There was a problem hiding this comment.
I'm just asking for opinion, if we should do allow it or not
There was a problem hiding this comment.
It is also a header at the end of the day. Maybe user wants to have a special json type? I think it is fine to allow it.
There was a problem hiding this comment.
Client should not impact the calls to subgraphs, and neither should subgraphs to clients. There should be complete separation of client<->router and router<->subgraph - the only thing that can propagate are the allowed headers as per the config - excluding content-type and accept.
|
Closed in favor of #692 |
Pull request was closed
ardatan
force-pushed
the
return-response-from-executor
branch
from
b21c13f to
02dca46
Compare
enisdenjo
left a comment
enisdenjo
left a comment
There was a problem hiding this comment.
When it comes to #620, the execution_pipeline can either return:
- an execution output with a graphql response
- at the time of writing, the graphql response is in
body: Vec<u8>in the subs pr
- at the time of writing, the graphql response is in
- or a subscriptions output with a stream of graphql responses
- at the time of writing, the graphql response is in
body: BoxStream<'static, Vec<u8>>in the subs pr
- at the time of writing, the graphql response is in
I don't mind it being a ntext response, as long as we can assemble it outside of an http request handler for subscriptions over websockets.
Just for context, in websockets, I mimic http requests by assembing an http request-like for each ws operation message - so the execute_query_plan still has the whole ClientRequestDetails deal happening. I figured a great and reliable way of doing that, so the execute_query_plan will continue getting the ClientRequestDetails.
bin/router/src/pipeline/execution.rs
Outdated
| let body = project_by_operation( | ||
| &Value::Null, | ||
| vec![], | ||
| &extensions, | ||
| planned_request.normalized_payload.root_type_name, | ||
| &[], | ||
| &None, | ||
| 0, | ||
| introspection_context.metadata, | ||
| ) | ||
| .with_plan_context(LazyPlanContext { | ||
| subgraph_name: || None, | ||
| affected_path: || None, | ||
| }) | ||
| .map_err(|err| { | ||
| tracing::error!( | ||
| "Failed to project query plan to extensions during dry-run: {}", | ||
| err | ||
| ); | ||
| PipelineError::PlanExecutionError(err) | ||
| })?; | ||
| let response = ntex::http::Response::Ok().json(&json!({ | ||
| "extensions": extensions, | ||
| })); |
lib/executor/src/execution/plan.rs
Outdated
| .content_type(ctx.response_content_type) | ||
| .body(body); | ||
|
|
||
| modify_client_response_headers(exec_ctx.response_headers_aggregator, response.headers_mut()) |
There was a problem hiding this comment.
Client should not impact the calls to subgraphs, and neither should subgraphs to clients. There should be complete separation of client<->router and router<->subgraph - the only thing that can propagate are the allowed headers as per the config - excluding content-type and accept.
bin/router/src/pipeline/mod.rs
Outdated
| supergraph: &SupergraphData, | ||
| shared_state: &Arc<RouterSharedState>, | ||
| schema_state: &Arc<SchemaState>, | ||
| response_content_type: &'static str, |
There was a problem hiding this comment.
Why does the execution_pipeline need to know about the response content-type? As I said in previous comment, they should be separate an the client should in no way influence the content-type of internal subgraph calls. Subscriptions, for example, would not have a content-type at all when dealing with WebSockets.
The execution_pipeline should be oblivious to the why or for whom it's resolving something.
There was a problem hiding this comment.
I moved the response header propagation to the pipeline executor. My point is to avoid creating a new HeaderMap in the execution then re-iterate it while creating ntex:http::Response. I think with my last commit, the plan executor becomes fully HTTP independent as you wish, right?
There was a problem hiding this comment.
It's looking great! I am just wondering why not simply HeaderMap? Why an abstraction over it? At the end of theday, there is no hiding from the fact that the executing a query plan needs headers, so why not just HeaderMap?
ardatan
changed the title
ardatan
force-pushed
the
return-response-from-executor
branch
from
79d2a0e to
a87e47c
Compare
3 participants
Extracted from #628
Combining with #651 , the aim is to reduce the number of extra structs.
Today,
PlanExecutionOutputwhich is the return type ofexecute_query_planinexecutor/execution/plan.rs, returnsbody: Vec<u8>andheaders: http::HeaderMap.Then, in the
router/pipeline,ResponseBuilderis called withBytes::fromofPlanExecutionOutput.bodyandPlanExecutionOutput.headersis converted tontex::HeaderMap, thencontent_typeis set here.With this PR, the response aggregator is sent with
PlanExecutionOutput, and during the response build, the response headers aggregator sets the headers intoResponseBuilderdirectly.So now
modify_client_headershelper acceptsResponseBuilder, nothttp:HeaderMap. And since this throwsHeaderPropagationErrorin the pipeline executor, I had to addHeaderPropagationErrortoPipelineError.I also extracted
PipelineErrortoFailedExecutionResultlogic to aFromtrait so that other future implementations that serializes and sets the body in a different way can use it and serializeFailedExecutionResulton their own.Currently it is used here only -> https://github.com/graphql-hive/router/pull/652/changes#diff-f3dc2f2d579374645281d6950414266d8984195dbf45c00f0a754ceca8550aadR95
Update:
After recent changes, I see that
project_by_operationis used fordry_runbut it is actually unnecessary as we just need to serializeextensionswhich has nothing to do with projection since it usessonic_rs::Value.