From 5445458d1a235cd08acfea5acecb6b75d0d1866f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Nov 2025 11:59:20 -0800 Subject: [PATCH 1/2] Add tests for various environment config issues These currently aren't working as expected. --- tests/testsuite/config.rs | 133 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index f5b14875a2..253cc0f797 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -204,6 +204,139 @@ ERROR Invalid configuration file unknown field `title`, expected `edition` +"#]]); + }); +} + +// An invalid top-level key in the environment. +#[test] +fn env_invalid_config_key() { + BookTest::from_dir("config/empty").run("build", |cmd| { + cmd.env("MDBOOK_FOO", "testing") + .expect_failure() + .expect_stdout(str![[""]]) + .expect_stderr(str![[r#" + +thread 'main' ([..]) panicked at [..] +unreachable: invalid key `foo` +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + +"#]]); + }); +} + +// An invalid value in the environment. +#[test] +fn env_invalid_value() { + BookTest::from_dir("config/empty") + .run("build", |cmd| { + cmd.env("MDBOOK_BOOK", r#"{"titlez": "typo"}"#) + .expect_stdout(str![[""]]) + .expect_stderr(str![[r#" + INFO Book building has started + INFO Running the html backend + INFO HTML book written to `[ROOT]/book` + +"#]]); + }) + .run("build", |cmd| { + cmd.env("MDBOOK_BOOK__TITLE", r#"{"looks like obj": "abc"}"#) + .expect_stdout(str![[""]]) + .expect_stderr(str![[r#" + INFO Book building has started + INFO Running the html backend + INFO HTML book written to `[ROOT]/book` + +"#]]); + }) + .check_file_contains("book/index.html", "Chapter 1") + .run("build", |cmd| { + cmd.env("MDBOOK_BOOK__TITLE", r#"{braces}"#) + .expect_stdout(str![[""]]) + .expect_stderr(str![[r#" + INFO Book building has started + INFO Running the html backend + INFO HTML book written to `[ROOT]/book` + +"#]]); + }) + .check_file_contains("book/index.html", "Chapter 1 - {braces}"); +} + +// Replacing the entire book table from the environment. +#[test] +fn env_entire_book_table() { + BookTest::init(|_| {}) + .change_file( + "book.toml", + "[book]\n\ + title = \"config title\"\n\ + ", + ) + .run("build", |cmd| { + cmd.env("MDBOOK_BOOK", r#"{"description": "custom description"}"#); + }) + .check_file_contains("book/index.html", "Chapter 1 - config title") + .check_file_contains( + "book/index.html", + r#""#, + ); +} + +// Replacing the entire output or preprocessor table from the environment. +#[test] +fn env_entire_output_preprocessor_table() { + BookTest::from_dir("config/empty") + .rust_program( + "mdbook-my-preprocessor", + r#" + fn main() { + let mut args = std::env::args().skip(1); + if args.next().as_deref() == Some("supports") { + return; + } + use std::io::Read; + let mut s = String::new(); + std::io::stdin().read_to_string(&mut s).unwrap(); + assert!(s.contains("custom preprocessor config")); + println!("{{\"items\": []}}"); + } + "#, + ) + .rust_program( + "mdbook-my-output", + r#" + fn main() { + use std::io::Read; + let mut s = String::new(); + std::io::stdin().read_to_string(&mut s).unwrap(); + assert!(s.contains("custom output config")); + } + "#, + ) + .run("build", |cmd| { + let mut paths: Vec<_> = + std::env::split_paths(&std::env::var_os("PATH").unwrap_or_default()).collect(); + paths.push(cmd.dir.clone()); + let path = std::env::join_paths(paths).unwrap().into_string().unwrap(); + + cmd.env( + "MDBOOK_OUTPUT", + r#"{"my-output": {"foo": "custom output config"}}"#, + ) + .env( + "MDBOOK_PREPROCESSOR", + r#"{"my-preprocessor": {"foo": "custom preprocessor config"}}"#, + ) + .env("PATH", path) + .expect_failure() + .expect_stdout(str![[""]]) + .expect_stderr(str![[r#" + +thread 'main' ([..]) panicked at [..] +unreachable: invalid key `output` +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + "#]]); }); } From 2afad43bdd46533964ca72be483919da3dd226b7 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 17 Nov 2025 14:38:58 -0800 Subject: [PATCH 2/2] Add error handling to env config handling This adds several changes to how environment variables are handled to more closely align with how configs are handled, and to fix an issue with replacing entire tables. The changes are: - Top-level tables like `MDBOOK_BOOK` now *replace* the contents of the `book` table instead of merging it. This adds consistency with how all the other environment objects work. - Fixed allowing top-level replacement of `MDBOOK_BOOK` and `MDBOOK_OUTPUT`. This was inadvertently recently broken. - Added ability to replace top-level `MDBOOK_RUST`. I don't recall why that wasn't included. - Reject invalid keys like `MDBOOK_FOO`. - Reject unknown keys, like `MDBOOK_BOOK='{"xyz": 123}'` - Reject invalid types, like `MDBOOK_BOOK='{"title": 123}'` --- CHANGELOG.md | 9 ++- crates/mdbook-core/src/config.rs | 73 +++++++++---------- crates/mdbook-driver/src/mdbook.rs | 2 +- .../configuration/environment-variables.md | 9 +-- tests/testsuite/config.rs | 37 +++++----- 5 files changed, 68 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d34664d17..49fda4dcab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,10 @@ This release also includes many new features described below. We have prepared a [0.5 Migration Guide](#05-migration-guide) to help existing authors switch from 0.4. -The final 0.5.0 release does not contain any changes since [0.5.0-beta.2](#mdbook-050-beta2). +The final 0.5.0 release only contains the following changes since [0.5.0-beta.2](#mdbook-050-beta2): + +- Added error handling to environment config handling. This checks that environment variables starting with `MDBOOK_` are correctly specified instead of silently ignoring. This also fixed being able to replace entire top-level tables like `MDBOOK_OUTPUT`. + [#2942](https://github.com/rust-lang/mdBook/pull/2942) ## 0.5 Migration Guide @@ -56,6 +59,10 @@ The following is a summary of the changes that may require your attention when u [#2775](https://github.com/rust-lang/mdBook/pull/2775) - Removed the very old legacy config support. Warnings have been displayed in previous versions on how to migrate. [#2783](https://github.com/rust-lang/mdBook/pull/2783) +- Top-level config values set from the environment like `MDBOOK_BOOK` now *replace* the contents of the top-level table instead of merging into it. + [#2942](https://github.com/rust-lang/mdBook/pull/2942) +- Invalid environment variables are now rejected. Previously unknown keys like `MDBOOK_FOO` would be ignored, or keys or invalid values inside objects like the `[book]` table would be ignored. + [#2942](https://github.com/rust-lang/mdBook/pull/2942) ### Theme changes diff --git a/crates/mdbook-core/src/config.rs b/crates/mdbook-core/src/config.rs index 166e921ef3..3368e5f6a8 100644 --- a/crates/mdbook-core/src/config.rs +++ b/crates/mdbook-core/src/config.rs @@ -123,11 +123,10 @@ impl Config { /// /// For example: /// - /// - `MDBOOK_foo` -> `foo` - /// - `MDBOOK_FOO` -> `foo` - /// - `MDBOOK_FOO__BAR` -> `foo.bar` - /// - `MDBOOK_FOO_BAR` -> `foo-bar` - /// - `MDBOOK_FOO_bar__baz` -> `foo-bar.baz` + /// - `MDBOOK_book` -> `book` + /// - `MDBOOK_BOOK` -> `book` + /// - `MDBOOK_BOOK__TITLE` -> `book.title` + /// - `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction` /// /// So by setting the `MDBOOK_BOOK__TITLE` environment variable you can /// override the book's title without needing to touch your `book.toml`. @@ -147,7 +146,7 @@ impl Config { /// The latter case may be useful in situations where `mdbook` is invoked /// from a script or CI, where it sometimes isn't possible to update the /// `book.toml` before building. - pub fn update_from_env(&mut self) { + pub fn update_from_env(&mut self) -> Result<()> { debug!("Updating the config from environment variables"); let overrides = @@ -162,19 +161,9 @@ impl Config { let parsed_value = serde_json::from_str(&value) .unwrap_or_else(|_| serde_json::Value::String(value.to_string())); - if key == "book" || key == "build" { - if let serde_json::Value::Object(ref map) = parsed_value { - // To `set` each `key`, we wrap them as `prefix.key` - for (k, v) in map { - let full_key = format!("{key}.{k}"); - self.set(&full_key, v).expect("unreachable"); - } - return; - } - } - - self.set(key, parsed_value).expect("unreachable"); + self.set(key, parsed_value)?; } + Ok(()) } /// Get a value from the configuration. @@ -266,24 +255,39 @@ impl Config { /// `output.html.playground` will set the "playground" in the html output /// table). /// - /// The only way this can fail is if we can't serialize `value` into a - /// `toml::Value`. + /// # Errors + /// + /// This will fail if: + /// + /// - The value cannot be represented as TOML. + /// - The value is not a correct type. + /// - The key is an unknown configuration option. pub fn set>(&mut self, index: I, value: S) -> Result<()> { let index = index.as_ref(); let value = Value::try_from(value) .with_context(|| "Unable to represent the item as a JSON Value")?; - if let Some(key) = index.strip_prefix("book.") { - self.book.update_value(key, value); + if index == "book" { + self.book = value.try_into()?; + } else if index == "build" { + self.build = value.try_into()?; + } else if index == "rust" { + self.rust = value.try_into()?; + } else if index == "output" { + self.output = value; + } else if index == "preprocessor" { + self.preprocessor = value; + } else if let Some(key) = index.strip_prefix("book.") { + self.book.update_value(key, value)?; } else if let Some(key) = index.strip_prefix("build.") { - self.build.update_value(key, value); + self.build.update_value(key, value)?; } else if let Some(key) = index.strip_prefix("rust.") { - self.rust.update_value(key, value); + self.rust.update_value(key, value)?; } else if let Some(key) = index.strip_prefix("output.") { - self.output.update_value(key, value); + self.output.update_value(key, value)?; } else if let Some(key) = index.strip_prefix("preprocessor.") { - self.preprocessor.update_value(key, value); + self.preprocessor.update_value(key, value)?; } else { bail!("invalid key `{index}`"); } @@ -703,18 +707,13 @@ pub struct SearchChapterSettings { /// This is definitely not the most performant way to do things, which means you /// should probably keep it away from tight loops... trait Updateable<'de>: Serialize + Deserialize<'de> { - fn update_value(&mut self, key: &str, value: S) { + fn update_value(&mut self, key: &str, value: S) -> Result<()> { let mut raw = Value::try_from(&self).expect("unreachable"); - - if let Ok(value) = Value::try_from(value) { - raw.insert(key, value); - } else { - return; - } - - if let Ok(updated) = raw.try_into() { - *self = updated; - } + let value = Value::try_from(value)?; + raw.insert(key, value); + let updated = raw.try_into()?; + *self = updated; + Ok(()) } } diff --git a/crates/mdbook-driver/src/mdbook.rs b/crates/mdbook-driver/src/mdbook.rs index 80ae692b92..e5dac9027f 100644 --- a/crates/mdbook-driver/src/mdbook.rs +++ b/crates/mdbook-driver/src/mdbook.rs @@ -56,7 +56,7 @@ impl MDBook { Config::default() }; - config.update_from_env(); + config.update_from_env()?; if tracing::enabled!(tracing::Level::TRACE) { for line in format!("Config: {config:#?}").lines() { diff --git a/guide/src/format/configuration/environment-variables.md b/guide/src/format/configuration/environment-variables.md index 0170e75936..bef0431f85 100644 --- a/guide/src/format/configuration/environment-variables.md +++ b/guide/src/format/configuration/environment-variables.md @@ -12,11 +12,10 @@ underscore (`_`) is replaced with a dash (`-`). For example: -- `MDBOOK_foo` -> `foo` -- `MDBOOK_FOO` -> `foo` -- `MDBOOK_FOO__BAR` -> `foo.bar` -- `MDBOOK_FOO_BAR` -> `foo-bar` -- `MDBOOK_FOO_bar__baz` -> `foo-bar.baz` +- `MDBOOK_book` -> `book` +- `MDBOOK_BOOK` -> `book` +- `MDBOOK_BOOK__TITLE` -> `book.title` +- `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction` So by setting the `MDBOOK_BOOK__TITLE` environment variable you can override the book's title without needing to touch your `book.toml`. diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 253cc0f797..e44649a84e 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -216,10 +216,7 @@ fn env_invalid_config_key() { .expect_failure() .expect_stdout(str![[""]]) .expect_stderr(str![[r#" - -thread 'main' ([..]) panicked at [..] -unreachable: invalid key `foo` -note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +ERROR invalid key `foo` "#]]); }); @@ -231,25 +228,26 @@ fn env_invalid_value() { BookTest::from_dir("config/empty") .run("build", |cmd| { cmd.env("MDBOOK_BOOK", r#"{"titlez": "typo"}"#) + .expect_failure() .expect_stdout(str![[""]]) .expect_stderr(str![[r#" - INFO Book building has started - INFO Running the html backend - INFO HTML book written to `[ROOT]/book` +ERROR unknown field `titlez`, expected one of `title`, `authors`, `description`, `src`, `language`, `text-direction` + "#]]); }) .run("build", |cmd| { cmd.env("MDBOOK_BOOK__TITLE", r#"{"looks like obj": "abc"}"#) + .expect_failure() .expect_stdout(str![[""]]) .expect_stderr(str![[r#" - INFO Book building has started - INFO Running the html backend - INFO HTML book written to `[ROOT]/book` +ERROR invalid type: map, expected a string +in `title` + "#]]); }) - .check_file_contains("book/index.html", "Chapter 1") + // This is not valid JSON, so falls back to be interpreted as a string. .run("build", |cmd| { cmd.env("MDBOOK_BOOK__TITLE", r#"{braces}"#) .expect_stdout(str![[""]]) @@ -276,7 +274,8 @@ fn env_entire_book_table() { .run("build", |cmd| { cmd.env("MDBOOK_BOOK", r#"{"description": "custom description"}"#); }) - .check_file_contains("book/index.html", "Chapter 1 - config title") + // The book.toml title is removed. + .check_file_contains("book/index.html", "Chapter 1") .check_file_contains( "book/index.html", r#""#, @@ -311,6 +310,7 @@ fn env_entire_output_preprocessor_table() { let mut s = String::new(); std::io::stdin().read_to_string(&mut s).unwrap(); assert!(s.contains("custom output config")); + eprintln!("preprocessor saw custom config"); } "#, ) @@ -329,14 +329,15 @@ fn env_entire_output_preprocessor_table() { r#"{"my-preprocessor": {"foo": "custom preprocessor config"}}"#, ) .env("PATH", path) - .expect_failure() .expect_stdout(str![[""]]) .expect_stderr(str![[r#" - -thread 'main' ([..]) panicked at [..] -unreachable: invalid key `output` -note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + INFO Book building has started + INFO Running the my-output backend + INFO Invoking the "my-output" renderer +preprocessor saw custom config "#]]); - }); + }) + // No HTML output + .check_file_list("book", str![[""]]); }