v0.5.4: code-cleanup pass (AppError, figment config, encoding dedup, typed status, deps)

Final cleanup before hardware testing. No behaviour changes; 248 tests green, clippy clean. #1 AppError newtype (http-api/src/error.rs) with one IntoResponse mapping (NotFound→404, Invalid→400, _→500) + From<core::Error>/From<io::Error>. Converted the clearly-safe handlers (sso_put, unattended_upload, branding_clear) to `?`; intentionally left handlers with bespoke status semantics (Invalid→404 on category, 409 on duplicate share / open upload) explicit so no asserted status changes. #2 figment-based Config::load (defaults → TOML → env). Keeps the historical flat OPENPXE_* names (Unraid/entrypoint compatible) AND adds the nested OPENPXE_SECTION__FIELD form; now covers every field (apply_env had silently skipped unattended_dir + bind addrs). 6 Jail tests prove backward-compat. Removed the hand-rolled apply_env. #3 thiserror 1→2; dropped unused mime/mime_guess/once_cell deps. #4 Re-evaluated: Duration::from_hours/from_mins are stable on the pinned 1.95 toolchain and clippy prefers them — kept the readable form (the "unstable" premise didn't hold; MSRV is intentionally 1.95). #5 insta snapshot of the rendered iPXE menu (version-filtered) + wiremock coverage of the SAML metadata-URL fetch (200 + non-2xx). #6 api_status → typed StatusResponse struct (was a 25-key json! blob) with a full_flow guard test asserting every UI key + the started_at string shape. Deferred the /api/docs typed conversion (lowest value, highest churn, zero functional benefit). #7 pct_encode/xml_escape de-duplicated into openpxe_core::encoding (were copied across app.rs + the SAML modules). No new crates. #8 UploadSessions registry → parking_lot::RwLock (sync, never held across .await); per-session lock stays tokio::Mutex. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-06-03 03:33:05 -04:00
parent 7358013093
commit 674a69f93b
18 changed files with 807 additions and 199 deletions
@@ -13,6 +13,7 @@ workspace = true
 serde.workspace = true
 serde_json.workspace = true
 toml.workspace = true
+figment.workspace = true
 thiserror.workspace = true
 anyhow.workspace = true
 tracing.workspace = true
@@ -38,6 +39,9 @@ base64.workspace = true

 [dev-dependencies]
 tempfile = "3.12"
+# v0.5.4: figment's `Jail` (hermetic env/file sandbox) for the config
+# loader tests lives behind the `test` feature.
+figment = { workspace = true, features = ["test"] }
 # v0.5.1: generate a throwaway self-signed signing cert/key so SAML
 # verification tests can produce genuinely signed SAMLResponses.
 rcgen = "0.13"
@@ -1,3 +1,5 @@
+use figment::providers::{Env, Format, Serialized, Toml};
+use figment::Figment;
 use serde::{Deserialize, Serialize};
 use std::net::{IpAddr, Ipv4Addr};
 use std::path::{Path, PathBuf};
@@ -56,6 +58,9 @@ pub enum DhcpMode {
    /// Disabled — rely on an external DHCP server that has been manually
    /// configured with `next-server` / `filename`. OpenPXE only serves TFTP
    /// + HTTP in this mode. Useful for home routers that can be pre-set.
+    // `off`/`none` are accepted as aliases for backward-compat with the old
+    // hand-rolled `apply_env`, which mapped them to Disabled.
+    #[serde(alias = "off", alias = "none")]
    Disabled,
 }

@@ -129,48 +134,162 @@ impl Config {
        toml::from_str(&text).map_err(|e| crate::Error::Config(e.to_string()))
    }

