From 674a69f93b479252639a9b4a9be56ff32cd6b8b1 Mon Sep 17 00:00:00 2001 From: Miles Ward Date: Wed, 3 Jun 2026 03:33:05 -0400 Subject: [PATCH] v0.5.4: code-cleanup pass (AppError, figment config, encoding dedup, typed status, deps) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/From. 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) --- Cargo.lock | 229 ++++++++++++++++-- Cargo.toml | 10 +- crates/core/Cargo.toml | 4 + crates/core/src/config.rs | 199 ++++++++++++--- crates/core/src/encoding.rs | 65 +++++ crates/core/src/lib.rs | 1 + crates/core/src/saml/authn_request.rs | 35 +-- crates/core/src/saml/metadata.rs | 17 +- crates/http-api/Cargo.toml | 8 +- crates/http-api/src/app.rs | 178 ++++++++------ crates/http-api/src/error.rs | 92 +++++++ crates/http-api/src/ipxe_script.rs | 16 ++ crates/http-api/src/lib.rs | 1 + crates/http-api/src/saml_routes.rs | 32 +++ ..._password_tests__render_menu_snapshot.snap | 36 +++ crates/http-api/src/uploads.rs | 20 +- crates/http-api/tests/full_flow.rs | 54 +++++ crates/openpxe/src/main.rs | 9 +- 18 files changed, 807 insertions(+), 199 deletions(-) create mode 100644 crates/core/src/encoding.rs create mode 100644 crates/http-api/src/error.rs create mode 100644 crates/http-api/src/snapshots/openpxe_http_api__ipxe_script__password_tests__render_menu_snapshot.snap diff --git a/Cargo.lock b/Cargo.lock index 3ce37a9..a077c1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -165,6 +165,16 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "assert-json-diff" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e4f2b81832e72834d7518d8487a0396a28cc408186a2e8854c0f98011faf12" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "async-trait" version = "0.1.89" @@ -176,6 +186,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "atomic" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a89cbf775b137e9b968e67227ef7f775587cde3fd31b0d8599dbd0f598a48340" +dependencies = [ + "bytemuck", +] + [[package]] name = "atomic-waker" version = "1.1.2" @@ -708,6 +727,17 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" +[[package]] +name = "console" +version = "0.16.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d64e8af5551369d19cf50138de61f1c42074ab970f74e99be916646777f8fc87" +dependencies = [ + "encode_unicode", + "libc", + "windows-sys 0.61.2", +] + [[package]] name = "const-oid" version = "0.9.6" @@ -859,6 +889,24 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d7a1e2f27636f116493b8b860f5546edb47c8d8f8ea73e1d2a20be88e28d1fea" +[[package]] +name = "deadpool" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0be2b1d1d6ec8d846f05e137292d0b89133caf95ef33695424c09568bdd39b1b" +dependencies = [ + "deadpool-runtime", + "lazy_static", + "num_cpus", + "tokio", +] + +[[package]] +name = "deadpool-runtime" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "092966b41edc516079bdf31ec78a2e0588d1d0c08f78b91d8307215928642b2b" + [[package]] name = "der" version = "0.7.10" @@ -1073,6 +1121,12 @@ version = "0.2.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e079f19b08ca6239f47f8ba8509c11cf3ea30095831f7fed61441475edd8c449" +[[package]] +name = "encode_unicode" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" + [[package]] name = "encoding_rs" version = "0.8.35" @@ -1141,6 +1195,22 @@ version = "0.2.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "28dea519a9695b9977216879a3ebfddf92f1c08c05d984f8996aecd6ecdc811d" +[[package]] +name = "figment" +version = "0.10.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8cb01cd46b0cf372153850f4c6c272d9cbea2da513e07538405148f95bd789f3" +dependencies = [ + "atomic", + "parking_lot", + "pear", + "serde", + "tempfile", + "toml", + "uncased", + "version_check", +] + [[package]] name = "find-msvc-tools" version = "0.1.9" @@ -1413,6 +1483,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + [[package]] name = "hex" version = "0.4.3" @@ -1762,6 +1838,12 @@ dependencies = [ "serde_core", ] +[[package]] +name = "inlinable_string" +version = "0.1.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c8fae54786f62fb2918dcfae3d568594e50eb9b5c25bf04371af6fe7516452fb" + [[package]] name = "inout" version = "0.1.4" @@ -1772,6 +1854,18 @@ dependencies = [ "generic-array", ] +[[package]] +name = "insta" +version = "1.47.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b4a6248eb93a4401ed2f37dfe8ea592d3cf05b7cf4f8efa867b6895af7e094e" +dependencies = [ + "console", + "once_cell", + "similar", + "tempfile", +] + [[package]] name = "ipnet" version = "2.12.0" @@ -2222,6 +2316,16 @@ dependencies = [ "libm", ] +[[package]] +name = "num_cpus" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91df4bbde75afed763b708b7eee1e8e7651e02d97f6d5dd763e89367e957b23b" +dependencies = [ + "hermit-abi", + "libc", +] + [[package]] name = "oid-registry" version = "0.8.1" @@ -2251,7 +2355,7 @@ checksum = "c08d65885ee38876c4f86fa503fb49d7b507c2b62552df7c70b2fce627e06381" [[package]] name = "openpxe" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "axum", @@ -2273,12 +2377,13 @@ dependencies = [ [[package]] name = "openpxe-core" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "base64", "bcrypt", "bergshamra", + "figment", "flate2", "parking_lot", "quick-xml", @@ -2287,7 +2392,7 @@ dependencies = [ "serde", "serde_json", "tempfile", - "thiserror 1.0.69", + "thiserror 2.0.18", "time", "tokio", "toml", @@ -2299,21 +2404,21 @@ dependencies = [ [[package]] name = "openpxe-dhcp-proxy" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "bytes", "dhcproto", "openpxe-core", "socket2 0.5.10", - "thiserror 1.0.69", + "thiserror 2.0.18", "tokio", "tracing", ] [[package]] name = "openpxe-http-api" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "axum", @@ -2323,9 +2428,8 @@ dependencies = [ "futures", "hyper", "image", + "insta", "lettre", - "mime", - "mime_guess", "openpxe-core", "openpxe-ipxe-assets", "openpxe-iso-store", @@ -2336,7 +2440,7 @@ dependencies = [ "serde", "serde_json", "tempfile", - "thiserror 1.0.69", + "thiserror 2.0.18", "time", "tokio", "tokio-stream", @@ -2345,21 +2449,22 @@ dependencies = [ "tower-http", "tracing", "uuid", + "wiremock", ] [[package]] name = "openpxe-ipxe-assets" -version = "0.5.3" +version = "0.5.4" dependencies = [ "openpxe-core", "rust-embed", - "thiserror 1.0.69", + "thiserror 2.0.18", "tracing", ] [[package]] name = "openpxe-iso-store" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "bcrypt", @@ -2376,7 +2481,7 @@ dependencies = [ "serde_json", "sha2 0.10.9", "tempfile", - "thiserror 1.0.69", + "thiserror 2.0.18", "time", "tokio", "tokio-util", @@ -2386,21 +2491,21 @@ dependencies = [ [[package]] name = "openpxe-tftp" -version = "0.5.3" +version = "0.5.4" dependencies = [ "anyhow", "bytes", "openpxe-core", "openpxe-ipxe-assets", "socket2 0.5.10", - "thiserror 1.0.69", + "thiserror 2.0.18", "tokio", "tracing", ] [[package]] name = "openpxe-webui" -version = "0.5.3" +version = "0.5.4" [[package]] name = "p256" @@ -2479,6 +2584,29 @@ dependencies = [ "hmac 0.12.1", ] +[[package]] +name = "pear" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bdeeaa00ce488657faba8ebf44ab9361f9365a97bd39ffb8a60663f57ff4b467" +dependencies = [ + "inlinable_string", + "pear_codegen", + "yansi", +] + +[[package]] +name = "pear_codegen" +version = "0.2.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4bab5b985dc082b345f812b7df84e1bef27e7207b39e448439ba8bd69c93f147" +dependencies = [ + "proc-macro2", + "proc-macro2-diagnostics", + "quote", + "syn 2.0.117", +] + [[package]] name = "pem" version = "3.0.6" @@ -2637,6 +2765,19 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proc-macro2-diagnostics" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", + "version_check", + "yansi", +] + [[package]] name = "pxfm" version = "0.1.29" @@ -2827,6 +2968,18 @@ dependencies = [ "bitflags 2.11.1", ] +[[package]] +name = "regex" +version = "1.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e10754a14b9137dd7b1e3e5b0493cc9171fdd105e0ab477f51b72e7f3ac0e276" +dependencies = [ + "aho-corasick", + "memchr", + "regex-automata", + "regex-syntax", +] + [[package]] name = "regex-automata" version = "0.4.14" @@ -3311,6 +3464,12 @@ version = "0.3.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "703d5c7ef118737c72f1af64ad2f6f8c5e1921f818cdcb97b8fe6fc69bf66214" +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "slab" version = "0.4.12" @@ -3889,6 +4048,15 @@ version = "1.20.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40ce102ab67701b8526c123c1bab5cbe42d7040ccfd0f64af1a385808d2f43de" +[[package]] +name = "uncased" +version = "0.9.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1b88fcfe09e89d3866a5c11019378088af2d24c3fbd4f0543f96b479ec90697" +dependencies = [ + "version_check", +] + [[package]] name = "unicase" version = "2.9.0" @@ -4353,6 +4521,29 @@ dependencies = [ "memchr", ] +[[package]] +name = "wiremock" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08db1edfb05d9b3c1542e521aea074442088292f00b5f28e435c714a98f85031" +dependencies = [ + "assert-json-diff", + "base64", + "deadpool", + "futures", + "http", + "http-body-util", + "hyper", + "hyper-util", + "log", + "once_cell", + "regex", + "serde", + "serde_json", + "tokio", + "url", +] + [[package]] name = "wit-bindgen" version = "0.51.0" @@ -4494,6 +4685,12 @@ dependencies = [ "time", ] +[[package]] +name = "yansi" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfe53a6657fd280eaa890a3bc59152892ffa3e30101319d168b781ed6529b049" + [[package]] name = "yasna" version = "0.5.2" diff --git a/Cargo.toml b/Cargo.toml index 9e6076a..eb7ca7e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ members = [ ] [workspace.package] -version = "0.5.3" +version = "0.5.4" edition = "2021" rust-version = "1.95" license = "MIT OR Apache-2.0" @@ -36,25 +36,25 @@ tower = "0.5" tower-http = { version = "0.6", features = ["fs", "trace", "cors", "limit"] } hyper = "1.4" reqwest = { version = "0.12", default-features = false, features = ["rustls-tls", "stream", "json"] } -mime = "0.3" -mime_guess = "2.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" toml = "0.8" +# v0.5.4: layered config (TOML file + env). Pure-Rust, no C deps; keeps the +# static-musl build OpenSSL-free. Replaces the hand-rolled apply_env mapping. +figment = { version = "0.10", features = ["toml", "env"] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter", "json"] } anyhow = "1.0" -thiserror = "1.0" +thiserror = "2.0" clap = { version = "4.5", features = ["derive", "env"] } uuid = { version = "1.10", features = ["v4", "serde"] } time = { version = "0.3", features = ["serde", "serde-human-readable", "formatting", "macros"] } sha2 = "0.10" hex = "0.4" bcrypt = "0.15" -once_cell = "1.19" parking_lot = "0.12" rust-embed = { version = "8.5", features = ["include-exclude"] } diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 8872a33..af61fce 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -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" diff --git a/crates/core/src/config.rs b/crates/core/src/config.rs index ea88e4c..d202dd4 100644 --- a/crates/core/src/config.rs +++ b/crates/core/src/config.rs @@ -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_
_`, 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_
__` (double underscore). + pub fn load(path: Option<&Path>) -> crate::Result { + 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(()) + }); } } diff --git a/crates/core/src/encoding.rs b/crates/core/src/encoding.rs new file mode 100644 index 0000000..feb97d6 --- /dev/null +++ b/crates/core/src/encoding.rs @@ -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("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + _ => 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\"d'e"), "a&b<c>"d'e"); + assert_eq!(xml_escape("plain text"), "plain text"); + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index f25d3a3..3ac6f7b 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -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; diff --git a/crates/core/src/saml/authn_request.rs b/crates/core/src/saml/authn_request.rs index c9be5d4..a8c6508 100644 --- a/crates/core/src/saml/authn_request.rs +++ b/crates/core/src/saml/authn_request.rs @@ -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 { 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("&"), - '<' => out.push_str("<"), - '>' => out.push_str(">"), - '"' => out.push_str("""), - '\'' => out.push_str("'"), - _ => out.push(c), - } - } - out -} +// `pct_encode` + `xml_escape` now live in `openpxe_core::encoding` (v0.5.4) +// — imported above. #[cfg(test)] mod tests { diff --git a/crates/core/src/saml/metadata.rs b/crates/core/src/saml/metadata.rs index 3972f43..9365744 100644 --- a/crates/core/src/saml/metadata.rs +++ b/crates/core/src/saml/metadata.rs @@ -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("&"), - '<' => out.push_str("<"), - '>' => out.push_str(">"), - '"' => out.push_str("""), - '\'' => out.push_str("'"), - _ => out.push(c), - } - } - out -} +// `xml_escape` now lives in `openpxe_core::encoding` (v0.5.4) — imported above. #[cfg(test)] mod tests { diff --git a/crates/http-api/Cargo.toml b/crates/http-api/Cargo.toml index ffb108a..21ac7fc 100644 --- a/crates/http-api/Cargo.toml +++ b/crates/http-api/Cargo.toml @@ -33,8 +33,6 @@ thiserror.workspace = true anyhow.workspace = true bytes.workspace = true futures.workspace = true -mime.workspace = true -mime_guess.workspace = true uuid.workspace = true # v0.4.5 Forms auth: lock-free session store and cookie helpers. parking_lot.workspace = true @@ -61,3 +59,9 @@ image = { version = "0.25", default-features = false, features = ["png"] } # replay, and IdP-initiated-gating flows exercise real signatures. rcgen = "0.13" bergshamra = { workspace = true } +# v0.5.4: snapshot the generated iPXE menu so any unintended drift (a +# dropped line, reordered item) is caught and reviewed, not silently shipped. +insta = "1.40" +# v0.5.4: stand up a mock HTTP server to exercise the SAML metadata-URL +# fetch path (previously untested because it did a real network GET). +wiremock = "0.6" diff --git a/crates/http-api/src/app.rs b/crates/http-api/src/app.rs index 81e8b6a..116b5c0 100644 --- a/crates/http-api/src/app.rs +++ b/crates/http-api/src/app.rs @@ -14,6 +14,7 @@ //! | `/api/*` | JSON/HTML API for the web UI | use crate::auth as auth_api; +use crate::error::AppError; use crate::ipxe_script::{ render_entry, render_family_menu, render_local_hdd, render_menu, render_nic_info, render_queue_entry, render_shell, render_tools_menu, render_util, @@ -31,15 +32,15 @@ use axum::{ Json, Router, }; use openpxe_core::{ - ext_for_mime, wol, BootEvent, ClientEvent, DeployProfile, Error, LogoSlot, NotifyConfig, - Settings, SsoConfig, ALLOWED_LOGO_MIMES, MAX_LOGO_BYTES, + encoding::pct_encode, ext_for_mime, wol, BootEvent, ClientEvent, DeployProfile, Error, + LogoSlot, NotifyConfig, Settings, SsoConfig, ALLOWED_LOGO_MIMES, MAX_LOGO_BYTES, }; use openpxe_ipxe_assets::asset_bytes; use openpxe_iso_store::{ - render_template, IsoCategory, IsoMeta, IsoSource, NfsAddRequest, SmbAddRequest, UnattendedKind, - UnattendedMeta, + render_template, IsoCategory, IsoMeta, IsoSource, NfsAddRequest, SmbAddRequest, SmbState, + UnattendedKind, UnattendedMeta, }; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use serde_json::json; use std::net::SocketAddr; use std::time::Duration; @@ -241,12 +242,12 @@ async fn api_sso_get(State(state): State) -> Json { Json(state.sso.snapshot()) } -async fn api_sso_put(State(state): State, Json(body): Json) -> Response { - match state.sso.replace(body) { - Ok(cfg) => (StatusCode::OK, Json(cfg)).into_response(), - Err(Error::Invalid(msg)) => (StatusCode::BAD_REQUEST, msg).into_response(), - Err(e) => (StatusCode::INTERNAL_SERVER_ERROR, format!("{e}")).into_response(), - } +async fn api_sso_put( + State(state): State, + Json(body): Json, +) -> Result, AppError> { + // v0.5.4: `?` + AppError centralizes Invalid→400 / _→500. + Ok(Json(state.sso.replace(body)?)) } // ─── UI ──────────────────────────────────────────────────────────────────── @@ -1198,14 +1199,12 @@ async fn api_branding_upload( async fn api_branding_clear( State(state): State, AxumPath(slot): AxumPath, -) -> Response { +) -> Result { let Some(slot) = LogoSlot::parse(&slot) else { - return (StatusCode::BAD_REQUEST, "unknown logo slot").into_response(); + return Ok((StatusCode::BAD_REQUEST, "unknown logo slot").into_response()); }; - match state.branding.clear_logo(slot) { - Ok(()) => StatusCode::NO_CONTENT.into_response(), - Err(e) => (StatusCode::INTERNAL_SERVER_ERROR, format!("{e}")).into_response(), - } + state.branding.clear_logo(slot)?; + Ok(StatusCode::NO_CONTENT.into_response()) } // ─── Unattended answer files (v0.5.2) ────────────────────────────────────── @@ -1221,7 +1220,9 @@ async fn api_unattended_list(State(state): State) -> Json, mut multipart: Multipart, -) -> Response { +) -> Result { + // v0.5.4: the answer-file add() maps Invalid→400 / _→500 via `?`+AppError. + // The multipart-shape 400s (missing field/filename) stay explicit. while let Ok(Some(field)) = multipart.next_field().await { let name = field.name().unwrap_or("").to_string(); if name != "file" && name != "unattended" { @@ -1229,19 +1230,18 @@ async fn api_unattended_upload( } let filename = field.file_name().map(str::to_string).unwrap_or_default(); if filename.trim().is_empty() { - return (StatusCode::BAD_REQUEST, "missing filename on upload").into_response(); + return Ok((StatusCode::BAD_REQUEST, "missing filename on upload").into_response()); } let bytes = match field.bytes().await { Ok(b) => b, - Err(e) => return (StatusCode::BAD_REQUEST, format!("read body: {e}")).into_response(), - }; - return match state.unattended.add(&filename, &bytes).await { - Ok(meta) => (StatusCode::CREATED, Json(meta)).into_response(), - Err(Error::Invalid(msg)) => (StatusCode::BAD_REQUEST, msg).into_response(), - Err(e) => (StatusCode::INTERNAL_SERVER_ERROR, format!("{e}")).into_response(), + Err(e) => { + return Ok((StatusCode::BAD_REQUEST, format!("read body: {e}")).into_response()) + } }; + let meta = state.unattended.add(&filename, &bytes).await?; + return Ok((StatusCode::CREATED, Json(meta)).into_response()); } - (StatusCode::BAD_REQUEST, "no 'file' part").into_response() + Ok((StatusCode::BAD_REQUEST, "no 'file' part").into_response()) } async fn api_unattended_delete( @@ -1379,23 +1379,7 @@ fn build_query(pairs: &[(&str, Option<&str>)]) -> String { out } -/// Minimal RFC 3986 percent-encoding for query values (unreserved set -/// passes through; everything else becomes `%XX`). -fn pct_encode(s: &str) -> String { - use std::fmt::Write as _; - 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 -} +// `pct_encode` lives in `openpxe_core::encoding` (v0.5.4) — imported above. /// Encode `(hostname, ip, mac)` into a single base64url path segment for /// the cloud-init seed directory. Empty values become empty fields. @@ -1948,7 +1932,49 @@ async fn api_list_clients(State(state): State) -> Json) -> Json { +/// Per-theme branding presence, nested under [`StatusResponse::branding`]. +#[derive(Serialize)] +struct BrandingStatus { + light: bool, + dark: bool, + client: bool, + rev: u64, +} + +/// Dashboard status payload. v0.5.4: this replaced a 25-key hand-built +/// `json!` blob — the typed struct makes the contract with the WebUI +/// compile-checked. Field names ARE the JSON keys; do not rename without +/// updating `crates/webui/src/app.js` (a `full_flow` test guards the set). +/// `settings` / `smb` / `started_at` embed their own `Serialize` impls so +/// the wire shape is byte-identical to the previous `json!` output. +#[derive(Serialize)] +struct StatusResponse { + version: &'static str, + public_base_url: String, + iso_count: usize, + client_count: usize, + queue_count: usize, + imaging_count: usize, + waiting_count: usize, + ipxe_assets: Vec, + settings: Settings, + smb: Option, + smb_share_count: usize, + smb_share_reachable: usize, + nfs_share_count: usize, + nfs_share_reachable: usize, + host_bindings: usize, + custom_logo: bool, + branding: BrandingStatus, + unattended_count: usize, + uptime_secs: i64, + started_at: time::OffsetDateTime, + nic_name: String, + subnet_mask: String, + gateway: String, +} + +async fn api_status(State(state): State) -> Json { let smb = state.smb.as_ref().map(|s| s.snapshot()); // v0.4.65+v0.4.67: external storage shares — SMB (userspace // smbclient) and NFS (in-process nfs3_client). Dashboard tile @@ -1981,39 +2007,39 @@ async fn api_status(State(state): State) -> Json { state.metrics.record_http(openpxe_core::HttpRoute::Api); let now = time::OffsetDateTime::now_utc(); let uptime_secs = (now - state.started_at).whole_seconds().max(0); - Json(json!({ - "version": env!("CARGO_PKG_VERSION"), - "public_base_url": state.public_base_url, - "iso_count": isos.len(), - "client_count": clients.len(), - "queue_count": queue_entries.len(), - "imaging_count": imaging, - "waiting_count": waiting, - "ipxe_assets": openpxe_ipxe_assets::list_assets(), - "settings": state.settings.snapshot(), - "smb": smb, - "smb_share_count": smb_shares.len(), - "smb_share_reachable": smb_reachable, - // v0.4.67: NFSv3 share counts. The dashboard tile sums these - // with the SMB counts above ("N shares reachable") so the - // top-line metric works regardless of protocol mix. - "nfs_share_count": nfs_shares.len(), - "nfs_share_reachable": nfs_reachable, - "host_bindings": state.hosts.len(), - "custom_logo": state.branding.has_any_web_logo(), - "branding": { - "light": state.branding.has_logo(LogoSlot::Light), - "dark": state.branding.has_logo(LogoSlot::Dark), - "client": state.branding.has_logo(LogoSlot::Client), - "rev": state.branding.logo_rev(), + Json(StatusResponse { + version: env!("CARGO_PKG_VERSION"), + public_base_url: state.public_base_url.clone(), + iso_count: isos.len(), + client_count: clients.len(), + queue_count: queue_entries.len(), + imaging_count: imaging, + waiting_count: waiting, + ipxe_assets: openpxe_ipxe_assets::list_assets(), + settings: state.settings.snapshot(), + smb, + smb_share_count: smb_shares.len(), + smb_share_reachable: smb_reachable, + // v0.4.67: NFSv3 share counts. The dashboard tile sums these with + // the SMB counts above ("N shares reachable") so the top-line + // metric works regardless of protocol mix. + nfs_share_count: nfs_shares.len(), + nfs_share_reachable: nfs_reachable, + host_bindings: state.hosts.len(), + custom_logo: state.branding.has_any_web_logo(), + branding: BrandingStatus { + light: state.branding.has_logo(LogoSlot::Light), + dark: state.branding.has_logo(LogoSlot::Dark), + client: state.branding.has_logo(LogoSlot::Client), + rev: state.branding.logo_rev(), }, - "unattended_count": state.unattended.len(), - "uptime_secs": uptime_secs, - "started_at": state.started_at, - "nic_name": state.nic_name, - "subnet_mask": state.subnet_mask, - "gateway": state.gateway, - })) + unattended_count: state.unattended.len(), + uptime_secs, + started_at: state.started_at, + nic_name: state.nic_name.clone(), + subnet_mask: state.subnet_mask.clone(), + gateway: state.gateway.clone(), + }) } async fn api_get_settings(State(state): State) -> Json { diff --git a/crates/http-api/src/error.rs b/crates/http-api/src/error.rs new file mode 100644 index 0000000..03d04f7 --- /dev/null +++ b/crates/http-api/src/error.rs @@ -0,0 +1,92 @@ +//! Uniform HTTP error mapping for the API layer (v0.5.4). +//! +//! Before this, ~40 handlers in `app.rs` hand-wrote +//! `match … { Err(e) => (StatusCode::…, format!("{e}")).into_response() }`, +//! and the `openpxe_core::Error` → status mapping drifted between them +//! (e.g. `Invalid` → 400 in most places, 404 in one). [`AppError`] wraps +//! `openpxe_core::Error` so a handler can return `Result` and +//! `?` its way out, getting one consistent status + body. The body stays +//! plain-text (matching the previous `(StatusCode, String)` responses) so +//! existing clients and tests see no shape change; 5xx detail is logged +//! and returned verbatim exactly as before. +//! +//! Handlers with *intentional* domain-specific statuses (e.g. a duplicate +//! share → 409, a still-open chunked upload → 409) keep their explicit +//! returns — `AppError` is for the common case, not a straitjacket. + +use axum::http::StatusCode; +use axum::response::{IntoResponse, Response}; +use openpxe_core::Error as CoreError; + +/// Newtype over [`openpxe_core::Error`] with a uniform [`IntoResponse`]. +#[derive(Debug)] +pub struct AppError(pub CoreError); + +impl From for AppError { + fn from(e: CoreError) -> Self { + AppError(e) + } +} + +impl From for AppError { + fn from(e: std::io::Error) -> Self { + AppError(CoreError::Io(e)) + } +} + +impl AppError { + /// The HTTP status this error maps to. Public so handlers (and tests) + /// can reason about the mapping in one place. + #[must_use] + pub fn status(&self) -> StatusCode { + match self.0 { + CoreError::NotFound(_) => StatusCode::NOT_FOUND, + CoreError::Invalid(_) => StatusCode::BAD_REQUEST, + CoreError::Config(_) | CoreError::Io(_) | CoreError::Other(_) => { + StatusCode::INTERNAL_SERVER_ERROR + } + } + } +} + +impl IntoResponse for AppError { + fn into_response(self) -> Response { + let status = self.status(); + // Match the prior hand-written responses: the 4xx arms returned the + // bare inner message (not the `Display` prefix), so a UI showing + // `await r.text()` reads "metadata too long", not "invalid input: + // metadata too long". 5xx keeps the full `Display` string. + let body = match &self.0 { + CoreError::Invalid(m) | CoreError::NotFound(m) => m.clone(), + other => other.to_string(), + }; + if status.is_server_error() { + // Log the full detail server-side; the body still carries it + // (unchanged from the prior `format!("{e}")` behaviour), but the + // log line is what an operator greps for. + tracing::error!(target: "openpxe::http", error = %self.0, "request failed"); + } + (status, body).into_response() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn status_mapping_is_consistent() { + assert_eq!( + AppError(CoreError::NotFound("x".into())).status(), + StatusCode::NOT_FOUND + ); + assert_eq!( + AppError(CoreError::Invalid("x".into())).status(), + StatusCode::BAD_REQUEST + ); + assert_eq!( + AppError(CoreError::Config("x".into())).status(), + StatusCode::INTERNAL_SERVER_ERROR + ); + } +} diff --git a/crates/http-api/src/ipxe_script.rs b/crates/http-api/src/ipxe_script.rs index 3ef1f19..865d086 100644 --- a/crates/http-api/src/ipxe_script.rs +++ b/crates/http-api/src/ipxe_script.rs @@ -693,6 +693,22 @@ mod password_tests { assert!(s.contains("arm64 UEFI"), "{s}"); } + // v0.5.4: a full snapshot of the rendered top menu. The fragment + // `assert!`s above check specific invariants; this catches *any* other + // drift (a reordered item, a dropped line, changed spacing) so it's + // reviewed deliberately. The OpenPXE version is filtered out so the + // snapshot doesn't churn on every release bump. + #[test] + fn render_menu_snapshot() { + // Normalize the compile-time version so the snapshot doesn't churn + // on every release bump (no insta `filters` feature needed). + let rendered = render_menu(&[], &Settings::default(), "http://10.0.0.5").replace( + concat!("OpenPXE v", env!("CARGO_PKG_VERSION")), + "OpenPXE vX.Y.Z", + ); + insta::assert_snapshot!(rendered); + } + #[test] fn generated_scripts_do_not_emit_bare_or_trailing_fallbacks() { let settings = Settings::default(); diff --git a/crates/http-api/src/lib.rs b/crates/http-api/src/lib.rs index dc631c6..25ed67d 100644 --- a/crates/http-api/src/lib.rs +++ b/crates/http-api/src/lib.rs @@ -15,6 +15,7 @@ pub mod app; pub mod auth; +pub mod error; pub mod ipxe_script; pub mod iso_fs; pub mod log_stream; diff --git a/crates/http-api/src/saml_routes.rs b/crates/http-api/src/saml_routes.rs index db8be85..97a8050 100644 --- a/crates/http-api/src/saml_routes.rs +++ b/crates/http-api/src/saml_routes.rs @@ -336,3 +336,35 @@ fn redirect_with_session(location: &str, session: &str) -> Response { IntoResponse::into_response, ) } + +#[cfg(test)] +mod tests { + use super::*; + use wiremock::matchers::method; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + // v0.5.4: exercise the SAML metadata-URL fetch against a mock server — + // previously this path did a real network GET and had no coverage. + #[tokio::test] + async fn fetch_metadata_returns_body_on_200() { + let server = MockServer::start().await; + let xml = "idp"; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(200).set_body_string(xml)) + .mount(&server) + .await; + let got = fetch_metadata(&server.uri()).await.expect("fetch ok"); + assert_eq!(got, xml); + } + + #[tokio::test] + async fn fetch_metadata_errors_on_non_2xx() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(503)) + .mount(&server) + .await; + let err = fetch_metadata(&server.uri()).await.unwrap_err(); + assert!(matches!(err, SamlError::Metadata(_)), "got {err:?}"); + } +} diff --git a/crates/http-api/src/snapshots/openpxe_http_api__ipxe_script__password_tests__render_menu_snapshot.snap b/crates/http-api/src/snapshots/openpxe_http_api__ipxe_script__password_tests__render_menu_snapshot.snap new file mode 100644 index 0000000..ad9d05b --- /dev/null +++ b/crates/http-api/src/snapshots/openpxe_http_api__ipxe_script__password_tests__render_menu_snapshot.snap @@ -0,0 +1,36 @@ +--- +source: crates/http-api/src/ipxe_script.rs +expression: rendered +--- +#!ipxe +# OpenPXE top-level menu - auto-generated, do not edit +set base-url http://10.0.0.5 +set esc:hex 1b +set cls ${esc:string}[2J +console --picture http://10.0.0.5/branding/pxe-logo --top 290 || console +set arch-label ${buildarch} ${platform} +iseq ${buildarch} i386 && iseq ${platform} pcbios && set arch-label x86 BIOS || iseq ${buildarch} x86_64 && iseq ${platform} efi && set arch-label x86_64 UEFI || iseq ${buildarch} arm64 && iseq ${platform} efi && set arch-label arm64 UEFI || true +:menu +menu OpenPXE - network boot menu +item --gap +item --gap -- ------------------------- Default ------------------------- +item local Boot from Local HDD +item --gap -- ----------------------- Installers ----------------------- +item --gap -- (no Linux ISOs uploaded) +item --gap -- (Windows support disabled in Settings) +item --gap -- -------------------------- Tools -------------------------- +item tools Tools > +item --gap -- ---------------------- Queued Deployment --------------------- +item queue Queued Deployment (join queue) +item --gap +item --key x exit Exit iPXE +item --gap +item --gap -- OpenPXE vX.Y.Z - ${arch-label} +choose --default queue --timeout 600000 target || goto menu +iseq ${target} local && chain http://10.0.0.5/boot/_local.ipxe || goto menu +iseq ${target} linux && chain http://10.0.0.5/boot/_linux_menu.ipxe || goto menu +iseq ${target} windows && chain http://10.0.0.5/boot/_windows_menu.ipxe || goto menu +iseq ${target} tools && chain http://10.0.0.5/boot/_tools_menu.ipxe || goto menu +iseq ${target} queue && chain http://10.0.0.5/boot/_queue.ipxe || goto menu +iseq ${target} exit && exit || goto menu +goto menu diff --git a/crates/http-api/src/uploads.rs b/crates/http-api/src/uploads.rs index 9141a01..ebe09ec 100644 --- a/crates/http-api/src/uploads.rs +++ b/crates/http-api/src/uploads.rs @@ -9,6 +9,7 @@ use bytes::Bytes; use openpxe_core::{Error, Result}; use openpxe_iso_store::{IsoMeta, IsoStore, UploadHandle}; +use parking_lot::RwLock; use serde::Serialize; use std::collections::HashMap; use std::sync::Arc; @@ -19,7 +20,11 @@ const DEFAULT_CHUNK_SIZE: u64 = 8 * 1024 * 1024; #[derive(Clone, Default)] pub struct UploadSessions { - inner: Arc>>>>, + // v0.5.4: the registry is a sync `parking_lot::RwLock` — it's only ever + // briefly read/inserted/removed to look up a session, never held across + // an `.await`. The per-session lock below stays a `tokio::sync::Mutex` + // because `write_chunk` / `finish` are awaited while it's held. + inner: Arc>>>>, } struct UploadSession { @@ -67,8 +72,7 @@ impl UploadSessions { }; self.inner - .lock() - .await + .write() .insert(upload_id.clone(), Arc::new(Mutex::new(session))); Ok(UploadStarted { @@ -88,7 +92,7 @@ impl UploadSessions { chunk: Bytes, complete: bool, ) -> Result { - let Some(session_lock) = self.inner.lock().await.get(upload_id).cloned() else { + let Some(session_lock) = self.inner.read().get(upload_id).cloned() else { return Err(Error::Invalid(format!("no such upload '{upload_id}'"))); }; @@ -119,7 +123,7 @@ impl UploadSessions { if let Err(e) = handle.write_chunk(&chunk).await { let handle = session.handle.take(); drop(session); - self.inner.lock().await.remove(upload_id); + self.inner.write().remove(upload_id); if let Some(handle) = handle { let _ = handle.abort().await; } @@ -156,11 +160,11 @@ impl UploadSessions { let meta = match handle.finish(store).await { Ok(meta) => meta, Err(e) => { - self.inner.lock().await.remove(upload_id); + self.inner.write().remove(upload_id); return Err(e); } }; - self.inner.lock().await.remove(upload_id); + self.inner.write().remove(upload_id); Ok(UploadAppend::Complete { offset: new_offset, iso: Box::new(meta), @@ -168,7 +172,7 @@ impl UploadSessions { } pub async fn abort(&self, upload_id: &str) -> Result<()> { - let Some(session_lock) = self.inner.lock().await.remove(upload_id) else { + let Some(session_lock) = self.inner.write().remove(upload_id) else { return Err(Error::Invalid(format!("no such upload '{upload_id}'"))); }; let mut session = session_lock.lock().await; diff --git a/crates/http-api/tests/full_flow.rs b/crates/http-api/tests/full_flow.rs index 312f9b9..c499e44 100644 --- a/crates/http-api/tests/full_flow.rs +++ b/crates/http-api/tests/full_flow.rs @@ -1536,6 +1536,60 @@ async fn status_exposes_custom_logo_flag() { ); } +/// v0.5.4 guard: the typed `StatusResponse` must keep every key the WebUI +/// (`crates/webui/src/app.js`) reads off `/api/status`. If a refactor drops +/// or renames one, the dashboard silently breaks — this catches it. +#[tokio::test] +async fn status_contract_has_all_ui_keys() { + let (state, _dir) = build_state().await; + let app = build_router(state); + let (s, body) = get(&app, "/api/status").await; + assert_eq!(s, StatusCode::OK); + let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); + for key in [ + "version", + "public_base_url", + "iso_count", + "client_count", + "queue_count", + "imaging_count", + "waiting_count", + "ipxe_assets", + "settings", + "smb_share_count", + "smb_share_reachable", + "nfs_share_count", + "nfs_share_reachable", + "host_bindings", + "custom_logo", + "branding", + "unattended_count", + "uptime_secs", + "started_at", + "nic_name", + "subnet_mask", + "gateway", + ] { + assert!( + v.get(key).is_some(), + "/api/status missing UI key '{key}': {v}" + ); + } + // Nested branding presence the Settings tab reads. + for key in ["light", "dark", "client", "rev"] { + assert!( + v["branding"].get(key).is_some(), + "/api/status branding missing '{key}': {v}" + ); + } + // started_at must remain an RFC3339 string (the UI does fmtUptime on + // uptime_secs but renders started_at as text), not a serialized struct. + assert!( + v["started_at"].is_string(), + "started_at should serialize as a string: {v}" + ); +} + async fn put_json(router: &axum::Router, path: &str, body: &str) -> (StatusCode, Vec) { let res = router .clone() diff --git a/crates/openpxe/src/main.rs b/crates/openpxe/src/main.rs index c2ebb2e..5621956 100644 --- a/crates/openpxe/src/main.rs +++ b/crates/openpxe/src/main.rs @@ -59,11 +59,10 @@ async fn main() -> anyhow::Result<()> { init_tracing(log_bus.clone()); let cli = Cli::parse(); - let mut config = match &cli.config { - Some(p) if p.exists() => Config::from_toml_file(p)?, - _ => Config::default(), - }; - config.apply_env(); + // v0.5.4: layered load via figment — defaults → optional TOML → env. + // The OPENPXE_* env layer keeps the historical flat names (see + // `Config::load`), so existing deployments are unaffected. + let config = Config::load(cli.config.as_deref())?; // Dispatch subcommands before bringing up the server. if let Some(cmd) = cli.command {