fix: tighten error handling and secret hygiene across helpers

A grab-bag of small but security- and operability-relevant fixes
uncovered while reworking the container system.

* `compose.yaml`: drop the inline `MyAccessToken` default and the
  inline SQLite `connect_url` default from the production compose
  file. Both are credentials/connection strings and now use bare
  `${VAR}` form per ADR-T-009 §8.1; the dev sandbox keeps its
  defaults in `compose.override.yaml`.

* `index-config`: stop printing config-loading messages to stdout.
  The TOML body may contain DB connect URLs, API tokens, or SMTP
  passwords — log only the env-var name through `tracing` (stderr)
  so we don't pollute the JSON-only stdout contract used by helper
  binaries (P9). Only the env-var *name* is ever emitted, never
  its value.

* `index-config`: fix the `[net.tls]` defaulting quirk where an
  empty table synthesised `Some("")` paths and later failed at TLS
  load time with a misleading "no such file" error. Missing fields
  now default to `None`, matching the empty-string behaviour. New
  regression test in `quirks.rs`.

* `index-health-check`: resolve `host:port` ourselves and use
  `TcpStream::connect_timeout` so the probe fails fast instead of
  inheriting the OS's multi-tens-of-seconds SYN-retransmit
  schedule — important for orchestrator health checks. Stop
  swallowing `set_{read,write}_timeout` errors silently. Return
  `ExitCode::FAILURE` on stdout-write failure (e.g. broken pipe)
  instead of panicking.

* `index-auth-keypair`: same broken-pipe fix — honour the helper's
  documented exit-code contract on stdout write failure rather
  than panicking out of `unwrap()`.

* `index-entry-script`: correct the `run_sh_with_args` doc-comment.
  Arguments are forwarded as `argv` entries to `sh -c` (with a
  `"sh"` placeholder for `$0`), not spliced into the script text;
  no quoting is performed or required.

* `require_permission` extractor: stop collapsing every database
  error into a 404 `UserNotFound`. Only the genuine
  `Error::UserNotFound` becomes a 404; DB/IO failures now surface
  as `DatabaseError` (500) and are logged, so operator-visible
  outages aren't masked as missing users.

* `AGENTS.md`: rewrite the misleading "POSIX paths must be
  null-terminated" guidance. Paths are opaque byte sequences;
  prefer `OsStr`/`PathBuf` (or `Utf8Path` when UTF-8 really is a
  precondition); NUL termination is only relevant at the
  libc/FFI boundary.

Author: peer-cat

AGENTS.md

@@ -51,7 +51,20 @@ Every test file (module) should maintain an index of the tests contained in the
 
 ## POSIX Paths
 
-All paths must be handled using null-terminated strings. No assumptions should be made about which characters may appear in file or directory names.
+Treat paths as opaque byte sequences. POSIX permits any byte except
+`\0` (NUL) and `/` (the path separator) in a file or directory name,
+and there is no guarantee that the bytes are valid UTF-8. Concretely:
+
+- Prefer `OsStr` / `OsString` / `Path` / `PathBuf` (or `Utf8Path`
+  when UTF-8 really is a precondition you intend to enforce) over
+  ad-hoc `String` handling.
+- Do not assume any particular character class — names may contain
+  spaces, newlines, control bytes, leading dashes, or arbitrary
+  non-UTF-8 bytes.
+- NUL termination is only required when crossing a libc/FFI
+  boundary (e.g. `CString` for `open(2)`); interior NUL bytes are
+  invalid for those APIs and must be rejected, not silently
+  truncated.
 
 ## Cross-Reference Conventions
 

compose.yaml

@@ -31,14 +31,15 @@ services:
       - TORRUST_INDEX_CONFIG_TOML
       - TORRUST_INDEX_DATABASE=${TORRUST_INDEX_DATABASE:-torrust_index}
       - TORRUST_INDEX_DATABASE_DRIVER=${TORRUST_INDEX_DATABASE_DRIVER:-sqlite3}
