Keep config permissions and hide config text in ts prebid client errors

.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

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 <url>` — 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<String> {
     let value = toml::from_str::<toml::Value>(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}"
+        );
+    }
 }

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<String> {
     let mut document = existing.parse::<DocumentMut>().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<toml::Value> {
-        let mut value = toml::from_str::<toml::Value>(document)
-            .map_err(|error| report_error(format!("failed to validate updated config: {error}")))?;
+        let mut value = toml::from_str::<toml::Value>(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::<toml::Value>(&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}"
+        );
+    }
 }

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<Vec<
 
 /// Validates one source config without echoing config values.
 ///
-/// Parse and schema errors report only a byte offset. Deploy validation errors
+/// Parse and schema errors report only a line and column. Deploy validation errors
 /// report only the failed phase because their messages can contain literal
 /// values, including validator parameters and invalid patterns.
 fn validate_source_config(source: &str) -> 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"
         );
     }

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::<toml::Value>(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");
     }
 

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<core::ops::Range<usize>>) -> 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"
+        );
+    }
+}

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"
+        );
+    }
+}

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;