Skip to content

fix(cli): avoid dangling symlink temp path#1467

Open
ShyamKarthickSec wants to merge 1 commit into
google:mainfrom
ShyamKarthickSec:fix-dangling-symlink
Open

fix(cli): avoid dangling symlink temp path#1467
ShyamKarthickSec wants to merge 1 commit into
google:mainfrom
ShyamKarthickSec:fix-dangling-symlink

Conversation

@ShyamKarthickSec
Copy link
Copy Markdown

Title
Fix dangling symlink handling for CLI temporary script paths

Description
This PR fixes a symlink-handling issue in the CLI temporary script file path selection used by --eval, stdin, and remote script execution.

Previously, getFilepath() used fs.existsSync() to determine whether a candidate temp path such as zx.mjs was available. existsSync() returns false for dangling symlinks, which meant a pre-existing dangling symlink could be treated as a free path. The subsequent fs.writeFile() call would then follow the symlink and create the symlink target.

This change replaces that availability check with an lstatSync()-based helper, so symlinks, including dangling symlinks, are treated as occupied paths and skipped.

Changes

  • Use lstatSync() instead of existsSync() when checking CLI temp path availability.
  • Mirror the generated build/cli.cjs output.
  • Add a regression test covering --eval with a dangling zx.mjs symlink.

Security Impact
Prevents attacker-controlled dangling symlinks in the working directory from being selected as temporary script paths and written through during CLI execution.

Testing

  • npx prettier --check src/cli.ts test/cli.test.js
  • npx tsc --project tsconfig.json --noEmit
  • Focused regression test added for dangling symlink temp path handling.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 15, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Copy Markdown

@kiwigitops kiwigitops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on the existsSync vs lstatSync semantics — the dangling-symlink case is subtle.

One clarifying question: this closes the gap where a pre-existing dangling symlink is selected as a "free" path, but there's still a TOCTOU window between the lstatSync() check in getFilepath() and the later fs.writeFile() call that actually writes the temp script. An attacker who can write into the same cwd could, in principle, race in a symlink (pointing anywhere the zx process can write) after the check returns "available" but before the write happens. Have you considered whether the write itself should be hardened too — e.g. opening with O_CREAT | O_EXCL | O_NOFOLLOW (or the Node equivalent, fs.open(path, 'wx') plus a post-open fstat/lstat comparison) so the create fails if the path exists or is a symlink at the moment of creation?

Not blocking — the lstat check already removes the most plausible "stale dangling symlink in CWD" footgun, and the random-suffix fallback makes deliberate exploitation noisier. Just wondering whether the threat model in your "Security Impact" note is intended to cover the race too, or only the static dangling-symlink case.

@ShyamKarthickSec
Copy link
Copy Markdown
Author

Thanks, that’s a fair point. This PR is scoped to the static dangling-symlink case described in the report: a pre-existing zx.mjs dangling symlink being treated as an available temp path because of existsSync() semantics.

I agree this does not fully eliminate the TOCTOU class between path selection and writeFile(). A stronger follow-up would be to make the creation itself atomic, for example with exclusive create semantics and no-follow behavior where supported, or by moving this flow to a securely-created temp directory. I kept this patch narrow to avoid changing the temp-file lifecycle more broadly in the security fix, but I’m happy to either update the Security Impact wording to make the scope explicit, or send a follow-up hardening PR for atomic temp-file creation if maintainers prefer.

So the intended coverage here is the static dangling-symlink footgun, not a full race-resistant temp-file primitive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants