Skip to content

fix(tool): normalize Windows paths in search and comments - #1255

Closed
zzz-yu wants to merge 1 commit into
alibaba:mainfrom
zzz-yu:fix/tool-path-normalization
Closed

zzz-yu wants to merge 1 commit into
alibaba:mainfrom
zzz-yu:fix/tool-path-normalization

Conversation

@zzz-yu

@zzz-yu zzz-yu commented Sep 14, 2026

Copy link
Copy Markdown

Description\n\nFixes #1088.\n\nGit pathspecs and diff paths are repository-relative and use / regardless of the host OS. On Windows, the code_search tool previously validated only / separators, so patterns such as ..\pkg or pkg\..\internal bypassed the traversal check and were passed to Git unchanged. Windows-style comment paths (for example pkg\util.go) also failed strict path matching and were silently dropped from review results.\n\nThis change:\n- normalizes backslashes in file_patterns before validation and Git invocation;\n- rejects traversal components after normalization;\n- normalizes comment paths with forward slashes and cleans ./ prefixes and duplicate separators;\n- adds regression coverage for Windows traversal attempts and path matching.\n\n## How Has This Been Tested?\n\n- go test ./... (all packages pass on Windows with Go 1.26.5)\n- gofmt and git diff --check\n\n## Checklist\n\n- [x] My code follows the project coding style (go fmt, go vet)\n- [x] I have performed a self-review of my code\n- [x] I have added tests that prove my fix is effective or my feature works\n- [x] New and existing unit tests pass locally with my changes\n- [ ] I have updated the documentation accordingly (not applicable)\n- [ ] I have signed the CLA\n- [x] I used AI/LLM assistance and disclosed it below; I reviewed every line and did not attribute commits to AI.\n\n### AI/LLM disclosure\n\nOpenAI Codex (GPT-5) assisted with issue investigation, patch drafting, and test design. Prompt used: “Identify a reproducible path-handling defect in this Go/Agent tool, implement a minimal fix with regression tests, and run the repository tests.” I reviewed the complete diff and verified the behavior with go test ./....\n\n## Related Issues\n\nCloses #1088

@CLAassistant

CLAassistant commented Sep 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment on lines 105 to +106
func hasTraversalPathComponent(pathspec string) bool {
for _, part := range strings.Split(pathspec, "/") {
for _, part := range strings.Split(normalizePathspec(pathspec), "/") {

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.

maintainability · low
Minor redundancy: the caller in Execute already normalizes s via normalizePathspec(s) before passing it to hasTraversalPathComponent. The second normalization inside this function is therefore unnecessary for the current call site. While harmless (the replacement is idempotent), it could be confusing to future readers about where the canonical normalization happens. Consider either documenting that this function intentionally re-normalizes for safety if called from other sites, or removing the inner normalization since the sole caller already handles it.

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Windows path normalization changes look correct and are well covered by regression tests, including traversal rejection and valid Windows-style path handling. The additional normalization in hasTraversalPathComponent() is slightly redundant but harmless and does not block the fix. I don't see any blocking issues in this PR.

@wu21-web

Copy link
Copy Markdown
Contributor

This is a duplicate of #1088 .

@brahmanandmathpati

Copy link
Copy Markdown

The fix itself is fine, honestly. The bug was real — Git pathspecs are always /-based, but the old traversal check only looked for /-style .., so on Windows you could sneak ..\pkg past it and it'd go straight to Git unmodified. That's a genuine bypass. Your fix normalizes backslashes to forward slashes before checking for traversal, which is the right order — you always want to validate the normalized form, not the raw input. Good call adding the regression tests for both that and the comment-path matching bug too.

@lizhengfeng101

Copy link
Copy Markdown
Contributor

This is a duplicate of #1089 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants