mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-13 22:16:23 +00:00
fix(gitignore): escape directory names before interpolating into pattern position (#1809)
* fix(gitignore): escape directory names before interpolating into pattern position GitignoreParser rescopes patterns from nested .gitignore files by prefixing the containing directory's relative path, but interpolates that path into pattern position without escaping it. A directory whose name contains a gitignore metacharacter (*, ?, [, ], !, #) is then read as glob syntax instead of a literal name, so a directory like a stray "***" venv can turn a scoped pattern into one matching most or all of the project. Fixes #1806 * fix(gitignore): build patterns with '/' only, never os.sep/os.path.join The escaping added in the prior commit inserts literal backslash escape characters before pattern metacharacters. On Windows, os.sep is also backslash, so os.path.join(rel_dir_pattern, line) mixed path-separator backslashes with escape backslashes, and the pre-existing trailing `.replace(os.sep, "/")` normalization step then converted ALL of them indiscriminately -- turning an escaped 'a\[1\]' into 'a/[1/]', which no longer matches the literal directory name. Verified with ntpath (Windows path semantics) on this Linux session: simulating a Windows relpath through the old os.sep/os.path.join/replace chain produces 'a/[1/]/mod.py', which pathspec does not match against 'a[1]/mod.py'; the new '/'-only construction produces 'a\[1\]/mod.py', which matches. Fixed by normalizing rel_dir to '/' immediately and joining every pattern component with a literal '/', removing the now-unneeded (and on Windows actively harmful) blanket separator replace. Also marks the two directory-creation regression tests that use '*'/'?' literally in a directory name as Windows-skipped (those characters are illegal in Windows filenames, so the directories cannot be created there -- a platform limitation of the test, not the fix), and adds a pure-function test for _escape_gitignore_path_component so the '*'/'?' escaping logic still has cross-platform coverage. Found via this PR's own Windows CI run (jobs 91787560091/91787560092/91787560183 on run 30843966565), which failed test_gitignore_dir_name_with_metachars_anchored_pattern with an assertion failure (not the OSError the other two tests hit), before any maintainer had looked at the PR.
This commit is contained in:
1 parent
8ebb247dae
commit
c687e751af
3 files changed
+115
-10
No files matched your search
@@ -4,6 +4,11 @@ Status of the `main` branch. Changes prior to the next official version change w
|
||||
|
||||
* General:
|
||||
- Fix: Race conditions in ProjectServer when used by multiple clients in parallel
|
||||
- Fix: `GitignoreParser` interpolated a directory's name unescaped into gitignore pattern position;
|
||||
a directory named with pattern metacharacters (e.g. a stray `***`) could turn a scoped pattern
|
||||
into one matching far more than intended, silently excluding most or all of the project from
|
||||
indexing #1806
|
||||
- Fix: the README, the Language Support docs page and the project template omitted several already-supported language servers
|
||||
- Fix: a tool call exceeding the timeout blocked the task executor indefinitely; the executor now
|
||||
recovers without user-induced cancellation
|
||||
- Add Grok Build support (context `grok`, setup CLI, hooks)
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
from collections.abc import Callable, Iterator
|
||||
from dataclasses import dataclass, field
|
||||
from pathlib import Path
|
||||
@@ -11,6 +12,18 @@ from sensai.util.logging import LogTime
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
||||
# Characters meaningful to pathspec's gitignore grammar: glob wildcards, bracket expressions,
|
||||
# the escape character itself, and '!'/'#' which change a whole pattern's meaning when they
|
||||
# are its first character. Backslash-escaping them makes a literal name safe to interpolate.
|
||||
_GITIGNORE_PATTERN_SPECIAL_CHARS_RE = re.compile(r"([\\*?\[\]!#])")
|
||||
|
||||
|
||||
def _escape_gitignore_path_component(component: str) -> str:
|
||||
"""Escape gitignore/pathspec pattern metacharacters in a single path component (no
|
||||
separators) so it is matched as a literal name rather than as glob syntax.
|
||||
"""
|
||||
return _GITIGNORE_PATTERN_SPECIAL_CHARS_RE.sub(r"\\\1", component)
|
||||
|
||||
|
||||
class ScanResult(NamedTuple):
|
||||
"""Result of scanning a directory."""
|
||||
@@ -223,11 +236,21 @@ class GitignoreParser:
|
||||
"""
|
||||
patterns = []
|
||||
|
||||
# Get the relative path from repo root to the gitignore directory
|
||||
rel_dir = os.path.relpath(gitignore_dir, self.repo_root)
|
||||
# Get the relative path from repo root to the gitignore directory. Normalize to
|
||||
# forward slashes immediately: os.path.relpath returns native separators, but
|
||||
# gitignore/pathspec patterns are always POSIX-style, and on Windows os.sep is
|
||||
# backslash -- the same character pathspec uses as its escape character. Building
|
||||
# the pattern with any raw os.sep would make a later blanket backslash->slash
|
||||
# normalization indistinguishable from the escape backslashes below.
|
||||
rel_dir = os.path.relpath(gitignore_dir, self.repo_root).replace(os.sep, "/")
|
||||
if rel_dir == ".":
|
||||
rel_dir = ""
|
||||
|
||||
# rel_dir is a filesystem path, but the code below interpolates it into pattern
|
||||
# position; escape each of its components so a directory name containing pattern
|
||||
# metacharacters (e.g. "***") is matched literally instead of as glob syntax.
|
||||
rel_dir_pattern = "/".join(_escape_gitignore_path_component(part) for part in rel_dir.split("/")) if rel_dir else rel_dir
|
||||
|
||||
for line in content.splitlines():
|
||||
# Strip trailing whitespace (but preserve leading whitespace for now)
|
||||
line = line.rstrip()
|
||||
@@ -256,20 +279,23 @@ class GitignoreParser:
|
||||
if is_anchored:
|
||||
line = line[1:]
|
||||
|
||||
# Adjust pattern based on gitignore file location
|
||||
# Adjust pattern based on gitignore file location. Joined with a literal "/",
|
||||
# never os.path.join/os.sep: gitignore patterns are always POSIX-style, and on
|
||||
# Windows os.sep is backslash, indistinguishable from the escape backslashes
|
||||
# rel_dir_pattern may already contain.
|
||||
if rel_dir:
|
||||
if is_anchored:
|
||||
# Anchored patterns are relative to the gitignore directory
|
||||
adjusted_pattern = os.path.join(rel_dir, line)
|
||||
adjusted_pattern = f"{rel_dir_pattern}/{line}"
|
||||
else:
|
||||
# Non-anchored patterns can match anywhere below the gitignore directory
|
||||
# We need to preserve this behavior
|
||||
if line.startswith("**/"):
|
||||
# Even if pattern starts with **, it should still be scoped to the subdirectory
|
||||
adjusted_pattern = os.path.join(rel_dir, line)
|
||||
adjusted_pattern = f"{rel_dir_pattern}/{line}"
|
||||
else:
|
||||
# Add the directory prefix but also allow matching in subdirectories
|
||||
adjusted_pattern = os.path.join(rel_dir, "**", line)
|
||||
adjusted_pattern = f"{rel_dir_pattern}/**/{line}"
|
||||
else:
|
||||
if is_anchored:
|
||||
# Anchored patterns in root should only match at root level
|
||||
@@ -283,9 +309,6 @@ class GitignoreParser:
|
||||
if is_negation:
|
||||
adjusted_pattern = "!" + adjusted_pattern
|
||||
|
||||
# Normalize path separators to forward slashes (gitignore uses forward slashes)
|
||||
adjusted_pattern = adjusted_pattern.replace(os.sep, "/")
|
||||
|
||||
patterns.append(adjusted_pattern)
|
||||
|
||||
return patterns
|
||||
|
||||
@@ -1,11 +1,13 @@
|
||||
import os
|
||||
import shutil
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
from pathspec import PathSpec
|
||||
|
||||
from serena.util.file_system import GitignoreParser, GitignoreSpec, match_path
|
||||
from serena.util.file_system import GitignoreParser, GitignoreSpec, _escape_gitignore_path_component, match_path
|
||||
|
||||
|
||||
class TestGitignoreParser:
|
||||
@@ -715,6 +717,81 @@ src/*.o
|
||||
# foo.txt in other/ should NOT be ignored (outside foo/ subtree)
|
||||
assert not parser.should_ignore("other/foo.txt"), "other/foo.txt should NOT be ignored by foo/.gitignore"
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="'*' is illegal in Windows filenames; this directory name cannot exist there")
|
||||
def test_gitignore_dir_name_with_glob_metachars_is_not_a_wildcard(self):
|
||||
"""A directory named with glob metacharacters (e.g. a stray '***' venv) must be
|
||||
matched literally, not interpreted as a pattern (issue #1806).
|
||||
"""
|
||||
test_dir = self.repo_path / "test_metachar_dirname"
|
||||
test_dir.mkdir()
|
||||
(test_dir / "pkg").mkdir()
|
||||
(test_dir / "***").mkdir()
|
||||
|
||||
(test_dir / "pkg" / "mod.py").touch()
|
||||
(test_dir / "***" / "junk.txt").touch()
|
||||
|
||||
gitignore = test_dir / "***" / ".gitignore"
|
||||
gitignore.write_text("*\n")
|
||||
|
||||
parser = GitignoreParser(str(test_dir))
|
||||
|
||||
# pkg/mod.py is outside the "***" directory and must not be affected by its gitignore
|
||||
assert not parser.should_ignore("pkg/mod.py"), "pkg/mod.py should not be ignored by ***/.gitignore"
|
||||
|
||||
# junk.txt inside "***" is still ignored by its own gitignore's "*" pattern
|
||||
assert parser.should_ignore("***/junk.txt"), "***/junk.txt should be ignored by its own gitignore"
|
||||
|
||||
def test_gitignore_dir_name_with_metachars_anchored_pattern(self):
|
||||
"""Same as above, for the anchored-pattern join site."""
|
||||
test_dir = self.repo_path / "test_metachar_dirname_anchored"
|
||||
test_dir.mkdir()
|
||||
(test_dir / "pkg").mkdir()
|
||||
(test_dir / "a[1]").mkdir()
|
||||
|
||||
(test_dir / "pkg" / "mod.py").touch()
|
||||
(test_dir / "a[1]" / "mod.py").touch()
|
||||
|
||||
gitignore = test_dir / "a[1]" / ".gitignore"
|
||||
gitignore.write_text("/mod.py\n")
|
||||
|
||||
parser = GitignoreParser(str(test_dir))
|
||||
|
||||
assert not parser.should_ignore("pkg/mod.py"), "pkg/mod.py should not be ignored by a[1]/.gitignore"
|
||||
assert parser.should_ignore("a[1]/mod.py"), "a[1]/mod.py should be ignored by its own /mod.py pattern"
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="'?' is illegal in Windows filenames; this directory name cannot exist there")
|
||||
def test_gitignore_dir_name_with_metachars_implicit_double_star_pattern(self):
|
||||
"""A non-anchored pattern with no leading '**/' is joined as (rel_dir, "**", line):
|
||||
the third join site. An unescaped '?' in the directory name would leak the pattern
|
||||
into a sibling directory whose name merely matches the wildcard (issue #1806).
|
||||
"""
|
||||
test_dir = self.repo_path / "test_metachar_dirname_implicit_doublestar"
|
||||
test_dir.mkdir()
|
||||
(test_dir / "q?").mkdir()
|
||||
(test_dir / "qA").mkdir()
|
||||
(test_dir / "qA" / "sub").mkdir()
|
||||
|
||||
(test_dir / "qA" / "sub" / "mod.py").touch()
|
||||
|
||||
gitignore = test_dir / "q?" / ".gitignore"
|
||||
gitignore.write_text("mod.py\n")
|
||||
|
||||
parser = GitignoreParser(str(test_dir))
|
||||
|
||||
# "q?/.gitignore" must not reach into the sibling "qA/" directory just because "?"
|
||||
# would match the "A" in "qA" if left as a wildcard.
|
||||
assert not parser.should_ignore("qA/sub/mod.py"), "qA/sub/mod.py should not be ignored by q?/.gitignore"
|
||||
|
||||
def test_escape_gitignore_path_component_escapes_all_metachars(self):
|
||||
"""Pure-function coverage for the '*' and '?' escaping cases the two directory-creation
|
||||
tests above cannot exercise on Windows (those characters are illegal in Windows
|
||||
filenames, so this runs on every platform instead of touching the filesystem).
|
||||
"""
|
||||
assert _escape_gitignore_path_component("***") == "\\*\\*\\*"
|
||||
assert _escape_gitignore_path_component("q?") == "q\\?"
|
||||
assert _escape_gitignore_path_component("a[1]") == "a\\[1\\]"
|
||||
assert _escape_gitignore_path_component("plain") == "plain"
|
||||
|
||||
|
||||
class TestGitignoreParserPermissionError:
|
||||
"""Test PermissionError handling in GitignoreParser."""
|
||||
|
||||
Reference in new issue
Block a user