Harden ls builtin: symlink safety, entry limits, pentest tests#60
Harden ls builtin: symlink safety, entry limits, pentest tests#60
Conversation
…pentest tests - Skip symlinks during -R recursion to match GNU ls default and prevent symlink-loop DoS attacks. Uses DirEntry.Type() (no symlink follow) instead of relying solely on FileInfo.IsDir() after e.Info() (follows). - Add MaxDirEntries (100,000) limit per directory to prevent OOM from huge directories. - Add 11 pentest tests covering symlink loops, recursion depth, entry count limits, flag injection, sandbox escape, and filename edge cases. - Add scenario test validating ls -R does not follow symlinks (asserted against bash). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall this is a solid hardening PR. The symlink skip logic is correct, the tests are thorough and well-structured, and the scenario test provides bash-compatibility validation. A few observations below.
P2 (Should Fix)
- MaxDirEntries check occurs after
ReadDirhas already loaded all entries into memory, reducing OOM protection effectiveness.
P3 (Nits / Minor)
MaxDirEntriesis exported solely for test access; consider an alternative.- The 100k-file pentest test is resource-heavy even outside short mode.
- Minor:
mustNotHanggoroutine leak on timeout.
|
I can only run on private repositories. |
|
Iteration 1 self-review result: COMMENT
No P0–P1 blockers. Proceeding to address comments. |
Acknowledge that ReadDir loads all entries into memory before the MaxDirEntries guard runs. The check still prevents expensive downstream processing (sorting, stat calls, recursion) on very large directories. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Re-review (iteration 2): All previous findings addressed. No issues remain.
P2 (MaxDirEntries post-ReadDir): Properly resolved with a clear four-line comment at ls.go:262-265 explaining that os.ReadDir does not support streaming, so the check prevents expensive downstream processing rather than the initial allocation. This is an accurate and honest documentation of the limitation.
P3 items (exported const, heavy CI test, goroutine leak): Satisfactorily explained as following existing codebase patterns.
Overall assessment:
- Symlink handling via
DirEntry.Type()is correct — avoids following symlinks during-Rrecursal, matching GNU ls default behaviour. MaxDirEntrieslimit is a reasonable safety bound against downstream processing costs.- Pentest coverage is thorough: symlink loops, deep nesting, entry limits, flag injection, sandbox escape, path traversal, and filename edge cases.
- Scenario test validates against bash (no
skip_assert_against_bash), which is appropriate since this matches standard bash/GNU ls behaviour. - No new issues found. Would approve if this were not a self-review.
|
I can only run on private repositories. |
|
Iteration 2 self-review result: APPROVE (no findings) Re-review confirms all iteration 1 findings have been properly addressed. No new issues found. The clarifying comment for MaxDirEntries accurately documents the Go |
![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: aa27d8f1d3
ℹ️ 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".
- Lower MaxDirEntries from 100,000 to 10,000 - Change from hard error (exit 1) to warning + suppress output (exit 0) - Print warning to stderr when limit exceeded, skip listing entries - Update pentest test to expect exit 0, warning on stderr, empty stdout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e76efda438
ℹ️ 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".
Add batched countDirEntries method to pathSandbox that reads directory entries in small batches (256) and bails out early once the count exceeds MaxDirEntries. This prevents OOM on directories with millions of entries by avoiding full materialization via ReadDir. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Lower MaxDirEntries from 10,000 to 1,000. When the cap is hit without explicit pagination flags, show the first 1,000 entries and exit 1 with a warning instead of suppressing all output. Add --offset/--limit flags so agents can paginate through large directories in safe batches. Replace countDirEntries with readDirLimited for single-pass reading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef74af5a8a
ℹ️ 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".
…owed ReadDir errors - Clamp opts.offset and opts.limit to >= 0 in listDir; guard readDirLimited against non-positive maxRead to prevent slice panics. - Restructure listDir to apply offset/limit to real entries only, then prepend `.`/`..` so they never consume limit slots. - Track lastErr in readDirLimited batch loop; only clear on io.EOF so non-EOF errors propagate via portablePathError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aee9e0132a
ℹ️ 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".
…fset Large --offset values (e.g. 999999) previously inflated maxRead (offset + effectiveLimit), defeating the DoS cap. Now maxRead is always MaxDirEntries and offset is clamped to MaxDirEntries before use. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b804c1b5e4
ℹ️ 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".
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce6c63ebf0
ℹ️ 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".
…on in multi-dir mode - ReadDirLimited now accepts an offset parameter that skips raw directory entries during reading (before sorting), achieving O(n) memory regardless of offset value where n = min(limit, MaxDirEntries). - Pagination flags (--offset/--limit) are silently cleared when multiple directory arguments are provided, matching the existing -R behavior. - Updated flag documentation to clarify offset operates on filesystem order. - Added tests for multi-dir limit ignored, large offset streaming, offset skipping at the readDirLimited level, and negative offset clamping. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
20 new pentest tests covering all review feedback from PR #60 rounds 1-4: negative limit, recursive/multi-dir pagination ignored, dot/dotdot with offset, truncation warning semantics, per-dir cap in recursive mode, dotfile DoS prevention, limit-only/offset-only, limit capping, sort flag interactions, -d/-a/-A with pagination, and exit code correctness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
15 declarative YAML scenario tests covering pagination behavior: limit-only, offset-only, offset+limit, offset beyond count, negative offset/limit clamping, zero values, recursive/multi-dir ignoring pagination, dot/dotdot not consumed by offset/limit, reverse sort with limit, and -d flag with limit. All scenarios use skip_assert_against_bash since --offset/--limit are non-standard flags. Offset-dependent tests use wc -l for filesystem- order-agnostic assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b235fdb11
ℹ️ 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".
…on after truncation - Synthesize . and .. before sorting so sort modifiers (-r, -S, -t) apply uniformly, matching bash behavior (e.g. ls -ar puts . and .. at the bottom). - Do not return early on implicit truncation — set failed flag and continue so -R recursion still descends into listed subdirectories. - Added ls -ar scenario test verified against bash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rEntries The MaxDirEntries cap exists to bound total work. Continuing recursion after truncation would defeat that purpose — an adversarial directory tree could trigger unbounded traversal. The early return is intentional. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef7ce13e8
ℹ️ 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".
Capture non-EOF errors from ReadDir before checking the truncation condition, since ReadDir can return partial entries alongside an error. Previously, a transient I/O failure could be silently discarded if the batch pushed entries past the maxRead threshold. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
…n test Refactor the batch-reading loop out of readDirLimited into a standalone collectDirEntries function that accepts a readBatch callback. This makes the error-during-truncation fix (f151963) directly testable with a mock reader that returns entries alongside a non-EOF error. New TestCollectDirEntries covers: error preserved when truncation occurs in the same batch, EOF not returned as error, offset skipping across batches, error without truncation, and sort correctness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
matt-dz
left a comment
matt-dz
left a comment
There was a problem hiding this comment.
Effective Go Review — PR #60
Scope: interp/allowed_paths.go, interp/builtins/ls/ls.go, interp/builtins/builtins.go, interp/runner_exec.go, tests, and scenario files.
Overall assessment: Has findings — 1 P2, 1 P3. The code is well-structured with thorough documentation, intentional DoS tradeoffs clearly explained, and comprehensive test coverage (unit, pentest, and scenario tests). Good use of the extracted collectDirEntries for testability.
| # | Priority | File | Finding | Rule |
|---|---|---|---|---|
| 1 |  |
interp/allowed_paths.go:262 |
err != io.EOF should use errors.Is |
#51 Sentinel errors |
| 2 |  |
interp/allowed_paths.go:246 |
Slice could pre-allocate with known capacity | #21 Slice init |
Positive Observations
- Excellent doc comments on
collectDirEntriesandreadDirLimitedexplaining the DoS tradeoffs - Good extraction of
collectDirEntrieswith a function callback for testability TestCollectDirEntriescovers the subtle error-during-truncation edge case well- Comprehensive scenario tests with
skip_assert_against_bashcorrectly applied to non-standard flags - Intentional bash divergences are clearly documented in code comments
- Use errors.Is(err, io.EOF) instead of direct comparison, matching the pattern used by all other builtins (tail, cat, head, etc.). - Pre-allocate entries slice with known maxRead capacity to avoid repeated slice growth during append. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex make a comprehensive code and security reviews |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
matt-dz
requested review from
AlexandreYang,
astuyve,
julesmcrt and
thieman
as code owners
2 participants
Summary
-Rrecursion to match GNU ls default behaviour and prevent symlink-loop DoS attacks. UsesDirEntry.Type()(does not follow symlinks) instead of relying onFileInfo.IsDir()aftere.Info()(which follows symlinks).MaxDirEntries(1,000) limit per directory to prevent OOM from directories with millions of entries.lswith--offsetand--limitflags for single-directory, non-recursive listings. Flags are silently ignored with-R.ls -Rdoes not follow symlinks (asserted against bash).Intentional DoS tradeoffs
Three review threads are left intentionally unresolved because they flag behaviour that is a deliberate security tradeoff, not a bug:
Hidden files consume
MaxDirEntriesquota (ls.goreadDir call) — TheMaxDirEntriescap operates on raw directory reads, before hidden-file filtering. If we applied the cap after filtering, an attacker could create a directory with 999,999 dotfiles and 1 visible file — the shell would have to read all entries to find visible ones, defeating the DoS protection entirely. Code comment added in ce6c63e.readDirLimitedtruncates before global sort (allowed_paths.go) — For directories larger thanmaxRead, the returned entries are sorted within the read window but may not be the globally-smallest names. Reading all entries to get globally-correct sorting would defeat the DoS protection — a directory with millions of files would OOM or stall. The truncation warning communicates that output is incomplete. Code comment added in ce6c63e.Test plan
go test ./interp/builtins/ls/ -v -run TestLsPentest— all pentest tests passgo test ./tests/ -v -run "TestShellScenarios/cmd/ls"— all scenario tests passgo test ./interp/... ./tests/... -timeout 120s— full test suite passesRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— bash comparison (requires Docker)🤖 Generated with Claude Code