Skip to content

Commit ff53ca3

Browse files
authored
feat(base-url)!: disallow relative local base to avoid confusion (#1857)
* feat(base-url)!: disallow relative directory paths in `--base-url` the previous behaviour accepted relative directory paths as bases but this led to later InvalidBaseJoin errors, because relative paths cannot be used to join relative URLs. this means that relative local bases were *only* useful for resolving root-relative links, and were confusingly problematic with ordinary relative links. see https://www.github.com/lycheeverse/lychee/issues/1574 which talks about errors when passing `--base ../network-documentation/` or, in a later comment, `--base build`. also, the previous behaviour would parse something like `--base-url google.com` to a local base pointing to `./google.com`. this would also lead to downstream errors and it's better to guard against this. it is my opinion that it is better to fail earlier in these cases, so the user is not hit with mysterious InvalidBaseJoin. after this pr, there will be a command-line argument parsing error: ``` error: invalid value 'build' for '--base-url <BASE_URL>': Error with base dir `build` : Base must either be a URL (with scheme) or an absolute path. Alternatively, if you want to resolve root-relative links in local files, see `--root-dir`. ``` in a slightly opinionated touch, i've mentioned --root-dir in the error message, since i think --root-dir is more suitable for most use cases where people try to use relative local bases. this agrees with later comments in https://www.github.com/lycheeverse/lychee/issues/1574. however, this does make the error message quite long. so i'm happy to take on feedback and changes about this. this pr implements (1) in my outline to reduce InvalidBaseJoin confusion, as described in https://www.github.com/lycheeverse/lychee/pull/1624#issuecomment-3274485963 * clippy * clipy 2 * review: add --help mention TODO: probably mention these restrictions and suggestions in --help too * add sentence to --help * move CLI-related suggestions into .context in lychee-bin this does have the side-effect of attaching the --help and --root-dir suggestions even to the "cannot be a base" error. ``` error: invalid value 'a:datafdajsio' for '--base-url <BASE_URL>': Error with base dir `a:datafdajsio` : The given URL cannot be used as a base URL. See `--help` for more information. If you want to resolve root-relative links in local files, also see `--root-dir`. ``` this could be a bit confusing, but idk a way around it without inspecting the error message of InvalidBase to determine which context to add. * shuffle extra notes about --base-url to the end of its help, and add a sentence about URL schemes to cover the "cannot be a base" error.
1 parent 73249e0 commit ff53ca3

File tree

4 files changed

+45
-6
lines changed

4 files changed

+45
-6
lines changed

README.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,9 @@ Options:
596596
Basically, the base URL option resolves links as if the local files were hosted
597597
at the given base URL address.
598598
599+
The provided base URL value must either be a URL (with scheme) or an absolute path.
600+
Note that certain URL schemes cannot be used as a base, e.g., `data` and `mailto`.
601+
599602
--root-dir <ROOT_DIR>
600603
Root directory to use when checking absolute links in local files. This option is
601604
required if absolute links appear in local files, otherwise those links will be

lychee-bin/src/options.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -750,7 +750,10 @@ of the base URL. For example, a base URL of `https://example.com/dir/page/` and
750750
a link of `a` would resolve to `https://example.com/dir/page/a`.
751751
752752
Basically, the base URL option resolves links as if the local files were hosted
753-
at the given base URL address."
753+
at the given base URL address.
754+
755+
The provided base URL value must either be a URL (with scheme) or an absolute path.
756+
Note that certain URL schemes cannot be used as a base, e.g., `data` and `mailto`."
754757
)]
755758
#[serde(default)]
756759
pub(crate) base_url: Option<Base>,

lychee-bin/src/parse.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,20 @@ pub(crate) fn parse_remaps(remaps: &[String]) -> Result<Remaps> {
1313
.context("Remaps must be of the form '<pattern> <uri>' (separated by whitespace)")
1414
}
1515

16-
pub(crate) fn parse_base(src: &str) -> Result<Base, lychee_lib::ErrorKind> {
17-
Base::try_from(src)
16+
pub(crate) fn parse_base(src: &str) -> Result<Base> {
17+
match Base::try_from(src) {
18+
Ok(x) => Ok(x),
19+
Err(e) => {
20+
// if context is defined, clap displays only the context string in
21+
// argument parse errors. to keep the message from within InvalidBase,
22+
// we need to retain it manually.
23+
let message = format!(
24+
"{e}. See `--help` for more information. If you want to resolve \
25+
root-relative links in local files, also see `--root-dir`."
26+
);
27+
Err(e).context(message)
28+
}
29+
}
1830
}
1931

2032
#[cfg(test)]

lychee-lib/src/types/base.rs

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,24 @@ impl TryFrom<&str> for Base {
5656
if url.cannot_be_a_base() {
5757
return Err(ErrorKind::InvalidBase(
5858
value.to_string(),
59-
"The given URL cannot be a base".to_string(),
59+
"The given URL cannot be used as a base URL".to_string(),
6060
));
6161
}
6262
return Ok(Self::Remote(url));
6363
}
64-
Ok(Self::Local(PathBuf::from(value)))
64+
65+
// require absolute paths in `Base::Local`. a local non-relative base is
66+
// basically useless because it cannot be used to join URLs and will
67+
// cause InvalidBaseJoin.
68+
let path = PathBuf::from(value);
69+
if path.is_absolute() {
70+
Ok(Self::Local(path))
71+
} else {
72+
Err(ErrorKind::InvalidBase(
73+
value.to_string(),
74+
"Base must either be a URL (with scheme) or an absolute local path".to_string(),
75+
))
76+
}
6577
}
6678
}
6779

@@ -96,14 +108,23 @@ mod test_base {
96108

97109
#[test]
98110
fn test_valid_local_path_string_as_base() -> Result<()> {
99-
let cases = vec!["/tmp/lychee", "/tmp/lychee/", "tmp/lychee/"];
111+
let cases = vec!["/tmp/lychee", "/tmp/lychee/"];
100112

101113
for case in cases {
102114
assert_eq!(Base::try_from(case)?, Base::Local(PathBuf::from(case)));
103115
}
104116
Ok(())
105117
}
106118

119+
#[test]
120+
fn test_invalid_local_path_string_as_base() {
121+
let cases = vec!["a", "tmp/lychee/", "example.com", "../nonlocal"];
122+
123+
for case in cases {
124+
assert!(Base::try_from(case).is_err());
125+
}
126+
}
127+
107128
#[test]
108129
fn test_valid_local() -> Result<()> {
109130
let dir = tempfile::tempdir().unwrap();

0 commit comments

Comments
 (0)