diff --git a/.gitignore b/.gitignore index ce2b3a6935..770aa83ef8 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,8 @@ # env .env* trusted-server.toml +# partial copies left if the ts CLI config writer is killed mid-write +.ts-write-* js-assets.toml # backup diff --git a/crates/trusted-server-cli/src/commands/audit/generate/mod.rs b/crates/trusted-server-cli/src/commands/audit/generate/mod.rs index 010522f8fb..57a1ff76f1 100644 --- a/crates/trusted-server-cli/src/commands/audit/generate/mod.rs +++ b/crates/trusted-server-cli/src/commands/audit/generate/mod.rs @@ -28,52 +28,14 @@ use crate::commands::audit::generate::slot_toml::{ render_slots, replace_key_in_section, resolve_network_id, splice_creative_slots, toml_string, }; use crate::commands::config::init::EXAMPLE_CONFIG; -use crate::error::{CliResult, cli_error, report_error}; +use crate::error::{CliResult, cli_error, report_error, toml_error_location}; +use crate::fs_util::write_file_atomically; use analyzer::{analyze_collected_page, extract_gtm_container_id}; pub(crate) use browser_collector::DeviceProfile; pub(crate) use crawl_plan::CrawlBudget; -/// Writes `contents` to `path` atomically: a same-directory temp file is -/// written and fsynced, then renamed over the target, then the directory entry -/// is fsynced. -/// -/// A plain `fs::write` truncates the destination before writing, so a full disk -/// or an interrupted run would leave an operator's `trusted-server.toml` empty -/// or half-written. `rename` within a directory is atomic, so a reader sees -/// either the old file or the complete new one. -/// -/// The target's existing permissions are carried onto the replacement, since -/// the temp file is created 0600 and the config may intentionally be broader. -/// -/// # Errors -/// -/// Returns the underlying I/O error when the temp file cannot be created, -/// written, synced, or renamed over `path`. -fn write_file_atomically(path: &Path, contents: &str) -> std::io::Result<()> { - let directory = path - .parent() - .filter(|parent| !parent.as_os_str().is_empty()) - .unwrap_or_else(|| Path::new(".")); - - let mut temp = tempfile::Builder::new() - .prefix(".ts-audit-") - .tempfile_in(directory)?; - temp.write_all(contents.as_bytes())?; - temp.as_file().sync_all()?; - if let Ok(metadata) = fs::metadata(path) { - temp.as_file().set_permissions(metadata.permissions())?; - } - temp.persist(path).map_err(|error| error.error)?; - - // Best-effort durability for the rename itself. Opening a directory handle - // is not portable (Windows rejects it), and the content is already safely - // on disk either way, so a failure here is not worth failing the command. - let _ = fs::File::open(directory).and_then(|handle| handle.sync_all()); - Ok(()) -} - /// Arguments for `ts audit generate ` — bootstraps draft Trusted Server /// config and JavaScript asset audit files from a live page (issue #800). #[derive(Debug, clap::Args)] @@ -1197,7 +1159,10 @@ pub(crate) fn run_update_slots( /// unrelated operator configuration from dry-run output. fn managed_creative_projection(document: &str) -> CliResult { let value = toml::from_str::(document).map_err(|error| { - report_error(format!("failed to parse config for dry-run diff: {error}")) + report_error(format!( + "failed to parse config for dry-run diff{}", + toml_error_location(document, error.span()) + )) })?; let creative = value .get("creative_opportunities") @@ -4376,4 +4341,20 @@ mod tests { "the concurrent edit must not be overwritten" ); } + + #[test] + fn managed_creative_projection_parse_error_does_not_echo_config_source() { + let source = "[publisher]\norigin_url = \"https://origin-private.example.com/internal\n"; + + let error = managed_creative_projection(source).expect_err("should reject invalid TOML"); + + assert!( + !error.contains("origin-private.example.com"), + "should not echo config source text: {error}" + ); + assert!( + error.contains("at line 2, column "), + "should report the error location: {error}" + ); + } } diff --git a/crates/trusted-server-cli/src/commands/audit/generate/slot_toml.rs b/crates/trusted-server-cli/src/commands/audit/generate/slot_toml.rs index c5d1627d85..2ecb2e4f2a 100644 --- a/crates/trusted-server-cli/src/commands/audit/generate/slot_toml.rs +++ b/crates/trusted-server-cli/src/commands/audit/generate/slot_toml.rs @@ -12,7 +12,7 @@ use trusted_server_core::creative_opportunities::{ #[cfg(test)] use crate::commands::audit::generate::gpt_slots; -use crate::error::{CliResult, cli_error, report_error}; +use crate::error::{CliResult, cli_error, report_error, toml_error_location}; /// A slot ready to render — the union of discovered and existing fields, without /// the core type's `pub(crate)` compiled-pattern cache. @@ -627,7 +627,8 @@ pub(super) fn splice_creative_slots( ) -> CliResult { let mut document = existing.parse::().map_err(|error| { report_error(format!( - "failed to parse target config before updating slots: {error}" + "failed to parse target config{} before updating slots", + toml_error_location(existing, error.span()) )) })?; let had_section = document.get("creative_opportunities").is_some(); @@ -700,8 +701,12 @@ pub(super) fn splice_creative_slots( /// Verifies that the structural update changed only generator-managed fields. fn ensure_only_managed_fields_changed(before: &str, after: &str) -> CliResult<()> { fn unmanaged(document: &str) -> CliResult { - let mut value = toml::from_str::(document) - .map_err(|error| report_error(format!("failed to validate updated config: {error}")))?; + let mut value = toml::from_str::(document).map_err(|error| { + report_error(format!( + "failed to validate updated config{}", + toml_error_location(document, error.span()) + )) + })?; if let Some(root) = value.as_table_mut() { let remove_empty = if let Some(creative) = root .get_mut("creative_opportunities") @@ -2466,4 +2471,21 @@ slot_id = "sidebar" toml::from_str::(&doc).expect("exotic targeting key renders as valid TOML"); } + + #[test] + fn ensure_only_managed_fields_changed_parse_error_does_not_echo_config_source() { + let source = "[publisher]\norigin_url = \"https://origin-private.example.com/internal\n"; + + let error = ensure_only_managed_fields_changed(source, source) + .expect_err("should reject invalid TOML"); + + assert!( + !error.contains("origin-private.example.com"), + "should not echo config source text: {error}" + ); + assert!( + error.contains("at line 2, column "), + "should report the error location: {error}" + ); + } } diff --git a/crates/trusted-server-cli/src/commands/audit/generate/validate.rs b/crates/trusted-server-cli/src/commands/audit/generate/validate.rs index 4334cb398d..a12e135100 100644 --- a/crates/trusted-server-cli/src/commands/audit/generate/validate.rs +++ b/crates/trusted-server-cli/src/commands/audit/generate/validate.rs @@ -9,7 +9,7 @@ use trusted_server_core::config::TrustedServerAppConfig; -use crate::error::{CliResult, cli_error}; +use crate::error::{CliResult, cli_error, toml_error_location}; /// Validates the candidate config text the generator is about to persist. /// @@ -49,14 +49,14 @@ pub(super) fn check_candidate(candidate: &str, baseline: &str) -> CliResult CliResult<()> { let config: TrustedServerAppConfig = toml::from_str(source).map_err(|error| { - error.span().map_or_else( - || "config could not be loaded".to_string(), - |span| format!("config could not be loaded at byte offset {}", span.start), + format!( + "config could not be loaded{}", + toml_error_location(source, error.span()) ) })?; TrustedServerAppConfig::new(config.into_settings()) @@ -93,10 +93,7 @@ mod tests { error.contains("refusing to write"), "should explain refusal" ); - assert!( - error.contains("byte offset"), - "should retain parser location" - ); + assert!(error.contains("at line "), "should retain parser location"); } #[test] @@ -111,7 +108,7 @@ mod tests { "should not disclose source bytes" ); assert!( - warnings[0].contains("byte offset"), + warnings[0].contains("at line "), "should retain parser location" ); } @@ -158,7 +155,7 @@ mod tests { "should cover schema as well as syntax errors" ); assert!( - error.contains("byte offset"), + error.contains("at line "), "should keep the parser location" ); } diff --git a/crates/trusted-server-cli/src/commands/audit/mod.rs b/crates/trusted-server-cli/src/commands/audit/mod.rs index 6da0171b96..1791566a0a 100644 --- a/crates/trusted-server-cli/src/commands/audit/mod.rs +++ b/crates/trusted-server-cli/src/commands/audit/mod.rs @@ -17,7 +17,7 @@ use clap::{Args, Subcommand}; use crate::app_config::AppConfigArgs; use crate::commands::audit::collector::{BrowserOpts, GenerateBrowserOpts}; use crate::commands::audit::page::PageAuditArgs; -use crate::error::{CliResult, cli_error}; +use crate::error::{CliResult, cli_error, toml_error_location}; use crate::run::RunOutcome; /// Parses and validates an `http`/`https` URL, rejecting all other schemes. @@ -427,10 +427,7 @@ fn creative_config( // Parser messages can quote literal secrets from the operator's config. // Keep the path and location, but never include the source or error text. let value = toml::from_str::(document).map_err(|error| { - let location = error - .span() - .map(|span| format!(" at byte offset {}", span.start)) - .unwrap_or_default(); + let location = toml_error_location(document, error.span()); format!( "failed to parse {}{location} before generating slots; fix the TOML syntax and re-run", path.display() @@ -527,10 +524,7 @@ mod tests { error.contains("fix the TOML syntax and re-run"), "should retain repair guidance, got {error}" ); - assert!( - error.contains("byte offset"), - "should retain parser location" - ); + assert!(error.contains("at line "), "should retain parser location"); assert!(!error.contains('\n'), "should not include a source excerpt"); } diff --git a/crates/trusted-server-cli/src/error.rs b/crates/trusted-server-cli/src/error.rs index 1ca36f0c4b..6706cda7aa 100644 --- a/crates/trusted-server-cli/src/error.rs +++ b/crates/trusted-server-cli/src/error.rs @@ -36,3 +36,48 @@ pub enum CliError { } impl Error for CliError {} + +/// Formats a TOML error's location without the parser's message. +/// +/// Parser and deserializer messages quote the offending source line or value, +/// which can hold data an operator keeps private, so only the 1-based line and +/// column of the error within `source` are reported. +pub(crate) fn toml_error_location(source: &str, span: Option>) -> String { + let Some(start) = span.map(|span| span.start) else { + return String::new(); + }; + let before = source.get(..start).unwrap_or(source); + let line = before.matches('\n').count() + 1; + let column = before + .rsplit('\n') + .next() + .map_or(0, |tail| tail.chars().count()) + + 1; + format!(" at line {line}, column {column}") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn toml_error_location_reports_line_and_column() { + let source = "a = 1\nb = \"x\n"; + + let location = toml_error_location(source, Some(10..11)); + + assert_eq!( + location, " at line 2, column 5", + "should map offset to line and column" + ); + } + + #[test] + fn toml_error_location_is_empty_without_span() { + assert_eq!( + toml_error_location("a = 1", None), + "", + "should omit location without span" + ); + } +} diff --git a/crates/trusted-server-cli/src/fs_util.rs b/crates/trusted-server-cli/src/fs_util.rs new file mode 100644 index 0000000000..36424e4f88 --- /dev/null +++ b/crates/trusted-server-cli/src/fs_util.rs @@ -0,0 +1,73 @@ +//! Filesystem helpers shared by `ts` subcommands. + +use std::fs; +use std::io::Write as _; +use std::path::Path; + +/// Writes `contents` to `path` atomically: a same-directory temp file is +/// written and fsynced, then renamed over the target, then the directory entry +/// is fsynced. +/// +/// A plain `fs::write` truncates the destination before writing, so a full disk +/// or an interrupted run would leave the target empty or half-written. `rename` +/// within a directory is atomic, so a reader sees either the old file or the +/// complete new one. If any step fails, the temp file is removed on drop. +/// +/// The temp file is created 0600. When the target already exists, its mode +/// bits are carried onto the replacement; a new file stays 0600. Because the +/// target is replaced by rename, its owner, group, ACLs and hard links are not +/// preserved, and a symlink at `path` is replaced by a regular file rather than +/// updating the file it points to. +/// +/// # Errors +/// +/// Returns the underlying I/O error when the temp file cannot be created, +/// written, synced, or renamed over `path`. +pub(crate) fn write_file_atomically(path: &Path, contents: &str) -> std::io::Result<()> { + let directory = path + .parent() + .filter(|parent| !parent.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + + let mut temp = tempfile::Builder::new() + .prefix(".ts-write-") + .tempfile_in(directory)?; + temp.write_all(contents.as_bytes())?; + temp.as_file().sync_all()?; + if let Ok(metadata) = fs::metadata(path) { + temp.as_file().set_permissions(metadata.permissions())?; + } + temp.persist(path).map_err(|error| error.error)?; + + // Best-effort durability for the rename itself. Opening a directory handle + // is not portable (Windows rejects it), and the content is already safely + // on disk either way, so a failure here is not worth failing the command. + let _ = fs::File::open(directory).and_then(|handle| handle.sync_all()); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn write_file_atomically_removes_temp_file_when_rename_fails() { + let temp = tempfile::TempDir::new().expect("should create temp dir"); + let target = temp.path().join("occupied"); + fs::create_dir(&target).expect("should create directory at target"); + fs::write(target.join("child"), "x").expect("should populate target directory"); + + write_file_atomically(&target, "contents") + .expect_err("should fail to rename over a non-empty directory"); + + let entries: Vec<_> = fs::read_dir(temp.path()) + .expect("should list temp dir") + .map(|entry| entry.expect("should read entry").file_name()) + .collect(); + assert_eq!( + entries, + vec![std::ffi::OsString::from("occupied")], + "should leave no temp file behind after a failed write" + ); + } +} diff --git a/crates/trusted-server-cli/src/lib.rs b/crates/trusted-server-cli/src/lib.rs index 24e001d853..7f6b9e6e73 100644 --- a/crates/trusted-server-cli/src/lib.rs +++ b/crates/trusted-server-cli/src/lib.rs @@ -5,6 +5,8 @@ mod app_config; #[cfg(not(target_arch = "wasm32"))] mod error; #[cfg(not(target_arch = "wasm32"))] +mod fs_util; +#[cfg(not(target_arch = "wasm32"))] mod prebid_bundle; #[cfg(not(target_arch = "wasm32"))] mod run; diff --git a/crates/trusted-server-cli/src/prebid_bundle.rs b/crates/trusted-server-cli/src/prebid_bundle.rs index 609c950531..98bc7d7450 100644 --- a/crates/trusted-server-cli/src/prebid_bundle.rs +++ b/crates/trusted-server-cli/src/prebid_bundle.rs @@ -8,6 +8,9 @@ use std::process::{Command, Stdio}; use serde::{Deserialize, Deserializer, Serialize}; use toml_edit::{DocumentMut, Item, table, value}; +use crate::error::toml_error_location; +use crate::fs_util::write_file_atomically; + pub(crate) type CliResult = Result; const NODE_MODULES_MISSING_HELP: &str = "Prebid bundling dependencies are missing. Run `cd crates/trusted-server-js/lib && npm ci`, then retry `ts prebid client`."; @@ -480,8 +483,9 @@ pub(crate) fn load_bundle_config(config_path: &Path) -> CliResult CliResult CliResu })?; let mut document = contents.parse::().map_err(|error| { report_error(format!( - "failed to parse config {} for metadata update: {error}", - config_path.display() + "failed to parse config {}{} for metadata update", + config_path.display(), + toml_error_location(&contents, error.span()) )) })?; @@ -779,7 +787,12 @@ fn patch_config_metadata(config_path: &Path, sha256: &str, sri: &str) -> CliResu prebid.insert("external_bundle_sha256", value(sha256)); prebid.insert("external_bundle_sri", value(sri)); - write_atomic(config_path, &document.to_string()) + write_file_atomically(config_path, &document.to_string()).map_err(|error| { + report_error(format!( + "failed to write config {}: {error}", + config_path.display() + )) + }) } fn table_like_mut<'a>( @@ -795,46 +808,13 @@ fn table_like_mut<'a>( }) } -fn write_atomic(path: &Path, contents: &str) -> CliResult<()> { - let parent = path - .parent() - .filter(|parent| !parent.as_os_str().is_empty()) - .unwrap_or_else(|| Path::new(".")); - fs::create_dir_all(parent).map_err(|error| { - report_error(format!( - "failed to create config parent directory {}: {error}", - parent.display() - )) - })?; - - let filename = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("trusted-server.toml"); - let tmp_path = parent.join(format!(".{filename}.tmp-{}", std::process::id())); - - fs::write(&tmp_path, contents).map_err(|error| { - report_error(format!( - "failed to write temporary config {}: {error}", - tmp_path.display() - )) - })?; - fs::rename(&tmp_path, path).map_err(|error| { - let _ = fs::remove_file(&tmp_path); - report_error(format!( - "failed to replace config {} with {}: {error}", - path.display(), - tmp_path.display() - )) - })?; - - Ok(()) -} - #[cfg(test)] mod tests { use super::*; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt as _; + fn managed_config(managed: &str, user_id: &str) -> String { format!( r#" @@ -1880,4 +1860,86 @@ real_time_data = ["exampleRtdProvider"] "error should instruct npm ci: {error:?}" ); } + + #[cfg(unix)] + #[test] + fn patch_config_metadata_preserves_config_permissions() { + // The temp file is created 0600, so 0640 is what proves the target's mode + // is copied onto the replacement rather than the temp file's default kept. + for mode in [0o600, 0o640] { + let (_temp, path) = write_config(&valid_config()); + fs::set_permissions(&path, fs::Permissions::from_mode(mode)) + .expect("should set config permissions"); + + patch_config_metadata(&path, &"a".repeat(64), "sha384-abc") + .expect("should patch config metadata"); + + let patched = fs::metadata(&path) + .expect("should stat patched config") + .permissions() + .mode() + & 0o777; + assert_eq!( + patched, mode, + "should keep the operator's config permissions {mode:o}" + ); + } + } + + // Success path only; the failed-rename cleanup is covered by + // `fs_util::tests::write_file_atomically_removes_temp_file_when_rename_fails`. + #[test] + fn patch_config_metadata_leaves_only_config_on_success() { + let (temp, path) = write_config(&valid_config()); + + patch_config_metadata(&path, &"a".repeat(64), "sha384-abc") + .expect("should patch config metadata"); + + let entries: Vec<_> = fs::read_dir(temp.path()) + .expect("should list config directory") + .map(|entry| entry.expect("should read entry").file_name()) + .collect(); + assert_eq!( + entries, + vec![std::ffi::OsString::from("trusted-server.toml")], + "should leave only the config file behind" + ); + } + + #[test] + fn load_bundle_config_parse_error_does_not_echo_config_source() { + let (_temp, path) = write_config( + "[publisher]\norigin_url = \"https://origin-private.example.com/internal\n", + ); + + let error = load_bundle_config(&path).expect_err("should reject invalid TOML"); + + assert!( + !error.contains("origin-private.example.com"), + "should not echo config source text: {error}" + ); + assert!( + error.contains("at line "), + "should report the error location: {error}" + ); + } + + #[test] + fn patch_config_metadata_parse_error_does_not_echo_config_source() { + let (_temp, path) = write_config( + "[publisher]\norigin_url = \"https://origin-private.example.com/internal\n", + ); + + let error = patch_config_metadata(&path, &"a".repeat(64), "sha384-abc") + .expect_err("should reject invalid TOML"); + + assert!( + !error.contains("origin-private.example.com"), + "should not echo config source text: {error}" + ); + assert!( + error.contains("at line "), + "should report the error location: {error}" + ); + } }