-    /// Apply environment variable overrides. Env var names follow the pattern
-    /// `OPENPXE_<SECTION>_<FIELD>`, uppercase. Unknown vars are ignored.
-    /// Call this after loading the TOML file so env takes precedence.
-    pub fn apply_env(&mut self) {
-        if let Ok(v) = std::env::var("OPENPXE_HTTP_PORT") {
-            if let Ok(p) = v.parse() {
-                self.server.http_port = p;
+    /// Load configuration with layered precedence (v0.5.4, via `figment`):
+    /// built-in [`Default`] → optional TOML file → `OPENPXE_*` environment
+    /// (highest). Replaces the old `from_toml_file` + `apply_env` two-step
+    /// and now covers **every** field automatically (the previous hand-rolled
+    /// mapping silently skipped `unattended_dir`, the bind addresses, etc.).
+    ///
+    /// The env layer preserves the historical flat names
+    /// (`OPENPXE_HTTP_PORT`, `OPENPXE_ISO_DIR`, …) so existing deployments
+    /// (the Unraid template, `entrypoint.sh`) keep working unchanged, and
+    /// additionally accepts the explicit nested form
+    /// `OPENPXE_<SECTION>__<FIELD>` (double underscore).
+    pub fn load(path: Option<&Path>) -> crate::Result<Self> {
+        let mut fig = Figment::from(Serialized::defaults(Config::default()));
+        if let Some(p) = path {
+            if p.exists() {
+                fig = fig.merge(Toml::file(p));
            }
        }
-        if let Ok(v) = std::env::var("OPENPXE_TFTP_PORT") {
-            if let Ok(p) = v.parse() {
-                self.server.tftp_port = p;
-            }
-        }
-        if let Ok(v) = std::env::var("OPENPXE_DHCP_PORT") {
-            if let Ok(p) = v.parse() {
-                self.network.dhcp_port = p;
-            }
-        }
-        if let Ok(v) = std::env::var("OPENPXE_PUBLIC_IP") {
-            if let Ok(ip) = v.parse() {
-                self.server.public_ip = Some(ip);
-            }
-        }
-        if let Ok(v) = std::env::var("OPENPXE_DHCP_MODE") {
-            self.network.dhcp_mode = match v.to_ascii_lowercase().as_str() {
-                "proxy" => DhcpMode::Proxy,
-                "disabled" | "off" | "none" => DhcpMode::Disabled,
-                _ => self.network.dhcp_mode,
+        fig = fig.merge(env_provider());
+        fig.extract()
+            .map_err(|e| crate::Error::Config(e.to_string()))
+    }
+}
+
+/// The `OPENPXE_*` environment provider. Maps the historical flat variable
+/// names onto the nested [`Config`] fields, and also accepts the explicit
+/// `OPENPXE_SECTION__FIELD` nested form. Keys that match nothing (e.g.
+/// `OPENPXE_CONFIG`, `OPENPXE_UID` from the entrypoint) become stray
+/// top-level keys that `Config` ignores on extract.
+fn env_provider() -> Env {
+    Env::prefixed("OPENPXE_")
+        .map(|key| {
+            // Lowercase so the match is robust regardless of how the OS
+            // reports the var's case.
+            let k = key.as_str().to_ascii_lowercase();
+            let mapped = match k.as_str() {
+                "http_port" => "server.http_port",
+                "http_bind" => "server.http_bind",
+                "tftp_port" => "server.tftp_port",
+                "tftp_bind" => "server.tftp_bind",
+                "public_ip" => "server.public_ip",
+                "dhcp_port" => "network.dhcp_port",
+                "dhcp_bind" => "network.dhcp_bind",
+                "dhcp_mode" => "network.dhcp_mode",
+                "pxe_port" => "network.pxe_port",
+                "iso_dir" => "paths.iso_dir",
+                "work_dir" => "paths.work_dir",
+                "ipxe_dir" => "paths.ipxe_dir",
+                "smb_dir" => "paths.smb_dir",
+                "wimboot_path" => "paths.wimboot_path",
+                "unattended_dir" => "paths.unattended_dir",
+                // Unknown: support the explicit nested form
+                // (OPENPXE_SERVER__HTTP_PORT). `replace` is a no-op for the
+                // already-handled flat names above.
+                other => return other.replace("__", ".").into(),
            };
-        }
-        if let Ok(v) = std::env::var("OPENPXE_ISO_DIR") {
-            self.paths.iso_dir = PathBuf::from(v);
-        }
-        if let Ok(v) = std::env::var("OPENPXE_WORK_DIR") {
-            self.paths.work_dir = PathBuf::from(v);
-        }
-        if let Ok(v) = std::env::var("OPENPXE_IPXE_DIR") {
-            self.paths.ipxe_dir = PathBuf::from(v);
-        }
-        if let Ok(v) = std::env::var("OPENPXE_SMB_DIR") {
-            self.paths.smb_dir = PathBuf::from(v);
-        }
+            mapped.into()
+        })
+        .split(".")
+}
+
+#[cfg(test)]
+mod tests {
+    // figment's `Jail::expect_with` closure returns `Result<(), figment::Error>`
+    // and `figment::Error` is large; that's the library's API, not ours.
+    #![allow(clippy::result_large_err)]
+    use super::*;
+
+    #[test]
+    fn defaults_load_when_no_file_or_env() {
+        figment::Jail::expect_with(|_jail| {
+            let c = Config::load(None).expect("load defaults");
+            assert_eq!(c.server.http_port, 80);
+            assert_eq!(c.network.dhcp_mode, DhcpMode::Proxy);
+            assert_eq!(c.paths.iso_dir, PathBuf::from("/var/lib/openpxe/isos"));
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn legacy_flat_env_vars_still_apply() {
+        figment::Jail::expect_with(|jail| {
+            jail.set_env("OPENPXE_HTTP_PORT", "8123");
+            jail.set_env("OPENPXE_TFTP_PORT", "6900");
+            jail.set_env("OPENPXE_DHCP_PORT", "6767");
+            jail.set_env("OPENPXE_PXE_PORT", "4444");
+            jail.set_env("OPENPXE_PUBLIC_IP", "10.20.30.40");
+            jail.set_env("OPENPXE_DHCP_MODE", "disabled");
+            jail.set_env("OPENPXE_ISO_DIR", "/data/isos");
+            jail.set_env("OPENPXE_WORK_DIR", "/data/work");
+            jail.set_env("OPENPXE_IPXE_DIR", "/data/ipxe");
+            jail.set_env("OPENPXE_SMB_DIR", "/data/smb");
+            // v0.5.4: a field the old apply_env never covered.
+            jail.set_env("OPENPXE_UNATTENDED_DIR", "/data/unattended");
+            let c = Config::load(None).expect("load with env");
+            assert_eq!(c.server.http_port, 8123);
+            assert_eq!(c.server.tftp_port, 6900);
+            assert_eq!(c.network.dhcp_port, 6767);
+            assert_eq!(c.network.pxe_port, 4444);
+            assert_eq!(c.server.public_ip, Some("10.20.30.40".parse().unwrap()));
+            assert_eq!(c.network.dhcp_mode, DhcpMode::Disabled);
+            assert_eq!(c.paths.iso_dir, PathBuf::from("/data/isos"));
+            assert_eq!(c.paths.work_dir, PathBuf::from("/data/work"));
+            assert_eq!(c.paths.ipxe_dir, PathBuf::from("/data/ipxe"));
+            assert_eq!(c.paths.smb_dir, PathBuf::from("/data/smb"));
+            assert_eq!(c.paths.unattended_dir, PathBuf::from("/data/unattended"));
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn dhcp_mode_off_alias_maps_to_disabled() {
+        figment::Jail::expect_with(|jail| {
+            jail.set_env("OPENPXE_DHCP_MODE", "off");
+            let c = Config::load(None).unwrap();
+            assert_eq!(c.network.dhcp_mode, DhcpMode::Disabled);
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn nested_double_underscore_form_also_works() {
+        figment::Jail::expect_with(|jail| {
+            jail.set_env("OPENPXE_SERVER__HTTP_PORT", "9001");
+            let c = Config::load(None).unwrap();
+            assert_eq!(c.server.http_port, 9001);
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn env_overrides_toml_file() {
+        figment::Jail::expect_with(|jail| {
+            jail.create_file(
+                "openpxe.toml",
+                "[server]\nhttp_port = 8080\n[paths]\niso_dir = \"/from/toml\"\n",
+            )?;
+            jail.set_env("OPENPXE_HTTP_PORT", "8443");
+            let c = Config::load(Some(Path::new("openpxe.toml"))).unwrap();
+            // env wins over TOML…
+            assert_eq!(c.server.http_port, 8443);
+            // …but TOML-only values still apply.
+            assert_eq!(c.paths.iso_dir, PathBuf::from("/from/toml"));
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn unrelated_openpxe_env_vars_are_ignored() {
+        figment::Jail::expect_with(|jail| {
+            // entrypoint.sh sets these; they must not break config load.
+            jail.set_env("OPENPXE_UID", "10001");
+            jail.set_env("OPENPXE_CONFIG", "/etc/openpxe.toml");
+            let c = Config::load(None).expect("stray vars ignored");
+            assert_eq!(c.server.http_port, 80);
+            Ok(())
+        });
    }
 }
@@ -0,0 +1,65 @@
+//! Small, dependency-free encoding helpers shared across crates.
+//!
+//! v0.5.4: `pct_encode` and `xml_escape` were duplicated in the SAML
+//! modules and the HTTP layer; they live here now. They're deliberately
+//! hand-rolled rather than pulling in `percent-encoding` / `url`: the
+//! unreserved set below is exactly the RFC 3986 set that iPXE's
+//! `:uristring` modifier and the SAML HTTP-Redirect binding both expect,
+//! and a general-purpose URL crate escapes a different set.
+
+use std::fmt::Write as _;
+
+/// Percent-encode `s` per RFC 3986: the unreserved set
+/// (`A-Z` `a-z` `0-9` `-` `_` `.` `~`) passes through unchanged; every
+/// other byte becomes `%XX` (uppercase hex).
+#[must_use]
+pub fn pct_encode(s: &str) -> String {
+    let mut out = String::with_capacity(s.len());
+    for b in s.bytes() {
+        match b {
+            b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => {
+                out.push(b as char);
+            }
+            _ => {
+                let _ = write!(out, "%{b:02X}");
+            }
+        }
+    }
+    out
+}
+
+/// Escape the five XML predefined entities so `s` is safe inside element
+/// text or a double-quoted attribute value.
+#[must_use]
+pub fn xml_escape(s: &str) -> String {
+    let mut out = String::with_capacity(s.len());
+    for c in s.chars() {
+        match c {
+            '&' => out.push_str("&amp;"),
+            '<' => out.push_str("&lt;"),
+            '>' => out.push_str("&gt;"),
+            '"' => out.push_str("&quot;"),
+            '\'' => out.push_str("&apos;"),
+            _ => out.push(c),
+        }
+    }
+    out
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn pct_encode_unreserved_passthrough_else_hex() {
+        assert_eq!(pct_encode("node-7.lab_1~"), "node-7.lab_1~");
+        assert_eq!(pct_encode("aa:bb cc/?&="), "aa%3Abb%20cc%2F%3F%26%3D");
+        assert_eq!(pct_encode(""), "");
+    }
+
+    #[test]
+    fn xml_escape_all_five_entities() {
+        assert_eq!(xml_escape("a&b<c>\"d'e"), "a&amp;b&lt;c&gt;&quot;d&apos;e");
+        assert_eq!(xml_escape("plain text"), "plain text");
+    }
+}
@@ -8,6 +8,7 @@ pub mod boot_log;
 pub mod branding;
 pub mod client;
 pub mod config;
+pub mod encoding;
 pub mod error;
 pub mod host_bindings;
 pub mod log_bus;
@@ -5,7 +5,6 @@
 //! appended as the `SAMLRequest` query parameter. AuthnRequests are sent
 //! unsigned in this release (the IdP must not require client signatures).

-use std::fmt::Write as _;
 use std::io::Write as _;

 use base64::Engine;
@@ -15,6 +14,7 @@ use time::format_description::well_known::Rfc3339;
 use time::OffsetDateTime;

 use super::{SamlError, SpParams};
+use crate::encoding::{pct_encode, xml_escape};

 const NS_PROTOCOL: &str = "urn:oasis:names:tc:SAML:2.0:protocol";
 const NS_ASSERTION: &str = "urn:oasis:names:tc:SAML:2.0:assertion";
@@ -79,37 +79,8 @@ fn deflate_base64(xml: &str) -> Result<String, SamlError> {
    Ok(base64::engine::general_purpose::STANDARD.encode(compressed))
 }

-/// Percent-encode a query-string component (RFC 3986 unreserved set passes
-/// through; everything else is `%XX`).
-fn pct_encode(s: &str) -> String {
-    let mut out = String::with_capacity(s.len() * 3);
-    for b in s.bytes() {
-        match b {
-            b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => {
-                out.push(b as char);
-            }
-            _ => {
-                let _ = write!(out, "%{b:02X}");
-            }
-        }
-    }
-    out
-}
-
-fn xml_escape(s: &str) -> String {
-    let mut out = String::with_capacity(s.len());
-    for c in s.chars() {
-        match c {
-            '&' => out.push_str("&amp;"),
-            '<' => out.push_str("&lt;"),
-            '>' => out.push_str("&gt;"),
-            '"' => out.push_str("&quot;"),
-            '\'' => out.push_str("&apos;"),
-            _ => out.push(c),
-        }
-    }
-    out
-}
+// `pct_encode` + `xml_escape` now live in `openpxe_core::encoding` (v0.5.4)
+// — imported above.

 #[cfg(test)]
 mod tests {
@@ -7,6 +7,7 @@
 use base64::Engine;

 use super::{SamlError, SpParams};
+use crate::encoding::xml_escape;

 /// SAML 2.0 binding URIs.
 pub const BINDING_REDIRECT: &str = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect";
@@ -148,21 +149,7 @@ fn node_text(n: &roxmltree::Node<'_, '_>) -> String {
        .collect()
 }

-/// Minimal XML attribute/text escaping for the values we interpolate.
-fn xml_escape(s: &str) -> String {
-    let mut out = String::with_capacity(s.len());
-    for c in s.chars() {
-        match c {
-            '&' => out.push_str("&amp;"),
-            '<' => out.push_str("&lt;"),
-            '>' => out.push_str("&gt;"),
-            '"' => out.push_str("&quot;"),
-            '\'' => out.push_str("&apos;"),
-            _ => out.push(c),
-        }
-    }
-    out
-}
+// `xml_escape` now lives in `openpxe_core::encoding` (v0.5.4) — imported above.

 #[cfg(test)]
 mod tests {