Skip to content

Commit 8333162

Browse files
committed
feat(apply): preserve mode + ownership across patches
`apply_file_patch` now treats target-file permissions as a strict round-trip: 1. **Existing file**: snapshot mode + uid + gid before writing. - If read-only, temporarily grant owner-write so the overwrite succeeds (Go module cache, npm linked symlinks, etc.). - After writing, restore the *exact* pre-patch mode (idempotent `set_permissions(from_mode(...))`) and chown back to the pre-patch uid/gid. `tokio::fs::write` truncates + rewrites the file in place, so owner usually survives, but pinning ownership explicitly stops a theoretical race where another process opens the file between truncate and write. 2. **New file** (created by the patch): chown to inherit owner/group from the parent directory, mode = `0o444` (read-only for all). Matches how a freshly-unpacked package tarball treats its files. Windows: no uid/gid concept; preserve the readonly attribute for existing files and force it on new ones. `restore_file_permissions` and the `chown_blocking` helper are split out of `apply_file_patch` for readability and unit testing. Four new tests pin the policy: readonly-mode preservation, executable (0o755) mode preservation, new-file default mode + parent ownership inheritance, and uid/gid round-trip on existing files. Assisted-by: Claude Code:opus-4-7
1 parent d96f110 commit 8333162

1 file changed

Lines changed: 255 additions & 6 deletions

File tree

  • crates/socket-patch-core/src/patch

crates/socket-patch-core/src/patch/apply.rs

Lines changed: 255 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,26 @@ pub async fn verify_file_patch(
202202
}
203203

