fix(workspace-state): keep the state file readable to other users (#17438)
The workspace state file was written through `private_named_tempfile_in`, so `node_modules/.pnpm-workspace-state-v1.json` ended up 0600 while pnpm 11 left it 0644 (`fs.writeFile`). Another user could not read it, so the deps-status check before `pnpm run` treated `node_modules` as stale and reinstalled. A Docker image that installs as root and runs as a non-root user hits this on every start. Add `pnpm_fs::umask_named_tempfile_in`, which creates the temp file with 0666 narrowed by the umask, and use it for the state file. The write stays without an fsync, as before.
This commit is contained in:
1 parent
38e9755f66
commit
64588da2d9
6 files changed
+68
-1
No files matched your search
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"pacquet": patch
|
||||
---
|
||||
|
||||
`pnpm install` now creates `node_modules/.pnpm-workspace-state-v1.json` with the same permissions as other new files. It was readable only by the user who ran the install, so a different user running `pnpm run` reinstalled the dependencies. This happened, for example, in a Docker image built as root and run as a non-root user [#17435](https://github.com/pnpm/pnpm/discussions/17435).
|
||||
@@ -12,6 +12,7 @@ use pnpm_testing_utils::{
|
||||
bin::{AddMockedRegistry, CommandTempCwd},
|
||||
command_env::CommandTestExt,
|
||||
};
|
||||
use pnpm_workspace_state::WORKSPACE_STATE_FILENAME;
|
||||
use std::{
|
||||
fs,
|
||||
os::unix::{fs::PermissionsExt, process::CommandExt},
|
||||
@@ -116,6 +117,16 @@ fn installs_materialize_files_at_the_current_umask_even_from_a_stale_store() {
|
||||
0o644,
|
||||
"the store link matches the umask that wrote the store",
|
||||
);
|
||||
assert_eq!(
|
||||
file_mode(&workspace.join("node_modules").join(WORKSPACE_STATE_FILENAME)),
|
||||
0o644,
|
||||
"the workspace state file follows the install's umask",
|
||||
);
|
||||
assert_eq!(
|
||||
file_mode(&other.join("node_modules").join(WORKSPACE_STATE_FILENAME)),
|
||||
0o600,
|
||||
"the workspace state file follows the install's umask",
|
||||
);
|
||||
|
||||
use std::os::unix::fs::MetadataExt;
|
||||
let inode = |path: &Path| fs::metadata(path).expect("stat").ino();
|
||||
|
||||
@@ -48,6 +48,7 @@ pub use secure_temp_lock::{
|
||||
};
|
||||
pub use symlink_dir::*;
|
||||
pub use sync_file::sync_file;
|
||||
pub use umask_named_tempfile::umask_named_tempfile_in;
|
||||
#[cfg(target_os = "wasi")]
|
||||
pub use wasi_fs::{
|
||||
check_file_owner, create_inheriting_mode as create_file_inheriting_mode,
|
||||
@@ -85,6 +86,7 @@ mod retry;
|
||||
mod secure_temp_lock;
|
||||
mod symlink_dir;
|
||||
mod sync_file;
|
||||
mod umask_named_tempfile;
|
||||
#[cfg(target_os = "wasi")]
|
||||
mod wasi_fs;
|
||||
mod write_atomic;
|
||||
|
||||
@@ -0,0 +1,26 @@
|
||||
use std::{io, path::Path};
|
||||
|
||||
/// Creates a named temporary file with the mode a new file from `fs::write` gets:
|
||||
/// 0666 narrowed by the umask on Unix and WASI.
|
||||
///
|
||||
/// For files other users may need to read, such as a `node_modules` built by root
|
||||
/// in one Docker stage and used by another user in the next.
|
||||
/// [`private_named_tempfile_in`](crate::private_named_tempfile_in) is the 0600 counterpart.
|
||||
#[inline]
|
||||
pub fn umask_named_tempfile_in(directory: impl AsRef<Path>) -> io::Result<tempfile::NamedTempFile> {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::PermissionsExt as _;
|
||||
tempfile::Builder::new()
|
||||
.permissions(std::fs::Permissions::from_mode(0o666))
|
||||
.tempfile_in(directory)
|
||||
}
|
||||
#[cfg(target_os = "wasi")]
|
||||
{
|
||||
tempfile::Builder::new().make_in(directory, |path| crate::create_new_with_mode(path, 0o666))
|
||||
}
|
||||
#[cfg(not(any(unix, target_os = "wasi")))]
|
||||
{
|
||||
tempfile::NamedTempFile::new_in(directory)
|
||||
}
|
||||
}
|
||||
@@ -436,7 +436,7 @@ pub fn update_workspace_state_in_modules(
|
||||
serde_json::to_string_pretty(state).map_err(UpdateWorkspaceStateError::SerializeJson)?;
|
||||
serialized.push('\n');
|
||||
let write = |source| UpdateWorkspaceStateError::WriteFile { path: file_path.clone(), source };
|
||||
let mut temp = pnpm_fs::private_named_tempfile_in(parent).map_err(write)?;
|
||||
let mut temp = pnpm_fs::umask_named_tempfile_in(parent).map_err(write)?;
|
||||
temp.write_all(serialized.as_bytes()).map_err(write)?;
|
||||
let temp = temp.into_temp_path();
|
||||
pnpm_fs::rename_with_retry(&temp, &file_path).map_err(write)
|
||||
|
||||
@@ -248,6 +248,29 @@ fn update_surfaces_write_file_error_when_target_is_a_directory() {
|
||||
assert_eq!(node_modules_entries(workspace_dir), vec![WORKSPACE_STATE_FILENAME.to_string()]);
|
||||
}
|
||||
|
||||
/// Another user must be able to read the state file, for example when root
|
||||
/// installs in one Docker stage and an unprivileged user runs scripts in the next.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn update_gives_the_state_file_the_mode_of_any_new_file() {
|
||||
use std::os::unix::fs::PermissionsExt as _;
|
||||
let tmp = tempdir().expect("create temp dir");
|
||||
let workspace_dir = tmp.path();
|
||||
let reference = workspace_dir.join("reference");
|
||||
std::fs::File::create(&reference).expect("create reference file");
|
||||
|
||||
update_workspace_state(workspace_dir, &WorkspaceState::default()).expect("write state");
|
||||
|
||||
let mode_of = |path: &std::path::Path| {
|
||||
std::fs::metadata(path)
|
||||
.expect("stat file")
|
||||
.permissions()
|
||||
.mode()
|
||||
& 0o777
|
||||
};
|
||||
assert_eq!(mode_of(&get_file_path(workspace_dir)), mode_of(&reference));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn update_leaves_no_temp_file_beside_the_state_file() {
|
||||
let tmp = tempdir().expect("create temp dir");
|
||||
|
||||
Reference in new issue
Block a user