fix(cron): write macOS launchd logs to ~/Library/Logs, not /tmp#29144
fix(cron): write macOS launchd logs to ~/Library/Logs, not /tmp#29144alii wants to merge 2 commits into
Conversation
|
Updated 7:27 AM PT - May 30th, 2026
✅ @autofix-ci[bot], your commit 6690afa797a653686757f98ac480c423d31470cc passed in 🧪 To try this PR locally: bunx bun-pr 29144That installs a local version of the PR into your bun-29144 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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:
WalkthroughReplace macOS cron stdout/stderr log paths from /tmp to the per-user directory ~/Library/Logs/bun/cron across implementation, tests, bindings, runtime wiring, and documentation; create the log directory and embed its XML-escaped path into generated launchd plists. ChangesmacOS Cron Log Paths
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — clean security fix moving log paths from world-writable /tmp to ~/Library/Logs/bun/cron, with proper directory creation, XML-escaping, and test coverage.
Extended reasoning...
Overview
The PR modifies three files: cron.zig (core macOS plist generation), cron.test.ts (test assertions and cleanup), and docs/runtime/cron.mdx (documentation). The change is narrow and self-contained: replace hardcoded /tmp/bun.cron.${title}.{stdout,stderr}.log paths with per-user paths under \~/Library/Logs/bun/cron/.
Security risks
This PR fixes a CWE-59/CWE-377 symlink/predictable-path vulnerability. The new path (\~/Library/Logs) is user-owned and not controllable by other local users. The log_dir is XML-escaped before embedding in the plist. No new security risks are introduced.
Level of scrutiny
Low-to-medium. The logic change is mechanical: swap a hardcoded /tmp prefix for a computed per-user directory with directory creation, XML escaping, and proper defer free() cleanup. The format string argument count increase (8 → 10) matches the two new {s} placeholders. Error paths follow the existing pattern. The pre-existing memory leak flagged by the bug hunter is unrelated to this PR and is already filed as a separate finding.
Other factors
The test now both asserts the new path and asserts absence of /tmp/, giving good regression coverage. Docs are updated in sync. No CODEOWNERS files cover this path, and the change is additive with no behavioral regression on Linux or Windows.
|
@robobun adopt |
|
✅ Ready to merge — the diff is green; the one red lane is CI infrastructure, not this change. The fix (live Rust
The only non-green check is |
|
@robobun rebase and get this mergable |
ea6e5f0 to
504c4a8
Compare
504c4a8 to
bb294ef
Compare
There was a problem hiding this comment.
LGTM — focused security fix that mirrors the existing LaunchAgents directory-creation pattern; tests and docs are in sync.
Extended reasoning...
Overview
This PR moves macOS Bun.cron() launchd log paths from /tmp/bun.cron.<title>.{stdout,stderr}.log to ~/Library/Logs/bun/cron/. It touches four files: src/runtime/api/cron.rs (creates the per-user log directory and uses it for StandardOutPath/StandardErrorPath in the plist, with XML-escaping), test/js/bun/cron/cron.test.ts (asserts the new path, asserts /tmp is not present, and updates e2e cleanup), docs/runtime/cron.mdx (updated log paths), and packages/bun-types/bun.d.ts (one-line JSDoc update to match).
Security risks
This is itself a security hardening — it eliminates a CWE-59/377 symlink-follow risk by moving from world-writable /tmp to a per-user directory. The new code introduces no new attack surface: the log dir is built from $HOME (already used one block above for ~/Library/LaunchAgents), XML-escaped via the same try_escape! macro as every other dynamic value in the plist, and created with the same Fd::cwd().make_path() call used for the LaunchAgents directory.
Level of scrutiny
Low-to-moderate. The Rust change is a near-verbatim copy of the launch_agents_dir block immediately above it (lines 512–524 vs 526–541), so it follows an established and already-reviewed pattern. The format-string change correctly adds {5} and a sixth positional argument. The test, docs, and JSDoc changes are mechanical path-string updates.
Other factors
Both of my prior inline comments are resolved: the pre-existing Zig memory-leak note was correctly marked out-of-scope (and the Zig file is now dead reference code post-#30412), and the stale bun.d.ts JSDoc was fixed in bb294ef. The bug-hunting system found no issues on the current revision. The packages/bun-types/ CODEOWNER (alii) is the one driving this PR. The updated test includes both positive and negative assertions plus a directory-existence check.
There was a problem hiding this comment.
LGTM — focused security hardening that mirrors the existing ~/Library/LaunchAgents mkdir pattern; both prior review threads addressed.
Extended reasoning...
Overview
Moves macOS Bun.cron() launchd log paths from world-writable /tmp/bun.cron.<title>.{stdout,stderr}.log to per-user ~/Library/Logs/bun/cron/. Touches four files: src/runtime/api/cron.rs (build the log dir path, Fd::cwd().make_path() it, XML-escape, interpolate as {5} in the plist template), test/js/bun/cron/cron.test.ts (assert the new path, assert not.toContain("<string>/tmp/"), assert the dir exists, update e2e cleanup), docs/runtime/cron.mdx and packages/bun-types/bun.d.ts (doc/JSDoc strings).
Security risks
This is the security fix itself — it removes a CWE-59/377 predictable-path symlink-follow vector in /tmp. The new code writes only under $HOME/Library/Logs/bun/cron, a directory the user already controls; make_path → mkdir_recursive_at so the multi-level create works on a fresh machine where ~/Library/Logs/bun doesn't exist yet. The log-dir string is run through the same xml_escape as the other plist fields, so a $HOME containing &/< won't break the plist. No new injection, auth, or data-exposure surface.
Level of scrutiny
Low–medium. The Rust hunk is 18 lines that copy the exact shape of the launch_agents_dir block 12 lines above (build → make_path → error → finish), then add one positional arg to an existing write!. The rest is doc strings and test expectations. macOS-only code path behind #[cfg(target_os = "macos")].
Other factors
Both of my earlier inline threads are resolved: the pre-existing Zig memory leak was correctly scoped out (and the code has since been ported to Rust anyway), and the stale bun.d.ts JSDoc was fixed in bb294ef — confirmed in the current diff. The bug-hunting pass found nothing this round. The one CI red is streams-leak.test.ts on a single Linux aarch64 shard, unrelated to a macOS-only plist change. The .d.ts file is CODEOWNED by alii, who is the one driving this PR.
35e35ce to
f540bd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/cron/cron.test.ts`:
- Around line 69-71: The test currently calls cronPlistForTesting via require()
inside the describe block; instead add a module-scope named import for
cronPlistForTesting from "bun:internal-for-testing" (matching the exported name
used in the diff) at the top of the file and remove the inline require() call
inside the describe; ensure any TypeScript typing (the function signature) is
preserved via an import type or local type annotation if needed so usages of
cronPlistForTesting in the tests remain correctly typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7cd4efa-37c4-4d50-8603-d269776653a8
📒 Files selected for processing (7)
docs/runtime/cron.mdxpackages/bun-types/bun.d.tssrc/js/internal-for-testing.tssrc/runtime/api/BunObject.bind.tssrc/runtime/api/cron.rssrc/runtime/hw_exports.rstest/js/bun/cron/cron.test.ts
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.
f9dbb74 to
082412e
Compare
There was a problem hiding this comment.
I didn't find any issues in this revision — all prior review threads are resolved — but since this is a security fix that also adds new unsafe FFI plumbing in hw_exports.rs, it's worth a quick human sign-off before merge.
Extended reasoning...
Overview
This PR fixes CWE-59/377 in Bun.cron() on macOS by moving the launchd plist's StandardOutPath/StandardErrorPath from world-writable /tmp to per-user ~/Library/Logs/bun/cron/. To make the fix testable on non-darwin CI, the plist body construction is extracted into a platform-independent build_launchd_plist() helper and exposed to JS via a new cronPlistForTesting bindgen function. The change spans 7 files: the Rust implementation (cron.rs), new FFI wiring (hw_exports.rs, BunObject.bind.ts, internal-for-testing.ts), tests, docs, and the bun.d.ts JSDoc.
Security risks
The PR is itself a security hardening — it removes a predictable-path symlink-follow vector in /tmp. The new log directory is created with Fd::cwd().make_path() under $HOME, and the path is XML-escaped before being embedded in the plist. I don't see new attack surface introduced; the cronPlistForTesting hook is a pure string-builder gated behind bun:internal-for-testing and doesn't touch the filesystem. No injection, auth, or data-exposure concerns beyond what the fix addresses.
Level of scrutiny
This warrants human review rather than bot approval: (1) it's a security fix, so a maintainer should confirm the new location and make_path semantics are what they want; (2) it adds ~70 lines of handwritten unsafe FFI in hw_exports.rs (raw pointer derefs, HOST_EXPORT markers, jsc_abi_extern! entries) — these closely follow the existing fmtString/getDeinitCountForTesting patterns in the same file, but unsafe ABI glue is exactly the kind of thing that benefits from a second pair of eyes; (3) the refactor moves the plist template into a shared helper and re-validates the schedule twice in start_mac — behavior-preserving but a structural change to a production path.
Other factors
All three prior review threads (pre-existing memory leak — out of scope; stale bun.d.ts JSDoc — fixed; require() → module import — fixed) are resolved. The new cross-platform unit tests for log paths and XML escaping are solid, and the macOS e2e test was updated to assert the new directory exists and /tmp is absent. The robobun build status shows linker warnings/failures on several lanes, but they look like the ambient -no-pie infrastructure noise rather than anything this PR introduced.
What
Bun.cron()on macOS wrote the launchd plist'sStandardOutPath/StandardErrorPathto/tmp/bun.cron.<title>.{stdout,stderr}.log. This PR moves them to~/Library/Logs/bun/cron/.Why
/tmpis world-writable and the log path is fully predictable from the job title. On a multi-user Mac, another local user can pre-create a symlink at that path before the job first runs; launchd will then open and append through the symlink as the registering user (CWE-59 / CWE-377).~/Library/Logsis per-user and not attacker-controllable.Change
cron.zig: create~/Library/Logs/bun/cronalongside~/Library/LaunchAgents, XML-escape the path, and use it for both log entries in the plist.cron.test.ts: the existing log-path test now asserts the new directory andnot.toContain("<string>/tmp/"); e2e cleanup paths updated.docs/runtime/cron.mdx: updated log paths.Supersedes #28299 (rebased onto current main; dropped the unrelated
WorkingDirectorychange which overlaps with #28283).Fixes #28298
Test