-
Notifications
You must be signed in to change notification settings - Fork 733
fix(scan): confine local diff inputs to the selected target #630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
97cb4a2
33a49c6
7c30deb
f3431c7
7210901
d21ffcc
f180e49
35329b6
564150f
4d2f5e5
446ff8f
8f35a48
b472e7a
059e7b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,12 @@ | |
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import errno | ||
| import hashlib | ||
| import json | ||
| import os | ||
| import re | ||
| import stat | ||
| import subprocess | ||
| import sys | ||
| from collections import Counter | ||
|
|
@@ -37,9 +39,17 @@ | |
|
|
||
| # Some plugin hosts launch Python with safe-path isolation enabled. | ||
| sys.path.insert(0, str(Path(__file__).resolve().parent)) | ||
| from finalize_scan_contract import ( | ||
| _descriptor_relative_reads_available, | ||
| _open_scan_local_directory, | ||
| _open_verified_scan_directory, | ||
| _windows_scan_local_files, | ||
| scan_root_identity, | ||
| ) | ||
| from rank_preview import ( | ||
| DEFAULT_PREVIEW_BYTES, | ||
| TEXT_CODE_EXTENSIONS, | ||
| is_binary_sample, | ||
| preview_for, | ||
| preview_for_bytes, | ||
| ) | ||
|
|
@@ -293,6 +303,121 @@ def path_is_excluded(path: Path) -> bool: | |
| return path.name.endswith((".min.js", ".map")) | ||
|
|
||
|
|
||
| def changed_path_parent_is_within_target(path: Path, target: Path) -> bool: | ||
| """Resolve the nearest existing parent without dereferencing the changed leaf.""" | ||
| target = target.resolve(strict=True) | ||
| candidate = path.parent | ||
| while True: | ||
| try: | ||
| candidate.lstat() | ||
| except (FileNotFoundError, NotADirectoryError): | ||
| parent = candidate.parent | ||
| if parent == candidate: | ||
| return False | ||
| candidate = parent | ||
| continue | ||
| break | ||
|
|
||
| resolved = candidate.resolve(strict=True) | ||
| if resolved.is_relative_to(target): | ||
| return True | ||
| for ancestor in (resolved, *resolved.parents): | ||
| try: | ||
| if ancestor.samefile(target): | ||
| return True | ||
| except OSError: | ||
| continue | ||
| return False | ||
|
|
||
|
|
||
| def _open_windows_changed_path_descriptor( | ||
| target: Path, relative_path: Path, expected_root_identity: tuple[int, int] | None | ||
| ) -> int: | ||
| """Preserve ordinary missing-leaf behavior without relaxing parent checks.""" | ||
|
|
||
| expected_path = target / relative_path | ||
| try: | ||
| return _windows_scan_local_files().open_read_fd( | ||
| target, | ||
| relative_path.as_posix(), | ||
| "changed Git working-tree path", | ||
| expected_root_identity=expected_root_identity, | ||
| ) | ||
| except OSError as error: | ||
| if error.filename is None or os.path.normcase( | ||
| os.path.normpath(os.fspath(error.filename)) | ||
| ) != os.path.normcase(os.path.normpath(os.fspath(expected_path))): | ||
| raise | ||
| if error.errno in {errno.ENOENT, 3}: | ||
| raise FileNotFoundError( | ||
| error.errno, error.strerror, error.filename | ||
| ) from error | ||
| if error.errno in {errno.EACCES, 5, 32, 33}: | ||
| raise PermissionError(error.errno, error.strerror, error.filename) from error | ||
| raise | ||
|
|
||
|
|
||
| def preview_for_changed_path( | ||
| path: Path, | ||
| target: Path, | ||
| preview_bytes: int, | ||
| *, | ||
| expected_root_identity: tuple[int, int] | None, | ||
| ) -> tuple[str, bool]: | ||
| """Bind working-tree reads to the checked repository and parent identities.""" | ||
|
|
||
| try: | ||
| relative_parent = path.parent.resolve(strict=True).relative_to(target) | ||
| except (FileNotFoundError, PermissionError) as error: | ||
| raise ValueError("changed Git working-tree parent became unavailable") from error | ||
| descriptor: int | None = None | ||
| try: | ||
| if os.name == "nt": | ||
| descriptor = _open_windows_changed_path_descriptor( | ||
| target, relative_parent / path.name, expected_root_identity | ||
| ) | ||
| elif _descriptor_relative_reads_available(): | ||
| root_descriptor = _open_verified_scan_directory(target, expected_root_identity) | ||
| try: | ||
| try: | ||
| parent_descriptor = _open_scan_local_directory( | ||
| root_descriptor, relative_parent.parts, create=False | ||
| ) | ||
| except (FileNotFoundError, PermissionError) as error: | ||
| raise ValueError( | ||
| "changed Git working-tree parent became unavailable" | ||
| ) from error | ||
| try: | ||
| descriptor = os.open( | ||
| path.name, | ||
| os.O_RDONLY | os.O_NOFOLLOW | getattr(os, "O_NONBLOCK", 0), | ||
| dir_fd=parent_descriptor, | ||
| ) | ||
| finally: | ||
| os.close(parent_descriptor) | ||
| finally: | ||
| os.close(root_descriptor) | ||
|
|
||
| if not stat.S_ISREG(os.fstat(descriptor).st_mode): | ||
| raise OSError("changed Git working-tree path is not a regular file") | ||
|
Comment on lines
+401
to
+402
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a selected regular file is replaced by a directory after the caller's AGENTS.md reference: AGENTS.md:L21-L24 Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed at Please treat this verified non-regular leaf as a skipped entry while retaining root/parent checks and symlink/reparse rejection. This also existed at |
||
| else: | ||
| raise OSError("changed Git working-tree input requires secure file operations") | ||
|
|
||
| try: | ||
| with os.fdopen(descriptor, "rb") as source: | ||
| descriptor = None | ||
| sample = source.read(4096) | ||
| if is_binary_sample(sample): | ||
| return "", True | ||
| data = sample + source.read() | ||
| except OSError: | ||
| return "", True | ||
| return preview_for_bytes(path, data, preview_bytes) | ||
| finally: | ||
| if descriptor is not None: | ||
| os.close(descriptor) | ||
|
|
||
|
|
||
| def windows_stream_component(path: Path) -> str | None: | ||
| """Return the first NTFS alternate-data-stream component.""" | ||
|
|
||
|
|
@@ -684,6 +809,7 @@ def make_diff_rank_input(args: argparse.Namespace) -> None: | |
| if not repo.is_dir(): | ||
| raise SystemExit(f"Repo path not found: {repo}") | ||
|
|
||
| root_identity = scan_root_identity(repo)[1] if args.mode != "revisions" else None | ||
| changed = [ | ||
| (path, status) | ||
| for path, status in git_changed_paths(repo, args.base, args.head, args.mode) | ||
|
|
@@ -708,6 +834,17 @@ def make_diff_rank_input(args: argparse.Namespace) -> None: | |
| rows: list[JsonRow] = [] | ||
| for path, status in changed: | ||
| rel = path.relative_to(repo) | ||
| if args.mode != "revisions": | ||
|
mldangelo-oai marked this conversation as resolved.
|
||
| try: | ||
| within_target = changed_path_parent_is_within_target(path, repo) | ||
| except (OSError, RuntimeError) as error: | ||
| raise SystemExit( | ||
| "Could not inspect a changed Git working-tree path." | ||
| ) from error | ||
| if not within_target: | ||
| raise SystemExit( | ||
| "Changed Git working-tree paths must stay inside the selected target." | ||
| ) | ||
|
|
||
| if status == "D": | ||
| preview = "" | ||
|
|
@@ -724,13 +861,17 @@ def make_diff_rank_input(args: argparse.Namespace) -> None: | |
| preview = "" | ||
|
mldangelo-oai marked this conversation as resolved.
|
||
| elif path.is_file(): | ||
| try: | ||
| path.resolve(strict=True).relative_to(repo) | ||
| except (OSError, ValueError): | ||
| preview = "" | ||
| else: | ||
| preview, is_binary = preview_for(path, args.preview_bytes) | ||
| if is_binary: | ||
| continue | ||
| preview, is_binary = preview_for_changed_path( | ||
| path, repo, args.preview_bytes, expected_root_identity=root_identity | ||
| ) | ||
| except (FileNotFoundError, PermissionError): | ||
| continue | ||
| except (OSError, RuntimeError, ValueError) as error: | ||
| raise SystemExit( | ||
| "Changed Git working-tree paths must stay inside the selected target." | ||
| ) from error | ||
| if is_binary: | ||
| continue | ||
| else: | ||
| preview = "" | ||
| rows.append({"path": rel.as_posix(), "area": args.area, "preview": preview}) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a non-symlink parent is exchanged after
changed_path_parent_is_within_targetreturns—by renamingrepo/srcaside and moving an unrelated same-filesystem directory intorepo/src—this opens the replacement because only the root identity is verified andO_NOFOLLOWaccepts the new ordinary directory. I reproduced this on POSIX and the generated rank preview contained the replacement directory's external marker. The fresh evidence beyond the prior junction and repository-root coverage is that a normal-directory parent swap bypasses both controls; retain or record the checked parent's identity and verify it while traversing the descriptor chain.AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L23
Useful? React with 👍 / 👎.