alibaba / alibaba/open-code-review
bug(tool): prevent path traversal bypass and normalize paths in code_search and code_comment
- Dominant language
- Go
- Stars
- 24.4k
- Forks
- 1.8k
- Avg merge
- 2d 4m
- Merged PRs (30d)
- 102
Description
### OpenCodeReview Version
`main` at `eda2548` (built from source)
### Operating System
Windows (also impacts cross-platform path handling across Windows/Linux/macOS)
### Installation Method
Built from source
### LLM Provider
Other OpenAI-compatible endpoint / Anthropic
### Bug Description
Two cross-platform path handling issues exist in `internal/tool`:
1. **Path traversal validation bypass in `code_search`**:
In `internal/tool/code_search.go`, `hasTraversalPathComponent` only checks for `/` path separators:
```go
func hasTraversalPathComponent(pathspec string) bool {
for _, part := range strings.Split(pathspec, "/") {
if part == ".." {
return true
}
}
return false
}
```
When `file_patterns` contain Windows-style backslashes (such as `..\pkg` or `pkg\..\internal`), `strings.Split` fails to separate path segments by `\`, allowing path traversal validation to be bypassed. Furthermore, `file_patterns` are forwarded directly to `git grep` without normalizing `\` to `/`, which causes `git grep` pathspec matching to fail.
2. **Silent comment dropping due to unnormalized path in `code_comment`**:
In `internal/tool/code_comment.go`, `parseCommentsInner` directly sets `cm.Path` from the tool arguments without normalizing path separators or redundant prefixes (e.g., `pkg\util.go`, `./pkg/util.go`, or `pkg//util.go`).
Downstream in `internal/agent/agent.go`, comments are looked up via `CommentCollector.CommentsForPath(d.NewPath)` where `d.NewPath` is the canonical forward-slash path (e.g. `pkg/util.go`). Because `CommentsForPath` performs strict equality (`cm.Path == path`), any comment generated with backslashes or `./` is silently dropped and never included in the review results.
### Steps to Reproduce
1. Call `CodeSearchProvider.Execute` with `file_patterns: ["..\\pkg"]`.
- Observe that `hasTraversalPathComponent` returns `false` and does not reject the pattern with `"Error: file_patterns must not contain .."`.
2. Call `CodeCommentProvider.Execute` (or `ParseComments`) with `path: "pkg\\util.go"` or `path: "./pkg/util.go"`.
- Store comments in `CommentCollector`.
- Call `CommentCollector.CommentsForPath("pkg/util.go")`.
- Observe that 0 comments are returned because the path was not normalized.
### Expected Behavior
1. `hasTraversalPathComponent` should normalize `\` to `/` before checking for `..`, and `file_patterns` should be normalized to forward slashes before being passed to `git grep`.
2. `parseCommentsInner` should normalize comment paths (`\` to `/`, stripping leading `./`, cleaning redundant slashes) so comments reliably match `CommentsForPath(d.NewPath)`.
### Additional Context
This aligns `code_search` and `code_comment` with the cross-platform path normalization patterns recently established in `file_find.go` (#1075).
Contributor guide
Research direction
Start with internal/tool/code_search.go and internal/tool/code_comment.go, including CodeSearchProvider.Execute, CodeCommentProvider.Execute or ParseComments, and the path handling established in file_find.go (#1075). Reproduce the Windows-style traversal and comment-path cases, then verify that traversal is rejected, git grep receives normalized paths, and CommentsForPath("pkg/util.go") retains comments from equivalent path forms.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- security, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100