mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-14 17:52:44 -04:00
fix(package-manifest): write package.json atomically via a temp file (#12642)
PackageManifest::save truncated package.json in place with File::create + write_all. A crash, OOM, or ENOSPC between the truncate and the completed write left the file empty or partial, destroying the user's dependency declarations with no recovery path. add/remove/update and set-script all go through this path. Write the serialized manifest to a sibling temp file, fsync it, then rename it over the target, matching the write-file-atomic guarantee the TypeScript pnpm CLI relies on. The crate's sibling workspace-manifest-writer already uses this pattern. --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -16,6 +16,7 @@ miette = { workspace = true }
|
||||
serde = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
strum = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
pipe-trait = { workspace = true }
|
||||
|
||||
@@ -9,6 +9,7 @@ use miette::Diagnostic;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use serde_json::{Map, Value, json};
|
||||
use strum::IntoStaticStr;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
#[derive(Debug, Display, Error, Diagnostic, From)]
|
||||
#[non_exhaustive]
|
||||
@@ -96,6 +97,28 @@ impl PackageManifest {
|
||||
Ok((manifest, contents))
|
||||
}
|
||||
|
||||
/// Write `contents` to `path` atomically: a sibling temp file is written
|
||||
/// and fsynced, then renamed over `path`. A crash or write error therefore
|
||||
/// never leaves a truncated or partial `package.json` behind, matching the
|
||||
/// `write-file-atomic` guarantee the TypeScript pnpm CLI relies on.
|
||||
fn write_atomic(path: &Path, contents: &str) -> io::Result<()> {
|
||||
let dir = path
|
||||
.parent()
|
||||
.filter(|parent| !parent.as_os_str().is_empty())
|
||||
.unwrap_or_else(|| Path::new("."));
|
||||
let mut tmp = NamedTempFile::new_in(dir)?;
|
||||
tmp.write_all(contents.as_bytes())?;
|
||||
tmp.as_file().sync_all()?;
|
||||
// A NamedTempFile is created 0o600; preserve the original file's mode
|
||||
// when overwriting an existing package.json (write-file-atomic does the
|
||||
// same) so the rename doesn't silently tighten its permissions.
|
||||
if let Ok(metadata) = fs::metadata(path) {
|
||||
tmp.as_file().set_permissions(metadata.permissions())?;
|
||||
}
|
||||
tmp.persist(path).map_err(|err| err.error)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn read_from_file(path: &Path) -> Result<Value, PackageManifestError> {
|
||||
let contents = fs::read_to_string(path)?;
|
||||
let mut value: Value = serde_json::from_str(&contents)?;
|
||||
@@ -158,8 +181,7 @@ impl PackageManifest {
|
||||
convert_dependencies_to_engines_runtime(&mut value, "devDependencies", "devEngines")?;
|
||||
convert_dependencies_to_engines_runtime(&mut value, "dependencies", "engines")?;
|
||||
let contents = serde_json::to_string_pretty(&value)?;
|
||||
let mut file = fs::File::create(&self.path)?;
|
||||
file.write_all(contents.as_bytes())?;
|
||||
Self::write_atomic(&self.path, &contents)?;
|
||||
Ok(value)
|
||||
}
|
||||
|
||||
|
||||
@@ -15,6 +15,50 @@ use crate::DependencyGroup;
|
||||
use serde_json::json;
|
||||
use std::io::Write;
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn save_leaves_the_original_intact_when_the_write_cannot_complete() {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let dir = tempdir().unwrap();
|
||||
let path = dir.path().join("package.json");
|
||||
let original = r#"{"name":"intact","version":"1.0.0"}"#;
|
||||
std::fs::write(&path, original).unwrap();
|
||||
|
||||
let manifest = PackageManifest::from_path(path.clone()).unwrap();
|
||||
|
||||
// Make the directory read-only so a sibling temp file cannot be created.
|
||||
// An atomic temp-file-then-rename write fails up front and leaves the
|
||||
// original untouched; a non-atomic in-place write would instead truncate
|
||||
// the existing package.json before failing.
|
||||
std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o555)).unwrap();
|
||||
let result = manifest.save();
|
||||
// Restore permissions before the assertions so tempdir cleanup succeeds.
|
||||
std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o755)).unwrap();
|
||||
|
||||
assert!(result.is_err());
|
||||
assert_eq!(std::fs::read_to_string(&path).unwrap(), original);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn save_preserves_the_existing_package_json_permissions() {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let dir = tempdir().unwrap();
|
||||
let path = dir.path().join("package.json");
|
||||
std::fs::write(&path, r#"{"name":"perm","version":"1.0.0"}"#).unwrap();
|
||||
std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o640)).unwrap();
|
||||
|
||||
let manifest = PackageManifest::from_path(path.clone()).unwrap();
|
||||
manifest.save().unwrap();
|
||||
|
||||
// The atomic temp-file-then-rename must keep the original mode, not leave
|
||||
// the NamedTempFile's default 0o600 behind.
|
||||
let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777;
|
||||
assert_eq!(mode, 0o640);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_init_package_json_content() {
|
||||
let manifest = PackageManifest::create_init_package_json("test");
|
||||
|
||||
Reference in New Issue
Block a user