Repository navigation
fix(gl): derive View: URL from node instead of hardcoding gitlawb.com (#370) #377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Gravirei
wants to merge
10
commits into
Twigpine:main
Choose a base branch
from
Gravirei:fix/issue-370-view-url-404
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4fc7ad3
fix(gl): derive View: URL from node instead of hardcoding gitlawb.com…
Gravirei 9ca4fcf
fix(gl): validate web_url response and trim trailing slashes (#370)
Gravirei 195a017
fix: address review feedback on View-URL PR (#370)
Gravirei 40350d8
fix: validate web_url format and surface node info failures (#370)
Gravirei 57276ca
fix: reject query/fragment web_urls and warn on node info errors (#370)
Gravirei 98d3795
fix(gl): cap node info body and reject control-byte web_urls (#370)
Gravirei 6c40812
chore: bump h2 to 0.4.19 for RUSTSEC-2026-0258
Gravirei a6113fd
Merge branch 'main' into fix/issue-370-view-url-404
Gravirei 4c48511
fix(gl): close merge regressions on #370 branch and pin View: with in…
Gravirei 10da6f6
fix(gl,node): normalize stored web_url and reject userinfo (#370 review)
Gravirei File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,12 @@ pub struct Config { | |
| #[arg(long, env = "GITLAWB_PUBLIC_URL")] | ||
| pub public_url: Option<String>, | ||
|
|
||
| /// Base URL for the web view of repos on this node (e.g. https://gitlawb.com). | ||
| /// When set, `GET /` advertises it as `web_url` so the CLI can print a | ||
| /// working `View:` link. Omit for nodes that have no web front-end. | ||
| #[arg(long, env = "GITLAWB_WEB_URL")] | ||
| pub web_url: Option<String>, | ||
|
|
||
| /// Comma-separated list of bootstrap peer URLs to announce to on startup | ||
| #[arg(long, env = "GITLAWB_BOOTSTRAP_PEERS", value_delimiter = ',')] | ||
| pub bootstrap_peers: Vec<String>, | ||
|
|
@@ -728,7 +734,13 @@ impl Config { | |
| /// Cross-field boot validation. Single-field ranges are enforced by clap; this | ||
| /// catches combinations that ship a denial-of-service under otherwise-valid | ||
| /// values. Call once at startup and fail fast on `Err`. | ||
| pub fn validate(&self) -> Result<(), String> { | ||
| /// | ||
| /// `&mut self` is required so the validator can normalize `web_url` to | ||
| /// its canonical form after validation: the configured value is | ||
| /// advertised on public `GET /`, and any consumer reading | ||
| /// `config.web_url` later must see the same string that was actually | ||
| /// validated. | ||
| pub fn validate(&mut self) -> Result<(), String> { | ||
| // A write pins one pooled connection for its whole duration (the | ||
| // connection-affine advisory lock in repo_store::acquire_write), and | ||
| // concurrent writes are capped at max_concurrent_git_pushes. If the pool | ||
|
|
@@ -746,10 +758,84 @@ impl Config { | |
| floor | ||
| )); | ||
| } | ||
| // GITLAWB_WEB_URL is advertised in GET / so the CLI can print a working | ||
| // View: link. Clap maps "" to Some("") which would produce a broken URL; | ||
| // reject early instead of serving a malformed link. Non-blank values must | ||
| // additionally parse as an absolute http(s) URL — the CLI appends | ||
| // `/{owner}/{repo}` to it, so scheme-less hosts ("gitlawb.com") and other | ||
| // garbage would render links no browser can follow. | ||
| if let Some(raw) = &self.web_url { | ||
| match validate_web_url(raw) { | ||
| Ok(()) => {} | ||
| Err(reason) => { | ||
| return Err(format!( | ||
| "GITLAWB_WEB_URL {reason} — \ | ||
| set it to an absolute URL like https://gitlawb.com or leave it unset." | ||
| )); | ||
| } | ||
| } | ||
| // Normalize to one canonical form so every consumer (the regular | ||
| // and degraded `GET /` handlers, anyone who reads | ||
| // `config.web_url` directly) sees the same string that was | ||
| // actually validated. The CLI's own defensive trim would paper | ||
| // over the divergence on the `View:` line, but that hides the | ||
| // bug from any other consumer that parses the value verbatim. | ||
| let canonical = raw | ||
| .trim() | ||
| .parse::<url::Url>() | ||
| .map(|u| u.to_string()) | ||
| .map_err(|_| "is not a valid absolute URL".to_string())?; | ||
| if canonical != *raw { | ||
| self.web_url = Some(canonical); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// Validate a `web_url` value for use as a browser-reachable base URL. | ||
| /// Blank values are rejected (clap maps `--web-url ""` to `Some("")`), and | ||
| /// non-blank values must parse as absolute `http`/`https` URLs with no | ||
| /// query, fragment, or userinfo — the CLI treats this as a string prefix | ||
| /// to append paths to, so anything else produces links no browser can | ||
| /// follow (`?a=1/owner/repo` puts the repo path inside the query string); | ||
| /// and userinfo is rejected outright because the configured value is | ||
| /// published on public `GET /` and reprinted on the CLI's `View:` line, | ||
| /// so an accidentally configured password would land in public metadata, | ||
| /// terminal scrollback, and CI logs. | ||
| pub(crate) fn validate_web_url(raw: &str) -> Result<(), String> { | ||
| if raw.trim().is_empty() { | ||
| return Err("must not be empty or whitespace-only".into()); | ||
| } | ||
| let parsed: url::Url = raw | ||
| .trim() | ||
| .parse() | ||
| .map_err(|_| "is not a valid absolute URL".to_string())?; | ||
| match parsed.scheme() { | ||
| "http" | "https" => {} | ||
| other => return Err(format!("must use http or https, got '{other}:'")), | ||
| } | ||
| // url::Url cannot represent a URL without a host for http/https schemes, so | ||
| // reaching here guarantees an absolute browser-usable base. | ||
| if parsed.query().is_some() { | ||
| return Err( | ||
| "must not contain a query string — it is used as a path prefix for View links".into(), | ||
| ); | ||
| } | ||
| if parsed.fragment().is_some() { | ||
| return Err( | ||
| "must not contain a fragment — it is used as a path prefix for View links".into(), | ||
| ); | ||
| } | ||
| if !parsed.username().is_empty() || parsed.password().is_some() { | ||
| return Err( | ||
| "must not contain a username or password — it is advertised publicly and printed on the View: line" | ||
| .into(), | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
@@ -1375,7 +1461,7 @@ mod tests { | |
| .expect("default config must validate"); | ||
|
|
||
| // An under-sized pool relative to the push cap is rejected (20 < 32 + 8). | ||
| let under = Config::parse_from([ | ||
| let mut under = Config::parse_from([ | ||
| "gitlawb-node", | ||
| "--db-max-connections", | ||
| "20", | ||
|
|
@@ -1388,7 +1474,7 @@ mod tests { | |
| ); | ||
|
|
||
| // Exactly at the floor validates (40 == 32 + 8). | ||
| let at_floor = Config::parse_from([ | ||
| let mut at_floor = Config::parse_from([ | ||
| "gitlawb-node", | ||
| "--db-max-connections", | ||
| "40", | ||
|
|
@@ -1401,6 +1487,129 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn web_url_rejects_empty_and_whitespace() { | ||
| // Unset is fine. | ||
| Config::parse_from(["gitlawb-node"]) | ||
| .validate() | ||
| .expect("no web_url must validate"); | ||
|
|
||
| // A real URL validates. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some("https://gitlawb.com".into()); | ||
| assert!(cfg.validate().is_ok()); | ||
|
|
||
| // Empty string is rejected. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some("".into()); | ||
| assert!(cfg.validate().is_err(), "empty web_url must be rejected"); | ||
|
|
||
| // Whitespace-only is rejected. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(" ".into()); | ||
| assert!( | ||
| cfg.validate().is_err(), | ||
| "whitespace-only web_url must be rejected" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn web_url_rejects_non_absolute_or_non_browser_urls() { | ||
| // Scheme-less host: parses nowhere, renders a broken View: link. | ||
| for bad in [ | ||
| "gitlawb.com", | ||
| "not a url", | ||
| "ftp://files.example.com", | ||
| "javascript:alert(1)", | ||
| "https://", // no host | ||
| ] { | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(bad.into()); | ||
| assert!(cfg.validate().is_err(), "web_url {bad:?} must be rejected"); | ||
| } | ||
|
|
||
| // Query strings and fragments corrupt the appended repo path, since | ||
| // web_url is used as a raw string prefix (`?a=1` would swallow | ||
| // `/{owner}/{repo}` into the query). | ||
| for bad in [ | ||
| "https://gitlawb.com?a=1", | ||
| "https://gitlawb.com/?utm_source=docs", | ||
| "https://gitlawb.com#section", | ||
| "https://gitlawb.com/#faq", | ||
| ] { | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(bad.into()); | ||
| assert!( | ||
| cfg.validate().is_err(), | ||
| "web_url {bad:?} (query or fragment) must be rejected" | ||
| ); | ||
| } | ||
|
|
||
| // Surrounding whitespace around a valid URL is tolerated. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(" https://gitlawb.com ".into()); | ||
| assert!( | ||
| cfg.validate().is_ok(), | ||
| "padded-but-valid web_url must validate" | ||
| ); | ||
|
|
||
| // Non-default ports and paths are fine. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some("http://localhost:8080/ui".into()); | ||
| assert!(cfg.validate().is_ok(), "port+path web_url must validate"); | ||
|
|
||
| // Userinfo must be rejected: `web_url` is published on public | ||
| // `GET /` and reprinted on the CLI's `View:` line, so an | ||
| // accidentally configured password would land in public metadata, | ||
| // terminal scrollback, and CI logs. Reject username-only and | ||
| // user+password forms. | ||
| for bad in [ | ||
| "https://[email protected]", | ||
| "https://user:[email protected]", | ||
| "https://:[email protected]", | ||
| ] { | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(bad.into()); | ||
| assert!( | ||
| cfg.validate().is_err(), | ||
| "web_url {bad:?} (userinfo) must be rejected" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// `validate()` accepts whitespace-padded input but the stored value | ||
| /// must match the canonical form actually validated, so every consumer | ||
| /// (the regular and degraded `GET /` handlers, anyone who reads | ||
| /// `config.web_url` directly) sees the same string. Without this, an | ||
| /// operator that sets `GITLAWB_WEB_URL=" https://git.example "` boots | ||
| /// a node that advertises `" https://git.example "` on public | ||
| /// metadata — different from what was validated, different from what | ||
| /// any other consumer would parse. | ||
| #[test] | ||
| fn web_url_is_normalized_to_canonical_form_after_validation() { | ||
| // Padded input is rewritten to the canonical form. `url::Url`'s | ||
| // own `to_string` is what we use for the canonicalization, and | ||
| // it always appends a path-separator slash to a bare-host URL, | ||
| // so the stored value is `https://git.example/` (which is also | ||
| // what every browser would normalize the operator-typed form to). | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some(" https://git.example ".into()); | ||
| cfg.validate().unwrap(); | ||
| assert_eq!( | ||
| cfg.web_url.as_deref(), | ||
| Some("https://git.example/"), | ||
| "stored web_url must be the canonical form, not the operator-typed one" | ||
| ); | ||
|
|
||
| // Already-canonical input is left alone after a no-op | ||
| // re-normalization — same form comes back out, so any consumer | ||
| // reading the stored value sees a stable string across calls. | ||
| let mut cfg = Config::parse_from(["gitlawb-node"]); | ||
| cfg.web_url = Some("https://git.example/".into()); | ||
| cfg.validate().unwrap(); | ||
| assert_eq!(cfg.web_url.as_deref(), Some("https://git.example/")); | ||
| } | ||
|
|
||
| /// The DECLARED default, read off the parser rather than out of a parse. | ||
| /// | ||
| /// `Config::parse_from` consults the process environment, so on a host that | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.