Write the config file in one place too, since both did it identically

Both projects saved by rendering, writing a temp file through `private`,
and renaming over the target. `format::write` is that, and it belongs with
the house rules rather than beside either schema: it is the two existing
modules used together, and the thing worth stating once is why they are
used together at all.

That thing is the temp file, and it is not obvious. It is not always new
-- a save killed partway leaves one behind, and reopening it keeps
whatever mode it already had, which is then renamed straight over the file
holding the enrolled token hashes. Opening through `private` sets the mode
on the way in, so the fresh and the leftover case are the same case.

There is a test that creates a 0644 leftover and asserts the config comes
out 0600, because that state cannot arise on a machine where nothing has
ever crashed mid-save -- which is every machine either project has been
developed on.

The temp name is now appended rather than substituted, so `config.ron`
yields `config.ron.tmp`. `with_extension("tmp")` would have produced
`config.tmp`, which is a name that could plausibly belong to something
else.
This commit is contained in:
iris committed 2026-08-28 18:12:21 -04:00
1 parent 4de8bff5f2
commit db4552f4e4
2 files changed
+92 -2

No files matched your search

+2 -2
View File
@@ -100,7 +100,7 @@ diffing the two copies and finding nothing but a name between them.
- **`netif`** — `wg_address()`, which fails closed when the tunnel is down, and `local_addresses()` for the certificate's SANs. The product name is a parameter so the failure reads as advice rather than as a library complaining. Both split the *lookup* from the *decision*, so the failure path can be tested on a machine that has a tunnel — every machine this runs on does.
- **`enroll`** — token generation, hex-SHA-256 storage, constant-time comparison, the `<scheme>://enroll?…` URI, and the terminal QR. The URI scheme is the parameter, because it is what routes a scan back to the right app.
- **`certs`** — the CA generated once and never replaced, the leaf reissued every start. `product` names the organisation and common name and is the whole of what is per-project. Shared because being written twice is worst here: a trust anchor built two ways can be built differently two ways, and the difference surfaces as an opaque handshake failure on a phone.
- **`format`** — the two RON house rules. This was byte-identical in both projects, which made it the clearest thing in the evidence table and the easiest deletion.
- **`format`** — the two RON house rules, plus `write`, which renders a value and replaces a file with it atomically and owner-only. This was byte-identical in both projects, which made it the clearest thing in the evidence table and the easiest deletion.
- **`private`** — owner-only files and directories, taken from ai-app's version, with an `append_file` alongside `create_file` because a transcript must never be truncated by being opened.
- **`xdg`** — where each project's state lives: `config_home(product)` and `data_home(product)`, resolving the XDG variable, ignoring it unless absolute, and falling back under `$HOME`. Shared because the *reason* is shared and is not obvious from the code — the repo is a mount that resolves at different absolute paths on each side, so config inside it records paths that work on only one, and a CA private key inside it would let the untrusted side mint a leaf the pinned app trusts.
@@ -157,7 +157,7 @@ nowhere near the generator. Worth revisiting if it ever needs a second fix.
**Should follow, in this order.** Each is already near-identical:
1. ~~The Kotlin `EnrollmentScanActivity`, `PinnedCert`, and the enrollment/Keystore half of `ServerConfig`.~~ Done — see `app/` above. It paid as predicted: ai-app's `ServerConfig` had gained `localNetworkAllowed` and the KTX `edit` block while dev-updater's had not, so the two had drifted in both directions exactly as the evidence suggested.
2. Atomic owner-only config save. Both do temp-file-then-rename with the mode set before the rename; only the schema differs.
2. ~~Atomic owner-only config save.~~ Done — `format::write`, since it is the RON house rules and the owner-only rules used together. The subtlety it now states once is that the temp file is *not always new*: a save killed partway leaves one behind, and reopening it keeps whatever mode it had, which is then renamed over the file holding the enrolled token hashes. There is a test for exactly that, because it cannot happen on a machine where nothing has ever crashed mid-save.
**Should not.** Naming these is the point of the exercise:
+90
View File
@@ -35,8 +35,13 @@
/// `#![enable(implicit_some)]` header every project file would have to
/// remember, and matched on the writing side by `skip_serializing_if` so
/// nothing writes back a `Some(...)` a person didn't type.
use std::path::Path;
use anyhow::{Context, Result};
use serde::{Serialize, de::DeserializeOwned};
use crate::private;
fn options() -> ron::Options {
ron::Options::default().with_default_extension(ron::extensions::Extensions::IMPLICIT_SOME)
}
@@ -51,6 +56,40 @@ pub fn render<T: Serialize>(value: &T) -> Result<String, ron::Error> {
Ok(unwrap_outer(&text))
}
/// Renders `value` and replaces `path` with it, atomically and owner-only.
///
/// Whole-file-and-rename rather than an in-place edit, because both
/// projects' config files are small, are read at startup, and hold the
/// enrolled token hashes -- a half-written one would take the server down
/// on its next start with no way to fix it from a phone. The rename is
/// what makes a reader see either the old file or the new one and never
/// part of both.
///
/// The temp file goes through [`private::write_file`] rather than
/// `std::fs::write`, and that is the subtle half: **the temp file is not
/// always new.** A save killed partway leaves one behind, and opening that
/// again keeps whatever mode it already had -- which is then renamed over
/// the file holding the token hashes. Setting the mode as it is opened
/// covers both the fresh and the leftover case.
pub fn write<T: Serialize>(path: &Path, value: &T) -> Result<()> {
if let Some(parent) = path.parent() {
private::create_dir(parent)?;
}
let text = render(value).context("serialize config")?;
// Appended rather than substituted, so `config.ron` yields
// `config.ron.tmp` and not `config.tmp` -- a name that cannot collide
// with a real file and that says what it is a temporary copy of.
let tmp = path.with_file_name(format!(
"{}.tmp",
path.file_name()
.unwrap_or_else(|| std::ffi::OsStr::new("config"))
.to_string_lossy()
));
private::write_file(&tmp, text.as_bytes())?;
std::fs::rename(&tmp, path)
.with_context(|| format!("replace {} with {}", path.display(), tmp.display()))
}
/// Strips the outer `(`/`)` the writer always emits and removes the
/// indent level they cost. Deliberately narrow: it accepts only the
/// exact shape `PrettyConfig` produces, and leaves anything else alone
@@ -146,6 +185,57 @@ mod tests {
);
}
#[test]
fn what_is_written_reads_back_and_is_owner_only() {
use std::os::unix::fs::PermissionsExt;
let dir = tempfile::tempdir().expect("tempdir");
let path = dir.path().join("nested").join("config.ron");
let value = Demo {
name: "thing".to_string(),
note: None,
};
write(&path, &value).expect("write");
assert_eq!(
parse::<Demo>(&std::fs::read_to_string(&path).expect("read")).expect("re-read"),
value,
);
let mode = std::fs::metadata(&path).expect("stat").permissions().mode();
assert_eq!(mode & 0o777, 0o600, "config holds token hashes: {mode:o}");
assert!(
!path.with_extension("ron.tmp").exists(),
"the temp file is renamed away, not left behind",
);
}
/// The case the whole thing turns on, and the one that cannot happen on
/// a machine where nothing has ever crashed mid-save: a leftover temp
/// file from an interrupted write is reopened, and if its mode came
/// along it would be renamed straight over the file holding the enrolled
/// token hashes.
#[test]
fn a_leftover_temp_file_cannot_widen_the_config() {
use std::os::unix::fs::PermissionsExt;
let dir = tempfile::tempdir().expect("tempdir");
let path = dir.path().join("config.ron");
let tmp = dir.path().join("config.ron.tmp");
std::fs::write(&tmp, b"leftover from a save that died").expect("stale temp");
std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o644)).expect("widen");
write(&path, &Demo::default()).expect("write");
let mode = std::fs::metadata(&path).expect("stat").permissions().mode();
assert_eq!(
mode & 0o777,
0o600,
"a world-readable leftover must not become the config: {mode:o}",
);
}
/// A parse error's line number has to point at the real line, which is
/// why the opening paren is not followed by a newline.
#[test]