Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid world-writable permissions for lockfiles #16018

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jbergstroem
Copy link
Contributor

@jbergstroem jbergstroem commented Dec 27, 2024

What does this PR do?

Use the file mode 0x644 for the plaintext lockfile. Also, switch the permission for the binary lockfile to not default to group/world writable (0x777 -> 0x755). Windows has a different behavior, so we test for that.

Finally, add tests for writing plaintext lockfiles and ensure its file stat properties.

How did you verify your code works?

Built and tested locally.

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

Closes: #15953

@jbergstroem
Copy link
Contributor Author

The codebase is new to me; can't seem to find tests around the plaintext lockfile. Should I add one that does something similar as the lockb test?

@dylan-conway
Copy link
Member

@jbergstroem thanks for opening this pr. For bun.lock tests, could you create a new file in test/cli/install, maybe bun.lock.test.ts or something similar? Ideally the test installs a local folder dependency, to avoid flakiness from requesting from a registry

@jbergstroem jbergstroem marked this pull request as ready for review December 29, 2024 10:14
@jbergstroem jbergstroem changed the title fix: don't set executable bit for plaintext lock-file fix: avoid world-writable permissions for lockfiles Dec 29, 2024
@jbergstroem
Copy link
Contributor Author

It seems windows permissions are different than the rest. Need to set up a local environment and figure it out.

@dylan-conway
Copy link
Member

@jbergstroem I think it would be okay to add:

import { isWindows } from "harness";
...
if (!isWindows) {
    // check permissions
}

Or hardcode it to the received value from CI (33206). We're only changing permissions on posix.

@jbergstroem
Copy link
Contributor Author

Or hardcode it to the received value from CI (33206). We're only changing permissions on posix.

I kept the test for windows so we pick up if permissions start changing on windows for some reason.

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.

Unnecessray file permissions on bun.lock
2 participants