feat(router): disallow inline argument values (ENG-9586)#3044
Draft
gausie wants to merge 21 commits into
Draft
Conversation
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a configurable ChangesDisallow Inline Arguments Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PreHandler
participant InlineArgumentsChecker
participant GraphQLHandler
participant WebSocketConnectionHandler
Client->>PreHandler: HTTP GraphQL request
PreHandler->>InlineArgumentsChecker: Check(operation, doc, clientInfo)
InlineArgumentsChecker-->>PreHandler: result, annotation, or error
alt enforce mode violation
PreHandler-->>Client: error response
else warn mode
PreHandler->>GraphQLHandler: operation with inlineArgumentsAnnotation
GraphQLHandler->>GraphQLHandler: inject extensions.inlineArguments
GraphQLHandler-->>Client: annotated response
end
Client->>WebSocketConnectionHandler: subscription message
WebSocketConnectionHandler->>InlineArgumentsChecker: Check(operation, doc)
InlineArgumentsChecker-->>WebSocketConnectionHandler: result or error
alt violation
WebSocketConnectionHandler-->>Client: GraphQL error payload
else warn/off
WebSocketConnectionHandler-->>Client: next message
end
Estimated code review effort: 4 (Complex) | ~60 minutes Compact metadata:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3044 +/- ##
==========================================
+ Coverage 61.46% 61.60% +0.14%
==========================================
Files 261 262 +1
Lines 30596 30792 +196
==========================================
+ Hits 18805 18970 +165
- Misses 10277 10301 +24
- Partials 1514 1521 +7
🚀 New features to boost your workflow:
|
gausie
commented
Jul 2, 2026
| ForwardInitialPayload: h.config.ForwardInitialPayload, | ||
| OperationProcessor: h.operationProcessor, | ||
| OperationBlocker: h.operationBlocker, | ||
| InlineArgumentsChecker: h.inlineArgumentsChecker, |
Contributor
Author
There was a problem hiding this comment.
I added this because it feels technically correct but... do we actually want to do this check for subscriptions?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
router/core/inline_arguments.go (1)
182-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPossible duplication between
extensionJSONandwriteInlineArgumentsError.Both build an
inlineArgumentsExtensionfrom the sameinlineArgumentsErrorfields. IfextensionJSONisn't consumed elsewhere (e.g.,errors.goin the next cohort), consider havingwriteInlineArgumentsErrorreuse it to avoid maintaining two marshal paths.♻️ Possible consolidation
func writeInlineArgumentsError(r *http.Request, w http.ResponseWriter, e *inlineArgumentsError, logger *zap.Logger, headerPropagation *HeaderPropagation) { + extJSON := e.extensionJSON(logger) body, err := json.Marshal(inlineArgumentsErrorResponse{ Errors: []inlineArgumentsErrorEntry{{ Message: e.message, Extensions: inlineArgumentsErrorExtensions{ Code: e.code, - InlineArguments: inlineArgumentsExtension{ - Code: e.code, - Message: e.message, - Arguments: e.arguments, - }, + InlineArguments: extJSON, }, }}, })Note: this requires changing
inlineArgumentsErrorExtensions.InlineArgumentstojson.RawMessage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/inline_arguments.go` around lines 182 - 235, The inline arguments error response is assembling the same inlineArgumentsExtension data twice, once in inlineArgumentsError.extensionJSON and again in writeInlineArgumentsError. If extensionJSON is the canonical builder, update writeInlineArgumentsError to reuse it and change inlineArgumentsErrorExtensions.InlineArguments to json.RawMessage so both paths share one marshal source and stay consistent; keep the existing symbols inlineArgumentsError, extensionJSON, and writeInlineArgumentsError as the main touchpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/graphql_handler.go`:
- Line 286: The final client write in the GraphQL handler is discarding errors,
so failures after annotation injection are lost when captureBuf is enabled.
Update the write path in the handler that uses hpw.Write to check and
propagate/log the returned error instead of ignoring both return values. Make
sure the fix preserves the existing captureBuf flow and only changes the final
response write handling in the GraphQL response pipeline.
In `@router/pkg/config/config.go`:
- Line 508: The DisallowInlineArguments security block in SecurityConfiguration
is currently only wired for YAML, so environment overrides won’t work even
though envDefault is present. Update the DisallowInlineArguments field and its
child config type to include the appropriate env tags, or add an envPrefix on
the SecurityConfiguration.DisallowInlineArguments block so it behaves like the
other security sections and can be overridden from the environment.
---
Nitpick comments:
In `@router/core/inline_arguments.go`:
- Around line 182-235: The inline arguments error response is assembling the
same inlineArgumentsExtension data twice, once in
inlineArgumentsError.extensionJSON and again in writeInlineArgumentsError. If
extensionJSON is the canonical builder, update writeInlineArgumentsError to
reuse it and change inlineArgumentsErrorExtensions.InlineArguments to
json.RawMessage so both paths share one marshal source and stay consistent; keep
the existing symbols inlineArgumentsError, extensionJSON, and
writeInlineArgumentsError as the main touchpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b5bdba0-4a6c-4b38-9d3d-504c45f31afd
📒 Files selected for processing (15)
router-tests/security/disallow_inline_arguments_test.gorouter/core/context.gorouter/core/errors.gorouter/core/graph_server.gorouter/core/graphql_handler.gorouter/core/graphql_prehandler.gorouter/core/inline_arguments.gorouter/core/inline_arguments_test.gorouter/core/websocket.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/otel/attributes.go
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…llow inline arguments
gausie
changed the title
…ing-lecun # Conflicts: # router/core/errors.go
… document Move detection from post-parse to after NormalizeOperation/ValidateOperation so only the executed operation is checked: non-executed sibling operations, unused fragments and statically resolved @skip/@include are no longer flagged. Detection walks the document via a pooled astvisitor walker (the flat doc.Arguments slice keeps orphaned nodes on a normalization cache miss), and line/column are dropped from the annotation payload since positions on the normalized document do not reliably map to the submitted text. Enforce errors are held and surfaced during validation, after the schema-validation error.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if logger := o.operationProcessor.logger; logger != nil { | ||
| logger.Warn("failed to look up persisted operation for a request carrying its own query body, classifying as APQ", | ||
| zap.String("sha256_hash", o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash), | ||
| zap.Error(err)) |
Fold inlineArgumentsError into the shared HttpError mechanism with a single graphqlError builder for the HTTP and WebSocket transports, restore the body+hash APQ short-circuit when nothing consumes the registered/APQ classification, drop redundant initial-part state from the defer writer, and dedupe the security test config boilerplate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the
security.disallow_inline_argumentsrouter policy:warnmode logs and annotates responses withextensions.inlineArguments,enforcemode rejects operations using inline argument values. Applies to both HTTP and WebSocket transports; persisted operations are exempt by default.RFC: #2998
Summary by CodeRabbit
security.disallow_inline_argumentsto detect hardcoded inline GraphQL argument values on both HTTP and WebSocket.off,warn, andenforcemodes, including configurable enforcement HTTP status and GraphQL errorcode/message.extensions.inlineArguments, telemetry (wg.operation.inline_arguments.count), and WebSocket response/error behavior.