-      # DEV-ONLY default — change for any public or production deployment.
-      - TORRUST_INDEX_CONFIG_OVERRIDE_TRACKER__TOKEN=${TORRUST_INDEX_CONFIG_OVERRIDE_TRACKER__TOKEN:-MyAccessToken}
+      # Credential — bare ${VAR}, no default. See ADR-T-009 §8.1.
+      # Dev sandbox defaults live in `compose.override.yaml`.
+      - TORRUST_INDEX_CONFIG_OVERRIDE_TRACKER__TOKEN=${TORRUST_INDEX_CONFIG_OVERRIDE_TRACKER__TOKEN}
       # Mandatory at the schema level (ADR-T-009 §D2): the shipped
       # default TOMLs no longer carry a `database.connect_url`, so the
-      # operator must inject one. The defaults below match the SQLite
-      # path materialised by the entry script and the MySQL service in
-      # this compose file.
-      - TORRUST_INDEX_CONFIG_OVERRIDE_DATABASE__CONNECT_URL=${TORRUST_INDEX_CONFIG_OVERRIDE_DATABASE__CONNECT_URL:-sqlite:///var/lib/torrust/index/database/sqlite3.db?mode=rwc}
+      # operator must inject one. Bare ${VAR} keeps the production
+      # baseline credential-clean; the dev sandbox supplies a SQLite
+      # default in `compose.override.yaml`.
+      - TORRUST_INDEX_CONFIG_OVERRIDE_DATABASE__CONNECT_URL=${TORRUST_INDEX_CONFIG_OVERRIDE_DATABASE__CONNECT_URL}
     networks:
       - server_side
     ports:

packages/index-auth-keypair/src/bin/torrust-index-auth-keypair.rs

@@ -38,8 +38,15 @@ fn main() -> ExitCode {
     match generate_keypair() {
         Ok(out) => {
             info!("Key pair generated successfully.");
-            emit(&out).unwrap();
-            ExitCode::SUCCESS
+            match emit(&out) {
+                Ok(()) => ExitCode::SUCCESS,
+                Err(e) => {
+                    // Writing to stdout failed (e.g. broken pipe). Honour
+                    // the helper's exit-code contract instead of panicking.
+                    error!(error = %e, "failed to write key pair to stdout");
+                    ExitCode::FAILURE
+                }
+            }
         }
         Err(e) => {
             error!(error = %e, "keypair generation failed");

packages/index-config/src/lib.rs

@@ -202,20 +202,28 @@ impl Info {
     pub fn new(default_config_toml_path: String) -> Result<Self, Error> {
         let info = Self::from_env(&default_config_toml_path);
 
-        if let Some(toml) = info.config_toml.as_ref() {
-            println!("Loading extra configuration from environment variable {toml} ...");
+        if info.config_toml.is_some() {
+            // The TOML body may contain secrets (DB connect URLs, API
+            // tokens, SMTP passwords, …) so log only the env-var name
+            // — never its value — and route through `tracing` (stderr)
+            // so we don't pollute the JSON-only stdout contract used
+            // by helper binaries (P9).
+            tracing::info!(
+                env_var = ENV_VAR_CONFIG_TOML,
+                "loading extra configuration from environment variable"
+            );
         }
 
         if env::var(ENV_VAR_CONFIG_TOML_PATH)
             .ok()
             .as_ref()
             .is_some_and(|s| !s.is_empty())
         {
-            println!("Loading extra configuration from file: `{}` ...", info.config_toml_path);
+            tracing::info!(path = %info.config_toml_path, "loading extra configuration from file");
         } else {
-            println!(
-                "Loading extra configuration from default configuration file: `{}` ...",
-                info.config_toml_path
+            tracing::info!(
+                path = %info.config_toml_path,
+                "loading extra configuration from default configuration file"
             );
         }
 
@@ -292,31 +300,27 @@ impl From<figment::Error> for Error {
 /// It's the port number `0`
 pub const FREE_PORT: u16 = 0;
 
+/// TLS configuration for the HTTPS endpoint.
+///
+/// Both fields default to `None` (treated as "not configured") when
+/// absent **or** when given as an empty string. An incomplete
+/// `[net.tls]` table — for example one missing `ssl_key_path` — therefore
+/// behaves identically to `[net.tls]` being absent altogether, rather
+/// than being silently coerced into a `Some("")` that would later fail
+/// at TLS load time with a misleading "no such file" error.
 #[serde_as]
 #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Default)]
 pub struct Tls {
     /// Path to the SSL certificate file.
     #[serde_as(as = "NoneAsEmptyString")]
-    #[serde(default = "Tls::default_ssl_cert_path")]
+    #[serde(default)]
     pub ssl_cert_path: Option<Utf8PathBuf>,
     /// Path to the SSL key file.
     #[serde_as(as = "NoneAsEmptyString")]
-    #[serde(default = "Tls::default_ssl_key_path")]
+    #[serde(default)]
     pub ssl_key_path: Option<Utf8PathBuf>,
 }
 
-impl Tls {
-    #[allow(clippy::unnecessary_wraps)]
-    fn default_ssl_cert_path() -> Option<Utf8PathBuf> {
-        Some(Utf8PathBuf::new())
-    }
-
-    #[allow(clippy::unnecessary_wraps)]
-    fn default_ssl_key_path() -> Option<Utf8PathBuf> {
-        Some(Utf8PathBuf::new())
-    }
-}
-
 /// Loads the settings from the [`Info`] struct.
 ///
 /// The whole configuration in TOML format is included in

packages/index-config/src/tests/quirks.rs

@@ -11,6 +11,7 @@
 //! | Test                                                 | What it proves                                                       |
 //! |------------------------------------------------------|----------------------------------------------------------------------|
 //! | `tls_empty_string_paths_deserialise_to_none`         | The `NoneAsEmptyString` adapter empties out → `None`.                |
+//! | `tls_section_with_no_fields_deserialises_to_none`    | An empty `[net.tls]` table leaves both paths as `None`.              |
 //! | `ipv6_bind_address_round_trips`                      | `[::1]:7000` survives parse → serialise → parse.                     |
 //! | `tracker_token_accepts_unicode_grapheme_clusters`    | A 🦀-laden token loads and pretty-prints intact.                     |
 //! | `tracker_validator_rejects_udp_private`              | The classic "you can't have a private UDP tracker" guard fires.      |
@@ -41,6 +42,18 @@ fn tls_empty_string_paths_deserialise_to_none() {
     assert!(tls.ssl_key_path.is_none(), "empty string must collapse to None");
 }
 
+#[test]
+fn tls_section_with_no_fields_deserialises_to_none() {
+    // A `[net.tls]` table with no fields at all must not synthesise empty
+    // paths — otherwise `make_rust_tls` would later try to load a cert
+    // from `""` and emit a misleading "no such file" error at startup.
+    let toml = format!("{MINIMUM_VALID_TOML}\n[net.tls]\n");
+    let settings = load_settings(&info_from(&toml)).expect("TOML must load");
+    let tls = settings.net.tls.expect("[net.tls] table must materialise as Some");
+    assert!(tls.ssl_cert_path.is_none(), "missing field must default to None");
+    assert!(tls.ssl_key_path.is_none(), "missing field must default to None");
+}
+
 #[test]
 fn ipv6_bind_address_round_trips() {
     let mut s = placeholder_settings();

packages/index-entry-script/src/lib.rs

@@ -108,8 +108,8 @@ pub fn run_sh(snippet: &str) -> Output {
         .expect("failed to spawn sh; is /bin/sh available?")
 }
 
-/// Same as [`run_sh`], but quotes each argument and passes
-/// them as `$1`, `$2`, … to the snippet.
+/// Same as [`run_sh`], but forwards each argument to the snippet
+/// as a positional parameter (`$1`, `$2`, …).
 ///
 /// This is the natural shape for testing functions that take
 /// positional arguments (e.g. `validate_auth_keys`):
@@ -121,10 +121,14 @@ pub fn run_sh(snippet: &str) -> Output {
 /// );
 /// ```
 ///
-/// Each argument is single-quoted; embedded single quotes are
-/// escaped using the standard `'\''` POSIX trick. This makes
-/// the wrapper safe for arbitrary string arguments — the
-/// tests never need to think about shell metacharacters.
+/// Arguments are **not** spliced into the script text — they are
+/// passed as additional `argv` entries to `sh -c`, after a `"sh"`
+/// placeholder for `$0`. `sh` then exposes them to the snippet as
+/// `$1`, `$2`, … (and through `"$@"`). Because nothing is
+/// interpolated into the script string, no quoting or
+/// metacharacter escaping is required: arbitrary byte sequences
+/// (single quotes, backslashes, spaces, …) are forwarded verbatim
+/// by the kernel's `execve` boundary.
 #[doc(hidden)]
 #[must_use]
 pub fn run_sh_with_args(snippet: &str, args: &[&str]) -> Output {

packages/index-health-check/src/bin/torrust-index-health-check.rs

@@ -33,10 +33,17 @@ fn main() -> ExitCode {
     });
 
     match do_health_check(&args.url) {
-        Ok(out) => {
-            emit(&out).unwrap();
-            ExitCode::SUCCESS
-        }
+        Ok(out) => match emit(&out) {
+            Ok(()) => ExitCode::SUCCESS,
+            Err(e) => {
+                // Writing the JSON object to stdout failed (e.g. broken
+                // pipe when the consumer has already exited). Surface
+                // the failure through the documented exit-code contract
+                // rather than letting Rust's panic handler set its own.
+                error!(error = %e, "failed to write health-check output to stdout");
+                ExitCode::FAILURE
+            }
+        },
         Err(e) => {
             error!(error = %e, "health check failed");
             ExitCode::FAILURE

packages/index-health-check/src/lib.rs

@@ -3,7 +3,7 @@
 //! Uses only `std::net::TcpStream` — no async runtime, no TLS crate.
 
 use std::io::{BufRead, BufReader, Write};
-use std::net::TcpStream;
+use std::net::{TcpStream, ToSocketAddrs};
 use std::time::{Duration, Instant};
 
 use serde::Serialize;
@@ -44,9 +44,26 @@ pub fn do_health_check(url: &str) -> Result<HealthCheckOutput, HealthCheckError>
         .map_or((stripped, "/"), |i| (&stripped[..i], &stripped[i..]));
 
     let timeout = Duration::from_secs(5);
-    let mut stream = TcpStream::connect(host_port).map_err(|e| HealthCheckError(format!("connect to {host_port}: {e}")))?;
-    stream.set_read_timeout(Some(timeout)).ok();
-    stream.set_write_timeout(Some(timeout)).ok();
+
+    // Resolve `host:port` ourselves so we can apply an explicit
+    // connect-timeout: `TcpStream::connect` falls back to the OS's
+    // SYN-retransmit schedule (often tens of seconds), which is far
+    // too long for a container orchestrator's health probe.
+    let mut addrs = host_port
+        .to_socket_addrs()
+        .map_err(|e| HealthCheckError(format!("resolve {host_port}: {e}")))?;
+    let addr = addrs
+        .next()
+        .ok_or_else(|| HealthCheckError(format!("resolve {host_port}: no addresses returned")))?;
+
+    let mut stream = TcpStream::connect_timeout(&addr, timeout)
+        .map_err(|e| HealthCheckError(format!("connect to {host_port} ({addr}): {e}")))?;
+    stream
+        .set_read_timeout(Some(timeout))
+        .map_err(|e| HealthCheckError(format!("set read timeout: {e}")))?;
+    stream
+        .set_write_timeout(Some(timeout))
+        .map_err(|e| HealthCheckError(format!("set write timeout: {e}")))?;
 
     let request = format!("GET {path} HTTP/1.1\r\nHost: {host_port}\r\nConnection: close\r\n\r\n");
     stream

src/web/api/server/v1/extractors/require_permission.rs

@@ -188,8 +188,16 @@ where
                         return Err(AuthError::UnrecognisedRole.into_response());
                     }
                 }
-                Err(_) => {
-                    return Err(AuthError::UserNotFound.into_response());
+                Err(err) => {
+                    // Preserve the underlying error category: only the
+                    // genuine "user not found" case becomes a 404. DB
+                    // / IO failures must surface as `DatabaseError`
+                    // (500), not as a misleading 404 that masks the
+                    // outage from operators.
+                    if !matches!(err, crate::databases::database::Error::UserNotFound) {
+                        tracing::error!(user_id, %err, "failed to load user for permission check");
+                    }
+                    return Err(AuthError::from(err).into_response());
                 }
             };