Skip to content

[code-scanning-fix] Fix workflow-graphql-id-unescaped: escape node IDs in GraphQL mutations#40757

Open
github-actions[bot] wants to merge 1 commit into
mainfrom
fix/graphql-id-unescaped-627-628-d08e45c37cbf4f35
Open

[code-scanning-fix] Fix workflow-graphql-id-unescaped: escape node IDs in GraphQL mutations#40757
github-actions[bot] wants to merge 1 commit into
mainfrom
fix/graphql-id-unescaped-627-628-d08e45c37cbf4f35

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Alert Numbers: #627, #628
Severity: Warning
Rule: workflow-graphql-id-unescaped
CWE: CWE-89

Vulnerability Description

In pkg/cli/project_command.go, two GraphQL mutations were constructing query strings using fmt.Sprintf with unescaped node ID values:

  1. createProjectV2 mutation (alert Add test agentic workflow for GitHub remote MCP #627, line 295): ownerId was interpolated directly, while the sibling title argument was correctly wrapped with escapeGraphQLString().
  2. linkProjectV2ToRepository mutation (alert Add execution duration tracking to tool usage reporting for both Codex and Claude engines #628, line 368): Both projectId and repositoryId were interpolated without escaping.

This inconsistency creates a defense-in-depth gap. While GitHub API node IDs are opaque and currently safe, failing to escape them means that if the API ever returns a node ID containing special characters (e.g., ", \), it could alter the structure of the GraphQL query (GraphQL injection, CWE-89).

Location

Fix Applied

Wrapped the unescaped node ID arguments with the already-available escapeGraphQLString() helper, making escaping consistent with the title argument and with the query construction on line 355 (which already used escapeGraphQLString for repoOwner and repoName).

Changes Made

  • Wrapped ownerId with escapeGraphQLString() in createProjectV2 mutation
  • Wrapped projectId with escapeGraphQLString() in linkProjectV2ToRepository mutation
  • Wrapped repoId with escapeGraphQLString() in linkProjectV2ToRepository mutation

Security Best Practices Applied

  • Consistent escaping: All string values interpolated into GraphQL queries now go through escapeGraphQLString()
  • Defense in depth: Values from trusted sources are still escaped, removing source-trustworthiness as a dependency
  • Minimal change: The existing escapeGraphQLString() helper is reused; no new dependencies introduced

Testing Considerations

  • Existing project creation and project-to-repo linking flows should be tested end-to-end
  • All existing tests for createProject and linkProjectToRepo should continue to pass

Automated by: Code Scanning Fixer Workflow
Run ID: §27940907624

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Generated by 🔒 Code Scanning Fixer · 49.1 AIC · ⌖ 17.3 AIC · ⊞ 5.8K ·

  • expires on Jun 24, 2026, 1:00 AM UTC-08:00

Wrap ownerId, projectId, and repositoryId with escapeGraphQLString() in
the createProjectV2 and linkProjectV2ToRepository GraphQL mutations.

Previously, title was consistently escaped but the node ID fields were
not, creating a defense-in-depth gap. While GitHub API node IDs are
opaque and currently safe, consistently applying escapeGraphQLString()
ensures that if the values ever contain special characters the queries
cannot be altered.

Fixes code scanning alerts #627 and #628.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor Author

Great work from the Code Scanning Fixer workflow catching and addressing alerts #627 and #628 — wrapping ownerId, projectId, and repoId with escapeGraphQLString() is exactly the right defense-in-depth fix for the GraphQL injection risk in pkg/cli/project_command.go.

One thing that would strengthen this before merge:

  • Add tests for the escaping behaviour — the PR itself notes that createProject and linkProjectToRepo flows should be tested end-to-end, but no test changes are included in the diff. A unit test (or table-driven test in project_command_test.go) that passes a node ID containing " or \ and asserts the mutation string is correctly escaped would lock in this fix and guard against regression.

If you would like a hand, you can assign this prompt to your coding agent:

In pkg/cli/project_command.go, the functions createProject and linkProjectToRepo now pass ownerId, projectId, and repoId through escapeGraphQLString() before interpolating them into GraphQL mutation strings.

Add unit tests in the corresponding test file (project_command_test.go or similar) to cover:
1. createProject: verify that an ownerId containing a double-quote character (e.g. abc"def) is correctly escaped in the generated mutation string.
2. linkProjectToRepo: verify that projectId and repoId containing backslash or double-quote characters are correctly escaped in the generated mutation string.
3. Verify that well-formed opaque node IDs (base64-like, no special chars) pass through unchanged.

Use table-driven tests following the existing test patterns in the file.

Generated by ✅ Contribution Check · 153 AIC · ⌖ 15.6 AIC · ⊞ 5.9K ·

@pelikhan pelikhan marked this pull request as ready for review June 22, 2026 14:14
Copilot AI review requested due to automatic review settings June 22, 2026 14:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR addresses code-scanning warnings for potential GraphQL injection by ensuring GitHub node IDs interpolated into GraphQL mutations are escaped consistently.

Changes:

  • Escape ownerId when building the createProjectV2 GraphQL mutation.
  • Escape projectId and repoId when building the linkProjectV2ToRepository GraphQL mutation.
Show a summary per file
File Description
pkg/cli/project_command.go Escapes node ID inputs embedded into GraphQL mutation strings to close a defense-in-depth injection gap.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

}
}
}`, ownerId, escapeGraphQLString(title))
}`, escapeGraphQLString(ownerId), escapeGraphQLString(title))
}
}
}`, projectId, repoId)
}`, escapeGraphQLString(projectId), escapeGraphQLString(repoId))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant