fix(scan): preserve tracked files without exposing ignored files - #320
fix(scan): preserve tracked files without exposing ignored files#320mldangelo-oai wants to merge 7 commits into
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45f56e891a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] | ||
| command = ["rg", "--files", "--hidden", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
When a repository force-tracks a file that still matches .gitignore, this command silently omits that committed file from the security inventory; for example, the tracked tracked-secret.py scenario in tests-ts/runtime.test.ts now produces only .gitignore, so both the existing regression test and scans of such repositories fail to cover committed source. Exclude ignored untracked files without dropping files reported by Git as tracked.
Useful? React with 👍 / 👎.
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] | ||
| command = ["rg", "--files", "--hidden", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
When --repo points to a supported non-Git directory snapshot, this still inventories files such as an ignored .env and descendants of ignored directories. The local rg --help documents the cause: “By default, ripgrep will only respect filter rules from source control ignore files when ripgrep detects” a Git repository. The new test initializes Git, so it misses this path; make ripgrep honor ignore files even when no .git directory is present.
Useful? React with 👍 / 👎.
| expect((await readFile(output, "utf8")).trimEnd().split("\n")).toEqual([ | ||
| "./.gitignore", | ||
| "./.visible-config", | ||
| "./src/handler.ts", |
There was a problem hiding this comment.
On every windows-latest test leg in .github/workflows/node-ci.yml, this exact array uses Unix separators even though the local rg --help says output “defaults to your platform's path separator”; ripgrep therefore emits backslash-separated paths on Windows and this newly added test fails despite a correct inventory. Normalize the returned paths or make the expected values platform-specific.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 45f56e891a
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| def generate_in_scope_files(repository: Path, scope: str, output: Path) -> int: | ||
| """Atomically write the exact ripgrep inventory sorted as ``LC_ALL=C``.""" | ||
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] | ||
| command = ["rg", "--files", "--hidden", "--glob", "!.git/**", "--", scope] |
There was a problem hiding this comment.
When a Deep Scan targets a repository where an untrusted contributor can commit ignore rules, removing --no-ignore lets those rules hide still-tracked source from rg. The bundled setup accepts the truncated inventory, gives it to every worker, and later derives deep_repository/repository coverage with no excluded paths; completion never reconciles it against Git. A contributor can therefore hide arbitrary production source and obtain a sealed “complete” scan that never reviews it. Enumerate tracked files independently and add only non-ignored untracked files.
Useful? React with 👍 / 👎.
mldangelo-oai
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6677984b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if relative and (repository / os.fsdecode(relative)).is_file(): | ||
| rows.add(prefix + relative + b"\n") |
There was a problem hiding this comment.
When an untrusted checkout force-tracks an ignored symlink whose target is a regular file outside the repository, Path.is_file() follows the link, so the new Git supplement adds that path even though ripgrep does not enumerate the symlink. Deep Scan workers are instructed to read every inventory entry, which can expose host files such as credentials to the scan; reject symlinks and verify that each resolved file remains beneath repository before adding it.
Useful? React with 👍 / 👎.
| "--", | ||
| scope, |
There was a problem hiding this comment.
When the requested scope has Git pathspec syntax in its literal name, Git interprets it rather than confining the query to that path; Git's pathspec documentation defines a leading : as a “magic signature,” and -- only terminates option parsing. For example, with a real directory named :(exclude)foo, this command returns ignored tracked files outside that directory while omitting its own tracked files, causing a scoped scan to inventory unauthorized paths. Invoke ls-files with literal-pathspec handling so scope is treated only as a filesystem path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: b6677984b7
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| "--ignored", | ||
| "--exclude-standard", |
There was a problem hiding this comment.
When an untrusted contributor can commit .ignore or .rgignore, this still lets a Deep Scan seal incomplete coverage: rg now honors those rules, but the recovery asks Git only for --exclude-standard, which does not know ripgrep-only rules. In a root Git repository with tracked hidden.py named by committed .ignore, the helper omitted hidden.py while git ls-files confirmed it was tracked. Workers receive only this canonical inventory, so the omission is not recorded as an exclusion. This is fresh evidence beyond the earlier .gitignore report: the new Git recovery fixes Git ignores but not ripgrep-only ignores. Union the inventory with all tracked regular files.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 540bbd4802
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| command = ["rg", "--files", "--hidden", "--no-ignore", "--glob", "!.git/**", "--", scope] | ||
| """Atomically inventory visible files and ignored files tracked by Git.""" | ||
| command = [ | ||
| "rg", |
There was a problem hiding this comment.
When RIPGREP_CONFIG_PATH names a config containing --follow, an untrusted checkout's symlink to a regular file outside the repository is emitted into this inventory and can expose that host file to scan workers. This reproduces independently of the earlier Git-supplement issue: the entry comes directly from ripgrep because its inherited config remains enabled. The inspected rg --help states that --follow traverses symbolic links and that --no-config prevents reading RIPGREP_CONFIG_PATH; add the latter so user configuration cannot alter this security boundary.
Useful? React with 👍 / 👎.
| rows = sorted(inventory) | ||
| rows = set(inventory) | ||
|
|
||
| if (repository / ".git").exists(): |
There was a problem hiding this comment.
When a supported non-Git directory snapshot merely contains a .git file or directory without valid repository metadata—for example, a copied submodule with a stale gitfile or a fixture with an empty .git directory—this condition runs git ls-files, receives status 128, and aborts the entire inventory instead of scanning it as a directory. This is inconsistent with the rev-parse-based Git classification in workbench_target.py; use an actual work-tree check rather than filesystem existence before enabling the supplement.
Useful? React with 👍 / 👎.
| "rg", | ||
| "--files", | ||
| "--hidden", | ||
| "--no-require-git", |
There was a problem hiding this comment.
When a non-Git snapshot is located beneath an unrelated parent .gitignore, that parent can silently remove files from this inventory; for example, a parent rule snapshot/hidden.py causes --repo snapshot --scope . to record only the other files. This is distinct from the earlier non-Git issue about honoring the snapshot's own ignore file: the inspected rg --help states that ripgrep ascends parent directories for ignore rules by default, so content outside --repo is controlling scan coverage. Pass --no-ignore-parent so only ignore rules within the requested repository affect the inventory.
Useful? React with 👍 / 👎.
| command = [ | ||
| "git", | ||
| "--literal-pathspecs", | ||
| "ls-files", |
There was a problem hiding this comment.
When the scanned repository's local .git/config sets core.fsmonitor to an executable hook, this newly introduced git ls-files call executes that hook with the scanner's privileges. I reproduced this through the helper with an executable core.fsmonitor value, and GIT_TRACE2=1 showed child_start for that executable during the exact ls-files --cached --ignored --exclude-standard command. Override core.fsmonitor=false for this invocation so inspecting an untrusted repository cannot execute repository-configured code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 540bbd4802
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| candidate = repository / os.fsdecode(relative) | ||
| if candidate.is_symlink() or not candidate.is_file(): | ||
| continue |
There was a problem hiding this comment.
When a whole-target Deep Scan contains an initialized submodule matched by an ignore rule, recurse into the submodule instead of dropping its gitlink. In a fixture where the superproject tracks module and ignores module/, git ls-files --ignored returns only module; candidate.is_file() is false, so module/security.py disappears while setup succeeds. Workers receive this truncated inventory, which can still back deep_repository coverage. Fresh evidence beyond the prior tracked-file reports is that this file is tracked in the nested submodule index, which the root-only recovery never queries. Enumerate initialized submodule indexes with the same containment checks.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52b82f9444
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise InventoryError(f"could not inspect Git worktree: {error}") from error | ||
| worktree = None | ||
|
|
||
| if worktree is not None and worktree.returncode not in (0, 128): |
There was a problem hiding this comment.
When an actual checkout makes rev-parse exit 128—for example, Git rejects a checkout owned by another UID with fatal: detected dubious ownership—this treats it as a non-Git snapshot and silently skips the tracked-file supplement. I reproduced this with a force-tracked ignored hidden.py: the helper exited successfully while inventorying only .gitignore, allowing incomplete scan coverage. Fresh evidence beyond the earlier stale-.git case is that status 128 also represents fatal errors in valid repositories, so distinguish “not a repository” from probe failures and fail closed for the latter.
Useful? React with 👍 / 👎.
| candidate.resolve(strict=True).relative_to(repository) | ||
| except (OSError, ValueError): | ||
| continue | ||
| rows.add(prefix + relative + b"\n") |
There was a problem hiding this comment.
On Windows, every visible tracked file can be added twice because ripgrep and Git spell the same path differently: the inspected rg --help says its output defaults to \ on Windows, while git ls-files emits slash-form index paths, and this byte-set union does not normalize either source. Fresh evidence beyond the earlier assertion-only issue is that its fixture tracks only files hidden from ripgrep, so normalizing expected strings masks the production duplication rather than exercising the overlap; a normal tracked source tree therefore gets inflated counts and duplicate worker assignments.
Useful? React with 👍 / 👎.
| "--files", | ||
| "--hidden", | ||
| "--no-require-git", | ||
| "--no-ignore-parent", |
There was a problem hiding this comment.
When a scoped scan targets a subdirectory, this flag also prevents ripgrep from applying the repository's .git/info/exclude, while the manual recovery below restores only .gitignore, .ignore, and .rgignore. I reproduced a sub/hidden.py that git check-ignore -v attributes to .git/info/exclude: a whole-repository inventory omitted it, but --scope sub included it and exited successfully. The inspected rg --help confirms that --no-ignore-parent suppresses applicable parent-directory rules, so preserve repository-local Git excludes explicitly or private ignored files can be sent to scan workers.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb7c5ce615
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if worktree.returncode == 128 and "not a git repository" in detail.lower(): | ||
| worktree = None |
There was a problem hiding this comment.
On hosts with Git translation catalogs and a non-English LC_MESSAGES, git rev-parse localizes this fatal diagnostic, so a supported non-Git directory snapshot no longer matches the English substring and the helper exits with an inventory error instead of scanning it. Force a stable locale for the Git subprocesses or distinguish the non-repository result without parsing localized stderr.
Useful? React with 👍 / 👎.
| if worktree is not None and worktree.stdout.strip() == b"true": | ||
| prefix = b"./" if scope == "." or scope.startswith("./") else b"" | ||
| listed: list[bytes] = [] | ||
| for arguments in (["--cached"], ["--others", "--exclude-standard"]): |
There was a problem hiding this comment.
When the scanning account configures core.excludesFile with a rule matching an untracked source file, git ls-files --others --exclude-standard omits that file and the allowed intersection later deletes it from ripgrep's output, even though ripgrep was deliberately invoked with --no-ignore-global. For example, a global *.py rule makes an untracked source.py disappear while the helper succeeds with incomplete coverage; the inspected git ls-files -h describes this option as adding the standard Git exclusions. Disable the global excludes source for this Git query while retaining repository-local ignore and .git/info/exclude behavior.
Useful? React with 👍 / 👎.
| for name in (".gitignore", ".ignore", ".rgignore"): | ||
| ignore = repository / name | ||
| if ignore.is_file() and not ignore.is_symlink(): | ||
| command.extend(["--ignore-file", str(ignore)]) |
There was a problem hiding this comment.
When a non-Git snapshot contains a root .gitignore, .ignore, or .rgignore symlink, this check prevents only the explicit --ignore-file argument; ripgrep still discovers and follows the symlink automatically. In a reproduced snapshot where .ignore pointed outside the target to a file containing hidden.py, the helper silently omitted the in-scope hidden.py, so outside-target filesystem contents can control scan coverage despite the apparent symlink guard. Disable automatic loading for these files and explicitly supply only validated regular ignore files, including any supported nested rules, or reject symlinked ignore files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: bb7c5ce615
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| "--files", | ||
| "--hidden", | ||
| "--no-require-git", | ||
| "--no-ignore-parent", | ||
| "--no-ignore-global", |
There was a problem hiding this comment.
On Unix, when a contributor can commit a symlinked .ignore, .rgignore, or .gitignore and an operator starts Deep Scan, ripgrep still auto-loads it: --no-config and the is_symlink() check do not disable automatic ignore discovery. A tracked .ignore -> /dev/zero made this helper exceed a 2-second timeout, while rg reached approximately 96 MB RSS after 0.5 seconds; the parent --no-ignore behavior returned immediately. Disable automatic discovery and load only contained regular ignore files, with a subprocess resource bound as defense in depth.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1baf11d67c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| worktree = subprocess.run( | ||
| [*git, "rev-parse", "--is-inside-work-tree"], |
There was a problem hiding this comment.
When a selected non-Git snapshot is nested beneath an unrelated Git working tree, this probe returns true and the later git ls-files --others --exclude-standard query applies the ancestor repository's exclusions; the local git ls-files -h describes that flag as adding the standard Git exclusions. I reproduced an ancestor .gitignore rule for snapshot/hidden.py causing that file to be removed by the allowed intersection even though ripgrep now uses --no-ignore-parent. Fresh evidence beyond the earlier ripgrep parent-ignore report is that this later Git classification reintroduces the ancestor rule; verify rev-parse --show-toplevel equals repository before enabling Git filtering, as the classification in workbench_target.py already does.
Useful? React with 👍 / 👎.
| if (path := normalized(row.rstrip(b"\r\n"))) in allowed | ||
| or any(path.startswith(worktree) for worktree in nested_worktrees) | ||
| } |
There was a problem hiding this comment.
When a relative scope is a symlink to a directory inside the repository, ripgrep emits paths using the requested alias but git ls-files reports only the canonical target paths (or the symlink itself), so this intersection removes every descendant. I reproduced a tracked d/a with linkdir -> d: --scope linkdir previously inventoried linkdir/a but now succeeds with zero files. Either reject symlinked directory scopes during resolution or reconcile the Git results using the resolved scope so an accepted scoped scan cannot silently become empty.
Useful? React with 👍 / 👎.
| rows = { | ||
| row | ||
| for row in rows | ||
| if (path := normalized(row.rstrip(b"\r\n"))) in allowed |
There was a problem hiding this comment.
On Unix, an untracked visible file whose name ends with a carriage return is silently removed because rstrip(b"\r\n") strips both the output delimiter and the carriage return that belongs to the filename, so the resulting key no longer matches the NUL-delimited git ls-files entry. I reproduced a repository containing only visible\r: ripgrep emitted ./visible\r\n, Git emitted visible\r\0, and the helper reported zero files. This filename form is intentionally handled by normalize_candidates.py::read_scope, so remove only the actual line delimiter rather than all trailing CR/LF bytes.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1baf11d67c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for arguments in (["--cached"], ["--others", "--exclude-standard"]): | ||
| try: | ||
| result = subprocess.run( | ||
| [*git, "ls-files", *arguments, "-z", "--", scope], |
There was a problem hiding this comment.
When --scope names a directory below an embedded Git worktree, such as nested/src, this outer-repository git ls-files ... -- nested/src query returns neither the descendant files nor the nested/ boundary entry that is returned for a whole-repository query. Consequently allowed and nested_worktrees are empty and the later intersection removes every file found by ripgrep, producing a successful zero-file scoped scan. Detect the enclosing nested-worktree boundary independently of the descendant pathspec or enumerate the nested worktree for such scopes.
Useful? React with 👍 / 👎.
| "GIT_WORK_TREE", | ||
| ): | ||
| environment.pop(name, None) | ||
| environment["GIT_LITERAL_PATHSPECS"] = "1" |
There was a problem hiding this comment.
When the scanner inherits GIT_GLOB_PATHSPECS=1 or GIT_ICASE_PATHSPECS=1, forcing GIT_LITERAL_PATHSPECS=1 leaves mutually incompatible global pathspec modes enabled. Git then aborts every repository inventory at ls-files with fatal: global 'literal' pathspec setting is incompatible with all other global pathspec settings, even for scope .. Remove the incompatible pathspec environment variables before selecting literal semantics so a user's Git environment cannot prevent scans from starting.
Useful? React with 👍 / 👎.
| worktree = subprocess.run( | ||
| [*git, "rev-parse", "--is-inside-work-tree"], | ||
| cwd=repository, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| env=environment, | ||
| check=False, |
There was a problem hiding this comment.
When the selected repository's local .git/config includes a FIFO or another blocking file, this newly added Git probe waits indefinitely because it has no timeout or resource bound. For example, an [include] path = /path/to/fifo entry causes the exact rev-parse invocation here to block before producing output, so setup for a scan of repository metadata that is not trusted never completes. Run these Git inspections with a finite timeout or otherwise prevent blocking config includes from stalling inventory generation.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Companion internal source PR: https://github.com/openai/openai/pull/1262966