diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index b02fa44fa..40e169110 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -76,7 +76,7 @@ Every subcommand accepts the same set of "global" flags via a single shared `Glo `--offline` means the same thing on every command (v3.0): never contact the network, fail loudly when a required local source is missing. On `repair`, `--offline` and `--download-only` are mutually exclusive (exit 2). `scan` and `get` need remote data for their core function (patch discovery / patch fetch), so `--offline` refuses them up front — exit 1 with an error naming the offline gate (JSON: `status: "error"`), before any crawl, client build, or network contact. This covers `scan --vendor` too: offline vendored staging is `vendor --offline`'s job. -The `--strict` mismatch policy applies to the in-place apply paths (apply/get/scan --apply/hook/go redirect). DEFAULT (v3.4): a file whose on-disk content matches neither the patch's beforeHash nor its afterHash is overwritten with the FULL verified patched content (the diff strategy self-disables on a wrong base; archive/blob writes are hash-gated to exactly afterHash; the missing blob is downloaded on demand) and surfaced as a `content_mismatch_overwritten` stderr warning + Skipped event. `--strict` turns that case into a hard error. `--force` overrides `--strict` and additionally skips missing files. Vendor staging is unaffected (it always auto-overwrites into its private stage). +The `--strict` mismatch policy applies to the in-place apply paths (apply/get/scan --apply/hook/go redirect). DEFAULT (v3.4): a file whose on-disk content matches neither the patch's beforeHash nor its afterHash is overwritten with the FULL verified patched content (the diff strategy self-disables on a wrong base; archive/blob writes are hash-gated to exactly afterHash; the missing blob is downloaded on demand) and surfaced as a `content_mismatch_overwritten` stderr warning + Skipped event. A file the patch adds (empty beforeHash) that already exists with other content is the same case. `--strict` turns that case into a hard error. Rollback of an added file deletes it; the content it replaced is not kept. `--force` overrides `--strict` and additionally skips missing files. Vendor staging is unaffected (it always auto-overwrites into its private stage). ## Per-subcommand arguments @@ -601,6 +601,17 @@ the purl its own `package.json` names, so `apply`, `rollback` and `vex` cover `n beside any plain `node_modules/left-pad` copy. Only real package dirs count: a link is a dependency edge, never an alias copy of its own. +### Links under `.socket/` and write containment (v5.0) + +socket-patch creates `.socket/` and everything under it itself and never writes a symlink or junction there, so a linked level below the project root is never its own. The rules below share one check (core `utils::containment`); each refusal names the linked path and carries the substring `is a symlink`. Levels at or above the project path (a symlinked home directory, `/tmp -> /private/tmp`) are never checked. + +- **Blob and diff cache writes** (agent-mode `get`, `scan --apply`, `apply` and `repair` downloads, and `rollback`'s before-blob fetch): a linked `.socket/blobs` or `.socket/diffs`, or a linked `.socket/blobs/` entry, is refused before anything is written. That blob is reported failed (a `get` patch fails as a whole, and its own new blobs are unwound); nothing is written at the link's target. A project that pointed `.socket/blobs` at a shared cache must replace the link with a real directory. +- **Inline blobs in `get`**: a patch view's `blobContent` / `beforeBlobContent` must hash (git-sha256) to the `afterHash` / `beforeHash` it is stored under, or the patch fails with `content hash mismatch: content hashes to ` before anything is written, the same rule a downloaded blob is held to. An existing blob that already verifies is never rewritten; every blob is staged and renamed into place, never truncated in place. +- **Ledgers**: the vendored ledger (`.socket/vendor/state.json`) and the pre-v5 hosted redirect ledger are refused when `.socket` itself, a directory below it or the ledger file is a link (`vendor_dir_symlink_unsupported` for the vendored flows), so a hosted run that still has to update a pre-v5 redirect ledger fails under a linked `.socket`. +- **Agent-mode apply and rollback outside the install tree**: `apply` and `rollback` (dry run included) refuse a package whose written directories resolve outside the install tree they were found in: a Composer path repository (`vendor//` linked to first-party source), a `flit install --symlink` / editable-by-link package, or a package directory a package manager links into `site-packages` from its own prefix (Homebrew's Cellar, a Nix store path). The per-package error carries `outside the install tree`. For `apply` the remedy is to patch that source directly or install the package as a copy; for `rollback` it is to restore that source from version control, since socket-patch does not write into a tree it does not own. + +Not covered: the agent-mode blob cache and manifest are checked from `.socket` down, so a `.socket` that is itself a link still redirects those writes (deciding that at lock time is deferred). + ## Vendor command contract `vendor` is `apply`'s committable sibling: instead of patching installed packages in place @@ -1283,7 +1294,7 @@ Every `--json` invocation emits a single JSON object that follows the **unified | `offline_eject_unavailable` | top-level `errorCode` | vendor eject under `--offline` / `SOCKET_OFFLINE` (v5.0): records and registry entries cannot be fetched offline; zero network requests, nothing touched, exit 1. | | `hosted_wiring_contested` | top-level `errorCode` (list: warning when it can still list) | rollback / remove / vendor eject / list (v5.0): a lockfile mentions a recognized hosted patch uuid that discovery rejected (or a pin with no lockfile), so the hosted set is not known exactly; refused with nothing touched, exit 1. Remedy: fix or `git checkout` the named lockfile. | | `vendor_pnpm_settings_elsewhere` | `failed` | vendor / scan / get `--mode vendored` (pnpm, v9 lock): the project directory is a pnpm workspace member with its own `pnpm-lock.yaml` and no `pnpm-workspace.yaml` of its own; pnpm reads `overrides:` only from the nearest ancestor `pnpm-workspace.yaml`, so an override wired into the member (its `package.json` or a nested workspace file) would be ignored, failing frozen installs on pnpm >= 11 and silently reinstalling the unpatched package on a plain install. Refused before any write (the pre-download preflight and `--dry-run` included); the detail names the governing file; remedy: `--mode hosted`. | -| `vendor_dir_symlink_unsupported` | `failed` | vendor / scan / get `--mode vendored` (every ecosystem): `.socket/vendor`, `.socket/vendor/` or the patch's `` dir is a symlink or junction. socket-patch creates those directories itself and never writes links, so a linked one is not ours; its target may be another project's vendor store. Refused before any write. The vendored revert (`vendor --revert`, `rollback`, `remove`, the vendored → hosted takeover) fails on the same check with the same detail before it edits a lock or deletes anything, so it never deletes another project's artifacts through the link. The detail names the linked path. Remedy: replace the link with a real directory and re-run. | +| `vendor_dir_symlink_unsupported` | `failed` | vendor / scan / get `--mode vendored` (every ecosystem): `.socket` itself (#887), `.socket/vendor`, `.socket/vendor/` or the patch's `` dir is a symlink or junction. socket-patch creates those directories itself and never writes links, so a linked one is not ours; its target may be another project's vendor store. Refused before any write. The vendored revert (`vendor --revert`, `rollback`, `remove`, the vendored → hosted takeover) fails on the same check with the same detail before it edits a lock or deletes anything, so it never deletes another project's artifacts through the link. The detail names the linked path. Remedy: replace the link with a real directory and re-run. | | `vendor_yarn_berry_cache_unsupported` | `failed` | vendor (yarn berry): lock `cacheKey ≠ 10c0` or non-default `.yarnrc.yml` `compressionLevel` — the cache-zip checksum is not reproducible. | | `vendor_yarn_berry_mixed_line_endings` | `failed` | vendor (yarn berry): `yarn.lock` or the root `package.json` mixes CRLF and LF line endings (or holds a bare CR) — no single ending can be kept, and yarn rewrites such a file wholesale on its next install (a mixed lock also fails `--immutable`, YN0028). Refused before any write; `yarn install` normalizes the files. A uniformly CRLF pair is vendored in CRLF. A hosted→vendored takeover (`vendor`, `scan`/`get --mode vendored`) raises this — and the berry `vendor_yarn_berry_cache_unsupported` gates — BEFORE restoring the hosted pin's upstream entry (dry run too), so a refused purl stays hosted. | | `vendor_override_conflict` | `failed` | vendor (pnpm/yarn-berry): a user-authored override/resolution for the package already exists. | @@ -1306,7 +1317,7 @@ Every `--json` invocation emits a single JSON object that follows the **unified | `vendor_artifact_redownload_failed` | `failed` | repair: download unavailable, integrity mismatch, or downloaded bytes/inventory differ from the ledger. Existing files are preserved. | | `vendor_artifact_unrepairable` | `failed` | repair: the ledger identity or patch record cannot be trusted or recovered. | | `vendor_uuid_mismatch` | `skipped` | repair: the manifest's patch uuid moved past the vendored artifact — a re-vendor (`vendor` / `scan --vendor`) is pending; repair does not cross patch generations. | -| `content_mismatch_overwritten` | `skipped` (warning) | apply (default policy): a file matched NEITHER beforeHash nor afterHash and was overwritten with the full verified patched content. `--strict` turns this case into a `failed` event instead. | +| `content_mismatch_overwritten` | `skipped` (warning) | apply (default policy): a file matched NEITHER beforeHash nor afterHash and was overwritten with the full verified patched content. This includes a file the patch adds (empty beforeHash) that already exists with other content. `--strict` turns this case into a `failed` event instead. | | `vendor_lock_checksums_unsupported` / `vendor_stale_lock_checksum` | `failed` | vendor (gem): an ambiguous/platform CHECKSUMS entry, or a v1-wired lock whose stale token blocks the hot path (run `vendor --revert` + re-vendor). | | `redirect_pypi_stale_install` | `redirect.warnings[]` (warning) | Hosted Python redirect: readable installed files differ from patched hashes. Read-only, repeated on re-scan, and excludes the package from same-run VEX. See the "Python stale-install guard" section. | | `redirect_gem_stale_install` | `redirect.warnings[]` (warning) | scan `--mode hosted` (gem): a stale UNPATCHED materialization (installed gem, or committed archive in bundler's cache dir — `vendor/cache` unless `cache_path` moves it) that `bundle install` will reuse instead of fetching the redirected patch; the detail carries the verified remedy. Full rules and flavors: the "Gem stale-install guard" section. | diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index d3a23819f..1cd8d8185 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -1806,7 +1806,7 @@ fn print_verbose_verification(results: &[ApplyResult]) { println!(" current: {h}"); } if let Some(ref h) = f.expected_hash { - println!(" expected: {h}"); + println!(" expected: {}", expected_hash_label(h)); } if let Some(ref h) = f.target_hash { println!(" target: {h}"); @@ -1815,6 +1815,18 @@ fn print_verbose_verification(results: &[ApplyResult]) { } } +/// The verbose `expected:` value: a new-file collision carries an empty +/// expected hash (the patch adds the file, so there is no beforeHash; see +/// core `mark_new_file_collision`), shown as `(new file)` instead of a +/// blank line. +fn expected_hash_label(hash: &str) -> &str { + if hash.is_empty() { + "(new file)" + } else { + hash + } +} + /// One gem-env fallback-home copy the fan-out skipped best-effort (a /// bundle-store copy applied; this shared-home copy mismatched or failed /// to write). Carries what the warning must name: the package, the copy's @@ -3108,6 +3120,15 @@ mod tests { AppliedVia as CoreAppliedVia, ApplyResult, VerifyResult, VerifyStatus, }; + /// A new-file collision's empty expected hash reads `(new file)` in the + /// verbose summary; a real hash is shown as is. + #[test] + fn expected_hash_label_names_a_new_file_collision() { + assert_eq!(expected_hash_label(""), "(new file)"); + let h = "a".repeat(64); + assert_eq!(expected_hash_label(&h), h); + } + /// Build a successful `ApplyResult` with one patched file and one /// verified file. Used as the base for action-routing tests. fn sample_applied(status: VerifyStatus) -> ApplyResult { diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index 0cbd9ce4b..65a8dc7c8 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -273,13 +273,17 @@ fn report_lock_failure( envelope } -/// Decode a base64 string and write it to `blobs_dir/hash`. Returns whether -/// the blob file was NEWLY created (`false`: a blob with this hash already -/// existed — content-addressed, so it is the same bytes — and was -/// overwritten in place), or a formatted error string referencing -/// `file_path` and `label` on failure. +/// Decode a base64 string and store it as the blob `blobs_dir/hash` +/// through the one verified writer, +/// [`store_verified_blob`](socket_patch_core::api::blob_fetcher::store_verified_blob): +/// the bytes must hash to `hash`, a linked `.socket/blobs` or +/// `.socket/blobs/` is refused, and the entry is staged and renamed +/// (#726). Returns whether the blob file was NEWLY created (`false`: a +/// verified blob with this hash already existed and was left untouched), +/// or a formatted error string referencing `file_path` and `label` on +/// failure. /// -/// `blobs_dir` is created here, lazily — only once a blob is actually +/// `blobs_dir` is created there, lazily — only once a blob is actually /// about to be persisted — so a run that records nothing (every fetch /// failed, every patch skipped, undecodable content) leaves no empty /// `.socket/blobs/` behind. @@ -297,18 +301,9 @@ async fn write_blob_entry( } let decoded = base64_decode(b64).map_err(|e| format!("Failed to decode {label} for {file_path}: {e}"))?; - tokio::fs::create_dir_all(blobs_dir) + socket_patch_core::api::blob_fetcher::store_verified_blob(blobs_dir, hash, &decoded) .await - .map_err(|e| format!("Failed to create blobs directory: {e}"))?; - let target = blobs_dir.join(hash); - // Probed BEFORE the (overwriting) write: a blob that already existed — - // a live record's revert data, or a sibling patch's shared after-blob - // written earlier this run — is never this call's to remove on unwind. - let existed = tokio::fs::try_exists(&target).await.unwrap_or(false); - tokio::fs::write(&target, &decoded) - .await - .map_err(|e| format!("Failed to write {label} for {file_path}: {e}"))?; - Ok(!existed) + .map_err(|e| format!("Failed to write {label} for {file_path} ({hash}): {e}")) } /// Write every after/before blob for `patch` into `blobs_dir`, reporting @@ -3972,42 +3967,27 @@ async fn run_get_vendored( } } -/// Decode a patch view's `blobContent` (canonical, padded base64 as the API -/// produces it). Hand-rolled; swapping in -/// `base64::engine::general_purpose::STANDARD.decode(input)` must keep -/// `DecodeError::InvalidByte(_, b)` mapped to the -/// `Invalid base64 character: ` message below (pinned by a unit test). +/// Decode a patch view's `blobContent` (canonical base64 as the API +/// produces it; line breaks and missing padding are tolerated). An invalid +/// byte keeps the `Invalid base64 character: ` message (pinned by a +/// unit test). pub(crate) fn base64_decode(input: &str) -> Result, String> { - let chars = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; - let mut table = [255u8; 256]; - for (i, &c) in chars.iter().enumerate() { - table[c as usize] = i as u8; - } - - let input = input.as_bytes(); - let mut output = Vec::with_capacity(input.len() * 3 / 4); - - let mut buf = 0u32; - let mut bits = 0u32; - - for &b in input { - if b == b'=' || b == b'\n' || b == b'\r' { - continue; - } - let val = table[b as usize]; - if val == 255 { - return Err(format!("Invalid base64 character: {}", b as char)); - } - buf = (buf << 6) | val as u32; - bits += 6; - if bits >= 8 { - bits -= 8; - output.push((buf >> bits) as u8); - buf &= (1 << bits) - 1; + use base64::engine::{general_purpose, DecodePaddingMode, GeneralPurpose}; + use base64::Engine; + const ENGINE: GeneralPurpose = GeneralPurpose::new( + &base64::alphabet::STANDARD, + general_purpose::PAD.with_decode_padding_mode(DecodePaddingMode::Indifferent), + ); + let compact: String = input + .chars() + .filter(|c| !matches!(c, '\n' | '\r')) + .collect(); + ENGINE.decode(compact).map_err(|e| match e { + base64::DecodeError::InvalidByte(_, b) => { + format!("Invalid base64 character: {}", b as char) } - } - - Ok(output) + other => format!("Invalid base64: {other}"), + }) } #[cfg(test)] @@ -4966,6 +4946,8 @@ mod tests { // "patched\n" in base64 — a valid payload so only the hash is at fault. const BLOB_B64: &str = "cGF0Y2hlZAo="; + /// base64 of `"pristine\n"`. + const PRISTINE_B64: &str = "cHJpc3RpbmUK"; #[tokio::test] async fn write_blob_entry_rejects_relative_traversal_hash() { @@ -5023,14 +5005,93 @@ mod tests { let blobs_dir = tmp.path().join("blobs"); tokio::fs::create_dir_all(&blobs_dir).await.unwrap(); - let hash = "1111111111111111111111111111111111111111111111111111111111111111"; - write_blob_entry(&blobs_dir, BLOB_B64, hash, "package/index.js", "blob") + let hash = git_sha256(b"patched\n"); + let created = write_blob_entry(&blobs_dir, BLOB_B64, &hash, "package/index.js", "blob") .await .expect("a canonical 64-hex hash must be accepted"); - let written = std::fs::read(blobs_dir.join(hash)).unwrap(); + assert!(created); + let written = std::fs::read(blobs_dir.join(&hash)).unwrap(); assert_eq!(written, b"patched\n"); } + fn git_sha256(bytes: &[u8]) -> String { + socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes(bytes) + } + + /// #726: inline content that does not hash to its name writes nothing, + /// and a verified blob already in the store is never replaced. + #[tokio::test] + async fn write_blob_entry_verifies_content_against_its_hash() { + let tmp = tempfile::tempdir().unwrap(); + let blobs_dir = tmp.path().join("blobs"); + + let wrong = "1111111111111111111111111111111111111111111111111111111111111111"; + let err = write_blob_entry(&blobs_dir, BLOB_B64, wrong, "package/index.js", "blob") + .await + .unwrap_err(); + assert!(err.contains("content hash mismatch"), "{err}"); + assert!(!blobs_dir.join(wrong).exists(), "nothing written"); + + let pristine = git_sha256(b"pristine\n"); + tokio::fs::create_dir_all(&blobs_dir).await.unwrap(); + tokio::fs::write(blobs_dir.join(&pristine), b"pristine\n") + .await + .unwrap(); + write_blob_entry(&blobs_dir, BLOB_B64, &pristine, "package/index.js", "blob") + .await + .unwrap_err(); + assert_eq!( + std::fs::read(blobs_dir.join(&pristine)).unwrap(), + b"pristine\n", + "a verified blob is byte-identical afterwards" + ); + let created = write_blob_entry(&blobs_dir, PRISTINE_B64, &pristine, "f", "blob") + .await + .unwrap(); + assert!(!created, "an existing verified blob is not re-created"); + } + + /// B24: a committed `.socket/blobs/` (or `.socket/blobs`) symlink + /// must not redirect the write out of the project. + #[cfg(unix)] + #[tokio::test] + async fn write_blob_entry_refuses_a_linked_blob_or_blobs_dir() { + let tmp = tempfile::tempdir().unwrap(); + let victim = tmp.path().join("victim"); + std::fs::write(&victim, b"precious").unwrap(); + let hash = git_sha256(b"patched\n"); + + let socket = tmp.path().join("p/.socket"); + let blobs_dir = socket.join("blobs"); + std::fs::create_dir_all(&blobs_dir).unwrap(); + std::os::unix::fs::symlink(&victim, blobs_dir.join(&hash)).unwrap(); + let err = write_blob_entry(&blobs_dir, BLOB_B64, &hash, "package/index.js", "blob") + .await + .unwrap_err(); + assert!(err.contains("is a symlink"), "{err}"); + assert_eq!(std::fs::read(&victim).unwrap(), b"precious"); + + let outside = tmp.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + let socket2 = tmp.path().join("q/.socket"); + std::fs::create_dir_all(&socket2).unwrap(); + std::os::unix::fs::symlink(&outside, socket2.join("blobs")).unwrap(); + let err = write_blob_entry(&socket2.join("blobs"), BLOB_B64, &hash, "f", "blob") + .await + .unwrap_err(); + assert!(err.contains("is a symlink"), "{err}"); + assert!( + !outside.join(&hash).exists(), + "nothing written through the link" + ); + } + + #[test] + fn base64_decode_tolerates_line_breaks_and_missing_padding() { + assert_eq!(base64_decode("cGF0\nY2hlZAo=").unwrap(), b"patched\n"); + assert_eq!(base64_decode("cGF0Y2hlZAo").unwrap(), b"patched\n"); + } + // --- short_uuid ------------------------------------------------------ // The `[update]` log line prints the first 8 chars of the manifest's // existing UUID. A naive `&uuid[..8]` panics on a short or non-ASCII @@ -5238,16 +5299,22 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let blobs_dir = tmp.path().join(".socket/blobs"); - let after = "a".repeat(64); + // A REAL git-sha256 name, so the after-blob verifies and lands + // before the before-blob's traversal hash is rejected. + let after = git_sha256(b"patched\n"); let mut files = HashMap::new(); let mut info = file_resp(Some("../escaped"), Some(&after)); info.blob_content = Some(BLOB_B64.to_string()); - info.before_blob_content = Some(BLOB_B64.to_string()); + info.before_blob_content = Some(PRISTINE_B64.to_string()); files.insert("package/index.js".to_string(), info); let patch = patch_with_files(files); let res = write_all_patch_blobs(&blobs_dir, &patch, /*quiet=*/ true).await; assert_eq!(res, Err(())); + assert!( + !tmp.path().join(".socket/escaped").exists(), + "nothing is written at the traversal target" + ); assert!( !blobs_dir.join(&after).exists(), "the after-blob written before the failure is unwound" @@ -5267,7 +5334,7 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let blobs_dir = tmp.path().join(".socket/blobs"); tokio::fs::create_dir_all(&blobs_dir).await.unwrap(); - let after = "a".repeat(64); + let after = git_sha256(b"patched\n"); tokio::fs::write(blobs_dir.join(&after), b"patched\n") .await .unwrap(); @@ -5275,7 +5342,7 @@ mod tests { let mut files = HashMap::new(); let mut info = file_resp(Some("../escaped"), Some(&after)); info.blob_content = Some(BLOB_B64.to_string()); - info.before_blob_content = Some(BLOB_B64.to_string()); + info.before_blob_content = Some(PRISTINE_B64.to_string()); files.insert("package/index.js".to_string(), info); let patch = patch_with_files(files); @@ -5288,11 +5355,11 @@ mod tests { ); // And a fully successful write reports exactly the NEW hashes. - let before = "b".repeat(64); + let before = git_sha256(b"pristine\n"); let mut files = HashMap::new(); let mut info = file_resp(Some(&before), Some(&after)); info.blob_content = Some(BLOB_B64.to_string()); - info.before_blob_content = Some(BLOB_B64.to_string()); + info.before_blob_content = Some(PRISTINE_B64.to_string()); files.insert("package/index.js".to_string(), info); let created = write_all_patch_blobs(&blobs_dir, &patch_with_files(files), true) .await diff --git a/crates/socket-patch-cli/tests/covgap_commands_get.rs b/crates/socket-patch-cli/tests/covgap_commands_get.rs index c98f057a7..40a8b7c6f 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_get.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_get.rs @@ -156,7 +156,9 @@ fn good_files() -> serde_json::Value { serde_json::json!({ "package/index.js": { "beforeHash": "0".repeat(64), - "afterHash": "1".repeat(64), + // The real git-sha256 of the content: inline blobs are + // verified against their name before they are stored (#726). + "afterHash": git_hash(b"patched\n"), "blobContent": "cGF0Y2hlZAo=", // base64("patched\n") } }) diff --git a/crates/socket-patch-cli/tests/get/get_invariants.rs b/crates/socket-patch-cli/tests/get/get_invariants.rs index c79166973..c3c860002 100644 --- a/crates/socket-patch-cli/tests/get/get_invariants.rs +++ b/crates/socket-patch-cli/tests/get/get_invariants.rs @@ -12,8 +12,10 @@ use crate::common; const ORG_SLUG: &str = "test-org"; const UUID: &str = "11111111-1111-4111-8111-111111111111"; -/// The `afterHash` embedded in `patch_response_json`; also the blob filename. -const AFTER_HASH: &str = "1111111111111111111111111111111111111111111111111111111111111111"; +/// The `afterHash` embedded in `patch_response_json` (the git-sha256 of +/// `BLOB_BYTES`, which `get` verifies inline blobs against); also the blob +/// filename. +const AFTER_HASH: &str = "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e"; /// base64 "cGF0Y2hlZAo=" decodes to exactly these bytes. const BLOB_BYTES: &[u8] = b"patched\n"; @@ -44,9 +46,9 @@ fn run_get(cwd: &Path, api_url: &str, identifier: &str, extra: &[&str]) -> (i32, /// PatchResponse JSON suitable as a `view/{uuid}` response. All fields /// are camelCase as the binary expects. fn patch_response_json(purl: &str, uuid: &str) -> serde_json::Value { - // base64 of "patched\n" — content is arbitrary, the save path - // doesn't verify content hash. The afterHash value is what gets - // used as the blob filename. + // base64 of "patched\n". `get` verifies an inline blob against the + // hash it is stored under, so afterHash is its real git-sha256; it is + // also the blob filename. serde_json::json!({ "uuid": uuid, "purl": purl, @@ -54,7 +56,7 @@ fn patch_response_json(purl: &str, uuid: &str) -> serde_json::Value { "files": { "package/index.js": { "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "afterHash": AFTER_HASH, "blobContent": "cGF0Y2hlZAo=", } }, diff --git a/crates/socket-patch-cli/tests/get/get_update_summary_e2e.rs b/crates/socket-patch-cli/tests/get/get_update_summary_e2e.rs index e68000d30..bf2bbddd0 100644 --- a/crates/socket-patch-cli/tests/get/get_update_summary_e2e.rs +++ b/crates/socket-patch-cli/tests/get/get_update_summary_e2e.rs @@ -75,7 +75,7 @@ async fn mount_mocks(mock: &MockServer) { "files": { "package/index.js": { "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "afterHash": common::git_sha256(b"patched\n"), "blobContent": "cGF0Y2hlZAo=", } }, diff --git a/crates/socket-patch-cli/tests/in_process_get.rs b/crates/socket-patch-cli/tests/in_process_get.rs index 5a6bd17fa..d146bd065 100644 --- a/crates/socket-patch-cli/tests/in_process_get.rs +++ b/crates/socket-patch-cli/tests/in_process_get.rs @@ -54,7 +54,7 @@ async fn make_view_mock(server: &MockServer, uuid: &str, purl: &str, tier: &str) "files": { "package/index.js": { "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", // base64("patched\n") } }, @@ -114,7 +114,9 @@ async fn start_wiremock() -> (MockServer, String) { /// The after_hash declared by `make_view_mock` and the exact decoded bytes /// of its `blobContent` (`base64("patched\n")`). Derived here independently /// of the production decode path so a regression that mangles the blob shows. -const AFTER_HASH: &str = "1111111111111111111111111111111111111111111111111111111111111111"; +/// `AFTER_HASH` is the real git-sha256 of `BLOB_BYTES`: inline blobs are +/// verified against their name before they are stored (#726). +const AFTER_HASH: &str = "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e"; const BEFORE_HASH: &str = "0000000000000000000000000000000000000000000000000000000000000000"; const BLOB_BYTES: &[u8] = b"patched\n"; /// The single patched file path declared by `make_view_mock`. The saved diff --git a/crates/socket-patch-cli/tests/in_process_get_corrupt_manifest.rs b/crates/socket-patch-cli/tests/in_process_get_corrupt_manifest.rs index 63a4af6aa..847ea3e75 100644 --- a/crates/socket-patch-cli/tests/in_process_get_corrupt_manifest.rs +++ b/crates/socket-patch-cli/tests/in_process_get_corrupt_manifest.rs @@ -30,8 +30,10 @@ async fn uuid_get_with_corrupt_manifest_fails_without_clobbering() { "publishedAt": "2024-06-01T00:00:00Z", "files": { "package/index.js": { - "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + // The real git-sha256 names of "original\n" and + // "patched\n": `get` verifies inline blobs against them. + "beforeHash": "9b8142d93991702a22920638c6d6768b150706c5e018b3605b74552d09fd160e", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", "beforeBlobContent": "b3JpZ2luYWwK", } diff --git a/crates/socket-patch-cli/tests/in_process_get_update_count.rs b/crates/socket-patch-cli/tests/in_process_get_update_count.rs index 9ac8bed45..5cd01c749 100644 --- a/crates/socket-patch-cli/tests/in_process_get_update_count.rs +++ b/crates/socket-patch-cli/tests/in_process_get_update_count.rs @@ -156,8 +156,8 @@ async fn successful_update_is_counted_once() { "publishedAt": "2024-06-01T00:00:00Z", "files": { "package/index.js": { - "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "beforeHash": "9b8142d93991702a22920638c6d6768b150706c5e018b3605b74552d09fd160e", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", "beforeBlobContent": "b3JpZ2luYWwK", } @@ -219,8 +219,8 @@ async fn corrupt_manifest_is_a_hard_error_not_silently_clobbered() { "publishedAt": "2024-06-01T00:00:00Z", "files": { "package/index.js": { - "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "beforeHash": "9b8142d93991702a22920638c6d6768b150706c5e018b3605b74552d09fd160e", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", "beforeBlobContent": "b3JpZ2luYWwK", } diff --git a/crates/socket-patch-cli/tests/in_process_get_uuid_fallback.rs b/crates/socket-patch-cli/tests/in_process_get_uuid_fallback.rs index dd131656d..e755bb3c1 100644 --- a/crates/socket-patch-cli/tests/in_process_get_uuid_fallback.rs +++ b/crates/socket-patch-cli/tests/in_process_get_uuid_fallback.rs @@ -17,7 +17,9 @@ use wiremock::{Mock, MockServer, ResponseTemplate}; const ORG: &str = "test-org"; const UUID: &str = "22222222-2222-4222-8222-222222222222"; const PURL: &str = "pkg:npm/fallback-pkg@1.0.0"; -const AFTER_HASH: &str = "1111111111111111111111111111111111111111111111111111111111111111"; +/// The git-sha256 of `patched\n`, the view's inline blob content (inline +/// blobs are verified against their name, #726). +const AFTER_HASH: &str = "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e"; #[tokio::test] #[serial] @@ -40,7 +42,7 @@ async fn stale_token_uuid_get_falls_back_to_proxy_end_to_end() { "publishedAt": "2024-06-01T00:00:00Z", "files": { "package/index.js": { - "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", + "beforeHash": "9b8142d93991702a22920638c6d6768b150706c5e018b3605b74552d09fd160e", "afterHash": AFTER_HASH, "blobContent": "cGF0Y2hlZAo=", "beforeBlobContent": "b3JpZ2luYWwK", diff --git a/crates/socket-patch-cli/tests/in_process_scan.rs b/crates/socket-patch-cli/tests/in_process_scan.rs index a920effb5..9fc5cb279 100644 --- a/crates/socket-patch-cli/tests/in_process_scan.rs +++ b/crates/socket-patch-cli/tests/in_process_scan.rs @@ -175,7 +175,7 @@ async fn mock_view_with_blob(server: &MockServer) { "files": { "package/index.js": { "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", } }, @@ -379,7 +379,7 @@ async fn scan_apply_wet_writes_manifest_and_blob() { // fixture's blobContent "cGF0Y2hlZAo=" decodes to exactly "patched\n"; // asserting the bytes (not just existence) catches a regression that // wrote an empty/garbled blob. - let after_hash = "1111111111111111111111111111111111111111111111111111111111111111"; + let after_hash = "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e"; let blob = tmp.path().join(".socket/blobs").join(after_hash); assert!(blob.exists(), "after-blob must be written"); assert_eq!( @@ -552,7 +552,7 @@ async fn mock_view_for(server: &MockServer, uuid: &str, published_at: &str) { "files": { "package/index.js": { "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "afterHash": "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e", "blobContent": "cGF0Y2hlZAo=", } }, @@ -817,7 +817,7 @@ async fn scan_sync_full_cycle_against_clean_project() { "sync manifest must record {PURL}; got {manifest}" ); - let after_hash = "1111111111111111111111111111111111111111111111111111111111111111"; + let after_hash = "d2802877eb1c2f442d30d18abcb281cb2830875571624544f2f1acb36480997e"; let blob = tmp.path().join(".socket/blobs").join(after_hash); assert!(blob.exists(), "sync must write the after-blob"); assert_eq!(std::fs::read(&blob).unwrap(), b"patched\n"); diff --git a/crates/socket-patch-core/src/api/blob_fetcher.rs b/crates/socket-patch-core/src/api/blob_fetcher.rs index 836cb67cc..cdb9a130c 100644 --- a/crates/socket-patch-core/src/api/blob_fetcher.rs +++ b/crates/socket-patch-core/src/api/blob_fetcher.rs @@ -2,7 +2,7 @@ use std::collections::HashSet; use std::path::Path; use crate::api::client::{ApiClient, ApiError, BinaryBody}; -use crate::hash::git_sha256::compute_git_sha256_from_reader; +use crate::hash::git_sha256::{compute_git_sha256_from_bytes, compute_git_sha256_from_reader}; use crate::manifest::operations::get_after_hash_blobs; use crate::manifest::schema::PatchManifest; use crate::patch::apply::PatchSources; @@ -389,6 +389,82 @@ fn concise_fetch_error<'a>(err: &'a str, id: &str) -> std::borrow::Cow<'a, str> err.into() } +/// Why [`store_verified_blob`] stored nothing. +#[derive(Debug)] +pub enum StoreBlobError { + /// The bytes hash to this, not to the blob's name. + HashMismatch(String), + /// A level of `.socket/blobs/` (the leaf included) is a link, + /// whose target is not socket-patch's to write (see + /// [`containment`](crate::utils::containment)). + Linked(std::io::Error), + /// A disk error while staging or renaming. + Write(std::io::Error), +} + +impl std::fmt::Display for StoreBlobError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::HashMismatch(actual) => { + write!(f, "content hash mismatch: content hashes to {actual}") + } + Self::Linked(e) | Self::Write(e) => write!(f, "{e}"), + } + } +} + +/// Store in-memory `bytes` as the content-addressed blob `blobs_dir/`: +/// the one writer for blobs that arrive inline (a patch view's +/// `blobContent` / `beforeBlobContent`), with the same rules as the +/// download path ([`stream_cache_entry_atomic`]): refused when any level +/// of the entry below `blobs_dir`'s parent is a link, verified against its +/// git-sha256 name BEFORE anything is written, then staged and renamed into +/// place (never truncated in place). An existing entry that already +/// verifies is never rewritten. Returns whether the entry was NEWLY +/// created. `blobs_dir` is created on demand. +pub async fn store_verified_blob( + blobs_dir: &Path, + hash: &str, + bytes: &[u8], +) -> Result { + let dest = blobs_dir.join(hash); + guard_cache_entry(&dest).map_err(StoreBlobError::Linked)?; + let actual = compute_git_sha256_from_bytes(bytes); + if !blob_hash_matches(hash, &actual) { + return Err(StoreBlobError::HashMismatch(actual)); + } + if let Ok(existing) = crate::utils::fs::read_regular_to_bytes(&dest).await { + if blob_hash_matches(hash, &compute_git_sha256_from_bytes(&existing)) { + return Ok(false); + } + } + let existed = tokio::fs::symlink_metadata(&dest).await.is_ok(); + tokio::fs::create_dir_all(blobs_dir) + .await + .map_err(StoreBlobError::Write)?; + let stage = crate::utils::fs::stage_path(&dest, ".socket-dl-"); + let written = async { + tokio::fs::write(&stage, bytes).await?; + tokio::fs::rename(&stage, &dest).await + } + .await; + if let Err(e) = written { + let _ = tokio::fs::remove_file(&stage).await; + return Err(StoreBlobError::Write(e)); + } + Ok(!existed) +} + +/// Refuse a cache entry (`/blobs/`, `/diffs/`) +/// when it, or a level between it and the directory holding `blobs/` / +/// `diffs/`, is a link: a committed `.socket/blobs` (or +/// `.socket/blobs/`) symlink would otherwise redirect the write out +/// of the project. +fn guard_cache_entry(dest: &Path) -> std::io::Result<()> { + let root = dest.parent().and_then(Path::parent).unwrap_or(dest); + crate::utils::containment::ensure_unlinked(root, dest, "write") +} + // ── Internal helpers ────────────────────────────────────────────────── /// Stream `body` to `dest` atomically: copy it chunk by chunk into a temp @@ -431,6 +507,7 @@ async fn stream_cache_entry_atomic( // commit, so a failure removes the directories this call created, while // they are still empty. An uncreatable parent surfaces as this entry's // write failure, like any other disk error. + guard_cache_entry(dest).map_err(EntryError::Write)?; let mut created_dirs = Vec::new(); for dir in parent.ancestors() { if dir.as_os_str().is_empty() || tokio::fs::symlink_metadata(dir).await.is_ok() { @@ -1065,6 +1142,56 @@ mod tests { stream_cache_entry_atomic(dest, &mut body, None).await } + /// The download path refuses a linked `.socket/blobs` (a committed + /// link to a shared cache or another project) and a linked + /// `.socket/blobs/` entry, end to end through + /// [`fetch_blobs_by_hash`]: the blob is reported failed and nothing is + /// written at the link's target. + #[cfg(unix)] + #[tokio::test] + async fn download_refuses_a_linked_blobs_dir_or_entry() { + let bytes = b"patched\n"; + let hash = compute_git_sha256_from_bytes(bytes); + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::any()) + .respond_with(wiremock::ResponseTemplate::new(200).set_body_bytes(bytes.to_vec())) + .mount(&server) + .await; + let client = ApiClient::new(crate::api::client::ApiClientOptions { + api_url: server.uri(), + api_token: None, + use_public_proxy: true, + org_slug: None, + }); + let tmp = tempfile::tempdir().unwrap(); + let hashes: HashSet = [hash.clone()].into_iter().collect(); + + // A linked `.socket/blobs` directory. + let shared = tmp.path().join("shared-cache"); + std::fs::create_dir_all(&shared).unwrap(); + let proj = tmp.path().join("proj"); + std::fs::create_dir_all(proj.join(".socket")).unwrap(); + std::os::unix::fs::symlink(&shared, proj.join(".socket/blobs")).unwrap(); + let res = fetch_blobs_by_hash(&hashes, &proj.join(".socket/blobs"), &client, None).await; + assert_eq!((res.downloaded, res.failed), (0, 1), "{res:?}"); + let err = res.results[0].error.clone().unwrap_or_default(); + assert!( + err.contains(crate::utils::containment::LINKED_LEVEL_MARKER), + "{err}" + ); + assert_eq!(std::fs::read_dir(&shared).unwrap().count(), 0); + + // A dangling `.socket/blobs/` link: `get_missing`-style + // presence probes see it as missing, and the write must not follow it. + let proj2 = tmp.path().join("proj2"); + std::fs::create_dir_all(proj2.join(".socket/blobs")).unwrap(); + let victim = tmp.path().join("victim"); + std::os::unix::fs::symlink(&victim, proj2.join(".socket/blobs").join(&hash)).unwrap(); + let res = fetch_blobs_by_hash(&hashes, &proj2.join(".socket/blobs"), &client, None).await; + assert_eq!((res.downloaded, res.failed), (0, 1), "{res:?}"); + assert!(!victim.exists(), "nothing is written through the link"); + } + #[tokio::test] async fn test_write_cache_entry_atomic_writes_exact_bytes_no_litter() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-core/src/patch/apply.rs b/crates/socket-patch-core/src/patch/apply.rs index 767f55674..fa7005178 100644 --- a/crates/socket-patch-core/src/patch/apply.rs +++ b/crates/socket-patch-core/src/patch/apply.rs @@ -841,6 +841,29 @@ impl crate::patch::store_copies::CopyFold for ApplyResult { /// keys on the same text. pub const OWNERSHIP_NOT_RESTORED_MARKER: &str = "ownership could not be restored"; +/// The message of a new-file collision (see [`mark_new_file_collision`]). +pub const NEW_FILE_COLLISION_MESSAGE: &str = + "File already exists, but the patch adds it as a new file (its content matches neither hash)"; + +/// A new-file entry (empty beforeHash) whose path already holds content +/// that is not the patched bytes is a mismatch like any other: the file +/// matches neither the patch's beforeHash (none) nor its afterHash +/// (CLI_CONTRACT "mismatch policy"). [`verify_file_patch`] reports it as +/// `Ready` (its callers only ask "can this be written?"); the apply engine +/// turns it into a `HashMismatch` with an empty `expected_hash`, so +/// `--strict` refuses it and the default policy overwrites it with the +/// `content_mismatch_overwritten` warning instead of silently. +fn mark_new_file_collision(verify: &mut VerifyResult, file_info: &PatchFileInfo) { + if file_info.before_hash.is_empty() + && verify.status == VerifyStatus::Ready + && verify.current_hash.is_some() + { + verify.status = VerifyStatus::HashMismatch; + verify.message = Some(NEW_FILE_COLLISION_MESSAGE.to_string()); + verify.expected_hash = Some(String::new()); + } +} + /// The single-copy apply engine behind [`apply_package_patch`]: verifies /// and patches the package at exactly the one `pkg_path` it is given. async fn apply_package_patch_at( @@ -892,6 +915,7 @@ async fn apply_package_patch_at( } let mut verify_result = verify_file_patch(pkg_path, file_name, file_info).await; + mark_new_file_collision(&mut verify_result, file_info); if verify_result.status != VerifyStatus::Ready && verify_result.status != VerifyStatus::AlreadyPatched @@ -2943,17 +2967,18 @@ mod tests { .await .unwrap(); assert_eq!(written, fresh, "divergent existing content is overwritten"); + // B23: ... but surfaced as a mismatch overwrite (the promoted + // signature the CLI's `content_mismatch_overwritten` keys on). + let v = &result.files_verified[0]; + assert_eq!(v.status, VerifyStatus::Ready); + assert!(v.expected_hash.is_some() && v.current_hash != v.expected_hash); } - /// CHARACTERIZATION: `--strict` does NOT protect divergent content at - /// a new-file path. `verify_file_patch` returns `Ready` (never - /// `HashMismatch`) for a new-file entry, so the Strict policy arm is - /// never consulted and the verified patched bytes overwrite the local - /// file — the deliberate "force overwrite" contract documented above - /// the `is_new_file` branch. If that contract ever changes to - /// fail-closed under Strict, flip these assertions. + /// B23: `--strict` refuses divergent content at a new-file path, like + /// any other mismatch (CLI_CONTRACT "mismatch policy"), and leaves the + /// local file untouched. #[tokio::test] - async fn test_apply_package_patch_new_file_overwrite_strict_characterization() { + async fn test_apply_package_patch_new_file_collision_refused_under_strict() { let pkg_dir = tempfile::tempdir().unwrap(); let blobs_dir = tempfile::tempdir().unwrap(); @@ -2987,12 +3012,14 @@ mod tests { ) .await; - assert!(result.success, "strict still overwrites at a new-file path"); - assert_eq!(result.files_patched, vec!["new.js".to_string()]); - let written = tokio::fs::read(pkg_dir.path().join("new.js")) + assert!(!result.success, "strict refuses a new-file collision"); + let err = result.error.as_deref().unwrap_or_default(); + assert!(err.contains(NEW_FILE_COLLISION_MESSAGE), "{err}"); + assert!(result.files_patched.is_empty()); + let on_disk = tokio::fs::read(pkg_dir.path().join("new.js")) .await .unwrap(); - assert_eq!(written, fresh); + assert_eq!(on_disk, b"stale local content", "local file untouched"); } // ── create_dir_all failure inside apply_file_patch_at ──────────── diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index fc9cd3354..764d5da7f 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -552,7 +552,7 @@ async fn rollback_package_patch_at( ) .await { - result.error = Some(store.refusal("roll back")); + result.error = Some(store.refusal(crate::patch::shared_store::ROLL_BACK_ACTION)); return result; } } @@ -1752,8 +1752,10 @@ mod tests { assert!(pkg.path().join("lib/old/c.js").exists()); } - /// A symlinked parent directory is never removed or followed by the - /// prune: only real, empty directories inside the package go. + /// A symlinked parent directory that leads out of the package is never + /// deleted through or pruned: the rollback refuses before touching + /// anything (B25 containment), so the file, `inner` and the link all + /// stay. #[cfg(unix)] #[tokio::test] async fn test_rollback_new_file_prune_never_follows_a_symlinked_dir() { @@ -1771,10 +1773,13 @@ mod tests { rollback_package_patch("pkg:npm/x@1.0.0", pkg.path(), &files, blobs.path(), false) .await; - assert!(result.success, "{:?}", result.error); - assert!(!target.join("inner/added.js").exists()); - // `inner` is reached through the symlink, so it lives outside the - // package and stays; so does `link` itself. + assert!(!result.success); + let err = result.error.as_deref().unwrap_or_default(); + assert!( + err.contains(crate::patch::shared_store::OUTSIDE_INSTALL_TREE_REFUSAL_MARKER), + "{err}" + ); + assert!(target.join("inner/added.js").exists(), "nothing deleted"); assert!(target.join("inner").is_dir()); assert!(tokio::fs::symlink_metadata(pkg.path().join("link")) .await diff --git a/crates/socket-patch-core/src/patch/shared_store.rs b/crates/socket-patch-core/src/patch/shared_store.rs index c75e902aa..c851d7eaf 100644 --- a/crates/socket-patch-core/src/patch/shared_store.rs +++ b/crates/socket-patch-core/src/patch/shared_store.rs @@ -35,6 +35,20 @@ //! linked `.store`) resolve inside a `node_modules` tree; the one exception, //! Yarn's pnpm-linker store relocated by `pnpmStoreFolder`, is recognized //! only for an active Yarn pnpm install and a registry entry's layout. +//! +//! Beyond those markers, every written directory must stay inside the +//! install tree it was found in ([`containment::resolves_within`]): each +//! directory a patch writes into must resolve inside the package directory, +//! and a Composer package (`//`, recognized by +//! `/composer/installed.json`) must resolve inside its vendor +//! dir. A Composer path repository symlinks `vendor//` to the +//! user's own source by default, and a link out of a package directory +//! (`flit install --symlink`, a hand-made link) is the same first-party +//! case; neither is an installed copy a reinstall restores. This is a +//! containment rule, not a list of stores: a layout nobody has seen yet +//! is refused rather than written through. +//! +//! [`containment::resolves_within`]: crate::utils::containment::resolves_within use std::path::{Path, PathBuf}; @@ -46,6 +60,13 @@ pub const SHARED_STORE_REFUSAL_MARKER: &str = "shared by other projects"; /// [`SharedStoreKind::LinkedSource`]). pub const LINKED_SOURCE_REFUSAL_MARKER: &str = "outside every node_modules tree"; +/// The substring every out-of-install-tree refusal carries (see +/// [`SharedStoreKind::OutsideInstallTree`]). +pub const OUTSIDE_INSTALL_TREE_REFUSAL_MARKER: &str = "outside the install tree"; + +/// The `action` [`SharedStore::refusal`] is given by rollback. +pub const ROLL_BACK_ACTION: &str = "roll back"; + /// A cross-project store a package directory resolves into. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SharedStoreKind { @@ -57,6 +78,10 @@ pub enum SharedStoreKind { /// `node_modules` tree (a workspace member, a `file:` / `link:` /// directory dependency, an `npm link` target). LinkedSource, + /// A written directory that resolves outside the install tree it was + /// found in: a Composer path repository's link into first-party + /// source, or a directory linked out of the package. + OutsideInstallTree, } /// Where a package directory really lives, when that is a shared store. @@ -81,6 +106,27 @@ impl SharedStore { "pnpm's global virtual store (enableGlobalVirtualStore)", "set enableGlobalVirtualStore to false and reinstall", ), + SharedStoreKind::OutsideInstallTree => { + // A rollback has nothing to "patch directly": the bytes an + // older in-place patch wrote there come back from the + // source's own history. + let remedy = if action == ROLL_BACK_ACTION { + "Restore that source from version control (for example `git checkout \ + -- ` in its repository) instead" + } else { + "Patch that source directly, or install the package as a copy (for \ + Composer, set the path repository's `symlink` option to false)" + }; + return format!( + "Refusing to {action} {path}: it is reached through a link and lies \ + {OUTSIDE_INSTALL_TREE_REFUSAL_MARKER} it was installed into (a Composer \ + path repository, a `--symlink` install, a package manager linking it \ + in from its own prefix, or another linked source directory), so it is \ + not an installed copy of the registry package that socket-patch owns. \ + {remedy}", + path = self.real_path.display(), + ); + } SharedStoreKind::LinkedSource => { return format!( "Refusing to {action} {path}: node_modules links to it, but it is \ @@ -142,8 +188,9 @@ pub async fn shared_store_of_patch_dirs<'a>( } Some(dir) })); - dirs.filter(|dir| seen.insert(dir.clone())) - .find_map(|dir| shared_store_of_blocking(&dir)) + dirs.filter(|dir| seen.insert(dir.clone())).find_map(|dir| { + shared_store_of_blocking(&dir).or_else(|| outside_install_tree(&pkg_path, &dir)) + }) }) .await .ok() @@ -187,6 +234,38 @@ fn shared_store_of_blocking(pkg_path: &Path) -> Option { linked_source_of(pkg_path, real) } +/// [`SharedStoreKind::OutsideInstallTree`]: `dir` (the package directory +/// itself or a directory a patch writes into below it) does not resolve +/// inside its install tree: the Composer vendor dir for the package +/// directory of a Composer install, the package directory for everything +/// below it. +fn outside_install_tree(pkg_path: &Path, dir: &Path) -> Option { + let root = if dir == pkg_path { + composer_vendor_dir(pkg_path)? + } else { + pkg_path + }; + if crate::utils::containment::resolves_within(root, dir) { + return None; + } + Some(SharedStore { + kind: SharedStoreKind::OutsideInstallTree, + real_path: std::fs::canonicalize(dir).ok()?, + }) +} + +/// The Composer vendor dir `pkg_path` (`//`) was +/// installed into, recognized by the `composer/installed.json` Composer +/// writes there; `None` for any other layout. +fn composer_vendor_dir(pkg_path: &Path) -> Option<&Path> { + let vendor = pkg_path.parent()?.parent()?; + vendor + .join("composer") + .join("installed.json") + .is_file() + .then_some(vendor) +} + /// [`SharedStoreKind::LinkedSource`]: `pkg_path` is spelled as a /// `node_modules` entry (`node_modules/` or /// `node_modules/@scope/`), yet its real path `real` is neither below @@ -748,6 +827,87 @@ mod tests { } } + /// B25: a Composer path repository (`vendor//` linked to the + /// user's own source) is outside its install tree; a real vendor dir + /// is not. + #[cfg(unix)] + #[tokio::test] + async fn composer_path_repository_is_outside_the_install_tree() { + let dir = tempfile::tempdir().unwrap(); + let vendor = dir.path().join("vendor"); + std::fs::create_dir_all(vendor.join("composer")).unwrap(); + std::fs::write(vendor.join("composer/installed.json"), "{}").unwrap(); + std::fs::create_dir_all(vendor.join("acme")).unwrap(); + let source = dir.path().join("packages/pkg"); + std::fs::create_dir_all(source.join("src")).unwrap(); + std::os::unix::fs::symlink(&source, vendor.join("acme/pkg")).unwrap(); + + let got = shared_store_of_patch_dirs(&vendor.join("acme/pkg"), ["src/a.php"]) + .await + .expect("refused"); + assert_eq!(got.kind, SharedStoreKind::OutsideInstallTree); + assert_eq!(got.real_path, std::fs::canonicalize(&source).unwrap()); + assert!(got + .refusal("patch") + .contains(OUTSIDE_INSTALL_TREE_REFUSAL_MARKER)); + // A rollback is told to restore the source from its history, not + // to "patch that source directly". + let rollback = got.refusal(ROLL_BACK_ACTION); + assert!( + rollback.contains(OUTSIDE_INSTALL_TREE_REFUSAL_MARKER), + "{rollback}" + ); + assert!(rollback.contains("version control"), "{rollback}"); + assert!(!rollback.contains("Patch that source"), "{rollback}"); + + let installed = vendor.join("acme/real"); + std::fs::create_dir_all(installed.join("src")).unwrap(); + assert_eq!( + shared_store_of_patch_dirs(&installed, ["src/a.php"]).await, + None + ); + // Without Composer's marker the layout is not recognized as a + // vendor dir, and the package dir itself is the install tree. + std::fs::remove_file(vendor.join("composer/installed.json")).unwrap(); + assert_eq!( + shared_store_of_patch_dirs(&vendor.join("acme/pkg"), ["src/a.php"]).await, + None + ); + } + + /// B25: a directory a patch writes into that is linked out of the + /// package root (`flit install --symlink` into site-packages) is + /// refused; a link that stays inside it is fine. + #[cfg(unix)] + #[tokio::test] + async fn written_dir_linked_out_of_the_package_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let site = dir.path().join("venv/lib/python3.12/site-packages"); + std::fs::create_dir_all(&site).unwrap(); + let source = dir.path().join("src/mypkg"); + std::fs::create_dir_all(&source).unwrap(); + std::os::unix::fs::symlink(&source, site.join("mypkg")).unwrap(); + let got = shared_store_of_patch_dirs(&site, ["mypkg/__init__.py"]) + .await + .expect("refused"); + assert_eq!(got.kind, SharedStoreKind::OutsideInstallTree); + + std::fs::create_dir_all(site.join("real/inner")).unwrap(); + std::os::unix::fs::symlink(site.join("real/inner"), site.join("alias")).unwrap(); + assert_eq!( + shared_store_of_patch_dirs(&site, ["real/x.py", "alias/y.py"]).await, + None + ); + + // A linked package root itself (a symlinked site-packages) is the + // user's layout, not a link out of the tree. + std::os::unix::fs::symlink(&site, dir.path().join("site-link")).unwrap(); + assert_eq!( + shared_store_of_patch_dirs(&dir.path().join("site-link"), ["real/x.py"]).await, + None + ); + } + #[test] fn refusal_names_store_and_remedy() { let s = SharedStore { diff --git a/crates/socket-patch-core/src/utils/containment.rs b/crates/socket-patch-core/src/utils/containment.rs new file mode 100644 index 000000000..3b32365a0 --- /dev/null +++ b/crates/socket-patch-core/src/utils/containment.rs @@ -0,0 +1,216 @@ +//! The one containment rule for every write and delete socket-patch makes +//! in a tree it does not fully own. +//! +//! Two questions recur wherever socket-patch touches the disk: +//! +//! * **Is any level of this path a link?** ([`linked_level`]) socket-patch +//! creates `.socket/` and everything under it itself and never writes a +//! symlink or junction there. A linked level is therefore never its own: +//! the target may be another project's `.socket/` (#887), another +//! project's vendor store (#664) or a file outside the repository that a +//! committed link points at (a planted `.socket/blobs/`). Writing +//! or deleting through it reaches that other tree. Every writer and +//! deleter of socket-patch state asks this before touching anything. +//! * **Does this directory really live inside the tree it was found in?** +//! ([`resolves_within`]) An installed package is patched in place, so its +//! directory must resolve inside the install tree the crawler found it +//! under. A package directory that is a link out of that tree (a +//! Composer path repository, a `flit install --symlink` package, a +//! workspace member) is first-party source or a shared store, and a write +//! would land there. +//! +//! Levels at or above `root` (the project path itself, `/tmp -> +//! /private/tmp`, a symlinked home directory) are the user's business and +//! are never checked. + +use std::path::{Path, PathBuf}; + +/// The outermost level of `path` strictly below `root` that is a symlink or +/// a Windows junction (lstat), or `None`. `path` itself counts as a level; +/// `root` and its ancestors do not. A level that does not exist (yet) is +/// not a link, so a tree the caller is about to create passes. When `path` +/// is not below `root`, only `path` itself is checked. A level that cannot +/// be probed (EACCES, ENOTDIR) is reported as no link; callers that must +/// fail closed on it use [`try_linked_level`]. +pub fn linked_level(root: &Path, path: &Path) -> Option { + try_linked_level(root, path).ok().flatten() +} + +/// [`linked_level`], but an lstat error other than `NotFound` on a level +/// (EACCES, ENOTDIR) is returned instead of being read as "not a link". +/// Levels are probed outermost first; a missing level ends the walk (nothing +/// below it can exist). +pub fn try_linked_level(root: &Path, path: &Path) -> std::io::Result> { + let levels: Vec<&Path> = if path.starts_with(root) && path != root { + path.ancestors().take_while(|a| *a != root).collect() + } else { + vec![path] + }; + for level in levels.into_iter().rev() { + match std::fs::symlink_metadata(level) { + Ok(meta) if meta.file_type().is_symlink() => return Ok(Some(level.to_path_buf())), + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(e), + } + } + Ok(None) +} + +/// Whether `path` is itself a symlink or a Windows junction (lstat; a +/// missing or unreadable path is not). The one link predicate: +/// [`crate::utils::fs::is_symlink`] and every link probe in the vendor +/// backends delegate here. +pub fn is_link(path: &Path) -> bool { + std::fs::symlink_metadata(path).is_ok_and(|meta| meta.file_type().is_symlink()) +} + +/// The refusal text for writing or deleting `what` through the linked +/// `link`. Every guarded writer and deleter shares it, so callers and tests +/// can match on [`LINKED_LEVEL_MARKER`]. +pub fn linked_level_refusal(action: &str, what: &Path, link: &Path) -> std::io::Error { + std::io::Error::other(format!( + "refusing to {action} {}: {} {LINKED_LEVEL_MARKER}, and its target is not \ + socket-patch's to change", + what.display(), + link.display() + )) +} + +/// The substring every [`linked_level_refusal`] carries. +pub const LINKED_LEVEL_MARKER: &str = "is a symlink"; + +/// Err with [`linked_level_refusal`] when any level of `path` below `root` +/// is a link, before the caller writes or deletes anything. +pub fn ensure_unlinked(root: &Path, path: &Path, action: &str) -> std::io::Result<()> { + match linked_level(root, path) { + Some(link) => Err(linked_level_refusal(action, path, &link)), + None => Ok(()), + } +} + +/// The nearest ancestor of `path` (inclusive) literally named `.socket`. +pub fn nearest_socket_dir(path: &Path) -> Option<&Path> { + path.ancestors().find(|a| { + a.file_name() + .is_some_and(|n| n == crate::constants::SOCKET_DIR) + }) +} + +/// The project root a path under `.socket/` belongs to: the parent of +/// [`nearest_socket_dir`]. `None` when `path` is not under a `.socket/`. +pub fn socket_project_root(path: &Path) -> Option<&Path> { + nearest_socket_dir(path).and_then(Path::parent) +} + +/// Whether `dir` really (canonically) lives inside `root`. Both must +/// exist; an unresolvable path is reported as contained, so the caller's +/// own missing-file handling runs instead of a spurious refusal. +pub fn resolves_within(root: &Path, dir: &Path) -> bool { + match (std::fs::canonicalize(root), std::fs::canonicalize(dir)) { + (Ok(root), Ok(dir)) => dir.starts_with(root), + _ => true, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn real_and_missing_levels_are_not_links() { + let tmp = tempfile::tempdir().unwrap(); + let p = tmp.path().join("p"); + std::fs::create_dir_all(p.join(".socket/vendor")).unwrap(); + assert_eq!(linked_level(&p, &p.join(".socket/vendor/npm/x")), None); + assert_eq!(linked_level(&p, &p), None); + assert!(ensure_unlinked(&p, &p.join(".socket/blobs/h"), "write").is_ok()); + } + + /// The outermost link wins, `.socket` itself counts (#887), and links + /// at or above `root` are ignored. + #[cfg(unix)] + #[test] + fn finds_the_outermost_link_below_root_only() { + use std::os::unix::fs::symlink; + let tmp = tempfile::tempdir().unwrap(); + let shared = tmp.path().join("shared"); + std::fs::create_dir_all(shared.join("vendor/npm")).unwrap(); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).unwrap(); + symlink(&shared, real.join(".socket")).unwrap(); + symlink(&real, tmp.path().join("alias")).unwrap(); + + let path = real.join(".socket/vendor/npm"); + assert_eq!(linked_level(&real, &path), Some(real.join(".socket"))); + let alias = tmp.path().join("alias"); + assert_eq!( + linked_level(&alias, &alias.join(".socket/vendor")), + Some(alias.join(".socket")), + "the project path being a link is fine; .socket below it is not" + ); + let err = ensure_unlinked(&real, &path, "delete").unwrap_err(); + assert!(err.to_string().contains(LINKED_LEVEL_MARKER), "{err}"); + + // A leaf link counts too (a planted `.socket/blobs/`). + let q = tmp.path().join("q"); + std::fs::create_dir_all(q.join(".socket/blobs")).unwrap(); + symlink(tmp.path().join("victim"), q.join(".socket/blobs/h")).unwrap(); + assert_eq!( + linked_level(&q.join(".socket"), &q.join(".socket/blobs/h")), + Some(q.join(".socket/blobs/h")) + ); + // Not below root: only the path itself is checked. + assert_eq!(linked_level(&tmp.path().join("elsewhere"), &path), None); + assert!(is_link(&real.join(".socket"))); + } + + /// `try_linked_level` surfaces a level it cannot probe (here ENOTDIR: + /// a regular file used as a directory) where `linked_level` reads it as + /// no link; a missing level is no link for both. + #[cfg(unix)] + #[test] + fn try_linked_level_propagates_unprobeable_levels() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::write(root.join("file"), b"x").unwrap(); + let through_file = root.join("file/below"); + assert!(try_linked_level(root, &through_file).is_err()); + assert_eq!(linked_level(root, &through_file), None); + assert_eq!( + try_linked_level(root, &root.join("missing/deeper")).unwrap(), + None + ); + } + + #[test] + fn socket_project_root_is_the_parent_of_the_nearest_socket_dir() { + let p = Path::new("/a/b/.socket/vendor/state.json"); + assert_eq!(socket_project_root(p), Some(Path::new("/a/b"))); + assert_eq!( + socket_project_root(Path::new("/a/b/.socket")), + Some(Path::new("/a/b")) + ); + assert_eq!(socket_project_root(Path::new("/a/b/state.json")), None); + } + + #[cfg(unix)] + #[test] + fn resolves_within_follows_links() { + use std::os::unix::fs::symlink; + let tmp = tempfile::tempdir().unwrap(); + let vendor = tmp.path().join("vendor/acme"); + let source = tmp.path().join("packages/pkg"); + std::fs::create_dir_all(&vendor).unwrap(); + std::fs::create_dir_all(&source).unwrap(); + symlink(&source, vendor.join("pkg")).unwrap(); + std::fs::create_dir_all(vendor.join("real")).unwrap(); + let root = tmp.path().join("vendor"); + assert!(!resolves_within(&root, &vendor.join("pkg"))); + assert!(resolves_within(&root, &vendor.join("real"))); + assert!( + resolves_within(&root, &vendor.join("missing")), + "unresolvable passes" + ); + } +} diff --git a/crates/socket-patch-core/src/utils/fs.rs b/crates/socket-patch-core/src/utils/fs.rs index 4f9e67581..17566a3b3 100644 --- a/crates/socket-patch-core/src/utils/fs.rs +++ b/crates/socket-patch-core/src/utils/fs.rs @@ -244,10 +244,7 @@ where /// a regular file (leaving the target stale) — they use this to refuse /// fail-closed before any write, mirroring the hosted replay guard. pub async fn is_symlink(path: &Path) -> bool { - tokio::fs::symlink_metadata(path) - .await - .map(|metadata| metadata.file_type().is_symlink()) - .unwrap_or(false) + crate::utils::containment::is_link(path) } /// Blocking twin of [`read_regular_to_string`] for the few synchronous @@ -327,12 +324,8 @@ pub async fn first_symlink<'a>( root: &Path, rels: impl IntoIterator, ) -> Option<&'a str> { - for rel in rels { - if is_symlink(&root.join(rel)).await { - return Some(rel); - } - } - None + rels.into_iter() + .find(|rel| crate::utils::containment::is_link(&root.join(rel))) } /// Remove the link at `path` itself — never its target. A symlink (file or diff --git a/crates/socket-patch-core/src/utils/group_commit.rs b/crates/socket-patch-core/src/utils/group_commit.rs index 3d7f411c2..0393df3df 100644 --- a/crates/socket-patch-core/src/utils/group_commit.rs +++ b/crates/socket-patch-core/src/utils/group_commit.rs @@ -670,7 +670,7 @@ impl GroupCommit { // the file other checkouts read, unpatched. Refuse the whole commit // before anything is written, as the hosted guard does. for change in &changes { - if is_symlink(&root.join(&change.rel)) { + if crate::utils::containment::is_link(&root.join(&change.rel)) { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, SymlinkedTarget(rel_string(&change.rel)), @@ -1070,32 +1070,11 @@ fn carries_commit(item: &Replay) -> Option { /// run, which captures nothing, to predict that refusal. pub fn symlinked_paths<'a>(root: &Path, rels: impl IntoIterator) -> Vec { rels.into_iter() - .filter(|rel| is_symlink(&root.join(rel))) + .filter(|rel| crate::utils::containment::is_link(&root.join(rel))) .map(str::to_string) .collect() } -fn is_symlink(path: &Path) -> bool { - std::fs::symlink_metadata(path).is_ok_and(|m| m.file_type().is_symlink()) -} - -/// Whether any existing level of `rel` below `root` — the file itself -/// included — is a symbolic link: a journal must never write through one -/// (out of the project, or onto a file it does not name). -fn crosses_symlink(root: &Path, rel: &Path) -> std::io::Result { - let mut at = root.to_path_buf(); - for component in rel.components() { - at.push(component); - match std::fs::symlink_metadata(&at) { - Ok(meta) if meta.file_type().is_symlink() => return Ok(true), - Ok(_) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(false), - Err(e) => return Err(e), - } - } - Ok(false) -} - fn write_sync(path: &Path, bytes: Option<&[u8]>, preserve_mode: bool) -> std::io::Result<()> { match bytes { Some(bytes) => { @@ -1158,7 +1137,12 @@ pub fn recover(project_root: &Path) -> std::io::Result { let rel = Path::new(&file.path); if relative_to(Path::new(""), rel).is_none() || !is_captured(rel) - || crosses_symlink(project_root, rel)? + // A journal must never write through a link (out of the + // project, or onto a file it does not name). A level that + // cannot be probed (EACCES, ENOTDIR) fails recovery closed: + // the journal stays and the locked command does not proceed. + || crate::utils::containment::try_linked_level(project_root, &project_root.join(rel))? + .is_some() { return set_aside(SetAsideOutcome::Refused); } @@ -1757,6 +1741,34 @@ mod tests { } } + /// A journaled path whose level cannot be probed (here ENOTDIR: a + /// regular file sits where a directory should) fails recovery closed: + /// `recover` errors, the journal stays in place, and nothing is + /// written, rather than the probe error being read as "not a link". + #[cfg(unix)] + #[test] + fn recovery_fails_closed_when_a_level_cannot_be_probed() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + std::fs::write(root.join("blocker"), b"file").unwrap(); + let changes = vec![ + change("b.lock", b"b", b"b2"), + Change { + rel: "blocker/x.lock".into(), + before: None, + after: Some(b"new".to_vec()), + preserve_mode: false, + }, + ]; + write_journal(root, &changes); + assert!(recover(root).is_err()); + assert!( + root.join(COMMIT_JOURNAL_REL).is_file(), + "the journal is kept" + ); + assert!(!root.join("b.lock").exists(), "nothing applied"); + } + /// #627: a commit never renames over a symbolic link (a shared lock, a /// linked `package.json` / `nuget.config`): it refuses before writing /// anything, naming the link, so the link and its target stay as they diff --git a/crates/socket-patch-core/src/utils/mod.rs b/crates/socket-patch-core/src/utils/mod.rs index efcb02f9c..6b071224e 100644 --- a/crates/socket-patch-core/src/utils/mod.rs +++ b/crates/socket-patch-core/src/utils/mod.rs @@ -1,6 +1,7 @@ pub mod cargo_workspace; pub mod composer_version; pub mod concurrent; +pub mod containment; pub(crate) mod digest; pub(crate) mod durability; pub mod env_compat; diff --git a/crates/socket-patch-core/src/utils/socket_dir.rs b/crates/socket-patch-core/src/utils/socket_dir.rs index a0285ee88..5de8496ca 100644 --- a/crates/socket-patch-core/src/utils/socket_dir.rs +++ b/crates/socket-patch-core/src/utils/socket_dir.rs @@ -20,6 +20,7 @@ use std::path::Path; use serde::Serialize; +use super::containment; use super::fs::{atomic_write_bytes, read_regular_to_bytes}; /// Best-effort: remove `dir` when it is empty, then each ancestor while it is @@ -46,7 +47,14 @@ pub async fn prune_empty_dirs(dir: &Path, stop_dir: &Path) { /// now-empty parents up to but excluding `stop_dir`. Any unlink error other /// than NotFound propagates BEFORE any pruning: a read-only parent leaves the /// file — and the caller's fail-closed error — exactly where they were. +/// +/// Refuses, before unlinking, when a directory level between the project +/// root and `path` (`.socket` included) is a link: the unlink would land in +/// the link's target (see [`remove_tree_and_prune`]). pub async fn remove_file_and_prune(path: &Path, stop_dir: &Path) -> std::io::Result<()> { + if let Some(parent) = path.parent() { + containment::ensure_unlinked(guard_root(stop_dir), parent, "delete")?; + } match super::fs::remove_file(path).await { Ok(()) => {} Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} @@ -67,18 +75,13 @@ pub async fn remove_file_and_prune(path: &Path, stop_dir: &Path) -> std::io::Res /// prune — the tree is still there. /// /// Refuses (before deleting anything) when `dir`, or any level between it -/// and `stop_dir`, is a symlink or junction: socket-patch never creates -/// links under `.socket/`, so a linked level points at a tree it does not -/// own — another project's vendor store, say — and deleting through it -/// would destroy that tree. +/// and the project root (`stop_dir`'s parent) — `.socket` itself included +/// (#887) — is a symlink or junction: socket-patch never creates links +/// there, so a linked level points at a tree it does not own (another +/// project's `.socket/` or vendor store, say) and deleting through it would +/// destroy that tree. See [`containment`]. pub async fn remove_tree_and_prune(dir: &Path, stop_dir: &Path) -> std::io::Result<()> { - if let Some(link) = linked_level(dir, stop_dir).await { - return Err(std::io::Error::other(format!( - "refusing to delete {}: {} is a symlink, and its target is not socket-patch's to remove", - dir.display(), - link.display() - ))); - } + containment::ensure_unlinked(guard_root(stop_dir), dir, "delete")?; crate::patch::copy_tree::remove_tree(dir).await?; if let Some(parent) = dir.parent() { prune_empty_dirs(parent, stop_dir).await; @@ -86,25 +89,10 @@ pub async fn remove_tree_and_prune(dir: &Path, stop_dir: &Path) -> std::io::Resu Ok(()) } -/// The outermost of `dir` and its ancestors strictly below `stop_dir` that -/// is a symlink (lstat), or `None`. Only `dir` itself is checked when it is -/// not under `stop_dir`. Levels at or above `stop_dir` (the project path -/// itself, `/tmp -> /private/tmp`) are the user's business. -async fn linked_level<'a>(dir: &'a Path, stop_dir: &Path) -> Option<&'a Path> { - let levels: Vec<&Path> = if dir.starts_with(stop_dir) { - dir.ancestors().take_while(|a| *a != stop_dir).collect() - } else { - vec![dir] - }; - for level in levels.into_iter().rev() { - if tokio::fs::symlink_metadata(level) - .await - .is_ok_and(|meta| meta.file_type().is_symlink()) - { - return Some(level); - } - } - None +/// The root the link guard starts below: the project that owns `stop_dir` +/// (`/.socket`), so `.socket` itself is a guarded level. +fn guard_root(stop_dir: &Path) -> &Path { + stop_dir.parent().unwrap_or(stop_dir) } /// Persist a committed JSON ledger: pretty-printed with a trailing newline @@ -123,7 +111,16 @@ async fn linked_level<'a>(dir: &'a Path, stop_dir: &Path) -> Option<&'a Path> { /// Unlike [`remove_file_and_prune`], nothing was written here, so pruning /// on the error path is the right asymmetry. A parent that already existed /// (the user's, or another run's) is never removed. +/// +/// A ledger under a `.socket/` is refused, before anything is read or +/// written, when any level between the project root and the ledger +/// (`.socket` and the ledger itself included) is a link: socket-patch +/// never creates one there, so its target belongs to another project or +/// lies outside this one (#887). pub(crate) async fn write_json_ledger(path: &Path, value: &T) -> std::io::Result<()> { + if let Some(root) = containment::socket_project_root(path) { + containment::ensure_unlinked(root, path, "write")?; + } let mut bytes = serde_json::to_vec_pretty(value).map_err(std::io::Error::other)?; bytes.push(b'\n'); if matches!(read_regular_to_bytes(path).await, Ok(existing) if existing == bytes) { @@ -138,7 +135,7 @@ pub(crate) async fn write_json_ledger(path: &Path, value: &T) -> s Ok(()) => Ok(()), Err(e) => { if created_parent { - if let Some(stop) = nearest_socket_dir(path) { + if let Some(stop) = containment::nearest_socket_dir(parent) { prune_empty_dirs(parent, stop).await; } } @@ -147,16 +144,6 @@ pub(crate) async fn write_json_ledger(path: &Path, value: &T) -> s } } -/// The nearest ancestor of `path` literally named `.socket` — the fence for -/// an error-path prune. `None` (a ledger that does not live under a -/// `.socket/`) means: never climb. -fn nearest_socket_dir(path: &Path) -> Option<&Path> { - path.ancestors().skip(1).find(|a| { - a.file_name() - .is_some_and(|n| n == crate::constants::SOCKET_DIR) - }) -} - #[cfg(test)] mod tests { use super::*; @@ -323,6 +310,50 @@ mod tests { assert!(shared.join("uuid/a.tgz").is_file(), "target untouched"); } + /// #887: `.socket` itself linked to a directory another project + /// shares. Deleting a unit, unlinking a ledger and writing a ledger all + /// refuse before touching the shared tree. + #[cfg(unix)] + #[tokio::test] + async fn a_linked_socket_dir_is_never_written_or_deleted_through() { + let tmp = tempfile::tempdir().unwrap(); + let shared = tmp.path().join("sock"); + let unit = shared.join("vendor/npm/uuid"); + tokio::fs::create_dir_all(&unit).await.unwrap(); + tokio::fs::write(unit.join("a.tgz"), b"x").await.unwrap(); + tokio::fs::write(shared.join("vendor/state.json"), b"{}") + .await + .unwrap(); + let project = tmp.path().join("a"); + tokio::fs::create_dir_all(&project).await.unwrap(); + std::os::unix::fs::symlink(&shared, project.join(".socket")).unwrap(); + let socket = project.join(".socket"); + + let err = remove_tree_and_prune(&socket.join("vendor/npm/uuid"), &socket) + .await + .unwrap_err(); + assert!(err.to_string().contains("is a symlink"), "{err}"); + let err = remove_file_and_prune(&socket.join("vendor/state.json"), &socket) + .await + .unwrap_err(); + assert!(err.to_string().contains("is a symlink"), "{err}"); + let ledger = Ledger { + version: 1, + entries: vec![], + }; + let err = write_json_ledger(&socket.join("vendor/state.json"), &ledger) + .await + .unwrap_err(); + assert!(err.to_string().contains("is a symlink"), "{err}"); + + assert!(unit.join("a.tgz").is_file(), "shared unit untouched"); + assert_eq!( + std::fs::read(shared.join("vendor/state.json")).unwrap(), + b"{}", + "shared ledger untouched" + ); + } + /// Links at or above `stop_dir` (a project reached through a linked /// path, `/tmp -> /private/tmp`) are not socket-patch's concern. #[cfg(unix)] diff --git a/crates/socket-patch-core/src/vendor/path.rs b/crates/socket-patch-core/src/vendor/path.rs index eead69e1e..6bf8c6011 100644 --- a/crates/socket-patch-core/src/vendor/path.rs +++ b/crates/socket-patch-core/src/vendor/path.rs @@ -70,43 +70,46 @@ pub fn vendor_uuid_dir_rel(eco: &str, uuid: &str) -> Option { Some(format!("{VENDOR_DIR}/{eco}/{uuid}")) } -/// The first of `.socket/vendor`, `.socket/vendor/` and +/// The outermost of `.socket`, `.socket/vendor`, `.socket/vendor/` and /// `.socket/vendor//` under `project_root` that is a symlink (or /// a Windows junction), project-relative and forward-slashed; `None` when /// none is. The `` level is only checked for a known ecosystem dir /// (`jvm` counts as `maven`, which also checks the JVM repository trees -/// `.socket/vendor/maven2` and `.socket/vendor/gradle`) and the `` level only for a canonical -/// uuid. +/// `.socket/vendor/maven2` and `.socket/vendor/gradle`) and the `` +/// level only for a canonical uuid. /// /// Vendor staging creates these dirs itself and never writes symlinks, so a /// linked level is never ours: its target may be another project's vendor -/// store (two projects sharing one `.socket/vendor/npm`). Writing a unit -/// through it, or deleting one, reaches that other project. Every vendor -/// and revert dispatch refuses on this before touching anything, as -/// [`sweep_vendor_dirs`] already skips a linked eco or uuid dir. +/// store (two projects sharing one `.socket/vendor/npm`, #664) or another +/// project's whole `.socket` (#887). Writing a unit through it, or deleting +/// one, reaches that other project. Every vendor and revert dispatch +/// refuses on this before touching anything, as [`sweep_vendor_dirs`] +/// already skips a linked eco or uuid dir. The check itself is +/// [`containment::linked_level`](crate::utils::containment::linked_level). pub fn vendor_dir_symlink(project_root: &Path, eco: &str, uuid: Option<&str>) -> Option { // A `jvm` ledger entry is reverted by the maven backend, and every // maven-family entry may own files in the JVM repository trees // (`.socket/vendor/maven2`, `.socket/vendor/gradle`) as well as a // `maven/` unit. let eco = if eco == "jvm" { "maven" } else { eco }; - let mut levels = vec![VENDOR_DIR.to_string()]; + let mut deepest = VENDOR_DIR.to_string(); + let mut trees: Vec = Vec::new(); if ECOSYSTEM_DIRS.contains(&eco) { - levels.push(format!("{VENDOR_DIR}/{eco}")); - if let Some(rel) = uuid.and_then(|u| vendor_uuid_dir_rel(eco, u)) { - levels.push(rel); - } + deepest = uuid + .and_then(|u| vendor_uuid_dir_rel(eco, u)) + .unwrap_or_else(|| format!("{VENDOR_DIR}/{eco}")); if eco == "maven" { - levels.extend( + trees.extend( super::jvm::apply::VENDOR_TREES .iter() .map(|t| t.to_string()), ); } } - levels.into_iter().find(|rel| { - std::fs::symlink_metadata(project_root.join(rel)) - .is_ok_and(|meta| meta.file_type().is_symlink()) + std::iter::once(deepest).chain(trees).find_map(|rel| { + let link = crate::utils::containment::linked_level(project_root, &project_root.join(rel))?; + let rel = link.strip_prefix(project_root).ok()?; + Some(rel.to_string_lossy().replace('\\', "/")) }) } @@ -1091,6 +1094,21 @@ mod tests { vendor_dir_symlink(&vendor, "jvm", None).as_deref(), Some(".socket/vendor") ); + + // #887: `.socket` itself shared with another project. Every + // ecosystem (known or not, with or without a uuid) reports it. + let shared = tmp.path().join("shared-socket"); + std::fs::create_dir_all(shared.join("vendor/npm").join(UUID)).unwrap(); + let linked = tmp.path().join("linked"); + std::fs::create_dir_all(&linked).unwrap(); + symlink(&shared, linked.join(".socket")).unwrap(); + for (eco, uuid) in [("npm", Some(UUID)), ("jvm", None), ("nope", None)] { + assert_eq!( + vendor_dir_symlink(&linked, eco, uuid).as_deref(), + Some(".socket"), + "{eco}" + ); + } } /// #664: a linked `.socket/vendor` makes every eco dir below it lstat diff --git a/crates/socket-patch-core/src/vendor/pypi_hatch.rs b/crates/socket-patch-core/src/vendor/pypi_hatch.rs index 6916e6e39..1a9a201dd 100644 --- a/crates/socket-patch-core/src/vendor/pypi_hatch.rs +++ b/crates/socket-patch-core/src/vendor/pypi_hatch.rs @@ -71,15 +71,11 @@ pub(super) async fn load( )); } let path = root.join(wheel); - let mut component_path = root.to_path_buf(); - for component in wheel.split('/') { - component_path.push(component); - if is_symlink(&component_path).await { - return Err(( - "pypi_hatch_pin_invalid", - "wheel path contains a symlink".into(), - )); - } + if crate::utils::containment::linked_level(root, &path).is_some() { + return Err(( + "pypi_hatch_pin_invalid", + "wheel path contains a symlink".into(), + )); } if tokio::fs::try_exists(&path).await.unwrap_or(true) && super::verify::file_sha256_hex(&path).await.as_deref() != Some(hash.as_str()) diff --git a/crates/socket-patch-core/src/vendor/redownload.rs b/crates/socket-patch-core/src/vendor/redownload.rs index 19d063096..3cab67c24 100644 --- a/crates/socket-patch-core/src/vendor/redownload.rs +++ b/crates/socket-patch-core/src/vendor/redownload.rs @@ -107,15 +107,8 @@ pub async fn restore( } Err(reason) => return Err(reason), }; - let mut cursor = root.to_path_buf(); - for part in entry.artifact.path.split('/') { - cursor.push(part); - if tokio::fs::symlink_metadata(&cursor) - .await - .is_ok_and(|m| m.file_type().is_symlink()) - { - return Err("vendor_path_unsafe: artifact path contains a symlink".into()); - } + if crate::utils::containment::linked_level(root, &root.join(&entry.artifact.path)).is_some() { + return Err("vendor_path_unsafe: artifact path contains a symlink".into()); } let file_shaped = !super::verify::is_vlt_dir_entry(entry) && super::verify::artifact_is_file_shaped(&entry.artifact.path); @@ -332,15 +325,8 @@ pub async fn restore( } // Same guard as the artifact's own path: never swap a tree // reached through a link (the swap deletes what it replaces). - let mut cursor = root.to_path_buf(); - for part in rel.split('/') { - cursor.push(part); - if tokio::fs::symlink_metadata(&cursor) - .await - .is_ok_and(|m| m.file_type().is_symlink()) - { - return Err("vendor_path_unsafe: artifact path contains a symlink".into()); - } + if crate::utils::containment::linked_level(root, &root.join(rel)).is_some() { + return Err("vendor_path_unsafe: artifact path contains a symlink".into()); } tokio::fs::create_dir_all(target.parent().ok_or("tree has no parent")?) .await diff --git a/crates/socket-patch-core/src/vendor/reuse.rs b/crates/socket-patch-core/src/vendor/reuse.rs index 5b9e84e62..00cf37790 100644 --- a/crates/socket-patch-core/src/vendor/reuse.rs +++ b/crates/socket-patch-core/src/vendor/reuse.rs @@ -183,16 +183,7 @@ pub(crate) async fn verify_committed_artifact( // artifact) would let bytes from outside the vendored tree be "reused" // and then pinned into the lock. `checked_artifact_path` already // rejected `..`/empty segments, so each prefix is a real path level. - let mut prefix = project_root.to_path_buf(); - for seg in rel_path.split('/') { - prefix.push(seg); - match tokio::fs::symlink_metadata(&prefix).await { - Ok(meta) if meta.file_type().is_symlink() => return Err(ReuseMiss::NotRegular), - Ok(_) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(ReuseMiss::Missing), - Err(_) => return Err(ReuseMiss::Unreadable), - } - } + ensure_unlinked_level(project_root, &rel_path)?; // One FIFO-safe open (O_NONBLOCK + fstat on the handle): the size gate // and every byte below come from the same inode. @@ -329,22 +320,32 @@ pub(crate) async fn reusable_committed_dir( { return Err(ReuseMiss::Ambiguous); } - let mut prefix = project_root.to_path_buf(); - for seg in rel_dir.split('/') { - prefix.push(seg); - match tokio::fs::symlink_metadata(&prefix).await { - Ok(meta) if meta.file_type().is_symlink() => return Err(ReuseMiss::NotRegular), - Ok(_) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(ReuseMiss::Missing), - Err(_) => return Err(ReuseMiss::Unreadable), - } - } + ensure_unlinked_level(project_root, rel_dir)?; super::verify::verify_vendored_patch_record(project_root, &entry, record) .await .map_err(ReuseMiss::MemberMismatch)?; Ok(entry.artifact.file_inventory.unwrap_or_default()) } +/// Refuse `project_root/rel` when any level of it below the project root +/// is a link ([`crate::utils::containment::try_linked_level`]): a symlinked +/// uuid dir (or artifact) would let bytes from outside the vendored tree be +/// "reused" and then pinned into the lock. A level that is missing is +/// [`ReuseMiss::Missing`], one that cannot be probed [`ReuseMiss::Unreadable`]. +fn ensure_unlinked_level(project_root: &Path, rel: &str) -> Result<(), ReuseMiss> { + let path = project_root.join(rel); + match crate::utils::containment::try_linked_level(project_root, &path) { + Ok(Some(_)) => return Err(ReuseMiss::NotRegular), + Ok(None) => {} + Err(_) => return Err(ReuseMiss::Unreadable), + } + match std::fs::symlink_metadata(&path) { + Ok(_) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Err(ReuseMiss::Missing), + Err(_) => Err(ReuseMiss::Unreadable), + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/docs/ecosystems.md b/docs/ecosystems.md index b8347d15b..0c1217836 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -237,6 +237,22 @@ active Yarn pnpm install (a `yarn.lock`, and `nodeLinker: pnpm` with git; never `workspace:`, `portal:` or `link:`), and only when that store does not contain the project. +The same rule covers every ecosystem, as a containment check rather than a +list of known stores: agent-mode `apply` and `rollback` refuse, dry run +included, any directory they would write into that resolves outside the +install tree it was found in. Each directory a patch writes into must +resolve inside the package directory (a `flit install --symlink` package +linked from `site-packages` into its source is refused), and a Composer +package must resolve inside its vendor dir: a Composer path repository +symlinks `vendor//` to your own source by default, and that +source is patched directly, or installed as a copy with the repository's +`symlink` option set to `false`. A package that a package manager links into +`site-packages` from its own prefix (Homebrew links formula Python packages +in from the Cellar, Nix from the store) resolves outside the install tree +too and is refused the same way; install it into a virtualenv to patch it. +`rollback` refuses the same directories: a file an older socket-patch +patched in place there is restored from that source's version control. + Every command that looks for installed npm copies walks these same trees, not only `scan`. A package installed only under a pruned directory is therefore "not installed" to `scan --prune` / `--sync`, which garbage-collect its