204204
/// Apply a patch to a single file.
205+
///
206+
/// **Permission policy** (per the user-visible contract — patched
207+
/// files must look identical to pre-patch perms-wise):
208+
///
209+
/// 1. **Existing file**. Snapshot mode + owner + group before writing.
210+
/// If the file is read-only, temporarily grant owner-write so the
211+
/// overwrite succeeds (e.g. Go's module cache marks sources read-only).
212+
/// After the write, restore the **exact** original mode and chown
213+
/// back to the pre-patch uid/gid. Owners stay put even when
214+
/// `tokio::fs::write` truncates and rewrites.
215+
///
216+
/// 2. **New file** (created by the patch). Inherit owner + group from
217+
/// the parent directory and force mode `0o444` (read-only for all).
218+
/// Mirrors how an unpacked tarball treats new package files —
219+
/// consumers expect package sources to be read-only by default.
220+
///
221+
/// On Windows there is no `uid`/`gid`, so the owner/group step is a
222+
/// no-op; the read-only attribute is preserved on existing files and
223+
/// set on new files to honor the read-only-by-default policy.
224+
///
205225
/// Writes the patched content and verifies the resulting hash.
206226
pub async fn apply_file_patch(
207227
pkg_path: &Path,
@@ -212,28 +232,51 @@ pub async fn apply_file_patch(
212232
let normalized = normalize_file_path(file_name);
213233
let filepath = pkg_path.join(normalized);
214234

215-
// Create parent directories if needed (e.g., new files added by a patch)
235+
// Snapshot pre-patch metadata so we can restore mode + ownership
236+
// after the write. `None` means the file is being created by this
237+
// patch — that path is handled below in the platform blocks.
238+
let existing_meta = tokio::fs::metadata(&filepath).await.ok();
239+
240+
// Create parent directories if needed (e.g., new files added by a patch).
216241
if let Some(parent) = filepath.parent() {
217242
tokio::fs::create_dir_all(parent).await?;
218243
}
219244

220-
// Make file writable if it exists and is read-only (e.g. Go module cache)
245+
// Temporarily grant owner-write if the existing file is read-only,
246+
// so the upcoming overwrite succeeds. The restore step below puts
247+
// the original mode back unconditionally — re-applying the exact
248+
// mode is idempotent, so we don't need to track whether we bumped it.
221249
#[cfg(unix)]
222-
if let Ok(meta) = tokio::fs::metadata(&filepath).await {
250+
if let Some(meta) = existing_meta.as_ref() {
223251
use std::os::unix::fs::PermissionsExt;
224252
let perms = meta.permissions();
225253
if perms.readonly() {
226254
let mode = perms.mode();
227-
let mut new_perms = perms;
255+
let mut new_perms = perms.clone();
228256
new_perms.set_mode(mode | 0o200);
229257
tokio::fs::set_permissions(&filepath, new_perms).await?;
230258
}
231259
}
260+
#[cfg(windows)]
261+
if let Some(meta) = existing_meta.as_ref() {
262+
let perms = meta.permissions();
263+
if perms.readonly() {
264+
let mut new_perms = perms.clone();
265+
new_perms.set_readonly(false);
266+
tokio::fs::set_permissions(&filepath, new_perms).await?;
267+
}
268+
}
232269

233-
// Write the patched content
270+
// Write the patched content.
234271
tokio::fs::write(&filepath, patched_content).await?;
235272

236-
// Verify the hash after writing
273+
// Restore (or set) the final permissions. On Unix this includes
274+
// chown back to the pre-patch uid/gid (or to the parent dir's
275+
// uid/gid for new files); on Windows we only manage the readonly
276+
// attribute.
277+
restore_file_permissions(&filepath, existing_meta.as_ref()).await?;
278+
279+
// Verify the hash after writing.
237280
let verify_hash = compute_file_git_sha256(&filepath).await?;
238281
if verify_hash != expected_hash {
239282
return Err(std::io::Error::new(
@@ -248,6 +291,89 @@ pub async fn apply_file_patch(
248291
Ok(())
249292
}
250293

294+
/// Restore the post-write permission state on `filepath`.
295+
///
296+
/// * `pre_patch` = `Some(meta)` → the file existed before the patch;
297+
/// restore its exact mode + uid/gid.
298+
/// * `pre_patch` = `None` → the file is new; inherit owner/group from
299+
/// the parent dir and set mode `0o444`.
300+
///
301+
/// Split out of `apply_file_patch` to keep that function readable and
302+
/// to make the platform branching unit-testable.
303+
async fn restore_file_permissions(
304+
filepath: &Path,
305+
pre_patch: Option<&std::fs::Metadata>,
306+
) -> Result<(), std::io::Error> {
307+
#[cfg(unix)]
308+
{
309+
use std::os::unix::fs::{MetadataExt, PermissionsExt};
310+
311+
match pre_patch {
312+
Some(meta) => {
313+
// Existing file: re-apply the original mode + ownership.
314+
let restored = std::fs::Permissions::from_mode(meta.mode());
315+
tokio::fs::set_permissions(filepath, restored).await?;
316+
let uid = meta.uid();
317+
let gid = meta.gid();
318+
chown_blocking(filepath.to_path_buf(), Some(uid), Some(gid)).await?;
319+
}
320+
None => {
321+
// New file. Inherit owner/group from the parent dir.
322+
if let Some(parent) = filepath.parent() {
323+
if let Ok(parent_meta) = tokio::fs::metadata(parent).await {
324+
let uid = parent_meta.uid();
325+
let gid = parent_meta.gid();
326+
chown_blocking(filepath.to_path_buf(), Some(uid), Some(gid))
327+
.await?;
328+
}
329+
}
330+
// Default new-file mode: read-only for all.
331+
let readonly = std::fs::Permissions::from_mode(0o444);
332+
tokio::fs::set_permissions(filepath, readonly).await?;
333+
}
334+
}
335+
}
336+
337+
#[cfg(windows)]
338+
{
339+
match pre_patch {
340+
Some(meta) => {
341+
// Re-apply the pre-patch readonly state; tokio::fs::write
342+
// does not preserve it across the truncate+rewrite.
343+
let perms = meta.permissions();
344+
tokio::fs::set_permissions(filepath, perms).await?;
345+
}
346+
None => {
347+
// New file: read-only by default.
348+
if let Ok(meta) = tokio::fs::metadata(filepath).await {
349+
let mut perms = meta.permissions();
350+
perms.set_readonly(true);
351+
tokio::fs::set_permissions(filepath, perms).await?;
352+
}
353+
}
354+
}
355+
}
356+
357+
let _ = filepath;
358+
let _ = pre_patch;
359+
Ok(())
360+
}
361+
362+
/// Synchronous `chown` wrapped to run on the blocking pool so we don't
363+
/// stall the async runtime. `std::os::unix::fs::chown` is a thin
364+
/// syscall wrapper — fast in the no-op case (uid/gid already match)
365+
/// but still nominally blocking.
366+
#[cfg(unix)]
367+
async fn chown_blocking(
368+
path: std::path::PathBuf,
369+
uid: Option<u32>,
370+
gid: Option<u32>,
371+
) -> Result<(), std::io::Error> {
372+
tokio::task::spawn_blocking(move || std::os::unix::fs::chown(&path, uid, gid))
373+
.await
374+
.map_err(|e| std::io::Error::other(e.to_string()))?
375+
}
376+
251377
/// Verify and apply patches for a single package.
252378
///
253379
/// For each file in `files`, this function:
@@ -705,6 +831,129 @@ mod tests {
705831
assert!(err.to_string().contains("Hash verification failed"));
706832
}
707833

834+
/// Existing read-only file: temporarily made writable for the
835+
/// overwrite, restored to read-only afterward, content updated.
836+
/// Mirrors the Go module cache scenario.
837+
#[cfg(unix)]
838+
#[tokio::test]
839+
async fn test_apply_file_patch_preserves_readonly_mode() {
840+
use std::os::unix::fs::PermissionsExt;
841+
842+
let dir = tempfile::tempdir().unwrap();
843+
let path = dir.path().join("index.js");
844+
let original = b"original";
845+
let patched = b"patched content";
846+
let patched_hash = compute_git_sha256_from_bytes(patched);
847+
848+
tokio::fs::write(&path, original).await.unwrap();
849+
// 0o444 = r--r--r--. Owner has no write bit.
850+
tokio::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o444))
851+
.await
852+
.unwrap();
853+
854+
apply_file_patch(dir.path(), "index.js", patched, &patched_hash)
855+
.await
856+
.unwrap();
857+
858+
// Content updated.
859+
let written = tokio::fs::read(&path).await.unwrap();
860+
assert_eq!(written, patched);
861+
// Mode preserved bit-for-bit.
862+
let mode_after = tokio::fs::metadata(&path).await.unwrap().permissions().mode()
863+
& 0o7777;
864+
assert_eq!(
865+
mode_after, 0o444,
866+
"mode must be restored to the pre-patch value after the write"
867+
);
868+
}
869+
870+
/// Non-default mode (e.g. 0o755 for an executable script) survives
871+
/// the patch round-trip unchanged.
872+
#[cfg(unix)]
873+
#[tokio::test]
874+
async fn test_apply_file_patch_preserves_executable_mode() {
875+
use std::os::unix::fs::PermissionsExt;
876+
877+
let dir = tempfile::tempdir().unwrap();
878+
let path = dir.path().join("bin.sh");
879+
let original = b"#!/bin/sh\necho old\n";
880+
let patched = b"#!/bin/sh\necho new\n";
881+
let patched_hash = compute_git_sha256_from_bytes(patched);
882+
883+
tokio::fs::write(&path, original).await.unwrap();
884+
tokio::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755))
885+
.await
886+
.unwrap();
887+
888+
apply_file_patch(dir.path(), "bin.sh", patched, &patched_hash)
889+
.await
890+
.unwrap();
891+
892+
let mode_after = tokio::fs::metadata(&path).await.unwrap().permissions().mode()
893+
& 0o7777;
894+
assert_eq!(mode_after, 0o755);
895+
}
896+
897+
/// New file created by the patch: default mode is read-only (0o444)
898+
/// and the parent directory's uid/gid get inherited (the uid/gid
899+
/// check is a smoke test — running as a regular user the new file
900+
/// would already inherit the user's uid, but the test still locks
901+
/// in that the new file's uid matches the parent's, which is what
902+
/// the chown call enforces).
903+
#[cfg(unix)]
904+
#[tokio::test]
905+
async fn test_apply_file_patch_new_file_is_readonly_and_inherits_dir_owner() {
906+
use std::os::unix::fs::{MetadataExt, PermissionsExt};
907+
908+
let dir = tempfile::tempdir().unwrap();
909+
let nested = "new-dir/new.js";
910+
let patched = b"brand new file content\n";
911+
let patched_hash = compute_git_sha256_from_bytes(patched);
912+
913+
// File does not yet exist — this is the new-file path.
914+
apply_file_patch(dir.path(), nested, patched, &patched_hash)
915+
.await
916+
.unwrap();
917+
918+
let path = dir.path().join(nested);
919+
// Default new-file mode is 0o444.
920+
let mode = tokio::fs::metadata(&path).await.unwrap().permissions().mode()
921+
& 0o7777;
922+
assert_eq!(mode, 0o444, "new files default to read-only");
923+
924+
// uid/gid inherited from the parent directory.
925+
let parent_meta = tokio::fs::metadata(path.parent().unwrap()).await.unwrap();
926+
let file_meta = tokio::fs::metadata(&path).await.unwrap();
927+
assert_eq!(file_meta.uid(), parent_meta.uid());
928+
assert_eq!(file_meta.gid(), parent_meta.gid());
929+
}
930+
931+
/// Existing patched file's uid/gid survive the round-trip. We can
932+
/// only verify "uid stays the same" without root, but that's
933+
/// enough to catch a regression that accidentally clobbered ownership.
934+
#[cfg(unix)]
935+
#[tokio::test]
936+
async fn test_apply_file_patch_preserves_uid_gid() {
937+
use std::os::unix::fs::MetadataExt;
938+
939+
let dir = tempfile::tempdir().unwrap();
940+
let path = dir.path().join("index.js");
941+
let original = b"orig";
942+
let patched = b"new";
943+
let patched_hash = compute_git_sha256_from_bytes(patched);
944+
945+
tokio::fs::write(&path, original).await.unwrap();
946+
let pre = tokio::fs::metadata(&path).await.unwrap();
947+
948+
apply_file_patch(dir.path(), "index.js", patched, &patched_hash)
949+
.await
950+
.unwrap();
951+
952+
let post = tokio::fs::metadata(&path).await.unwrap();
953+
assert_eq!(pre.uid(), post.uid());
954+
assert_eq!(pre.gid(), post.gid());
955+
}
956+
708957
#[tokio::test]
709958
async fn test_apply_package_patch_success() {
710959
let pkg_dir = tempfile::tempdir().unwrap();

0 commit comments

Comments
 (0)