Move time.Now() into restrictedtime package for sandboxing#113
Move time.Now() into restrictedtime package for sandboxing#113AlexandreYang wants to merge 7 commits intomainfrom
Conversation
…xing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mbols Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rp allowlist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changes SummaryProblem
SolutionReplaced What changedNew
|
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
This PR refactors time-related operations out of builtins (find, ls) into a new restrictedtime package. The key design change is that builtins no longer have access to time.Now() directly — instead, they receive boolean-returning closures (MatchMtime, MatchMmin, IsRecentEnough) via CallContext, with the invocation timestamp captured once inside NewCallbacks().
Overall assessment: Safe to merge with minor observations.
What was reviewed
restrictedtime/timecomp.go— new package with time-comparison logicrestrictedtime/timecomp_test.go— relocated and adapted unit testsbuiltins/builtins.go—CallContextAPI change (Now→ three callback fields)builtins/find/eval.go,find.go— delegation toCallContextcallbacksbuiltins/find/now_test.go— adapted test for new APIbuiltins/ls/ls.go—formatTimenow usesIsRecentEnoughcallbackinterp/runner_exec.go— wiringNewCallbacks()intoCallContextallowedsymbols/— symbol list updates and newrestrictedtimeallowlist
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
restrictedtime/timecomp.go:101 |
NewCallbacks() captures time.Now() per command — no unit test for the closure wiring |
| 2 | |
restrictedtime/timecomp.go:65 |
Missing overflow guard for CmpExact in MatchMmin when diff is extreme |
| 3 | |
builtins/find/now_test.go:73 |
Weakened assertion: test no longer verifies timestamp consistency across root paths |
Finding Details
1. 
No unit test for NewCallbacks() closure wiring
Severity: P3 — Code Quality / Test Coverage
Location: restrictedtime/timecomp.go:96-111
NewCallbacks() is the public API that captures time.Now() and returns closures. The individual functions (MatchMtime, MatchMmin, IsRecentEnough) are well tested with explicit now parameters, but there's no test that verifies NewCallbacks() itself returns working closures that delegate correctly. A simple test calling NewCallbacks() and invoking each returned function would provide coverage of the wiring.
2. MatchMmin CmpExact has no overflow guard for extreme diff
Severity: P3 — Correctness (hardening)
Location: restrictedtime/timecomp.go:78-80
For CmpExact, diff.Minutes() returns a float64, and math.Ceil is applied before converting to int64. If diff is extremely large (e.g., now and modTime are centuries apart), diff.Minutes() could produce a float64 that exceeds int64 range, causing undefined behavior in the int64() cast. The same pattern existed in the old code, so this is not a regression — just a hardening opportunity. In practice, file mtimes won't produce such extreme values.
The same applies to MatchMtime's CmpExact path with diff.Hours() / 24.
3. Weakened test assertion in now_test.go
Severity: P3 — Test Coverage
Location: builtins/find/now_test.go:73
The old test (TestNowCalledOnce) verified that Now() was called exactly once per find invocation — guaranteeing timestamp consistency across multiple root paths. The new test (TestMatchMminCalledConsistently) only checks that MatchMmin is called at least once. The consistency guarantee still holds (because NewCallbacks() captures now once in the runner), but the test no longer verifies this invariant at the find level. This is acceptable since the architecture now guarantees it by construction, but worth noting.
Positive Observations
-
Excellent security hardening — Moving
time.Now()out of builtins and into a controlled package with its own symbol allowlist is a strong defense-in-depth measure. Builtins can no longer access wall-clock time directly. -
Clean separation of concerns — The
restrictedtimepackage has a minimal, well-scoped allowlist. Thetime.Nowcall is isolated to a singleNewCallbacks()function. -
Behavioral preservation — The time-comparison logic was moved verbatim from
find/eval.gotorestrictedtime/timecomp.go. The tests were faithfully migrated and all pass. -
Overflow guards preserved —
MaxMtimeNandMaxMminNconstants and their guards were correctly moved. -
Symbol allowlist enforcement — The new
allowedsymbols/symbols_restrictedtime.goand its verification tests ensure therestrictedtimepackage can't silently add dangerous imports. -
cmpOpvalues alignment — TheCmpLess/CmpExact/CmpMoreconstants inrestrictedtime(-1/0/1) exactly matchfind'scmpOpvalues, and theint(cmp)cast inevalMtime/evalMminis correct.
Coverage Summary
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
| MatchMmin ceiling bucketing | find mmin scenarios | restrictedtime/timecomp_test.go:TestMatchMminCeiling |
Covered |
| MatchMmin overflow | find mtime_int64_overflow scenario | restrictedtime/timecomp_test.go:TestMatchMminOverflow |
Covered |
| MatchMtime floor bucketing | find mtime scenarios | restrictedtime/timecomp_test.go:TestMatchMtimeFloor |
Covered |
| IsRecentEnough | ls long_format scenarios | restrictedtime/timecomp_test.go:TestIsRecentEnough |
Covered |
| NewCallbacks() wiring | — | — | Not covered (P3) |
| Symbol allowlist enforcement | — | allowedsymbols/symbols_restrictedtime_verification_test.go |
Covered |
Overall coverage: Adequate — one minor gap noted above.
|
Iteration 1 self-review result: COMMENT
|
- Add TestNewCallbacks to verify NewCallbacks() closure wiring - Strengthen TestMatchMminCalledConsistently to assert MatchMmin is called at least once per file across both root paths, with a comment explaining timestamp consistency is guaranteed by construction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 2)
Re-reviewed the full diff after the previous review identified 3 P3 findings. All have been addressed or were acceptable as-is.
Overall assessment: Safe to merge.
Previous Findings Status
| # | Previous Finding | Status |
|---|---|---|
| 1 | P3: No unit test for NewCallbacks() closure wiring |
Fixed — TestNewCallbacks added in restrictedtime/timecomp_test.go |
| 2 | P3: MatchMmin CmpExact no overflow guard for extreme diff |
Acceptable — pre-existing behavior, not a regression, impractical to trigger with real file mtimes |
| 3 | P3: Weakened test assertion in now_test.go |
Acceptable — timestamp consistency is now guaranteed by construction (closures capture now once in NewCallbacks()) |
New Findings
No new issues found on re-review.
Verification
- All tests pass:
restrictedtime/,builtins/find/,builtins/ls/,allowedsymbols/ time.Now()is not called directly in any builtin production codecmpOpvalues (-1/0/1) align correctly betweenfindandrestrictedtimeIsRecentEnoughlogic is equivalent to the oldformatTimecondition- Symbol allowlists are correctly updated (removed from builtins/interp, added to restrictedtime)
- No remaining references to the old
CallContext.Nowfield
Positive Observations
- Clean security hardening — builtins can no longer access wall-clock time directly; they only receive boolean callbacks.
- Faithful logic migration — comparison logic moved verbatim with all overflow guards preserved.
- Strong test coverage — the new
TestNewCallbackstest addresses the wiring gap from iteration 1. - Well-scoped allowlist —
restrictedtimehas its own minimal symbol allowlist with verification tests.
Coverage Summary
| Code path | Test | Status |
|---|---|---|
| MatchMmin ceiling bucketing | TestMatchMminCeiling |
Covered |
| MatchMmin overflow | TestMatchMminOverflow |
Covered |
| MatchMtime floor bucketing | TestMatchMtimeFloor |
Covered |
| IsRecentEnough | TestIsRecentEnough |
Covered |
| NewCallbacks() wiring | TestNewCallbacks |
Covered (new) |
| Symbol allowlist enforcement | TestTimecompAllowedSymbols + verification tests |
Covered |
|
Iteration 2 self-review result: APPROVE (no new findings)
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 3 self-review result: APPROVE (no findings)
|
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
|
Iteration 4 self-review result: APPROVE (no blocking findings)
|
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
AlexandreYang
changed the title
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz and
thieman
as code owners
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 5 — stability check)
Reviewed the extraction of time.Now() from builtins into the new restrictedtime package. This is a pure security-hardening refactor that confines wall-clock access to a single call site.
Overall assessment: safe to merge — no findings. (Note: cannot submit APPROVE on own PR; this would be an APPROVE.)
What was reviewed
restrictedtime/timecomp.go— new package withMatchMtime,MatchMmin,IsRecentEnoughfunctions andNewCallbacks()factorybuiltins/builtins.go—CallContextAPI change fromNow func() time.Timeto three boolean-returning callbacksbuiltins/find/eval.go— delegation of-mtime/-mmintoCallContextcallbacksbuiltins/find/find.go— removal ofnowplumbing throughwalkOptionsandwalkPathbuiltins/ls/ls.go—formatTimenow usesIsRecentEnoughcallbackinterp/runner_exec.go—NewCallbacks()wiring inr.call()allowedsymbols/— symbol list migration and newrestrictedtimeallowlist with verification tests- Tests — migrated and new unit tests in
restrictedtime/timecomp_test.go, updatedfind/now_test.go
Security
time.Now()is now confined torestrictedtime.NewCallbacks()— builtins cannot obtain raw wall-clock time.NewCallbacks()is called once per builtin invocation inr.call(), ensuring consistent timestamps across multi-rootfindinvocations.- No new imports of unsafe/restricted packages. The
restrictedtimepackage only usesmathandtime. - Symbol allowlist is properly enforced via
TestTimecompAllowedSymbolsand verification tests. - Builtins do not import
restrictedtimedirectly (verified by grep).
Correctness
- The
cmpOpvalues (-1,0,1) infind/expr.goexactly matchCmpLess,CmpExact,CmpMoreinrestrictedtime— theint(cmp)conversion is safe. - The time-comparison logic is a direct code move with no behavioral changes.
- Overflow guards (
MaxMtimeN,MaxMminN) are preserved. IsRecentEnoughcorrectly replicates the previousformatTimelogic (6-month cutoff + future detection).
Test Coverage
| Code path | Test | Status |
|---|---|---|
| MatchMmin ceiling bucketing | restrictedtime/timecomp_test.go:TestMatchMminCeiling |
Covered |
| MatchMmin overflow | restrictedtime/timecomp_test.go:TestMatchMminOverflow |
Covered |
| MatchMtime floor bucketing | restrictedtime/timecomp_test.go:TestMatchMtimeFloor |
Covered |
| IsRecentEnough | restrictedtime/timecomp_test.go:TestIsRecentEnough |
Covered |
| NewCallbacks wiring | restrictedtime/timecomp_test.go:TestNewCallbacks |
Covered |
| Consistent invocation across roots | find/now_test.go:TestMatchMminCalledConsistently |
Covered |
| Symbol allowlist enforcement | allowedsymbols/symbols_restrictedtime_test.go |
Covered |
| Symbol allowlist verification | allowedsymbols/symbols_restrictedtime_verification_test.go |
Covered |
All tests pass locally (go test on all 4 affected packages).
Positive Observations
- Clean separation of concerns: builtins receive only boolean-returning closures, eliminating the ability to leak raw time values.
- Comprehensive allowlist verification tests that detect both unlisted symbols and exempt internal imports.
- The refactor preserves all existing GNU find/ls behavioral semantics.
- Overflow guards were correctly migrated, not lost.
Iteration 5 stability check: consistent with previous 4 approvals. No new concerns.
|
Iteration 5 self-review result: APPROVE (no findings)
|
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 6 — Stability Check)
Reviewed: 14 changed files extracting time.Now() from builtins into a dedicated restrictedtime package.
Overall assessment: APPROVE (submitted as COMMENT due to GitHub self-review restriction)
What was reviewed
-
Security: The core change confines
time.Now()to a single call site inrestrictedtime.NewCallbacks(). Builtins (find,ls) can no longer obtain raw wall-clock time — they only receive boolean-returning closures viaCallContext. The allowlist changes are consistent:time.Nowremoved from interp,math.Ceil/math.Floor/time.Duration/time.Hour/time.Minute/time.Secondremoved from builtins (moved torestrictedtime). No new filesystem access, no unsafe imports, no sandbox bypasses. -
Correctness:
MatchMtimepreserves GNU find’s floor-bucketed day semantics for exact match and second-precision truncation for+N/-N.MatchMminpreserves ceiling-bucketed minute semantics for exact match and raw-second comparison for+N/-N.IsRecentEnoughcorrectly handles the 6-month window boundary and future dates. Overflow guards (MaxMtimeN,MaxMminN) prevent integer overflow on large N values. -
Bash compatibility: No behavioral change — the time comparison logic is unchanged, only relocated. The
cmpOpvalues (-1,0,1) map correctly toCmpLess/CmpExact/CmpMoreconstants. -
Test coverage: Comprehensive unit tests in
restrictedtime/timecomp_test.gocover ceiling rounding, floor rounding, overflow, boundary conditions, and theNewCallbacksfactory.find/now_test.goverifies the delegation integration. All tests pass locally. -
Allowlist integrity:
symbols_restrictedtime.gocorrectly lists only the symbols needed by the new package. Verification tests enforce the allowlist cannot be circumvented.time.Nowproperly removed from interp allowlist sincerunner_exec.gonow calls it viarestrictedtime.NewCallbacks(). -
Platform compatibility: No platform-specific code introduced; the change is pure Go with no OS dependencies.
Positive Observations
- Clean separation of concerns: builtins cannot reconstruct wall-clock time from the boolean callbacks
NewCallbacks()capturestime.Now()once per command invocation, ensuring consistent time reference across all predicates- Overflow protection is thorough with
MaxMtimeNandMaxMminNconstants - The allowlist enforcement tests verify the new package cannot sneak in unauthorized imports
- Good test coverage with both unit tests and integration tests
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings |
|
Iteration 6 self-review result: APPROVE (no findings)
|
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 7 — Stability Check)
Verdict: APPROVE (submitted as COMMENT due to GitHub self-review restriction)
Reviewed all 14 changed files across allowedsymbols/, builtins/find/, builtins/ls/, interp/, and the new restrictedtime/ package.
Overall Assessment: Safe to merge
This is a well-executed security hardening change that extracts time.Now() from builtins into a dedicated restrictedtime package. The key security properties are:
time.Now()is confined to a single call site (restrictedtime.NewCallbacks()), captured once per command invocation.- Builtins only receive boolean-returning closures — they cannot obtain raw wall-clock time.
- Allowlist changes are correct —
time.Now,time.Duration,time.Hour,time.Minute,time.Second,math.Ceil,math.Floorremoved from builtins allowlist;time.Nowadded only torestrictedtime-specific allowlist. - No new unsafe imports —
restrictedtimeonly usesmathandtime. - Overflow protection preserved —
MaxMtimeNandMaxMminNconstants with proper guards. - Test coverage is adequate — unit tests for
MatchMtime,MatchMmin,IsRecentEnough,NewCallbacks, overflow cases, and allowlist verification tests all present. - GNU find semantics preserved — floor bucketing for
-mtime, ceiling bucketing for-mmin, truncation to second precision for+N/-Ncomparisons.
Findings
No P0, P1, P2, or P3 findings.
Positive Observations
- The callback pattern (
NewCallbacksreturning closures) is a clean design that enforces the security boundary at the type level — builtins physically cannot accesstime.Now(). - The
restrictedtimeallowlist verification tests (TestVerificationTimecomp*) provide defense-in-depth: even if someone adds a new import to the package, the allowlist check will catch it. - Overflow guards for large
-mtime/-mminvalues prevent integer overflow without panicking. - The single
time.Now()capture per invocation ensures consistent time reference across multiple root paths infind.
|
Iteration 7 self-review result: APPROVE (no findings) |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed all 14 changed files across the restrictedtime package, builtins/find, builtins/ls, interp/runner_exec.go, and allowedsymbols/.
Overall assessment: safe to merge (APPROVE — submitted as COMMENT because GitHub prevents approving own PR)
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings |
Security
This PR strengthens the sandbox by confining time.Now() to a single call site in restrictedtime.NewCallbacks(). Previously, time.Now was in the interp allowlist and builtins had direct access to time-comparison math symbols (math.Ceil, math.Floor, time.Duration, time.Hour, time.Minute, time.Second). Now:
- Builtins receive only boolean-returning closures (
MatchMtime,MatchMmin,IsRecentEnough) viaCallContext— they cannot obtain raw wall-clock time. time.Nowis removed from the interp allowlist and confined torestrictedtimeAllowedSymbols.- The
restrictedtimepackage has its own dedicated allowlist enforced byTestTimecompAllowedSymbols. - Verified: no
time.Now()calls remain in any builtin production code.
Correctness
MatchMtime,MatchMmin, andIsRecentEnoughlogic is preserved identically (moved from find/eval.go to restrictedtime/timecomp.go).NewCallbacks()capturestime.Now()once per command invocation (runner_exec.go:269), ensuring consistent time reference across all predicates evaluated during a singlefindorlsexecution.evalMtime/evalMminin find properly delegate throughCallContextcallbacks.formatTimein ls correctly uses theIsRecentEnoughcallback.- Overflow guards (
MaxMtimeN,MaxMminN) are preserved with the same logic.
Test Coverage
Adequate coverage across all changed code paths:
| Code path | Test | Status |
|---|---|---|
| MatchMmin ceiling bucketing | restrictedtime/timecomp_test.go:TestMatchMminCeiling |
Covered |
| MatchMmin overflow | restrictedtime/timecomp_test.go:TestMatchMminOverflow |
Covered |
| MatchMtime floor bucketing | restrictedtime/timecomp_test.go:TestMatchMtimeFloor |
Covered |
| IsRecentEnough boundary | restrictedtime/timecomp_test.go:TestIsRecentEnough |
Covered |
| NewCallbacks closure wiring | restrictedtime/timecomp_test.go:TestNewCallbacks |
Covered |
| find -mmin delegation | builtins/find/now_test.go:TestMatchMminCalledConsistently |
Covered |
| Allowlist enforcement | allowedsymbols/symbols_restrictedtime_test.go |
Covered |
| Allowlist verification (clean/unlisted/exempt) | allowedsymbols/symbols_restrictedtime_verification_test.go |
Covered |
Positive Observations
- Clean separation of concerns: time-comparison logic in its own package with its own allowlist.
- The callback pattern (boolean-returning closures) is an effective defense-in-depth measure — even if a builtin is compromised, it cannot extract the current time.
NewCallbacks()capturestime.Now()once, ensuring consistent time reference and preventing TOCTOU issues in predicates like-mminevaluated across many files.- Thorough verification tests for the allowlist itself (clean pass, unlisted symbol detection, exempt import handling).
- All tests pass locally.
|
Iteration 8 self-review result: APPROVE (no findings) |
|
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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the extraction of time.Now() from builtins into a dedicated restrictedtime package. This PR strengthens the sandbox by confining wall-clock access to a single call site, replacing the previous CallContext.Now() function with three boolean-returning closures (MatchMtime, MatchMmin, IsRecentEnough).
Overall assessment: safe to merge (would APPROVE but cannot approve own PR)
No P0, P1, P2, or P3 findings.
Security
time.Now()confinement is correct. The only call site isrestrictedtime.NewCallbacks(), invoked once per command ininterp/runner_exec.go. Builtins cannot import therestrictedtimepackage (blocked by the builtin import allowlist) and receive only boolean-returning closures viaCallContext.- No raw time leakage. The
nowvalue captured inNewCallbacks()is a closure-local variable; builtins cannot extract it. - Allowlist changes are correct.
time.Nowmoved frominterpAllowedSymbolstorestrictedtimeAllowedSymbols.math.Ceil,math.Floor,time.Duration,time.Hour,time.Minute,time.Secondremoved from builtin allowlists and added to the restrictedtime allowlist. No new capabilities granted to builtins. - No filesystem access outside sandbox. No new
os.Open,os.Stat, etc. calls introduced.
Bash Compatibility
MatchMtimelogic is a direct extraction fromevalMtime— same floor-bucketing for exact, same truncation to second precision for +N/-N, same overflow guards.MatchMminlogic is a direct extraction fromevalMmin— same ceiling-bucketing for exact, same raw-second comparison for +N/-N, same overflow guards.IsRecentEnoughinlsis semantically equivalent to the oldformatTimelogic:!modTime.Before(cutoff) && !modTime.After(now)=== old!(t.Before(sixMonthsAgo) || t.After(now)).
Correctness
NewCallbacks()capturestime.Now()once, ensuring consistent timestamps across all predicate evaluations within a single command invocation (same guarantee as the oldcallCtx.Now()approach).- The
cmpOptointconversion at the call site (int(cmp)) correctly maps the find-internal constants to the restrictedtime constants (CmpLess=-1, CmpExact=0, CmpMore=1). - The
nowfield removed fromevalContextandwalkOptions— no remaining references. Clean removal.
Test Coverage
| Code path | Test | Status |
|---|---|---|
| MatchMmin ceiling bucketing | restrictedtime/timecomp_test.go:TestMatchMminCeiling |
Covered |
| MatchMmin overflow | restrictedtime/timecomp_test.go:TestMatchMminOverflow |
Covered |
| MatchMtime floor bucketing | restrictedtime/timecomp_test.go:TestMatchMtimeFloor |
Covered |
| NewCallbacks closure wiring | restrictedtime/timecomp_test.go:TestNewCallbacks |
Covered |
| IsRecentEnough boundary cases | restrictedtime/timecomp_test.go:TestIsRecentEnough |
Covered |
| find delegates to MatchMmin | builtins/find/now_test.go:TestMatchMminCalledConsistently |
Covered |
| restrictedtime allowlist | allowedsymbols/symbols_restrictedtime_test.go |
Covered |
| restrictedtime verification | allowedsymbols/symbols_restrictedtime_verification_test.go |
Covered |
| MatchMtime overflow guards | — | Pre-existing gap (not introduced by this PR) |
All tests pass locally (go test ./restrictedtime/ ./builtins/find/ ./builtins/ls/ ./allowedsymbols/ ./tests/).
Positive Observations
- Excellent security architecture: builtins can only ask boolean questions about file times, never obtain raw
time.Timevalues. - Clean separation of concerns with the
restrictedtimepackage having its own import allowlist and verification tests. - The
NewCallbacks()factory pattern is elegant — capturestime.Now()once and returns typed closures, making it impossible for builtins to calltime.Now()repeatedly or obtain the captured instant. - Thorough test migration: all time-comparison tests moved from
find/eval_test.gotorestrictedtime/timecomp_test.gowith newTestNewCallbacksandTestIsRecentEnoughtests added.
|
Iteration 9 self-review result: APPROVE (no findings) |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
|
@codex review this PR Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary (Iteration 10 — final stability check)
All previous iterations APPROVE. This iteration confirms: safe to merge.
Reviewed the extraction of time.Now() from builtins into a dedicated restrictedtime package. This is a well-structured security hardening change that confines wall-clock access to a single call site.
Overall Assessment: Safe to merge (no P0/P1/P2 findings)
What was reviewed
restrictedtime/timecomp.go— new package withMatchMtime,MatchMmin,IsRecentEnoughfunctions andNewCallbacks()factorybuiltins/builtins.go—CallContextAPI change:Now func() time.Timereplaced with three boolean-returning callbacksbuiltins/find/eval.go—evalMtime/evalMminnow delegate toCallContextcallbacksbuiltins/find/find.go— removednowcapture and threading throughwalkOptions/evalContextbuiltins/ls/ls.go—formatTimenow usesIsRecentEnoughcallback instead of direct time comparisoninterp/runner_exec.go— wiresrestrictedtime.NewCallbacks()intoCallContextallowedsymbols/—time.Nowmoved from interp to restrictedtime;math.Ceil/math.Floor/time.Duration/etc. moved from builtins to restrictedtime- Tests — unit tests migrated correctly;
TestMatchMminCalledConsistentlyreplacesTestNowCalledOnce; verification tests for symbol allowlist
Security Analysis
- Sandbox integrity: Strengthened. Builtins can no longer call
time.Now()or obtain raw wall-clock values. They can only ask boolean questions about file modification times via closures. - time.Now() confinement: Single call site in
NewCallbacks()captures the timestamp once per command invocation — matches GNU find's single-snapshot semantics. - No new imports in builtins: The
findbuiltin dropped itsmathimport;lsdropped itsnowparameter threading. Neither importsrestrictedtimedirectly. - Allowlist enforcement: New
restrictedtimeAllowedSymbolslist is properly tested with verification tests (clean pass, unlisted symbol, exempt import). - cmpOp to int mapping:
find'scmpLess/cmpExact/cmpMorevalues (-1/0/1) matchrestrictedtime'sCmpLess/CmpExact/CmpMoreexactly. Theint(cmp)conversion inevalMtime/evalMminis safe.
Correctness Analysis
MatchMtime/MatchMminlogic is a direct move frombuiltins/find/eval.gowith the same overflow guards, truncation behavior, and bucketing semantics.IsRecentEnoughlogic correctly inverts the oldformatTimecondition: oldt.Before(sixMonthsAgo) || t.After(now)maps to new!modTime.Before(cutoff) && !modTime.After(now).NewCallbacks()capturestime.Now()once and returns closures — each command invocation gets a consistent timestamp.
Test Coverage
| Code path | Test | Status |
|---|---|---|
MatchMmin ceiling bucketing |
restrictedtime/TestMatchMminCeiling |
Covered |
MatchMmin overflow |
restrictedtime/TestMatchMminOverflow |
Covered |
MatchMtime floor bucketing |
restrictedtime/TestMatchMtimeFloor |
Covered |
NewCallbacks wiring |
restrictedtime/TestNewCallbacks |
Covered |
IsRecentEnough |
restrictedtime/TestIsRecentEnough |
Covered |
MatchMmin delegation from find |
find/TestMatchMminCalledConsistently |
Covered |
MatchMtime overflow |
— | Pre-existing gap (P3) |
| Allowlist enforcement | allowedsymbols/TestTimecompAllowedSymbols + verification tests |
Covered |
Positive Observations
- Clean separation of concerns: time logic is isolated in a dedicated package with its own symbol allowlist
- The boolean-callback pattern effectively prevents builtins from extracting wall-clock information
- Tests were migrated faithfully — same test cases, just with updated function signatures
- Verification tests for the allowlist catch both unlisted symbols and exempt internal imports
- All tests pass locally
Minor Note (not a finding)
MatchMtime overflow handling (MaxMtimeN guard) is not explicitly unit-tested, unlike MatchMmin overflow. This is a pre-existing gap that predates this PR.
|
Iteration 10 self-review result: APPROVE (no findings) |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
Review-Fix Loop Summary
Iteration log
Final state
Remaining issuesNone. PR is clean and ready for human review. |
1 participant
Summary
Extracts
time.Now()out of builtins into a newrestrictedtimepackage to strengthen the sandbox. Builtins no longer have direct access to wall-clock time — they receive boolean-returning closures (MatchMtime,MatchMmin,IsRecentEnough) viaCallContext.Changes
restrictedtimepackage — containsMatchMtime,MatchMmin,IsRecentEnoughfunctions and aNewCallbacks()factory that capturestime.Now()once per command invocationCallContextAPI change — replacedNow func() time.Timewith three callback fields:MatchMtime,MatchMmin,IsRecentEnoughfindbuiltin — delegates-mtime/-mminevaluation toCallContextcallbacks instead of computing time comparisons directlylsbuiltin —formatTimeusesIsRecentEnoughcallback instead of direct time comparisontime.Nowmoved from interp allowlist torestrictedtime-specific allowlist; time/math symbols removed from builtins allowlist (no longer needed)find/eval_test.gotorestrictedtime/timecomp_test.go; addedTestNewCallbacksfor closure wiring; strengthenedTestMatchMminCalledConsistentlySecurity
time.Now()is now confined to a single call site inrestrictedtime.NewCallbacks(). Builtins cannot obtain the raw current time — they can only ask boolean questions about file modification times relative to the invocation timestamp.