[code-scanning-fix] Fix go/unsafe-quoting: unsafe quoting in import cycle error message - #58189
Conversation
…le error message Fixes go/unsafe-quoting alert where circular import chain names were manually wrapped in single quotes via fmt.Sprintf, allowing a single quote in an import path to break out of the enclosing quotes. Replaced with Go's %q verb which safely escapes special characters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #58189 does not have the implementation label and has only 4 added lines in default business logic directories (<=100 threshold).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
Lean already. Ship. 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
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
🟡 Changes recommended
The security fix lacks regression coverage for quote-bearing import paths.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens import-cycle errors by safely quoting untrusted import paths.
Changes:
- Replaces manual quoting with Go’s
%qformatting. - Updates existing error-message expectations.
File summaries
| File | Review |
|---|---|
pkg/parser/import_error.go |
Safely quotes paths in import-cycle errors. |
pkg/parser/import_cycle_test.go |
Updates expected formatting, but needs coverage for embedded single and double quotes with exact escaping assertions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| "circular import detected: b.md → c.md → d.md → b.md", | ||
| "Imports must form a directed acyclic graph", | ||
| "remove the import of 'b.md' from 'd.md'", | ||
| "remove the import of \"b.md\" from \"d.md\"", |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Reviewed this 4-line security fix using /diagnosing-bugs and /codebase-design lenses — no actionable issues found.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: replaced manual
'%s'quoting with Go's%qverb, which safely escapes embedded quotes/control characters — the correct fix for the CWE-78/89/94 class of issue described. - Regression coverage present: existing tests (
TestImportCycleDetection_SelfImport,TestImportCycleError_FormattedOutput) were updated to assert on the new%q-quoted output, so the format change is pinned and any future regression to unsafe quoting would be caught. - Change is scoped and consistent: only the one
Sprintfcall and its two dependent test assertions were touched; no unrelated code was modified.
Positive Highlights
- ✅ Uses the language-provided safe formatting primitive (
%q) instead of ad-hoc escaping logic. - ✅ Tests updated in the same PR rather than left to bit-rot.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 16.8 AIC · ⌖ 14.5 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with the Impeccable skills lens (change classified as mixed_unclear -> critique/audit); this PR has no UI surface, so those Impeccable modes don't meaningfully apply here.
The change itself is a correct, minimal security fix: replaces manual '%s' quoting with Go's %q verb in ImportCycleError.Error(), safely escaping untrusted import-path strings and eliminating the quote-breakout risk (CWE-78/89/94). Tests were updated consistently to match the new double-quoted, escaped output format.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 16.1 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
I don't see a changed-line defect here that warrants blocking this PR. The %q switch is the correct hardening change, and the test updates match the new escaping behavior.
Highlights
The patch is narrowly scoped to the vulnerable formatting path, and the updated assertions cover the externally visible message change without widening behavior elsewhere.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 26.9 AIC · ⌖ 7.42 AIC · ⊞ 21.8K
Comment /review to run again
|
@copilot Please take the next forward-progress pass on this PR.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added regression coverage in f18eecc. The test uses import paths containing both embedded single quotes ( |
|
@copilot Please take the next forward-progress pass on this PR.
|
HEAD is f18eecc and contains |
|
@copilot Please take the next forward-progress pass on this PR.
|
No code work remains. HEAD |
|
@copilot Please take the next forward-progress pass on this PR.
|
No code work remains. Reconciled refreshed PR state: HEAD is still |
|
@copilot Please take another pass on this PR. Please review the current status, refresh the branch if needed, address any outstanding review feedback or failing checks, and then use the
|
Reconciled the current PR state. No code work remains: HEAD is unchanged at |
|
🎉 This pull request is included in a new release. Release: |
Security Fix: Unsafe Quoting in Import Cycle Error Message
Alert Numbers: #671, #670
Severity: Critical
Rule: go/unsafe-quoting
CWE: CWE-78, CWE-89, CWE-94
Vulnerability Description
ImportCycleError.Error()inpkg/parser/import_error.gomanually wrappeduntrusted import-path strings (
importer,imported) in single quotes usingfmt.Sprintf("... '%s' from '%s' ...", ...). If an import path contained asingle quote character, it could break out of the enclosing quotes and alter
the structure of the resulting message, which is later displayed to users
and could be embedded in other formatted/templated output.
Location
pkg/parser/import_error.goFix Applied
Replaced the manual
'%s'quoting with Go's built-in%qformat verb, whichsafely quotes and escapes special characters (including embedded quotes)
using standard Go string-escaping rules, eliminating the injection risk.
Changes Made:
'%s' from '%s'to%q from %qin theError()method ofImportCycleError.pkg/parser/import_cycle_test.go) to expectthe new double-quoted, safely-escaped output format.
Security Best Practices
%q) instead ofmanual string concatenation/quoting for untrusted data embedded in messages.
Testing Considerations
go test ./pkg/parser/...— all tests pass, including the updatedTestImportCycleDetection_SelfImportandTestImportCycleError_FormattedOutputcases that assert on the exact error message format.
Automated by: Code Scanning Fixer Workflow
Run ID: 33725870616
Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/33740403993