Skip to content

fix(npm): handle read-only bin files when setting up node_modules/.bin#32632

Merged
bartlomieju merged 6 commits intodenoland:mainfrom
bartlomieju:fix/npm-bin-readonly-permissions
Mar 11, 2026
Merged

fix(npm): handle read-only bin files when setting up node_modules/.bin#32632
bartlomieju merged 6 commits intodenoland:mainfrom
bartlomieju:fix/npm-bin-readonly-permissions

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju commented Mar 11, 2026

Summary

  • Some npm tarballs ship bin files with read+execute but no write permission (e.g. @formatjs/cli@2.15.0 uses mode 555). make_executable_if_exists() was opening these files with O_RDWR unconditionally, causing EACCES ("Permission denied").
  • Fix: open read-only first to check the execute bit. Only reopen with write if chmod is actually needed.
$ curl -sL https://registry.npmjs.org/@formatjs/cli/-/cli-2.15.0.tgz | tar tvzf - | grep bin/formatjs
-r-xr-xr-x  0 0      0          81 26 Oct  1985 package/bin/formatjs

Closes #29847

bartlomieju and others added 2 commits March 11, 2026 09:25
Some npm tarballs ship bin files with read+execute but no write
permission (e.g. `@formatjs/cli@2.15.0` uses mode 555). The
`make_executable_if_exists()` function was opening these files with
O_RDWR unconditionally, which fails with EACCES on such files.

The fix opens the file read-only first to check if the execute bit is
already set. Only if the execute bit is missing does it reopen with
write access to chmod. This avoids the permission error for packages
that already ship executable bin files without write permission.

Closes denoland#29847

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Readonly bin file (mode 555): verifies no EACCES error
- Non-executable file (mode 644): verifies execute bit is added
- Nonexistent file: verifies returns false without error

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

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

lgtm, nice fix

opening read-only first to check permissions, then only reopening with write if chmod is needed, is the right approach.

the drop(file) before reopening is important — good that you included it.

solid test coverage for both the readonly case (mode 555) and the non-executable case (mode 644).

bartlomieju and others added 2 commits March 11, 2026 09:45
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM, but CI is failing

bartlomieju and others added 2 commits March 11, 2026 13:47
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@bartlomieju bartlomieju merged commit cbdfed1 into denoland:main Mar 11, 2026
112 checks passed
@bartlomieju bartlomieju deleted the fix/npm-bin-readonly-permissions branch March 11, 2026 13:28
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.

deno install permission denied formatjs

3 participants