From 6830b65cc9a94e1e5d7ef0119cd24b8a844b22c1 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:22:43 -0400 Subject: [PATCH 1/2] Make Go module copies writable on Windows Go extracts module-cache files read-only, and on Windows that is the read-only attribute. apply and vendor copy the module out of the cache and patch the copy with stage + rename, but Windows refuses to rename over a read-only file, so every Go apply/vendor on Windows failed with "Access is denied. (os error 5)" and nothing was patched. fresh_copy now grants owner-write on each copied file that arrived read-only. The copy is a fresh private inode, so the module cache and any shared inode keep their permissions. Fixes #346 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../socket-patch-core/src/patch/copy_tree.rs | 66 +++++++++++++++++-- .../src/patch/redirect/golang_local.rs | 47 +++++++++++++ 2 files changed, 109 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-core/src/patch/copy_tree.rs b/crates/socket-patch-core/src/patch/copy_tree.rs index 5d2fb5b48..0fefe3244 100644 --- a/crates/socket-patch-core/src/patch/copy_tree.rs +++ b/crates/socket-patch-core/src/patch/copy_tree.rs @@ -18,10 +18,12 @@ fn to_io(e: E) -> std::io::Error { /// Directories are created fresh (writable, subject to umask) rather than /// mirroring the cache's read-only modes, so the copy can be patched and later /// removed without a chmod dance. File *contents* are copied via -/// `std::fs::copy`, which also carries the source's mode bits (often `0o444` in -/// the cache); the downstream apply pipeline replaces files via stage + -/// rename (no write grant needed), and -/// [`remove_tree`] relaxes perms on cleanup. Symlinks / specials are skipped — +/// `std::fs::copy`, which also carries the source's permissions (`0o444` / +/// the Windows read-only attribute in Go's module cache), so each copied file +/// that lands read-only is then granted owner-write: the copy is a private, +/// freshly created inode, and the apply pipeline's stage + rename cannot +/// replace a read-only destination on Windows (`MoveFileEx` refuses it with +/// `ERROR_ACCESS_DENIED`, #346). [`remove_tree`] relaxes perms on cleanup. Symlinks / specials are skipped — /// crates.io registry and Go module-cache sources contain none, and copying a /// dangling link would be unsafe. pub(crate) async fn fresh_copy( @@ -78,11 +80,35 @@ fn copy_tree_blocking(src: &Path, dst: &Path, skip_file_name: Option<&str>) -> s } } std::fs::copy(entry.path(), &target)?; + make_owner_writable(&target)?; } } Ok(()) } +/// Grant owner-write on a file [`copy_tree_blocking`] just created, when +/// the copy carried a read-only mode/attribute over from its source. Only +/// ever called on the fresh inode `std::fs::copy` wrote, never on a source +/// or a link, so no shared inode's permissions change. +fn make_owner_writable(path: &Path) -> std::io::Result<()> { + let mut perms = std::fs::metadata(path)?.permissions(); + if !perms.readonly() { + return Ok(()); + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + perms.set_mode(perms.mode() | 0o200); + } + #[cfg(not(unix))] + { + // Windows: clears the read-only attribute, nothing else. + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); + } + std::fs::set_permissions(path, perms) +} + /// The previous [`copy_tree_blocking`], which created every file's parent, /// kept as the equivalence oracle. #[cfg(test)] @@ -113,6 +139,7 @@ fn copy_tree_blocking_reference( std::fs::create_dir_all(p)?; } std::fs::copy(entry.path(), &target)?; + make_owner_writable(&target)?; } } Ok(()) @@ -513,6 +540,37 @@ mod tests { fs::set_permissions(src.path().join("ro"), fs::Permissions::from_mode(0o755)).unwrap(); } + /// #346: a read-only source file (Go's module cache; the Windows + /// read-only attribute) yields an owner-writable copy, so the apply + /// pipeline's stage + rename can replace it on every platform. The + /// source keeps its read-only permission. + #[tokio::test] + async fn fresh_copy_files_are_writable_even_from_readonly_source() { + let src = tempfile::tempdir().unwrap(); + let dst = tempfile::tempdir().unwrap(); + let d = dst.path().join("copy"); + let f = src.path().join("lib.go"); + fs::write(&f, b"package lib\n").unwrap(); + let mut ro = fs::metadata(&f).unwrap().permissions(); + ro.set_readonly(true); + fs::set_permissions(&f, ro).unwrap(); + + fresh_copy(src.path(), &d, None).await.unwrap(); + + let copied = fs::metadata(d.join("lib.go")).unwrap().permissions(); + assert!(!copied.readonly(), "the copy must be writable: {copied:?}"); + #[cfg(unix)] + assert_eq!(copied.mode() & 0o777, 0o644, "only owner-write is added"); + assert!(fs::metadata(&f).unwrap().permissions().readonly()); + assert_eq!(fs::read(d.join("lib.go")).unwrap(), b"package lib\n"); + + // Let the temp dir clean up on Windows. + let mut rw = fs::metadata(&f).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + rw.set_readonly(false); + fs::set_permissions(&f, rw).unwrap(); + } + #[cfg(unix)] #[tokio::test] async fn remove_tree_does_not_follow_symlink_out_of_tree() { diff --git a/crates/socket-patch-core/src/patch/redirect/golang_local.rs b/crates/socket-patch-core/src/patch/redirect/golang_local.rs index 2c5868ec2..b8c47b1df 100644 --- a/crates/socket-patch-core/src/patch/redirect/golang_local.rs +++ b/crates/socket-patch-core/src/patch/redirect/golang_local.rs @@ -878,6 +878,53 @@ mod tests { assert_eq!(e.version.as_deref(), Some(VERSION)); } + /// #346: Go extracts module-cache files read-only (on Windows, the + /// read-only attribute). The copy must still be patchable — Windows' + /// rename refuses to replace a read-only destination — and the cache + /// itself must stay read-only and pristine. + #[tokio::test] + async fn test_apply_redirect_over_a_read_only_module_cache() { + let (dir, blobs, pristine, files, _after) = fixture().await; + let root = dir.path(); + let set_readonly = |readonly: bool| { + for name in ["bar.go", "go.mod"] { + let path = pristine.join(name); + let mut perms = std::fs::metadata(&path).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(readonly); + std::fs::set_permissions(&path, perms).unwrap(); + } + }; + set_readonly(true); + let sources = PatchSources::blobs_only(&blobs); + + for base in [GO_PATCHES_DIR, ".socket/vendor/golang/u"] { + let result = apply_go_redirect( + PURL, + MODULE, + VERSION, + &pristine, + root, + base, + &files, + &sources, + None, + false, + MismatchPolicy::Warn, + ) + .await; + assert!(result.success, "{base}: apply failed: {:?}", result.error); + let copy = root.join(base).join("github.com/foo/bar@v1.4.2"); + assert_eq!(std::fs::read(copy.join("bar.go")).unwrap(), PATCHED); + } + assert_eq!(std::fs::read(pristine.join("bar.go")).unwrap(), PRISTINE); + assert!(std::fs::metadata(pristine.join("bar.go")) + .unwrap() + .permissions() + .readonly()); + set_readonly(false); + } + #[tokio::test] async fn test_apply_is_idempotent_byte_for_byte() { let (dir, blobs, pristine, files, _after) = fixture().await; From f7bc3d9612012f47a03f431a793d152bbac054fc Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 18:26:30 -0400 Subject: [PATCH 2/2] Drop stale uuid arg from new Go read-only redirect test The merge group failed `cargo check --all-targets` (lib test target): the read-only module-cache test added by this PR called apply_go_redirect with 11 arguments, but #1049 ("Remove --download-mode and the diff download path") removed the `uuid: Option<&str>` parameter on main. Pass the current 10-argument signature; behavior and test coverage are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-core/src/patch/redirect/golang_local.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/socket-patch-core/src/patch/redirect/golang_local.rs b/crates/socket-patch-core/src/patch/redirect/golang_local.rs index f567d0f5f..42a95d1e3 100644 --- a/crates/socket-patch-core/src/patch/redirect/golang_local.rs +++ b/crates/socket-patch-core/src/patch/redirect/golang_local.rs @@ -904,7 +904,6 @@ mod tests { base, &files, &sources, - None, false, MismatchPolicy::Warn, )