docs(rfc): disallow inline argument values (ENG-9586)#2998
Conversation
RFC for an optional router policy that detects, and optionally rejects, GraphQL operations carrying hardcoded inline argument values instead of variables (ticket #2498 / ENG-9586). Design highlights: - Detection during Parse() in graphql-go-tools parseArgumentList: no extra walk, sees the un-normalized operation, rejects before any cache write. - Comprehensive flagging: field args, directive args (@skip/@include), introspection args, and args that @skip/@include would normalize out. - Three modes: off (default) / enabled-non-enforcing (log + extensions hint) / enabled-enforcing (configurable status code, error code, message). - Persisted operations exempt by default; opt-in via include_persisted_operations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughA new RFC document is added at ChangesDisallow Inline Arguments RFC
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
| 1. Add an opt-in field to `Parser` (e.g. `reportInlineArguments bool` plus a `[]InlineArgumentInfo` accumulator), | ||
| settable per parse and reset at the start of each parse (alongside the parser's existing per-parse reset). | ||
| When the flag is off, the only cost is one predictable branch per argument — effectively free. | ||
|
|
||
| 2. In `parseArgumentList`, immediately after `value := p.ParseValue()`, | ||
| record the argument when the flag is set and `value.Kind != ast.ValueKindVariable`. | ||
| Because this is the common path for field **and** directive arguments, it captures: | ||
| - field arguments (`userById(userId: "12345")`), | ||
| - directive arguments (`@include(if: true)`, `@skip(if: false)`, any directive), | ||
| - introspection-field arguments (`__type(name: "User")`), | ||
| - arguments inside fields/fragments that `@skip`/`@include` would later remove (nothing is pruned yet). | ||
|
|
||
| It naturally **excludes** variable-definition defaults (`$x: Int = 5`), because those are parsed | ||
| on a different path (variable-definition parsing), not through `parseArgumentList` — so the one | ||
| intended exclusion requires no special-casing. | ||
|
|
||
| ```go | ||
| type InlineArgumentInfo struct { | ||
| ArgumentName string // e.g. "userId" — available as name.Literal at parser.go:430 | ||
| ValueKind ast.ValueKind // e.g. ValueKindString — from value.Kind | ||
| InDirective string // directive name if this is a directive argument, else "" | ||
| Position position.Position | ||
| } |
There was a problem hiding this comment.
I'm not sure if it is a good idea to include this feature in the parser logic. I think we can just include the extra step in one of our validation walkers. Otherwise all parsing will additionally have to check each time if reportInlineArguments is set to true.
I'm personally against the recommended approach here
There was a problem hiding this comment.
I think we can do this much simpler. After parsing, we simply look into the AST and verify if we have any inline arguments that are not variables. If that's the case we flag this as error and stop the rest of the request pipeline. For that we don't need any visitors but just a single lookup in the request pipeline that we otherwise don't do. So the operation by itself is rather cheap and in case of disabled check a no op.
| | # | Approach | Flags directive / introspection / skip-removed args? | Reject before 1st cache write? | Extra walk? | Verdict | | ||
| | --- | --- | --- | --- | --- | --- | | ||
| | A | Detect during parse in `parseArgumentList` (RECOMMENDED) | **Yes** — sees the un-normalized operation | **Yes** — runs during `Parse()`, before `NormalizeOperation` and any cache | **No** — rides the parse already performed | Chosen | | ||
| | B | Dedicated argument visitor over the parsed op, after `Parse()` | **Yes** — also sees the un-normalized operation | Yes | Yes — one extra argument-only walk | Fallback (see § 11) | |
There was a problem hiding this comment.
No sure if we need a dedicated visitor here, I think we already have some validation in place that could potentially be re-used? @devsergiy
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Something seems off here. We seem to have a path which is dot separated and the last element is the actual argument. not the enclosing field. while we have "argument" as well
so basically we have some kind of duplication here.
"path": "field.field.argument"
"argument": "argument"
If we use the proposed data structure as a reference to create errors we won't have the information about either path or enclosing field
type InlineArgumentInfo struct {
ArgumentName string // e.g. "userId" — available as name.Literal at parser.go:430
ValueKind ast.ValueKind // e.g. ValueKindString — from value.Kind
InDirective string // directive name if this is a directive argument, else ""
Position position.Position
}
| ArgumentName string // e.g. "userId" — available as name.Literal at parser.go:430 | ||
| ValueKind ast.ValueKind // e.g. ValueKindString — from value.Kind | ||
| InDirective string // directive name if this is a directive argument, else "" | ||
| Position position.Position |
There was a problem hiding this comment.
Positions might not be always correct, right? We sometimes have multiple stages of parsing where we also append to operation documents or rewrite certain things (e.g. astnormalization).
We might end up reparsing again for different visitors so the position might be different to the position in the original operation that was sent to the router.
| userById(userId: $userId) { | ||
| loginName | ||
| } | ||
| } |
There was a problem hiding this comment.
The idea is to not have too many distinct operations, but technically we could also have different variable names. I suppose this would be up to the user to use the feature properly? Or do we also want to flag use of different variable names for the same operations?
query GetUserById($userId1: ID!) {
userById(userId: $userId2) {
loginName
}
}
query GetUserById($userId2: ID!) {
userById(userId: $userId2) {
loginName
}
}There was a problem hiding this comment.
we cannot prevent this
Adopt the simplest detection approach per reviewer feedback: after Parse(), scan ast.Document.Arguments once in the router pipeline. No parser change, no visitor, no graphql-go-tools change at all. - jensneuse: single AST lookup, no visitors, no-op when disabled. - Noroth: drop parser instrumentation (taxes every parse); remove path/argument duplication in the report struct; clarify positions are accurate (scan runs pre-normalization). - Note variable-name variance cannot be prevented (out of scope). - Demote parser instrumentation and validation-walker reuse to rejected alternatives; keep the dedicated visitor as the richer-info fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rfc/disallow-inline-args/RFC.md (1)
254-259: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve enforce-mode reporting contradiction (first-hit vs full list).
Line 254 and Line 288 define enforce mode as stopping on the first inline argument, but Line 629 expects enforce responses to report both
firstandifin one request. Please pick one contract and align §§4, 6, and 10 (plus examples) to avoid implementation/test drift.Also applies to: 288-288, 629-630
🤖 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 `@rfc/disallow-inline-args/RFC.md` around lines 254 - 259, The enforce-mode contract is inconsistent across the RFC: some sections say scanning stops at the first inline argument, while another example expects multiple violations like `first` and `if` to be reported together. Pick one behavior for `inlineArgumentsError`/`reportError` and update the related text in §§4, 6, and 10 plus the examples so `NormalizeOperation()` and the prehandler flow describe the same single contract everywhere.
🤖 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 `@rfc/disallow-inline-args/RFC.md`:
- Around line 232-239: Add a language tag to the unlabeled fenced pipeline block
in the RFC so it satisfies markdown lint; update the fence around the
Parse/NormalizeOperation pipeline snippet to use a text-style language label,
keeping the content unchanged while making the docs tooling happy.
---
Outside diff comments:
In `@rfc/disallow-inline-args/RFC.md`:
- Around line 254-259: The enforce-mode contract is inconsistent across the RFC:
some sections say scanning stops at the first inline argument, while another
example expects multiple violations like `first` and `if` to be reported
together. Pick one behavior for `inlineArgumentsError`/`reportError` and update
the related text in §§4, 6, and 10 plus the examples so `NormalizeOperation()`
and the prehandler flow describe the same single contract everywhere.
🪄 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: ebb8e64b-8eb8-491a-b669-8b6a570360e1
📒 Files selected for processing (1)
rfc/disallow-inline-args/RFC.md
| ``` | ||
| Parse (687) | ||
| === scan doc.Arguments here (only when the feature is enabled) === | ||
| === enforce mode: stop the pipeline here, before any cache is consulted or written === | ||
| NormalizeOperation (830) -> first cache write at :885 | ||
| NormalizeVariables (855) -> variables cache | ||
| ... RemapVariables / Validate / Plan, each with its own cache | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced pipeline block.
The code fence starting at Line 232 is unlabeled (```). Add a language (for example text) to satisfy markdown lint and keep docs tooling clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@rfc/disallow-inline-args/RFC.md` around lines 232 - 239, Add a language tag
to the unlabeled fenced pipeline block in the RFC so it satisfies markdown lint;
update the fence around the Parse/NormalizeOperation pipeline snippet to use a
text-style language label, keeping the content unchanged while making the docs
tooling happy.
Source: Linters/SAST tools
| for ref := range o.kit.doc.Arguments { | ||
| if o.kit.doc.Arguments[ref].Value.Kind != ast.ValueKindVariable { | ||
| // inline argument found | ||
| // enforce mode: record it and stop the pipeline here | ||
| // log mode: collect and continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Imagine a more complex operation. Can we still provide good UX/DX here without knowing where the argument is coming from?
query GetDashboard(
$tenantId: ID!
$userId: ID!
$limit: Int!
$includeArchived: Boolean!
) @cacheControl(maxAge: 60, scope: "PRIVATE")
@trace(enabled: true, sampleRate: 0.25)
{
tenant(id: $tenantId)
@auth(role: "admin")
@cacheControl(maxAge: 30, scope: "PUBLIC")
{
id
users(
filter: { status: "ACTIVE", role: "admin" }
limit: $limit
orderBy: { field: "createdAt", direction: "DESC" }
) @include(if: true)
@rateLimit(limit: 100, window: "1m")
{
id
name
posts(
filter: { published: true, tag: "graphql" }
limit: 10
) @skip(if: false)
{
id
title
comments(
filter: { flagged: false }
limit: 5
) @include(if: $includeArchived)
{
id
text
}
}
}
auditLogs(
filter: { actorId: $userId, action: "LOGIN" }
limit: 50
) @cacheControl(maxAge: 5, scope: "PRIVATE")
{
id
action
createdAt
}
}
}We would get something like:
{
"extensions": {
"inlineArguments": {
"code": "INLINE_ARGUMENT_VALUES_NOT_ALLOWED",
"message": "Inline argument values are not allowed. Use variables instead.",
"arguments": [
{
"argument": "maxAge",
"valueKind": "Int",
"line": 5,
"column": 30
},
{
"argument": "scope",
"valueKind": "String",
"line": 5,
"column": 43
},
{
"argument": "role",
"valueKind": "String",
"line": 9,
"column": 17
},
{
"argument": "filter",
"valueKind": "Object",
"line": 14,
"column": 7
},
{
"argument": "limit",
"valueKind": "Int",
"line": 15,
"column": 7
},
{
"argument": "filter",
"valueKind": "Object",
"line": 24,
"column": 9
},
{
"argument": "limit",
"valueKind": "Int",
"line": 25,
"column": 9
},
{
"argument": "filter",
"valueKind": "Object",
"line": 34,
"column": 11
},
{
"argument": "limit",
"valueKind": "Int",
"line": 35,
"column": 11
}
]
}
}
}I think something beter could be:
Field:
{
"path": "tenant.users.posts",
"kind": "Field",
"name": "posts",
"argument": "filter",
"valueKind": "Object",
"line": 24,
"column": 9
}Directive
{
"path": "tenant.users.posts",
"kind": "Directive",
"name": "skip",
"argument": "if",
"valueKind": "Boolean",
"line": 27,
"column": 15
}
|
The RFC specifies different shapes for the two modes, is that intentional? In enforce mode, Clients reading the argument list need to branch on mode: |
3 participants
Summary
RFC for an optional Cosmo Router policy that detects — and optionally rejects — GraphQL operations that carry hardcoded inline argument values instead of variables.
This PR adds the design document only (
rfc/disallow-inline-args/RFC.md); no router code yet.Design highlights
Parse()in graphql-go-toolsparseArgumentList— no extra AST walk, sees the operation before any normalization, and rejects before any internal cache is written.@skip/@includeand any directive), introspection-field arguments, and arguments inside selections that@skip/@includewould normalize out.off(default) /enabled-non-enforcing(structured log +extensionsmigration hint on a successful response) /enabled-enforcing(reject with configurable HTTP status, error code, and message).include_persisted_operations.off → log-only → enforce, with structured logs and an OTEL per-client attribute to find affected clients before enforcing.Status
Draft — design review only. Open questions are tracked in §9 (subscriptions exemption, nested-path granularity, per-argument allowlist).
🤖 Generated with Claude Code
Summary by CodeRabbit