Fix panic on invalid BEACON_BASE_PATH; make config loading fallible#242
Merged
Conversation
Setting BEACON_BASE_PATH to a value without a leading slash (e.g. 'mybeacon') crashed the server at startup: the raw value was passed straight to axum's Router::nest() and utoipa, which assert paths begin with '/'. - Normalize base_path to a canonical form (exactly one leading '/', no trailing '/'); empty/blank serves at root. So 'foo', '/foo', and '/foo/' are equivalent. - Validate path characters (URL unreserved set + '/'); reject anything else and empty internal segments with a descriptive error. - Make config loading fallible: replace the panicking Config::init() with Config::load() -> Result and a OnceLock-backed beacon_config::init(), called from main() so configuration errors surface as a clean anyhow error instead of a panic. CONFIG is now a Deref handle, keeping all existing call sites unchanged. - Add unit tests for normalize_base_path and document the behavior. Fixes #240
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a startup panic caused by malformed BEACON_BASE_PATH by normalizing/validating the value during configuration loading and making config initialization fallible so errors surface as clean anyhow failures.
Changes:
- Added
normalize_base_pathto canonicalizeBEACON_BASE_PATHand reject invalid characters/segments before the value reaches axum/utoipa. - Replaced the panicking config initialization with
Config::load() -> anyhow::Result<Config>and aOnceLock-backed global config handle plusbeacon_config::init(). - Updated the Beacon API binary to initialize config up front; added unit tests and documentation for the base path behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/docs/1.7.0/data-lake/configuration.md | Documents base path normalization/validation behavior. |
| Cargo.lock | Adds anyhow dependency for beacon-config. |
| beacon-config/src/lib.rs | Implements base path normalization/validation, fallible config loading, and OnceLock-based global config. |
| beacon-config/Cargo.toml | Adds anyhow as a workspace dependency. |
| beacon-api/src/main.rs | Calls beacon_config::init() early so config errors exit cleanly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `BEACON_HOST` - IP address to listen on. (Default is `0.0.0.0`) | ||
| - `BEACON_PORT` - Port number to listen on. (Default is `5001`) | ||
| - `BEACON_BASE_PATH` - Optional URL path prefix to serve the HTTP API, OpenAPI document, and Swagger UI under (default is empty, i.e. served at the root). Useful when running Beacon behind a reverse proxy or on a shared subpath, e.g. `/beacon`. | ||
| - `BEACON_BASE_PATH` - Optional URL path prefix to serve the HTTP API, OpenAPI document, and Swagger UI under (default is empty, i.e. served at the root). Useful when running Beacon behind a reverse proxy or on a shared subpath, e.g. `/beacon`. The value is normalized to exactly one leading slash and no trailing slash, so `beacon`, `/beacon`, and `/beacon/` are equivalent. Only URL-safe characters are allowed (letters, digits, `-`, `_`, `.`, `~`, and `/` as a separator); any other character (e.g. spaces, `?`, `#`, `%`) causes Beacon to exit at startup with a descriptive error. |
Comment on lines
+407
to
+415
| pub fn init() -> anyhow::Result<&'static Config> { | ||
| if let Some(config) = CONFIG_CELL.get() { | ||
| return Ok(config); | ||
| } | ||
| let config = Config::load()?; | ||
| // A concurrent caller may have won the race; either value is equally valid. | ||
| let _ = CONFIG_CELL.set(config); | ||
| Ok(CONFIG_CELL.get().expect("config cell populated above")) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Fixes #240. Setting
BEACON_BASE_PATHto a value without a leading slash (e.g.mybeacon) crashed the server at startup — the raw value was passed straight toaxum's
Router::nest()and utoipa, which assert that paths begin with/.Changes
base_pathto a canonical form: exactly one leading/, no trailing/; empty/blank serves at root. Sofoo,/foo, and/foo/are equivalent./as separator); rejectanything else (spaces,
?,#,%, …) and empty internal segments with adescriptive error.
Config::init()with afallible
Config::load() -> anyhow::Result<Config>plus aOnceLock-backedbeacon_config::init(), called frommain()so config errors surface as a cleananyhowerror and the process exits non-zero.CONFIGis now aDerefhandle, soall existing
beacon_config::CONFIG.<field>call sites are unchanged.normalize_base_pathand documented the behavior in theconfiguration docs.
Verification
cargo test -p beacon-config— 5 tests pass (normalization + validation).cargo check --workspace— compiles clean.BEACON_BASE_PATH="bad path":