fix: path traversal bypass via backslash separators on Windows in code_search

[Sunil56224972]

Summary

Security Fix: hasTraversalPathComponent() in internal/tool/code_search.go only splits on '/' to detect .. path traversal components in file_patterns. On Windows (an explicitly supported platform), backslash \ is a valid path separator, so patterns like ..\..\etc\passwd bypass the traversal check entirely.

Root Cause

The function splits the pathspec on '/' only. When a backslash-separated path like ..\..\..\etc is provided, strings.Split(s, "/") produces a single component ..\..\..\etc which does NOT equal .., so the traversal is not detected.

Impact

An LLM (potentially via prompt injection in reviewed code) could craft file_patterns arguments to code_search using backslash path separators to search files outside the repository root when OCR runs on Windows.

Fix

Normalize the pathspec via filepath.ToSlash() (converts \ to /) and filepath.Clean() (collapses redundant separators and .. segments) before splitting to check for traversal components.

Testing

Added TestCodeSearchProvider_Execute_RejectsBackslashTraversal with test cases for:

• Pure backslash traversal: ..\..\\etc
• Backslash mid-path: pkg\..\internal
• Mixed separators: pkg\../..\secret

All existing tests continue to pass.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

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

• ✅ Successfully posted inline: 1 comment(s)

[github-actions]

Security Issue: filepath.Clean resolves .. components by collapsing them with parent directories (e.g., "a/../b" becomes "b"). This means pathspecs that previously would have been rejected (because they contained literal .. segments) may now pass the check after being cleaned.

The stated goal is to also catch Windows-style backslash traversals like "..\\..\\..". Instead of using filepath.Clean (which semantically resolves paths), you should only normalize the separator characters so that .. segments remain intact for detection.

Consider replacing filepath.Clean with just filepath.ToSlash, or alternatively manually replace backslashes before splitting.

Suggestion:

Suggested change

	normalized := filepath.ToSlash(filepath.Clean(pathspec))
	normalized := filepath.ToSlash(pathspec)

[lizhengfeng101]

Review

Thanks for the contribution! The identified gap is real — hasTraversalPathComponent only splits on /, so backslash-separated paths like ..\..\..\etc bypass the check on Windows where \ is a valid path separator.

That said, the practical exploitability is very low: git grep only searches tracked files (or untracked files within the worktree with --untracked), and modern git rejects pathspecs that resolve above the repository root. So this is more of a defense-in-depth hardening than a directly exploitable vulnerability.

Issue with filepath.Clean

The current fix uses filepath.ToSlash(filepath.Clean(pathspec)), which introduces platform-dependent behavior:

• On Linux: filepath.Clean("pkg\\..\\internal") → "pkg\\..\\internal" (backslash is a literal character, not a separator) → then ToSlash converts it → "pkg/../internal" → .. detected ✓
• On Windows: filepath.Clean("pkg\\..\\internal") → "internal" (resolves the ..) → .. gone → not detected ✗

This means the test case "backslash middle parent" would fail on Windows, and the behavior diverges across platforms.

Suggested simpler approach

A plain string replacement achieves the goal without platform-dependent semantics:

func hasTraversalPathComponent(pathspec string) bool {
	normalized := strings.ReplaceAll(pathspec, "\\", "/")
	for _, part := range strings.Split(normalized, "/") {
		if part == ".." {
			return true
		}
	}
	return false
}

Advantages:

• Identical behavior on all platforms
• Preserves the original strict policy (any .. component is rejected)
• No new path/filepath import needed
• Easier to audit — no need to reason about filepath.Clean semantics
• All test cases pass consistently everywhere

[Sunil56224972]

Great catch on the filepath.Clean platform dependency! You're right — on Windows it resolves the .. before we can detect it, making the check silently pass.

I've adopted your suggested approach:

func hasTraversalPathComponent(pathspec string) bool {
    normalized := strings.ReplaceAll(pathspec, "\\", "/")
    for _, part := range strings.Split(normalized, "/") {
        if part == ".." {
            return true
        }
    }
    return false
}

Advantages (as you noted):

• Identical behavior on all platforms
• Preserves the original strict policy
• No new path/filepath import needed
• Easier to audit
• All test cases pass consistently everywhere

Also rebuilt both files from upstream to fix encoding.