[code-scanning-fix] Fix go/bad-redirect-check: incomplete leading-slash validation in manifest path check

[github-actions]

test

Generated by PR Description Updater for #56219 · copilot · auto · 50.4 AIC · ⌖ 4.17 AIC · ⊞ 7.7K · ◷

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped. PR changes only production code (pkg/cli/add_package_manifest_includes.go) to fix CWE-601 security validation.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ 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 does not have the implementation label and has only 6 new lines of code in business logic directories (threshold: 100).

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ Ponytail Reviewer completed successfully!

Lean already. Ship.

Generated by Ponytail Reviewer for #56219

[github-actions]

Comment Memory

reviewed_at: 2026-08-27T04:23:30Z
review_event: COMMENT
top_themes:
  - no actionable changed-line issues found
  - missing unit tests noted but not attributable to changed lines enough to block
files_reviewed:
  - pkg/cli/add_package_manifest_includes.go
comment_count: 0

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.04 AIC · ⌖ 6.86 AIC · ⊞ 7K · ◷
_{Comment /review to run again}

[github-actions]

Verdict

Non-blocking: this patch is narrowly correct, and I did not find a changed-line bug worth holding the PR for.

What I checked

• The new guard rejects all double slash/backslash prefixes (//, /\\, \\/, \\\\) before path.Clean can normalize them into something misleading.
• The existing single-leading-slash and Windows drive checks still cover the simpler absolute-path cases.
• I do not love shipping a security fix with zero targeted tests, but that gap pre-existed this PR and the changed lines themselves are not obviously broken.

🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.04 AIC · ⌖ 6.86 AIC · ⊞ 7K
_{Comment /review to run again}

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs — requesting changes on a critical correctness issue.

📋 Key Finding

Critical Issue

• Dead code / ineffective fix: The new guard at line 152 (slashed[0] == '/' || slashed[0] == '\\') is unreachable. The existing check on line 146 uses strings.HasPrefix(slashed, "/") and strings.HasPrefix(slashed, "\\"), which already reject all paths whose first character is / or \. No path that starts with // or /\ can ever reach line 152.

This means the CodeQL alert (go/bad-redirect-check) is likely not actually fixed by this PR. Before merging, the double-slash cases should be tested against the original code to confirm whether they were already rejected.

Suggested action

1. Add the regression test described in the inline comment to verify the real behavior.
2. If the original code already rejected double-slash paths → the CodeQL alert is a false positive; the PR should document this and the dead code should be removed.
3. If any double-slash path slipped through → restructure the check so the new guard is actually reached (e.g. check p before filepath.ToSlash, or combine into one condition).

Positive Highlights

• ✅ Minimal, surgical change scoped to one function.
• ✅ Existing error message reused consistently.
• ✅ Good comment explaining the intent.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 21.6 AIC · ⌖ 9.99 AIC · ⊞ 7.6K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] This new guard is unreachable dead code — the existing check on line 146 already rejects any path whose first character is / or \, so slashed[0] == '/' || slashed[0] == '\\' can never be true here. The CodeQL alert is not actually fixed by this change; it is purely redundant.

💡 Root cause analysis & suggested test

strings.HasPrefix(slashed, "/") on line 146 catches all paths starting with /, which is a strict superset of slashed[0] == '/'. Execution never reaches line 152.

To confirm this before merging, add a regression test that runs against the original code:

func TestCleanManifestRelativePathDoubleSlash(t *testing.T) {
    cases := []string{"(etc/redacted), "/\\evil", "\\/evil", "\\\\server\\share"}
    for _, tc := range cases {
        _, err := cleanManifestRelativePath(tc)
        if err == nil {
            t.Errorf("expected error for %q, got nil", tc)
        }
    }
}

If these cases already return an error with the original code, the double-slash concern was never a real vulnerability and this PR should be reverted. If any case returns nil, the fix needs to be placed where it is actually reachable (e.g. applied to p before filepath.ToSlash, or the logic restructured so the double-slash condition is checked when slashed[0] is not already caught).

@copilot please address this.

[Copilot]

Pull request overview

Attempts to harden manifest path validation against protocol-relative and UNC-style prefixes.

Changes:

• Adds a two-character slash/backslash prefix guard.
• Documents the intended security protection.

Show a summary per file

File	Description
pkg/cli/add_package_manifest_includes.go	Adds manifest path-prefix validation.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

[github-actions]

The fix addresses a real CodeQL go/bad-redirect-check concern, but the new double-slash guard (lines 149–154) is dead code.

filepath.ToSlash converts every \ → / before any check runs, so (foo/redacted) becomes (foo/redacted). The existing strings.HasPrefix(slashed, "/") check on line 146 already matches any string that starts with /, including //. The new block is therefore unreachable and should be removed.

See the inline comment for details.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 22.9 AIC · ⌖ 9.15 AIC · ⊞ 6.2K

[github-actions]

Dead code — the new check is unreachable.

The guard added here (lines 152–154) can never execute. Line 146 already calls strings.HasPrefix(slashed, "/"), which returns true for any string whose first character is / — including // and /\. Control never reaches line 152 when slashed starts with /.

The double-slash UNC / protocol-relative concern is valid, but filepath.ToSlash already converts every \ to /, so (foo/redacted) → (foo/redacted), which is then caught by the existing leading-slash check on line 146. The new block adds no real coverage.

Suggested fix: remove lines 149–154 entirely. If you want to make the intent explicit, add a comment to line 146 noting that HasPrefix(slashed, "/") also covers //-prefixed paths.

@copilot please address this.

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Completed a pr-finisher pass and pushed 6de0199.

• Removed the unreachable double-slash guard in cleanManifestRelativePath
• Added regression coverage for //, /\, and double-leading-backslash include sources
• Ran local validation: make fmt, make lint, go test ./pkg/cli -run TestResolveRepositoryPackage_IncludeMappings, make test-unit, make test, make recompile, and make agent-report-progress
• Ran code review + CodeQL check (CodeQL reported DB-size skip, no alerts)

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.87.8