fix(cron): use user-specific log directory instead of /tmp on macOS#28299
fix(cron): use user-specific log directory instead of /tmp on macOS#28299robobun wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSet cron job WorkingDirectory to the script’s directory (import.meta.dir semantics); change macOS stdout/stderr log paths from Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward security fix moving macOS cron logs from world-readable /tmp/ to user-specific ~/Library/Logs/bun/cron/.
Extended reasoning...
Overview
This PR modifies three files: src/bun.js/api/cron.zig (core logic to create and use ~/Library/Logs/bun/cron/ for launchd plist log paths), docs/runtime/cron.mdx (documentation update), and test/js/bun/cron/cron.test.ts (test expectations updated). The change is ~20 lines of new Zig code plus mechanical updates to docs and tests.
Security risks
This PR improves security by moving log files out of the globally shared /tmp/ directory into a user-specific location. The new path ~/Library/Logs/bun/cron/ follows macOS conventions (similar to the existing ~/Library/Caches/bun/ usage for transpiler cache). The xml_log_dir value is properly XML-escaped before embedding in the plist template. No new security risks introduced.
Level of scrutiny
Low scrutiny needed. The change is small, follows established patterns already present in the same function (directory creation with makePath, xmlEscape, format string construction), and the format string arguments (10 total) correctly match the 10 {s} placeholders in the plist template. Memory management uses the same defer/free pattern as surrounding code.
Other factors
No prior reviewer comments to address. Tests pass per the PR description (54 pass, 0 fail). The macOS CI checkbox is unchecked but that is expected for a PR that just landed. No CODEOWNERS concerns for these files. The claude label indicates this was AI-generated, but the change is mechanical and correct.
|
@robobun adopt |
|
⏳ CI infrastructure outage — all 4 darwin test jobs expired (runners unavailable), all Linux jobs failing across every PR in the repo. Not code-related. Will retry when CI stabilizes. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
While we're at it, can we also make it set the cwd to import.meta.dir?
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from world-readable /tmp to user-specific ~/Library/Logs/bun/cron/, with proper XML escaping, updated tests, and docs.
Extended reasoning...
Overview
This PR changes macOS cron log file paths from /tmp/bun.cron.<title>.{stdout,stderr}.log to ~/Library/Logs/bun/cron/bun.cron.<title>.{stdout,stderr}.log, and adds a WorkingDirectory plist key set to the script's directory. Three files are modified: cron.zig (implementation), cron.test.ts (test updates), and cron.mdx (documentation).
Security risks
This PR is itself a security improvement — /tmp/ is globally shared and world-readable, so moving logs to a user-specific directory under ~/Library/Logs/ mitigates information disclosure and tampering risks. The new directory is created with makePath following the same pattern already used for ~/Library/LaunchAgents. All new dynamic values (log_dir, script_dir) are properly XML-escaped before embedding in the plist. No new security concerns introduced.
Level of scrutiny
This is a low-risk, well-contained change. The implementation follows existing patterns in the same file (directory creation + XML escaping + format string). I verified the format string argument count and ordering (11 placeholders, 11 arguments, correctly mapped). The change is macOS-only and doesn't affect Linux or Windows code paths.
Other factors
No bugs were found by the automated bug hunting system. CodeRabbit also found no actionable issues. Tests are updated to assert the new paths, including a new test for WorkingDirectory. A maintainer (alii) initiated the adoption of this PR. The test plan shows 54 pass / 0 fail locally.
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from world-readable /tmp to user-specific ~/Library/Logs/bun/cron/, with matching WorkingDirectory addition.
Extended reasoning...
Overview
This PR modifies 3 files: src/bun.js/api/cron.zig (implementation), test/js/bun/cron/cron.test.ts (tests), and docs/runtime/cron.mdx (documentation). The changes move macOS launchd cron log files from /tmp/ to ~/Library/Logs/bun/cron/ and add a WorkingDirectory key to the generated plist set to the script's parent directory.
Security risks
This PR improves security by eliminating the use of the globally shared /tmp/ directory for cron log output, which was world-readable and could allow other users to read or tamper with cron job output. The new ~/Library/Logs/bun/cron/ path is user-specific and follows macOS conventions. No new security risks are introduced.
Level of scrutiny
The implementation change is small and mechanical: creating a new directory, XML-escaping the path, and inserting it into the plist template. The format string has 11 placeholders matching 11 arguments in the correct order. All new allocations have corresponding defer free statements. The pattern follows the existing launch_agents_dir creation exactly. This warrants standard review but is low-risk.
Other factors
- Tests were updated to assert the new log paths and a new test was added for WorkingDirectory
- Documentation was updated consistently
- CodeRabbit found no issues
- No bugs were found by the bug hunting system
- No outstanding reviewer comments to address
- The
xmlEscapefunction always returns a fresh allocation (viadupeortoOwnedSlice), so no double-free risk with thedefer freepattern
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
93dc9f6 to
6040b75
Compare
There was a problem hiding this comment.
LGTM — straightforward security fix moving logs from /tmp to ~/Library/Logs/bun/cron/, with proper error handling and tests.
Extended reasoning...
Overview
This PR changes 3 files: src/bun.js/api/cron.zig (macOS plist generation), test/js/bun/cron/cron.test.ts (test updates), and docs/runtime/cron.mdx (documentation). It moves macOS cron log files from the world-readable /tmp/ to the user-specific ~/Library/Logs/bun/cron/ directory, and adds a WorkingDirectory key to the launchd plist pointing to the script's directory.
Security risks
This PR improves security by moving logs out of /tmp/ which is globally readable/writable. The new location ~/Library/Logs/bun/cron/ is user-scoped, following macOS conventions (similar to how Bun already uses ~/Library/Caches/bun/). All dynamic values embedded in the plist XML continue to go through xmlEscape(), preventing injection. No new security risks introduced.
Level of scrutiny
The implementation changes are mechanical and follow the exact same patterns already used in the function: allocPrint + defer free, xmlEscape + defer free, makePath for directory creation. The format string has 11 placeholders matching 11 arguments in the correct order (verified). The new WorkingDirectory derivation uses bun.path.dirname with a fallback to / for empty paths. This is low-risk, pattern-following code.
Other factors
- No CODEOWNERS match for these files
- CodeRabbit found no issues
- Bug hunting system found no bugs
- Tests are updated (log path assertions) and a new test for WorkingDirectory is added
- Documentation is updated to reflect the new paths and working directory behavior
- The PR has been rebased on latest main; previous CI failures were unrelated to this change
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from world-readable /tmp to ~/Library/Logs/bun/cron/, with matching doc/test updates.
Extended reasoning...
Overview
This PR modifies three files: src/bun.js/api/cron.zig (macOS plist generation), test/js/bun/cron/cron.test.ts (test expectations and cleanup paths), and docs/runtime/cron.mdx (documentation). The changes move macOS cron log file paths from /tmp/bun.cron.<title>.{stdout,stderr}.log to ~/Library/Logs/bun/cron/bun.cron.<title>.{stdout,stderr}.log and add a WorkingDirectory key to the generated launchd plist.
Security risks
This PR is itself a security fix — it eliminates writing cron logs to the globally shared /tmp/ directory where other users could read or tamper with them. The new location ~/Library/Logs/bun/cron/ is user-specific. The directory is created with makePath (inheriting default permissions). All dynamic values embedded in the plist XML are properly XML-escaped via the existing xmlEscape helper. No new security risks are introduced.
Level of scrutiny
The core logic change is small: adding ~30 lines of Zig to create a log directory, derive the script directory, XML-escape both, and update the format string. I verified the 11 format string placeholders match the 11 tuple arguments exactly. The pattern follows the same error-handling style used throughout the function (allocate, check error, setErr, finish, return). This warrants a standard review but is not high-risk.
Other factors
Tests are updated to expect the new paths and a new test validates the WorkingDirectory plist key. The e2e test cleanup also uses the new paths. Documentation is updated consistently. No outstanding reviewer comments. A maintainer adopted the PR. No bugs were found by the automated bug hunting system.
|
✅ CI Build #41394 — 52/53 pass. All macOS ✅, all Windows ✅, all debian/ubuntu/alpine test runners ✅, ASAN ✅. Single failure: |
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from /tmp to ~/Library/Logs/bun/cron/ with proper error handling, XML escaping, and tests.
Extended reasoning...
Overview
This PR modifies 3 files: src/bun.js/api/cron.zig (plist generation), test/js/bun/cron/cron.test.ts (test updates), and docs/runtime/cron.mdx (doc updates). The scope is limited to macOS launchd plist generation — it moves StandardOutPath/StandardErrorPath from /tmp/ to ~/Library/Logs/bun/cron/ and adds a WorkingDirectory key derived from the script's directory.
Security risks
This PR fixes a security issue (world-readable /tmp/ log files). The new path is under ~/Library/Logs/ which is user-specific. All dynamic values embedded in the plist XML are properly escaped via xmlEscape(). No new attack surface is introduced. The format string has 11 placeholders matching 11 arguments in the correct order — verified manually.
Level of scrutiny
This is a low-risk, well-scoped change. The macOS plist generation code follows established patterns already present in the file (XML escaping, allocation/deallocation with defer, error handling). The changes are mechanical: create a new directory, derive paths, pass them through the existing template. Tests are updated to assert the new paths, and a new test verifies the WorkingDirectory key.
Other factors
Maintainer (alii) has adopted the PR. CodeRabbit found no issues. The bug hunting system found no bugs. All new allocations have corresponding defer free statements. The fallback for empty script_dir to / is reasonable. CI passed on macOS/Windows (only failure was unrelated ASAN OOM).
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
656e017 to
019e123
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/cron.zig`:
- Around line 311-315: The code is using bun.FD.cwd().makePath(u8, log_dir) to
create directories recursively; replace this call with the repository-standard
bun.makePath(bun.FD.cwd(), log_dir) to align with guidelines. Locate the snippet
that calls bun.FD.cwd().makePath in the cron code and swap it to
bun.makePath(...) while preserving the existing error handling (this.setErr(...)
and this.finish() on catch) so behavior and messages remain unchanged.
In `@test/js/bun/cron/cron.test.ts`:
- Around line 1067-1070: The test compares raw filesystem paths (logDir) against
plist XML, which will fail if HOME or temp paths contain XML-sensitive chars;
update the assertions to XML-escape the expected path strings before matching
(escape &, <, >, " and ') and use the escaped values in the
expect(...).toContain checks for `<string>...</string>` for the stdout and
stderr assertions (and likewise for the similar assertions around the other
block referenced at 1083-1084); reference the variables plist and logDir in
cron.test.ts and perform escaping via a small helper (e.g., xmlEscape or
equivalent) used where the expectations are constructed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 365d0ae0-ffcb-4c61-b2de-7d5f207c9824
📒 Files selected for processing (3)
docs/runtime/cron.mdxsrc/bun.js/api/cron.zigtest/js/bun/cron/cron.test.ts
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from /tmp to ~/Library/Logs/bun/cron/ with proper directory creation, XML escaping, and test coverage.
Extended reasoning...
Overview
This PR changes 3 files: src/bun.js/api/cron.zig (macOS plist generation), test/js/bun/cron/cron.test.ts (updated assertions), and docs/runtime/cron.mdx (updated log paths in docs). The zig changes create ~/Library/Logs/bun/cron/ at registration time, XML-escape the path, derive the script directory for a new WorkingDirectory plist key, and update the format string from 8 to 11 arguments. All new allocations have corresponding defer free calls.
Security risks
This PR improves security by moving log files out of the globally shared /tmp/ directory (which is world-readable) into a user-specific ~/Library/Logs/bun/cron/ path. The directory is created with default permissions inherited from ~/Library/. Dynamic values embedded in the plist XML are properly escaped via xmlEscape. No new attack surface is introduced.
Level of scrutiny
This is a low-risk, well-scoped change. The core logic is mechanical: updating path strings in the plist template and creating the target directory. The format string placeholder count (11 {s} placeholders matching 11 tuple arguments) has been independently verified. The change follows existing patterns — Bun already uses ~/Library/Caches/bun/ for transpiler cache.
Other factors
Maintainer alii approved. Tests are updated to assert the new paths and would fail on main. A new test for WorkingDirectory is added. CodeRabbit pre-merge checks all passed. No unresolved review comments. No bugs were found by the bug hunting system.
There was a problem hiding this comment.
LGTM — straightforward security fix moving cron logs from /tmp to ~/Library/Logs/bun/cron/ with proper error handling, XML escaping, and test coverage.
Extended reasoning...
Overview
This PR touches 3 files: src/bun.js/api/cron.zig (implementation), test/js/bun/cron/cron.test.ts (test updates), and docs/runtime/cron.mdx (documentation). The changes move macOS cron log files from the globally shared /tmp/ directory to the user-specific ~/Library/Logs/bun/cron/ directory, and add a WorkingDirectory key to the generated launchd plist pointing to the script's directory.
Security risks
This PR fixes a security issue (world-readable /tmp/ log files) rather than introducing one. The new path ~/Library/Logs/bun/cron/ is user-owned and follows macOS conventions. All dynamic values interpolated into the plist XML are properly XML-escaped via the existing xmlEscape() helper. No new attack surface is introduced.
Level of scrutiny
The core change is mechanical: swapping /tmp/ for a user-specific log directory and adding a WorkingDirectory plist key. The most error-prone aspect — the format string with 11 placeholders matching 11 arguments — has been manually verified correct. All new allocations have corresponding defer free statements. The change follows established patterns in the file (e.g., bun.FD.cwd().makePath used identically 5 lines above for launch_agents_dir). This warrants standard review, not elevated scrutiny.
Other factors
- Maintainer
aliihas already approved - All CodeRabbit review threads resolved (minor nits about
makePathstyle and XML escaping in tests, both correctly dismissed by the author) - No bugs found by the bug hunting system
- Tests updated to assert new paths (would fail on main), plus a new WorkingDirectory test added
- CI passing on macOS platforms
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
63d55d1 to
6da72c3
Compare
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
c93a3d5 to
e7080d2
Compare
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
d67453f to
7b90a41
Compare
There was a problem hiding this comment.
The memory leak I flagged in spawnBootstrap() has been fixed. LGTM.
Extended reasoning...
Overview
This PR moves macOS cron log files from the world-readable /tmp/ directory to ~/Library/Logs/bun/cron/, adds a WorkingDirectory key to the launchd plist (set to the script's directory), and fixes a pre-existing memory leak in spawnBootstrap(). Changes span src/bun.js/api/cron.zig, the test file, and documentation. The s3-fd-validation.test.ts change is a trivial import reorder.
Security risks
The primary motivation of the PR is a security fix — removing world-readable /tmp/ log files in favor of user-scoped ~/Library/Logs/bun/cron/. The new path is user-specific and follows macOS conventions. No new security risks are introduced. The makePath call before plist generation ensures the log directory exists before writing. All dynamic values inserted into the plist XML go through xmlEscape().
Memory management
A memory leak I identified in a prior review (plist_path string was never freed on the success path in spawnBootstrap()) was fixed in commit 7b90a41 via defer bun.default_allocator.free(plist_path). The three new allocations (log_dir, xml_log_dir, xml_script_dir) each have corresponding defer free calls, following the existing pattern. All allocations appear correctly balanced.
Level of scrutiny
This is a targeted bug fix + minor feature addition (WorkingDirectory) touching macOS-only cron registration code. The changes are well-scoped, follow established patterns in the file, and the bug hunting system found no issues. Tests are updated to assert the new paths and would fail against the old code, providing confidence in correctness.
Other factors
CodeRabbit inline comments were both addressed/resolved. Maintainer alii adopted the PR. CI failures noted in the timeline were infra-related (expired runners, unrelated ASAN OOM), not code issues. The fix is self-consistent and the description clearly explains the rationale for ~/Library/Logs/ over NSTemporaryDirectory().
There was a problem hiding this comment.
LGTM — the memory leak I flagged in the previous round was fixed in 7b90a41; the remaining nit (hardcoded tilde path in the error message) is cosmetic and does not affect correctness.
Extended reasoning...
Overview
This PR moves macOS cron log files from the globally-readable /tmp/ to ~/Library/Logs/bun/cron/, adds a WorkingDirectory key to the launchd plist, updates docs, and adjusts tests accordingly. It also contains a minor import-order fix in an unrelated S3 test file.
Security risks
The PR fixes a security issue (world-readable /tmp/ logs, #28298) rather than introducing one. No new security surface area is added. Dynamic values embedded in the plist continue to be XML-escaped via xmlEscape().
Level of scrutiny
This is a medium-complexity macOS-specific change. The logic is straightforward: allocate, XML-escape, and embed new paths into the plist template. All three new allocations (log_dir, xml_log_dir, xml_script_dir) have defer free. The memory leak I identified in the previous review (plist_path string leaked after tmp_path = null) was fixed in 7b90a41. CI passes on all macOS, Windows, and Linux jobs; the only failures are in bundler_compile.test.ts across multiple Linux runners, which is an unrelated pre-existing flake.
Other factors
The one remaining bug (inline comment) is a nit: the makePath failure error message hardcodes ~/Library/Logs/bun/cron instead of interpolating the log_dir variable. This is cosmetic — it only appears in a rare error path and does not affect behavior. Maintainer alii approved; CodeRabbit pre-merge checks all passed; 52/53 CI jobs pass.
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
a4fe481 to
42953c8
Compare
There was a problem hiding this comment.
LGTM — both issues I flagged (memory leak in spawnBootstrap(), misleading tilde in error message) have been fixed.
Extended reasoning...
Overview
This PR moves macOS cron log files from the globally-shared /tmp/ to ~/Library/Logs/bun/cron/, adds a WorkingDirectory key to the launchd plist, and fixes a memory leak in spawnBootstrap(). Four files changed: the Zig implementation, tests, docs, and a minor import-sort in an unrelated S3 test.
Security risks
The PR is itself a security fix removing world-readable log paths. The new path is user-scoped and follows macOS conventions. No new attack surface is introduced. XML-escaping of the new log_dir and script_dir values is applied correctly through the existing xmlEscape() helper.
Level of scrutiny
Moderate. The Zig changes are additive (new allocations with matching defers), follow established patterns in the file, and are well-tested. The memory leak fix is straightforward and matches the pattern used in spawnBootout(). The WorkingDirectory key addition is a one-liner using an already-computed value.
Other factors
CI passed 52/53 jobs (one unrelated flaky alpine runner). Maintainer alii approved. CodeRabbit inline suggestions were correctly resolved. Both bugs I previously identified were addressed promptly. Tests assert the new paths and would fail on main, confirming correctness.
There was a problem hiding this comment.
Both issues I flagged have been addressed — LGTM.
Extended reasoning...
Overview
This PR moves macOS cron log files from the world-readable /tmp/ to ~/Library/Logs/bun/cron/, adds a WorkingDirectory key to the launchd plist, updates docs, and fixes a pre-existing memory leak in spawnBootstrap() where plist_path was nulled to prevent plist deletion but never freed.
Security risks
The change is itself a security improvement — removing world-readable log output from /tmp/. No new security surface is introduced. The new directory is user-owned and not accessible to other users.
Level of scrutiny
This touches production Zig memory management and macOS launchd plist generation, so it deserves real review. However, the scope is narrow and self-contained: three new allocations each paired with defer free, one new makePath call with error handling, and updated format-string arguments (11 placeholders, 11 args, verified). The maintainer has approved.
Other factors
Both inline bugs I flagged in prior runs were fixed: the plist_path memory leak (now freed via defer in spawnBootstrap) and the hardcoded tilde in the error message (now interpolates the actual log_dir). CI passed 52/53 with the single failure being an unrelated alpine flake. CodeRabbit comments were resolved. No bugs were found in this review run.
There was a problem hiding this comment.
Both previously raised issues are fixed; LGTM.
Extended reasoning...
Overview
This PR moves macOS cron log files from /tmp/bun.cron.* to ~/Library/Logs/bun/cron/bun.cron.*, adds WorkingDirectory to the launchd plist (set to the script's directory), fixes a memory leak in spawnBootstrap(), and updates tests and docs accordingly. Four files changed: cron.zig, cron.test.ts, cron.mdx, and a trivial import-sort fix in s3-fd-validation.test.ts.
Security risks
The PR fixes a security issue (world-readable /tmp/ logs) rather than introducing one. The new ~/Library/Logs/bun/cron/ directory is user-owned and not world-accessible. All dynamic values written into the plist XML continue to go through xmlEscape(). No auth, crypto, or permissions regressions.
Level of scrutiny
Medium — this touches runtime macOS cron registration logic (cron.zig) but the changes are well-scoped: a directory constant swap, makePath call for the new dir, WorkingDirectory key addition, and two memory-management fixes. All new allocations have defer free. The format-string argument count (11 placeholders, 11 args) was verified independently. Tests would fail on main, confirming they actually exercise the new paths.
Other factors
Maintainer approved. CI build #40597 passed 52/53 (single unrelated alpine flake). Both inline comments I raised in prior reviews were fixed in commits 7b90a41 and 42953c8. CodeRabbit's two suggestions were addressed and confirmed by the author. No outstanding reviewer threads.
|
@robobun Get this mergable |
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
4e1108e to
2f393b8
Compare
There was a problem hiding this comment.
LGTM — both prior concerns (memory leak in spawnBootstrap, log_dir error message interpolation) were addressed; the remaining launch_agents_dir tilde issue is pre-existing, cosmetic-only, and not introduced by this PR.
Extended reasoning...
Overview
This PR touches src/bun.js/api/cron.zig, docs/runtime/cron.mdx, and test/js/bun/cron/cron.test.ts. The core change moves macOS cron log files from the globally shared /tmp/ to the per-user ~/Library/Logs/bun/cron/, fixing a security exposure (world-readable cron output). It also sets WorkingDirectory in the launchd plist to the script directory.
Security risks
The PR reduces security risk by removing world-readable /tmp/ log paths. No new injection vectors: all dynamic values are XML-escaped before plist embedding. The new log_dir and xml_script_dir allocations are properly deferred-freed. The defer bun.default_allocator.free(plist_path) fix in spawnBootstrap() (from a prior review cycle) closes a small heap leak.
Level of scrutiny
Moderate complexity — process spawning, memory management, XML generation — but follows established patterns in the file. The logic is straightforward and changes are scoped tightly to the macOS plist generation path. CI passed 52/53 (one unrelated alpine flake). A maintainer approved. Tests assert the new paths and would fail on main.
Other factors
The one remaining flagged issue (the launch_agents_dir error message still using a hardcoded tilde) is pre-existing code that predates this PR and is purely cosmetic — only error message wording is wrong, not behavior. Both bugs I surfaced in earlier review rounds were resolved before this approval.
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
2f393b8 to
227bc7a
Compare
There was a problem hiding this comment.
LGTM — all previously flagged issues (memory leak, hardcoded error paths) have been addressed.
Extended reasoning...
Overview
This PR moves macOS Bun.cron() log files from the globally-shared /tmp/ to ~/Library/Logs/bun/cron/, sets WorkingDirectory in the launchd plist to the script's directory, and includes memory management fixes. Changes touch src/bun.js/api/cron.zig, test/js/bun/cron/cron.test.ts, and docs/runtime/cron.mdx.
Security risks
The PR improves security by eliminating world-readable /tmp/ log files. No new security risks introduced — all dynamic values written into the plist XML are properly XML-escaped via xmlEscape(), the new log_dir and xml_log_dir allocations are correctly defer-freed, and plist_path is freed with defer in spawnBootstrap() after tmp_path is set to null.
Level of scrutiny
Moderate — macOS-only runtime path with Zig memory management. Three issues I flagged in prior review rounds were all fixed: (1) memory leak of plist_path string in spawnBootstrap(), (2) hardcoded ~/Library/Logs/bun/cron in the log_dir makePath error message, and (3) same hardcoded path in the pre-existing launch_agents_dir error message. The format string has 11 placeholders matching 11 arguments. All new allocations have defer free.
Other factors
CI Build #40597 passed 52/53 jobs (one unrelated alpine flake). Maintainer alii approved and requested merge. The bug hunting system found no additional bugs. Tests assert the new ~/Library/Logs/bun/cron/ paths and the WorkingDirectory key — both would fail on main without this PR, confirming test validity.
There was a problem hiding this comment.
All previously flagged issues are resolved — LGTM.
Extended reasoning...
Overview
This PR modifies three files: src/bun.js/api/cron.zig (core macOS launchd plist generation), test/js/bun/cron/cron.test.ts (updated assertions + new WorkingDirectory test), and docs/runtime/cron.mdx. The main changes: move log files from world-readable /tmp/ to ~/Library/Logs/bun/cron/ (security fix), set WorkingDirectory in the plist to the script directory, and a memory leak fix for plist_path in spawnBootstrap().
Security risks
The PR is a security improvement. New allocations (log_dir, xml_log_dir, xml_script_dir) all have matching defer free calls. Format string verified: 11 placeholders, 11 arguments. No new security concerns.
Level of scrutiny
macOS-only runtime change in a self-contained function. Three bugs found and fixed across prior review rounds: memory leak in spawnBootstrap(), and two hardcoded tilde strings in error messages. All corrected in 227bc7a.
Other factors
CI passed 52/53 (single failure is an unrelated aarch64 C++ compiler crash). Maintainer approved. CodeRabbit suggestions addressed. No bugs found by the automated system on this revision.
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR moves macOS cron log files from the globally-shared /tmp/ to ~/Library/Logs/bun/cron/, fixing a security issue where cron job output was world-readable. Changes touch src/bun.js/api/cron.zig (log path construction, XML generation, WorkingDirectory support), tests (updated assertions, new WorkingDirectory test), and docs.
Security risks
The PR improves security by moving from world-readable /tmp/ to a user-owned ~/Library/Logs/bun/cron/ directory. No new security risks introduced. Dynamic values in the plist are XML-escaped via the existing xmlEscape() helper.
Level of scrutiny
Moderate — macOS-specific runtime feature, not core auth/crypto. All previously flagged issues (memory leak in spawnBootstrap, hardcoded tilde in two error messages) were fixed in subsequent commits (7b90a41, 42953c8, 227bc7a).
Other factors
CI passed 52/53 (single unrelated alpine C++ compiler crash). Maintainer alii approved. CodeRabbit inline suggestions resolved. Format string has 11 placeholders matching 11 arguments. All new allocations have defer free. Tests assert new paths and would fail on main.
Move cron log files from /tmp/ (globally shared) to ~/Library/Logs/bun/cron/ to prevent other users on the system from reading or tampering with cron job output. This follows macOS conventions and addresses the security concern described in Apple's secure coding guidelines. Closes #28298 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set the launchd plist WorkingDirectory key to the directory containing the cron script file (equivalent to import.meta.dir), so relative paths in cron jobs resolve from the script's location.
The plist_path string was allocated in startMac() but never freed on the success path because spawnBootstrap() sets tmp_path = null to prevent file deletion, which also prevented deinit() from freeing the string memory.
robobun
force-pushed
the
claude/fix-cron-log-dir-28298
branch
from
3a44bb7 to
7412c01
Compare
There was a problem hiding this comment.
LGTM — all prior bugs (memory leak in spawnBootstrap, hardcoded tilde error messages) were fixed, no new issues found.
Extended reasoning...
Overview
This PR moves macOS cron log files from the world-readable /tmp/ to ~/Library/Logs/bun/cron/, adds WorkingDirectory to the generated launchd plist, and updates tests and docs accordingly. The changes touch src/bun.js/api/cron.zig (plist generation logic), test/js/bun/cron/cron.test.ts (assertions and cleanup), and docs/runtime/cron.mdx.
Security risks
The PR is itself a security fix — moving logs from a globally shared /tmp/ to a user-specific ~/Library/Logs/ directory prevents other local users from reading or tampering with cron output. The new directory is created with makePath before the plist is written, with proper early-exit error handling. No new security concerns introduced.
Level of scrutiny
Moderate: the Zig changes generate XML-embedded in a plist, which requires correct escaping and correct format-string argument ordering. These were verified (11 {s} placeholders match 11 tuple arguments in the std.fmt.allocPrint call). All new allocations (log_dir, xml_log_dir, xml_script_dir) have defer free. The memory leak in spawnBootstrap I flagged earlier was fixed.
Other factors
All three inline comments from my prior reviews were addressed and resolved. The bug hunting system found no new issues. CI build #41394 passed 52/53 — the single failure was an unrelated linux-aarch64 C++ compiler crash. The maintainer approved. Tests assert the new ~/Library/Logs/bun/cron/ paths and the new WorkingDirectory key, so they would fail on main, confirming they exercise the new behavior.
The launchd plist hard-coded StandardOutPath/StandardErrorPath under
/tmp with a predictable name (bun.cron.<title>.{stdout,stderr}.log). On
a multi-user machine another local user can pre-create a symlink at that
path and have launchd follow it when the job first runs, writing through
to an arbitrary file as the registering user (CWE-59/377).
Create ~/Library/Logs/bun/cron at registration time and point both log
paths there instead. The directory is per-user and not world-writable.
Supersedes #28299.
|
Superseded by #29144 — rebased onto current main with tests added/updated. |
The launchd plist hard-coded StandardOutPath/StandardErrorPath under
/tmp with a predictable name (bun.cron.<title>.{stdout,stderr}.log). On
a multi-user machine another local user can pre-create a symlink at that
path and have launchd follow it when the job first runs, writing through
to an arbitrary file as the registering user (CWE-59/377).
Create ~/Library/Logs/bun/cron at registration time and point both log
paths there instead. The directory is per-user and not world-writable.
Fixes #28298. Supersedes #28299.
The launchd plist hard-coded StandardOutPath/StandardErrorPath under
/tmp with a predictable name (bun.cron.<title>.{stdout,stderr}.log). On
a multi-user machine another local user can pre-create a symlink at that
path and have launchd follow it when the job first runs, writing through
to an arbitrary file as the registering user (CWE-59/377).
Create ~/Library/Logs/bun/cron at registration time and point both log
paths there instead. The directory is per-user and not world-writable.
Fixes #28298. Supersedes #28299.
The launchd plist hard-coded StandardOutPath/StandardErrorPath under
/tmp with a predictable name (bun.cron.<title>.{stdout,stderr}.log). On
a multi-user machine another local user can pre-create a symlink at that
path and have launchd follow it when the job first runs, writing through
to an arbitrary file as the registering user (CWE-59/377).
Create ~/Library/Logs/bun/cron at registration time and point both log
paths there instead. The directory is per-user and not world-writable.
The plist body is built by a platform-independent helper
(build_launchd_plist) so the log-path and XML-escaping logic can be
unit-tested on any host via a bun:internal-for-testing hook — the real
start_mac path is darwin-only-compiled. Also updates the Bun.cron()
JSDoc and docs.
Fixes #28298. Supersedes #28299.
The launchd plist hard-coded StandardOutPath/StandardErrorPath under
/tmp with a predictable name (bun.cron.<title>.{stdout,stderr}.log). On
a multi-user machine another local user can pre-create a symlink at that
path and have launchd follow it when the job first runs, writing through
to an arbitrary file as the registering user (CWE-59/377).
Create ~/Library/Logs/bun/cron at registration time and point both log
paths there instead. The directory is per-user and not world-writable.
The plist body is built by a platform-independent helper
(build_launchd_plist) so the log-path and XML-escaping logic can be
unit-tested on any host via a bun:internal-for-testing hook — the real
start_mac path is darwin-only-compiled. Also updates the Bun.cron()
JSDoc and docs.
Fixes #28298. Supersedes #28299.
3 participants
Summary
Bun.cron()log files from/tmp/to~/Library/Logs/bun/cron/on macOS/tmp/is globally shared and world-readable, allowing other users to read or tamper with cron job output~/Library/Logs/bun/cron/is user-specific and follows macOS conventions (similar to how Bun already uses~/Library/Caches/bun/for transpiler cache)Why not
NSTemporaryDirectory()?The original issue suggested using
NSTemporaryDirectory()(which returns a per-user temp dir like/var/folders/xx/.../T/). We intentionally chose~/Library/Logs/bun/cron/instead because:~/Library/Logs/is the standard macOS location for application logs.NSTemporaryDirectory()paths are buried in/var/folders/with randomized names, making it hard for users to find andtail -ftheir cron logs.~/Library/Logs/bun/cron/is predictable and human-friendly.~/Library/subdirectories on macOS (e.g.,~/Library/Caches/bun/for transpiler cache).Both approaches fix the security issue (no more world-readable
/tmp/), but~/Library/Logs/is the more correct home for log files.Closes #28298
Test plan
bun bdcompiles successfullybun bd test test/js/bun/cron/cron.test.ts— 54 pass, 0 fail~/Library/Logs/bun/cron/pathsVerification (robobun): CI Build #40322 in progress; prior build #40275 passed all macOS/Windows jobs (only failure was unrelated ASAN OOM/signal-kill). Diff is clean — no TODO/FIXME, no unrelated changes. Format string verified: 11 placeholders match 11 arguments in correct order. All new allocations properly freed. Tests updated to assert new paths (would fail on main). New WorkingDirectory test added per maintainer request. Maintainer alii approved; CodeRabbit found no issues.
Verification (robobun): CI Build #41449 in progress (latest commit is empty 'retry CI'). Prior build #41394 passed 52/53 — all macOS, Windows, Linux, ASAN green; single failure was unrelated linux-aarch64-build-cpp compiler crash. Diff clean: no TODO/FIXME/HACK. Format string verified: 11 {s} placeholders match 11 tuple arguments in correct order. All new allocations (log_dir, xml_log_dir, xml_script_dir) have defer free. Memory leak fix for plist_path in spawnBootstrap confirmed correct. Tests assert new ~/Library/Logs/bun/cron/ log paths (would fail on main) and new WorkingDirectory plist key (didn't exist on main). No unresolved review threads.