diff --git a/.devflow/features/mds-fmt/KNOWLEDGE.md b/.devflow/features/mds-fmt/KNOWLEDGE.md index e39dc8c3..14872959 100644 --- a/.devflow/features/mds-fmt/KNOWLEDGE.md +++ b/.devflow/features/mds-fmt/KNOWLEDGE.md @@ -211,7 +211,7 @@ Notable divergences worth knowing before touching this file: - **Partials are reformatted but `is_partial` still gates output emission elsewhere.** Don't conflate the two meanings of "partial" across `fmt` vs. `build`/`check`. - **Hidden FILES are still collected by the walker.** `is_default_excluded_dir` excludes hidden *directories* from recursion, not hidden `.mds` files at the traversed level. A `.dotfile.mds` at the root of a traversed directory is collected and formatted. - **`atomic_write_file` is the write primitive — not `std::fs::write`.** `fmt.rs` calls `atomic_write_file` from `output.rs` (shared with `lint.rs`). It provides a TOCTOU guard, Unix permission preservation (`mode & 0o7777`), and `sync_all()` + atomic rename. This means a failed write leaves the original file intact — it does NOT leave a partially-written file. And, since #227, `write_output`/dir-mode build/`.map` sidecars route through it too, with `Durability::RenameOnly` (fmt/lint pass `Fsync`). -- **`crates/mds-cli/tests/write_funnel.rs` fails CI on any raw `fs::write(` / `File::create(` in `crates/mds-cli/src` outside the two allow-listed sites** (`mds init` in main.rs; the test-only readiness marker in watch.rs). +- **`crates/mds-cli/tests/write_funnel.rs` fails CI on any raw `fs::write(` / `File::create(` in `crates/mds-cli/src` outside the one allow-listed site** (the test-only readiness marker in watch.rs; `mds init` joined the funnel in #386). ## Deferred follow-ups (recorded to avoid re-flagging as new debt) diff --git a/.devflow/features/mds-lint/KNOWLEDGE.md b/.devflow/features/mds-lint/KNOWLEDGE.md index 4dde7c35..63805bb5 100644 --- a/.devflow/features/mds-lint/KNOWLEDGE.md +++ b/.devflow/features/mds-lint/KNOWLEDGE.md @@ -549,7 +549,7 @@ LintDiagnostic.fix_removals (FixLineSpan) OR .fix_edits (TextEdit) **atomic_write_file temp prefix**: The temp file prefix is `.mds-tmp-`. Both lint and fmt share the same `atomic_write_file` from `output.rs`. Shared with `build`/`watch` since #227; third parameter `Durability` (`Fsync` for lint/fmt, `RenameOnly` for build/watch). -**`crates/mds-cli/tests/write_funnel.rs` fails CI on any raw `fs::write(` / `File::create(` in `crates/mds-cli/src`** outside the two allow-listed sites (`mds init` in main.rs; the test-only readiness marker in watch.rs). +**`crates/mds-cli/tests/write_funnel.rs` fails CI on any raw `fs::write(` / `File::create(` in `crates/mds-cli/src`** outside the one allow-listed site (the test-only readiness marker in watch.rs; `mds init` joined the funnel in #386). **Python `LintDiagnostic.fix_edits` getter vs `#[pyo3(get)]`**: `Vec` does not implement `IntoPy`. Use the custom `#[getter]` which calls `value_to_py`. Stored internally as `Option>`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ec2e7f8..06817f2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,7 +108,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `cannot get metadata` message that #240 emitted on every first write of a not-yet-existing file is gone; a stat failure other than "not found" is now a hard error rather than a warning (#225). A new test, `write_funnel.rs`, fails CI on any raw - `fs::write`/`File::create` in the CLI outside the two justified sites. + `fs::write`/`File::create` in the CLI outside the one justified site (the test-only + readiness marker in `watch.rs`; `mds init` joined the funnel in #386). - **Fix stale `lint_str` rustdoc and lint-rule Tier tables (#329).** `mds-core`'s `lint_str` rustdoc said "applies the 9 lint rules" after a 10th rule (`legacy-interpolation`) had shipped; the Tier tables in `lint/tier.rs` and @@ -134,6 +135,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 triggered by template input, only by a defect, and the messages carry no source text. On the CLI a tripped invariant is a Rust panic (exit code 101); the napi, WASM and Python bindings convert it to `mds::internal` as before. +- **`mds init` no longer writes through a symlink at the target path (#386).** The + starter file is written by the same replace-by-rename primitive as every other CLI + write (`atomic_write_file`): a symlink at the target — live under `--force`, or + dangling without it — is refused with `cannot write : refusing to replace a + symlink` (exit 1) and neither the link nor its target is touched. Previously a + dangling link was written through (creating its target) and `--force` truncated a + live link's target in place. `--force` on a regular file still replaces it, now + atomically with its permission bits preserved; a new file keeps the umask default + mode. The `init` entry is gone from the `write_funnel.rs` allow-list, so the CLI's + only remaining raw write is the test-only readiness marker in `watch.rs`. - **`mds lint ` fails closed on a path it cannot name (#217).** The directory-mode `files[].file` key and the sort key are the entry's path relative to the lint root. A path that is not valid UTF-8, or that is not under the lint root, previously produced diff --git a/SECURITY.md b/SECURITY.md index 4b4855ab..684a12fe 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -51,9 +51,9 @@ input. The compiler enforces several defense-in-depth controls: derived from a lossy string. A root that is empty or not valid UTF-8 is treated as "no root" — entries degrade to basenames — so it can never make the containment check vacuous. -- **Replace-by-rename writes**: `mds fmt`, `mds lint --fix`, and `mds build`/`mds - watch` outputs and `.map` sidecars are written to a same-directory temp file and - renamed over the target after a final symlink re-check (`mds-cli/src/output.rs`, +- **Replace-by-rename writes**: `mds fmt`, `mds lint --fix`, `mds init`, and `mds + build`/`mds watch` outputs and `.map` sidecars are written to a same-directory + temp file and renamed over the target after a final symlink re-check (`mds-cli/src/output.rs`, `atomic_write_file`; enforced by `crates/mds-cli/tests/write_funnel.rs`), so a crash never leaves a truncated target and a symlinked output path is refused. Consequence: hard links, ACLs, xattrs, and owner/group of a pre-existing target diff --git a/crates/mds-cli/src/main.rs b/crates/mds-cli/src/main.rs index 8d2a6b20..7e7ae28d 100644 --- a/crates/mds-cli/src/main.rs +++ b/crates/mds-cli/src/main.rs @@ -418,15 +418,12 @@ Your items: - {{item}} @end "; - // Raw std::fs::write is deliberate (#227). What it is NOT justified by: "init only - // ever creates a file". `--force` skips the exists-check above and truncates in - // place, and both `exists()` and `write` follow a symlink, so a link at `filename` - // — dangling, or live under `--force` — is written through to its target. The raw - // write stays because the blast radius is small and entirely user-directed: - // `filename` is typed on the command line and `starter` is a fixed public template - // that a re-run reproduces. Allow-listed in tests/write_funnel.rs. - std::fs::write(&filename, starter) - .map_err(|e| miette::miette!("cannot write {}: {e}", filename.display()))?; + // #386: the same replace-by-rename primitive as every other CLI write — a symlink + // at `filename` (live under `--force`, dangling without it) is refused instead of + // written through; `--force` replaces a regular file by rename with its mode + // preserved. `RenameOnly` because the starter is a fixed public template a re-run + // reproduces. + output::atomic_write_file(&filename, starter, output::Durability::RenameOnly)?; if !quiet { eprintln!( "Created {}\n Try: mds build {}", diff --git a/crates/mds-cli/tests/cli_commands.rs b/crates/mds-cli/tests/cli_commands.rs index aa2fcedc..e83b6433 100644 --- a/crates/mds-cli/tests/cli_commands.rs +++ b/crates/mds-cli/tests/cli_commands.rs @@ -131,6 +131,171 @@ fn init_does_not_overwrite_existing_file() { assert_eq!(content, "original content"); } +/// T-D1-1 (#386): `mds init --force` at a live symlink is refused; the link and its +/// target survive untouched. Positive control in the same test: `mds init` on a plain +/// (non-symlink) path still succeeds normally. +#[cfg(unix)] +#[test] +fn init_force_symlink_target_refused_plain_path_created() { + let dir = tempfile::tempdir().unwrap(); + let real = dir.path().join("real.txt"); + std::fs::write(&real, "REAL").unwrap(); + let link = dir.path().join("link.mds"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + let refused = mds_bin() + .args(["init", link.to_str().unwrap(), "--force"]) + .output() + .unwrap(); + + let stderr = String::from_utf8_lossy(&refused.stderr); + assert_eq!( + refused.status.code(), + Some(1), + "init --force onto a symlink must exit 1; stderr: {stderr}" + ); + assert!( + stderr.contains("refusing to replace a symlink"), + "the refusal must say why; got: {stderr}" + ); + assert_eq!( + std::fs::read_to_string(&real).unwrap(), + "REAL", + "the symlink target must not be written through" + ); + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the symlink itself must survive the refusal" + ); + + // Positive control: init on a plain (non-symlink) path still succeeds. + let plain = dir.path().join("plain.mds"); + let ok = mds_bin() + .args(["init", plain.to_str().unwrap()]) + .output() + .unwrap(); + assert_eq!( + ok.status.code(), + Some(0), + "init on a plain path must still succeed; stderr: {}", + String::from_utf8_lossy(&ok.stderr) + ); + assert!( + std::fs::read_to_string(&plain) + .unwrap() + .contains("Hello {{name}}!"), + "init must write the starter template" + ); +} + +/// T-D1-2 (#386): `mds init` (no `--force`) at a dangling symlink is refused; the +/// target is never created and the link survives. Positive control in the same test: +/// `mds init` on a plain (non-symlink) path still succeeds normally. +#[cfg(unix)] +#[test] +fn init_dangling_symlink_refused_without_force() { + let dir = tempfile::tempdir().unwrap(); + let victim = dir.path().join("victim.txt"); + let link = dir.path().join("link.mds"); + std::os::unix::fs::symlink(&victim, &link).unwrap(); + + let refused = mds_bin() + .args(["init", link.to_str().unwrap()]) + .output() + .unwrap(); + + let stderr = String::from_utf8_lossy(&refused.stderr); + assert_eq!( + refused.status.code(), + Some(1), + "init onto a dangling symlink must exit 1; stderr: {stderr}" + ); + assert!( + stderr.contains("refusing to replace a symlink"), + "the refusal must say why; got: {stderr}" + ); + assert_eq!( + victim.symlink_metadata().unwrap_err().kind(), + std::io::ErrorKind::NotFound, + "the dangling link's target must not be created" + ); + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the symlink itself must survive the refusal" + ); + + // Positive control: init on a plain (non-symlink) path still succeeds. + let plain = dir.path().join("plain.mds"); + let ok = mds_bin() + .args(["init", plain.to_str().unwrap()]) + .output() + .unwrap(); + assert_eq!( + ok.status.code(), + Some(0), + "init on a plain path must still succeed; stderr: {}", + String::from_utf8_lossy(&ok.stderr) + ); + assert!( + std::fs::read_to_string(&plain) + .unwrap() + .contains("Hello {{name}}!"), + "init must write the starter template" + ); +} + +/// T-D1-3 (#386, pin): `mds init --force` on a regular file replaces it atomically via +/// rename, preserving its permission bits, and leaves no `.mds-tmp-` temp file behind. +/// Already green on `main` before this PR — this test pins the contract stated in the +/// PR body so a future regression is caught. +#[cfg(unix)] +#[test] +fn init_force_regular_file_replaced_atomically_mode_preserved() { + use std::os::unix::fs::PermissionsExt as _; + + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("keep.mds"); + std::fs::write(&target, "original content").unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o600)).unwrap(); + + let output = mds_bin() + .args(["init", target.to_str().unwrap(), "--force"]) + .output() + .unwrap(); + + assert_eq!( + output.status.code(), + Some(0), + "init --force on a regular file must succeed; stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let content = std::fs::read_to_string(&target).unwrap(); + assert!( + content.contains("Hello {{name}}!"), + "the overwritten file must contain the starter template; got: {content:?}" + ); + let mode = std::fs::metadata(&target).unwrap().permissions().mode(); + assert_eq!( + mode & 0o777, + 0o600, + "the replaced file must preserve the original permission bits" + ); + let leftover_tmp = std::fs::read_dir(dir.path()) + .unwrap() + .filter_map(|e| e.ok()) + .any(|e| e.file_name().to_string_lossy().starts_with(".mds-tmp-")); + assert!( + !leftover_tmp, + "no .mds-tmp- temp file should remain in the directory" + ); +} + #[test] fn set_flag_cli_overrides() { // --set name=Test should override the frontmatter variable 'name' diff --git a/crates/mds-cli/tests/write_funnel.rs b/crates/mds-cli/tests/write_funnel.rs index 95177217..fe72f6e1 100644 --- a/crates/mds-cli/tests/write_funnel.rs +++ b/crates/mds-cli/tests/write_funnel.rs @@ -40,22 +40,12 @@ const NEEDLES: &[&str] = &["fs::write(", "File::create("]; /// reported as dead by [`write_sites_are_funnelled`] and by /// [`every_allowlist_entry_is_live`], so a removed site cannot leave a stale licence /// behind for a future raw write to hide under. -const ALLOWED_RAW_WRITES: &[(&str, &str, usize, &str)] = &[ - ( - "main.rs", - "fs::write(", - 1, - "mds init scaffolds a fixed public template at a user-typed path, reproducible by \ - a re-run — NOT because it only ever creates: --force truncates in place and the \ - write follows a symlink at the target (#227)", - ), - ( - "watch.rs", - "fs::write(", - 1, - "test-only readiness marker: written to .tmp then renamed — already atomic", - ), -]; +const ALLOWED_RAW_WRITES: &[(&str, &str, usize, &str)] = &[( + "watch.rs", + "fs::write(", + 1, + "test-only readiness marker: written to .tmp then renamed — already atomic", +)]; #[test] fn write_sites_are_funnelled() { diff --git a/spec.md b/spec.md index f3ac070a..e790d517 100644 --- a/spec.md +++ b/spec.md @@ -926,7 +926,7 @@ mds build src/ --out-dir dist # Mirror subtree: src/a/b.mds → dis In all paths, `` is `md` for Markdown templates and `json` for messages templates. -**Output writing.** Compiled outputs and `.map` sidecars written by `mds build` and `mds watch`, and `.mds` sources rewritten by `mds fmt` and `mds lint --fix`, are written to a temporary file in the target's directory and then renamed over the target, after a final symlink re-check of the target: a crash, kill or full disk never leaves a truncated file behind, and a destination path that is a symlink is refused. Because the output is created as a sibling temporary file and renamed into place, the destination must be a regular-file path inside a writable directory: device files such as `/dev/null` and FIFOs are not supported as `-o` targets (write to stdout instead). Source rewrites (`fmt`, `lint --fix`) are additionally fsynced before the rename; compiled outputs and sidecars rely on the rename alone — they are regenerable, and an unconditional fsync made directory-mode startup several times slower on macOS. Because the rename gives the target a new inode, a pre-existing target's hard links (other links keep the old content), ACLs, extended attributes, and owner/group are not preserved; permission bits are preserved on Unix. This is enforced for the CLI's write sites by `crates/mds-cli/tests/write_funnel.rs`. +**Output writing.** Compiled outputs and `.map` sidecars written by `mds build` and `mds watch`, `.mds` sources rewritten by `mds fmt` and `mds lint --fix`, and the starter file written by `mds init`, are written to a temporary file in the target's directory and then renamed over the target, after a final symlink re-check of the target: a crash, kill or full disk never leaves a truncated file behind, and a destination path that is a symlink is refused. Because the output is created as a sibling temporary file and renamed into place, the destination must be a regular-file path inside a writable directory: device files such as `/dev/null` and FIFOs are not supported as `-o` targets (write to stdout instead). Source rewrites (`fmt`, `lint --fix`) are additionally fsynced before the rename; compiled outputs and sidecars rely on the rename alone — they are regenerable, and an unconditional fsync made directory-mode startup several times slower on macOS. Because the rename gives the target a new inode, a pre-existing target's hard links (other links keep the old content), ACLs, extended attributes, and owner/group are not preserved; permission bits are preserved on Unix. This is enforced for the CLI's write sites by `crates/mds-cli/tests/write_funnel.rs`. ### 7.3 `mds check` @@ -1274,7 +1274,7 @@ mds init my-prompt.mds # Creates my-prompt.mds mds init my-prompt.mds --force # Overwrite if file already exists ``` -Creates a compilable starter template. Path traversal (e.g. `../escaped.mds`) is rejected. +Creates a compilable starter template. Path traversal (e.g. `../escaped.mds`) is rejected. The file is written through the replace-by-rename primitive of §7.2 "Output writing": a symlink at the target — live or dangling — is refused (exit 1, `cannot write : refusing to replace a symlink`) rather than written through; `--force` replaces a regular file atomically, preserving its permission bits. ### 7.7 Auto-Detection