From d5b76ec8d8c03b277ba6b00e74c529037a5d15f1 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Thu, 17 Sep 2026 17:00:22 +0200 Subject: [PATCH] new: validate wizard defaults like typed answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ask_text accepted its default on Enter without running the question's validator, so a probed upstream version like 1.0-2 or v1.0 sailed through the whole questionnaire and crashed resolve() at the end, and an invalid git-derived maintainer default (e.g. 'Name <>') was accepted verbatim. ask_text now takes the validator and applies it to both typed answers and the offered default — a default that fails validation is withheld and an invalid answer re-asks — and all question call sites (incl. the maintainer loop) route through it. --- src/new/questions.rs | 239 +++++++++++++++++++++++++++++++------------ 1 file changed, 171 insertions(+), 68 deletions(-) diff --git a/src/new/questions.rs b/src/new/questions.rs index 5c94d36..d64736c 100644 --- a/src/new/questions.rs +++ b/src/new/questions.rs @@ -250,7 +250,10 @@ async fn run_wizard(mut cli: NewCli) -> Result> { let mut origin = packaged_dir.as_deref().and_then(GitOrigin::detect); // 4. Upstream version: probed project version, then the tag HEAD sits - // on, then `+git.`, then 0.1.0. + // on, then `+git.`, then 0.1.0. A raw probe + // that fails `validate_upstream_version` (e.g. `1.0-2` or `v1.0`) is + // not offered as the default — the same validator `resolve` applies + // decides, so Enter can never accept it and crash late. if cli.upstream_version.is_none() { let default = probe .as_ref() @@ -259,9 +262,8 @@ async fn run_wizard(mut cli: NewCli) -> Result> { .or_else(|| origin.as_ref().and_then(GitOrigin::git_version)) .unwrap_or_else(|| "0.1.0".to_string()); let revision = cli.revision.unwrap_or(1); - let answer = ask_text("Upstream version", &default, move |version: &str| { - options::validate_upstream_version(version, revision) - })?; + let validate = move |version: &str| options::validate_upstream_version(version, revision); + let answer = ask_text("Upstream version", &default, validate)?; cli.upstream_version = Some(answer.clone()); // The typed version names an existing tag HEAD is not on: offer to @@ -334,31 +336,25 @@ async fn run_wizard(mut cli: NewCli) -> Result> { .as_ref() .and_then(|p| p.description.clone()) .unwrap_or_default(); - loop { - let answer = ask_text( - "One-line description", - &default, - required_answer("the description"), - )?; - if !answer.trim().is_empty() { - cli.description = Some(answer); - break; - } - log::warn!("A one-line description is required to scaffold a package"); - } + let validate = required_answer("the description"); + let answer = ask_text("One-line description", &default, validate)?; + cli.description = Some(answer); } - // 7. Homepage. + // 7. Homepage (blank skips; a probed default must still be a valid URL). if cli.homepage.is_none() { let default = probe .as_ref() .and_then(|p| p.homepage.clone()) .unwrap_or_default(); - let answer = ask_text( - "Homepage (blank to skip)", - &default, - options::validate_homepage, - )?; + let validate = |url: &str| { + if url.is_empty() { + Ok(()) + } else { + options::validate_homepage(url) + } + }; + let answer = ask_text("Homepage (blank to skip)", &default, validate)?; if !answer.is_empty() { cli.homepage = Some(answer); } @@ -378,17 +374,8 @@ async fn run_wizard(mut cli: NewCli) -> Result> { options.contains(&answer.to_string()) })?; if answer == LICENSE_OTHER { - let license = loop { - let candidate = ask_text( - "License (SPDX identifier)", - &custom_default, - required_answer("the license identifier"), - )?; - if !candidate.trim().is_empty() { - break candidate; - } - log::warn!("A license identifier is required when picking the free-text entry"); - }; + let validate = required_answer("the license identifier"); + let license = ask_text("License (SPDX identifier)", &custom_default, validate)?; cli.license = Some(license); } else { cli.license = Some(answer); @@ -396,43 +383,41 @@ async fn run_wizard(mut cli: NewCli) -> Result> { } // 9. Command name (skipped for the empty template, where nothing is - // installed). + // installed). The probed default must pass the required-answer check + // too, or the question is asked without one. if cli.command.is_none() && template != TemplateId::Empty { let default = probe .as_ref() .and_then(|p| p.command.clone()) .unwrap_or_else(|| cli.name.clone().unwrap_or_default()); - let command = ask_text( - "Command name", - &default, - required_answer("the command name"), - )?; - cli.command = Some(if command.is_empty() { - cli.name.clone().unwrap_or_default() - } else { - command - }); + let validate = required_answer("the command name"); + let command = ask_text("Command name", &default, validate)?; + cli.command = Some(command); } - // 10. Maintainer, defaulting to the DEBEMAIL/git-config identity (an - // empty answer re-asks). + // 10. Maintainer, defaulting to the DEBEMAIL/git-config identity. The + // git-derived default goes through `parse_maintainer` like typed + // input: an empty git user.email yields a `Name <>` default that is + // ignored (the question is asked without one) instead of accepted + // verbatim and only failing late in `resolve`. An empty answer + // re-asks. if cli.maintainer.is_none() { - let default = crate::changelog::get_maintainer_info() + let candidate = crate::changelog::get_maintainer_info() .map(|(name, email)| format!("{name} <{email}>")) .unwrap_or_default(); - let maintainer = loop { - let answer = ask_text("Maintainer", &default, |answer: &str| { - options::parse_maintainer(answer).map(|_| ()) - })?; - if !answer.is_empty() { - break answer; - } + let default = if options::parse_maintainer(&candidate).is_ok() { + candidate + } else { + String::new() + }; + if default.is_empty() { log::warn!( "Could not determine a maintainer default (no DEBFULLNAME/\ DEBEMAIL and no git user config): answer as 'Name '" ); - }; - cli.maintainer = Some(maintainer); + } + let validate = |answer: &str| options::parse_maintainer(answer).map(|_| ()); + cli.maintainer = Some(ask_text("Maintainer", &default, validate)?); } // 11. Target distribution. @@ -472,10 +457,11 @@ async fn run_wizard(mut cli: NewCli) -> Result> { // 13. Metapackage Depends (empty template only). if template == TemplateId::Empty && cli.depends.is_empty() { + let validate = |answer: &str| options::validate_depends(answer).map(|_| ()); let answer = ask_text( "Depends (metapackage, comma-separated, blank for an empty base)", "", - |answer: &str| options::validate_depends(answer).map(|_| ()), + validate, )?; if !answer.trim().is_empty() { cli.depends = vec![answer]; @@ -799,17 +785,54 @@ fn select_from( } } +/// The default a question is offered with: a probed default must clear the +/// same `validate` bar as typed input, so an unusable probe (e.g. an +/// upstream version like `1.0-2` carrying a Debian revision) is withheld — +/// the question is asked without a default instead of offering one that +/// Enter would accept verbatim. +fn offered_default<'a>(default: &'a str, validate: &prompt::Validator) -> &'a str { + if default.is_empty() || validate(default).is_ok() { + default + } else { + "" + } +} + +/// One round of [`ask_text`]: an empty answer takes the (pre-validated) +/// `default`, and whatever answer is finally proposed — typed or the +/// default — must pass `validate`. `Err` carries the validation error so the +/// caller re-asks with it. +fn accept_answer( + answer: &str, + default: &str, + validate: &prompt::Validator, +) -> Result { + let answer = if answer.is_empty() { default } else { answer }; + validate(answer).map(|_| answer.to_string()) +} + /// One free-text question implementing the spec's "Enter accepts the -/// default": an empty answer falls back to `default` (`Esc` keeps its -/// prompt-level meaning of restoring the default). `validate` only ever -/// sees non-empty answers — the empty one is accepted by the prompt loop so -/// it can take the default path; callers that require an answer re-check -/// the result. +/// default": an empty answer falls back to the default (`Esc` keeps its +/// prompt-level meaning of restoring it). Both the typed answer and the +/// default go through `validate`: a probed default that fails validation is +/// never offered ([`offered_default`]), and an empty answer without a valid +/// default is treated like any other invalid answer (re-ask with the +/// validation error) — probe data can never bypass validation and only blow +/// up later in [`options::resolve`]. fn ask_text( label: &str, default: &str, validate: impl Fn(&str) -> Result<(), String> + 'static, ) -> Result> { + // The offered default must clear the same bar as typed input: an + // unusable probe is withheld and the question is asked without one. + let default = offered_default(default, &validate); + // Enter/Esc (and the non-TTY fallback) return the default without + // validation, so pre-decide what an empty answer yields: the default + // itself (it passed the bar above), or the rejection it shares with any + // invalid answer when no valid default exists. Computed here because + // `validate` moves into the prompt wrapper below. + let empty_answer = accept_answer("", default, &validate); let accept_empty = move |answer: &str| { if answer.is_empty() { Ok(()) @@ -817,12 +840,20 @@ fn ask_text( validate(answer) } }; - let answer = prompt::text(label, default, Some(&accept_empty))?; - Ok(if answer.is_empty() { - default.to_string() - } else { - answer - }) + loop { + let answer = prompt::text(label, default, Some(&accept_empty))?; + // Typed non-empty answers were already validated by the prompt; only + // an empty one resolves to the default, pre-decided above. + let answer = if answer.is_empty() { + empty_answer.clone() + } else { + Ok(answer) + }; + match answer { + Ok(answer) => return Ok(answer), + Err(error) => log::warn!("{error}"), + } + } } /// A validator requiring a non-empty answer. @@ -1404,6 +1435,78 @@ mod tests { assert!(required_answer("x")("ok").is_ok()); } + /// A probed upstream-version default must clear the same + /// [`options::validate_upstream_version`] bar that `resolve` applies: a + /// raw probe carrying a Debian revision or a `v` prefix fails it, so the + /// wizard withholds it instead of offering it for blind Enter-acceptance + /// and crashing late in `resolve` (regression). + #[test] + fn probed_version_defaults_fail_upstream_validation() { + assert!(options::validate_upstream_version("1.0-2", 1).is_err()); + assert!(options::validate_upstream_version("v1.0", 1).is_err()); + assert!(options::validate_upstream_version("1.0.0", 1).is_ok()); + + let validate = |version: &str| options::validate_upstream_version(version, 1); + // Invalid probes are not offered as the default... + assert_eq!(offered_default("1.0-2", &validate), ""); + assert_eq!(offered_default("v1.0", &validate), ""); + // ...clean probes keep theirs, and no default stays none. + assert_eq!(offered_default("1.0.0", &validate), "1.0.0"); + assert_eq!(offered_default("", &validate), ""); + } + + /// [`accept_answer`] routes typed answers and Enter-taken defaults + /// through the same validation: valid typed input accepted verbatim, + /// invalid typed input rejected (re-ask), an empty answer taking a valid + /// default, and an empty answer with an invalid or missing default + /// rejected like any other invalid answer. + #[test] + fn accept_answer_validates_typed_answers_and_defaults() { + let validate = |answer: &str| { + if answer == "ok" { + Ok(()) + } else { + Err(format!("not ok: {answer}")) + } + }; + + assert_eq!(accept_answer("ok", "ok", &validate), Ok("ok".to_string())); + assert!(accept_answer("bad", "ok", &validate).is_err()); + + // An empty answer takes the default, which must itself validate: an + // invalid (or missing) default is rejected, not taken. + assert_eq!(accept_answer("", "ok", &validate), Ok("ok".to_string())); + assert!(accept_answer("", "bad", &validate).is_err()); + assert!(accept_answer("", "", &validate).is_err()); + + // The upstream-version scenario end to end: Enter on an invalid + // probed default (offered as none) fails instead of taking it + // verbatim, while a valid default is taken. + let version = |v: &str| options::validate_upstream_version(v, 1); + assert!(accept_answer("", "1.0-2", &version).is_err()); + assert_eq!( + accept_answer("", "1.0.0", &version), + Ok("1.0.0".to_string()) + ); + } + + /// The git-derived maintainer default goes through + /// [`options::parse_maintainer`] before it is offered: an empty git + /// email yields a `Name <>` default that parses to an error and is + /// ignored, instead of being accepted verbatim and only failing late in + /// `resolve` (regression). + #[test] + fn maintainer_default_validated_by_parse_maintainer() { + assert!( + options::parse_maintainer("Jane Doe ").is_ok(), + "a well-formed identity is offered as the default" + ); + // Empty git user.email (or DEBEMAIL): `Name <>` must not parse. + assert!(options::parse_maintainer("Jane Doe <>").is_err()); + // An empty git user.name: `<>` (or ``) must not parse either. + assert!(options::parse_maintainer(" ").is_err()); + } + #[test] fn select_labels_carry_their_own_separator() { // prompt::select renders `>