Use parameterized GraphQL variables instead of Sprintf interpolation - #51827
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Replaces interpolated GraphQL queries with parameterized variables to address CodeQL injection findings.
Changes:
- Adds deterministic, typed GraphQL variable forwarding.
- Converts pull-request intent lookup to a static query.
- Removes obsolete escaping code and updates tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/project_command.go |
Removes manual GraphQL escaping. |
pkg/cli/project_command_test.go |
Removes obsolete escaping tests. |
pkg/cli/outcome_eval.go |
Adds variable-based GraphQL execution. |
pkg/cli/outcome_eval_test.go |
Updates stubs and parameterization assertions. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| for _, name := range slices.Sorted(maps.Keys(variables)) { | ||
| switch value := variables[name].(type) { | ||
| case string: | ||
| // -f sends the value literally, avoiding gh's @file / {placeholder} expansion. | ||
| args = append(args, "-f", name+"="+value) |
| assert.Equal(t, "mapped", report.AttributionStatus) | ||
| assert.Equal(t, "closing_issue", report.AttributionSource) | ||
|
|
||
| assert.NotContains(t, capturedQuery, "owner/repo", "query should not interpolate values into the GraphQL document") |
There was a problem hiding this comment.
Updated the assertions to check each quoted literal individually ("owner", "repo", and the raw 77) rather than just the concatenated owner/repo string.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51827 does not have the 'implementation' label and has only 33 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Good security improvement. Replacing fmt.Sprintf interpolation with parameterized GraphQL variables eliminates injection risk and the escapeGraphQLString workaround. The two existing Copilot inline comments (#3751352069, #3751352151) already capture the actionable gaps (CLI encoding not tested by unit tests; weak NotContains assertion). No additional blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.9 AIC · ⌖ 7.06 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #51827 · auto · 23.4 AIC · ⌖ 4.27 AIC · ⊞ 6.8K
Comment /ponytail to run again
| func ghAPIGraphQL(ctx context.Context, query string, variables map[string]any, repo string) (map[string]any, error) { | ||
| ownerRepo, host := repoutil.NormalizeRepoForAPI(repo) | ||
| args := []string{"api", "graphql", "-f", "query=" + query} | ||
| for _, name := range slices.Sorted(maps.Keys(variables)) { |
There was a problem hiding this comment.
L317: yagni: sorts variable map keys for deterministic arg order, but the only caller passes 3 fixed keys and no test checks arg order. Iterate variables directly, drop the maps/slices imports.
There was a problem hiding this comment.
Kept the sorted iteration — it's now exercised directly by TestBuildGraphQLArgs's "deterministic key order" subtest, so it's no longer untested dead weight.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the security fix is correct and well-scoped, but two pre-existing review comments and one new finding flag test coverage gaps worth addressing.
📋 Key Themes & Highlights
Key Themes
- Untested encoding path: the
-f/-Farg-building loop inghAPIGraphQL(line 321) is bypassed by the test stub, so the security-critical encoding is never actually exercised - Weak injection assertion:
NotContains(capturedQuery, "owner/repo")(line 310) would also pass on the old interpolated query, so it doesn’t prove injection safety - Unguarded
%vformatting: non-string variable types usefmt.Sprintf("%v", value)without documenting or enforcing supported types (line 328)
Positive Highlights
- ✅ Clean removal of hand-rolled
escapeGraphQLString— eliminating the unsafe helper is exactly right - ✅ Sorted key order for deterministic CLI arg emission
- ✅ Correct
-f(raw) /-F(typed) split forgh api graphql - ✅
assert.EqualoncapturedVariablesgives a strong check that values arrive correctly at the call site
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.8 AIC · ⌖ 7.53 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| var output []byte | ||
| var err error | ||
| if host != "" { |
There was a problem hiding this comment.
[/diagnosing-bugs] fmt.Sprintf("%v", value) for non-string variables relies on Go's default formatting, but gh api graphql -F expects specific value forms. For current int usage this is fine, but future callers passing float64, bool, or custom types could silently emit wrong values.
💡 Suggestion
Document supported types explicitly, or guard against unexpected ones:
default:
return nil, fmt.Errorf("ghAPIGraphQL: unsupported variable type %T for key %q", variables[name], name)This makes the contract explicit and surfaces bugs early.
@copilot please address this.
There was a problem hiding this comment.
Added the explicit type guard as suggested — unsupported variable types now return an error naming the key and %T instead of silently formatting them. Covered by a new test case in TestBuildGraphQLArgs.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 modified test)
Verdict
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
CodeQL (
workflow-go-graphql-injection-sprintf, alerts #651/#652) flagged GraphQL queries built by string interpolation. The last such call site was the closing-issues query inpkg/cli/outcome_eval.go, which relied on a hand-rolledescapeGraphQLStringhelper defined inpkg/cli/project_command.go.Changes
ghAPIGraphQLaccepts variables — newvariables map[string]anyparameter, forwarded togh api graphql. Strings use-f(raw field) so gh's@file/{placeholder}expansion can't be triggered by interpolated values; other types use-Ffor correct GraphQL typing. Args are emitted in sorted key order for determinism.loadPullRequestIntentDataquery is now static, declaring$owner,$name,$number.escapeGraphQLStringand its test — manual escaping is no longer needed anywhere. All other GraphQL calls inproject_command.gowere already variable-based.Before:
After: