diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eaaa09..d593020 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ All notable changes to CodeSnake are documented here. The format follows ### Security +- **A directory walk no longer follows a `.py` symlink out of the scan root.** + `os.walk` does not follow directory symlinks, but a *file* symlink was yielded and + read through, so `src/x.py -> ~/.ssh/id_rsa` was opened when scanning `src/`. Worse + than the read itself, a syntax error prints the offending source line into the + report, so a line of the target file reached the output — and on a public repository, + the CI log. Symlinks resolving inside the root are still followed; an explicit file + argument is still read as given. + +### Security + - **The release workflow refuses a tag that is not reachable from `main`.** Branch protection governs `main`, not tags, so anyone able to push a tag and publish a GitHub Release could previously ship a commit that never passed review — the diff --git a/SECURITY.md b/SECURITY.md index b1e8b0b..9b3d318 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -34,6 +34,8 @@ Two places reach outside the process, both only when you ask: Directory walks skip `.gitignore` matches, but **only for files git does not track** — matching git's own rule that an ignore pattern has no effect on a committed file. A file added with `git add -f` is therefore still analyzed, and so is a tracked file inside an ignored directory. `--no-ignore` disables ignore handling entirely, for scanning a working tree that is not a git repository or whose ignored output you want covered. Venvs, caches, and `.git` are skipped in every mode. +A directory walk does not follow a `.py` symlink out of the tree being scanned. `src/x.py -> ~/.ssh/id_rsa` is skipped rather than read, because a parse failure prints the offending source line into the report. Symlinks that resolve inside the scan root are followed normally. An explicit file argument is still read as given — naming a file is a deliberate act. + ### In scope - Anything that makes CodeSnake execute code from a file it is analyzing diff --git a/src/codesnake/checker.py b/src/codesnake/checker.py index 0515f8e..a714302 100644 --- a/src/codesnake/checker.py +++ b/src/codesnake/checker.py @@ -432,6 +432,19 @@ def issue_ignored_by_pragma(code: str, line: int, source_lines: Sequence[str]) - return code.upper() in codes +def _within(path: Path, root: Path) -> bool: + """True if ``path`` resolves to somewhere inside ``root``. + + Reading through a symlink that leaves the tree the caller asked to scan + discloses files outside it, so the walk declines to follow one. + """ + try: + resolved = path.resolve() + except OSError: + return False + return resolved == root or resolved.is_relative_to(root) + + def _load_ignore_state( root_resolved: Path, ) -> Tuple['_IgnoreStack', Set[Path], Set[Path]]: @@ -511,6 +524,12 @@ def iter_python_files(root: Path, *, respect_gitignore: bool = True) -> Iterable if not name.endswith('.py'): continue candidate = current / name + # os.walk does not follow directory symlinks, but a *file* symlink is + # yielded and would be read through. `src/x.py -> ~/.ssh/id_rsa` would + # otherwise be opened, and a syntax error prints the offending line + # into the report. Only follow links that stay inside the scan root. + if candidate.is_symlink() and not _within(candidate, root_resolved): + continue if ( respect_gitignore and candidate not in tracked diff --git a/test/test_codesnake.py b/test/test_codesnake.py index 58c4f21..a989f0b 100644 --- a/test/test_codesnake.py +++ b/test/test_codesnake.py @@ -881,6 +881,64 @@ def _git(self, root, *args): import subprocess subprocess.run(['git', *args], cwd=root, check=True, capture_output=True, timeout=30) + def test_symlink_escaping_the_root_is_not_followed(self): + """`src/x.py -> /outside/secret` must not be read. + + os.walk does not follow directory symlinks, but a file symlink is + yielded and would be read through, and a syntax error prints the + offending line into the report. + """ + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp).resolve() + (root / 'project' / 'src').mkdir(parents=True) + outside = root / 'outside' + outside.mkdir() + secret = outside / 'secret.txt' + secret.write_text('DB_PASSWORD = hunter2!$nope\n', encoding='utf-8') + (root / 'project' / 'src' / 'real.py').write_text('x = 1\n', encoding='utf-8') + try: + (root / 'project' / 'src' / 'leaked.py').symlink_to(secret) + except (OSError, NotImplementedError): + self.skipTest('symlinks unavailable') + + targets, _ = expand_python_targets([str(root / 'project' / 'src')]) + self.assertEqual({Path(t).name for t in targets}, {'real.py'}) + + def test_symlink_staying_inside_the_root_is_followed(self): + """Containment, not a blanket ban on symlinks.""" + from codesnake.checker import iter_python_files + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp).resolve() + (root / 'src' / 'pkg').mkdir(parents=True) + target = root / 'src' / 'pkg' / 'shared.py' + target.write_text('x = 1\n', encoding='utf-8') + try: + (root / 'src' / 'alias.py').symlink_to(target) + except (OSError, NotImplementedError): + self.skipTest('symlinks unavailable') + + found = {p.name for p in iter_python_files(root / 'src')} + self.assertEqual(found, {'shared.py', 'alias.py'}) + + def test_escaping_symlink_content_never_reaches_the_report(self): + """The disclosure that made this worth fixing: the source line is printed.""" + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp).resolve() + (root / 'project' / 'src').mkdir(parents=True) + secret = root / 'secret.env' + secret.write_text('DEBUG = False\nDB_PASSWORD = hunter2!$nope\n', encoding='utf-8') + (root / 'project' / 'src' / 'ok.py').write_text('x = 1\n', encoding='utf-8') + try: + (root / 'project' / 'src' / 'config.py').symlink_to(secret) + except (OSError, NotImplementedError): + self.skipTest('symlinks unavailable') + + out = StringIO() + run_check([str(root / 'project' / 'src')], stream=out, + color=False, show_banner=False) + self.assertNotIn('DB_PASSWORD', out.getvalue()) + self.assertNotIn('hunter2', out.getvalue()) + def test_committed_file_is_not_hidden_by_gitignore(self): """git ignores .gitignore for tracked files; so must the walk.