fix: Provide path to avoid UTF-8 panics

Before, validating UTF-8 was all-or-nothing and would cause a `panic` if someone used the right API with non-UTF-8 input. Now, all arguments are validated for UTF-8, unless opted-out. This ensures a non-panicing path forward at the cost of people using the builder API that previously did `value_of_os` need to now set this flag. Fixes clap-rs#751
epage · Aug 18, 2021 · 47133d2 · 47133d2
1 parent a4137d6
commit 47133d2
Show file tree

Hide file tree

Showing 17 changed files with 564 additions and 186 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -40,6 +40,12 @@ Added `unicode_help`, `env` features.
   * **ErrorKind**
     * `ErrorKind::MissingArgumentOrSubcommand` => `ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand`
 * **Changed**
+  * `AppSettings::StrictUtf8` is now default and it and `AppSettings::AllowInvalidUtf8` are replaced by
+    * `AppSettings::AllowInvalidUtf8ForExternalSubcommands`
+      * This only applies to the subcommand args.  Before we paniced if the
+        subcommand itself was invalid but now we will report an error up to the
+        user.
+    * `ArgSettings::AllowInvalidUtf8`
   * Allowing empty values is the default again with `ArgSettings::AllowEmptyValues` changing to
     `ArgSettings::ForbidEmptyValues`
   * `AppSettings::GlobalVersion` renamed to `AppSettings::PropagateVersion` and it is not applied

diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs
@@ -228,7 +228,22 @@ pub fn gen_augment(
                     ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
                         .validator_os(|s| #func(s).map(|_: #convert_type| ()))
                     },
-                    _ => quote!(),
+                    ParserKind::FromStr
+                    | ParserKind::FromOsStr
+                    | ParserKind::FromFlag
+                    | ParserKind::FromOccurrences => quote!(),
+                };
+                let allow_invalid_utf8 = match *parser.kind {
+                    _ if attrs.is_enum() => quote!(),
+                    ParserKind::FromOsStr | ParserKind::TryFromOsStr => {
+                        quote_spanned! { func.span()=>
+                            .allow_invalid_utf8(true)
+                        }
+                    }
+                    ParserKind::FromStr
+                    | ParserKind::TryFromStr
+                    | ParserKind::FromFlag
+                    | ParserKind::FromOccurrences => quote!(),
                 };
 
                 let value_name = attrs.value_name();
@@ -250,6 +265,7 @@ pub fn gen_augment(
                             .value_name(#value_name)
                             #possible_values
                             #validator
+                            #allow_invalid_utf8
                         }
                     }
 
@@ -260,6 +276,7 @@ pub fn gen_augment(
                         .max_values(1)
                         .multiple_values(false)
                         #validator
+                            #allow_invalid_utf8
                     },
 
                     Ty::OptionVec => quote_spanned! { ty.span()=>
@@ -268,6 +285,7 @@ pub fn gen_augment(
                         .multiple_values(true)
                         .min_values(0)
                         #validator
+                            #allow_invalid_utf8
                     },
 
                     Ty::Vec => {
@@ -285,6 +303,7 @@ pub fn gen_augment(
                             .multiple_values(true)
                             #possible_values
                             #validator
+                            #allow_invalid_utf8
                         }
                     }
 
@@ -311,6 +330,7 @@ pub fn gen_augment(
                             .required(#required)
                             #possible_values
                             #validator
+                            #allow_invalid_utf8
                         }
                     }
                 };

diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
@@ -131,8 +131,34 @@ fn gen_augment(
 
             match &*kind {
                 Kind::ExternalSubcommand => {
-                    quote_spanned! { kind.span()=>
-                        let app = app.setting(clap::AppSettings::AllowExternalSubcommands);
+                    let ty = match variant.fields {
+                        Unnamed(ref fields) if fields.unnamed.len() == 1 => &fields.unnamed[0].ty,
+
+                        _ => abort!(
+                            variant,
+                            "The enum variant marked with `external_subcommand` must be \
+                             a single-typed tuple, and the type must be either `Vec<String>` \
+                             or `Vec<OsString>`."
+                        ),
+                    };
+                    match subty_if_name(ty, "Vec") {
+                        Some(subty) => {
+                            if is_simple_ty(subty, "OsString") {
+                                quote_spanned! { kind.span()=>
+                                    let app = app.setting(clap::AppSettings::AllowExternalSubcommands).setting(clap::AppSettings::AllowInvalidUtf8ForExternalSubcommands);
+                                }
+                            } else {
+                                quote_spanned! { kind.span()=>
+                                    let app = app.setting(clap::AppSettings::AllowExternalSubcommands);
+                                }
+                            }
+                        }
+
+                        None => abort!(
+                            ty.span(),
+                            "The type must be either `Vec<String>` or `Vec<OsString>` \
+                             to be used with `external_subcommand`."
+                        ),
                     }
                 }
 
@@ -348,7 +374,7 @@ fn gen_from_arg_matches(
 
                     _ => abort!(
                         variant,
-                        "The enum variant marked with `external_attribute` must be \
+                        "The enum variant marked with `external_subcommand` must be \
                          a single-typed tuple, and the type must be either `Vec<String>` \
                          or `Vec<OsString>`."
                     ),

diff --git a/clap_derive/tests/ui/external_subcommand_wrong_type.stderr b/clap_derive/tests/ui/external_subcommand_wrong_type.stderr
@@ -10,7 +10,7 @@ error: The type must be either `Vec<String>` or `Vec<OsString>` to be used with
 13 |     Other(String),
    |           ^^^^^^
 
-error: The enum variant marked with `external_attribute` must be a single-typed tuple, and the type must be either `Vec<String>` or `Vec<OsString>`.
+error: The enum variant marked with `external_subcommand` must be a single-typed tuple, and the type must be either `Vec<String>` or `Vec<OsString>`.
   --> $DIR/external_subcommand_wrong_type.rs:18:5
    |
 18 | /     #[clap(external_subcommand)]

diff --git a/clap_derive/tests/utf8.rs b/clap_derive/tests/utf8.rs
@@ -0,0 +1,226 @@
+#![cfg(not(windows))]
+
+use clap::{Clap, ErrorKind};
+use std::ffi::OsString;
+use std::os::unix::ffi::OsStringExt;
+
+#[derive(Clap, Debug, PartialEq, Eq)]
+struct Positional {
+    arg: String,
+}
+
+#[derive(Clap, Debug, PartialEq, Eq)]
+struct Named {
+    #[clap(short, long)]
+    arg: String,
+}
+
+#[test]
+fn invalid_utf8_strict_positional() {
+    let m = Positional::try_parse_from(vec![OsString::from(""), OsString::from_vec(vec![0xe9])]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn invalid_utf8_strict_option_short_space() {
+    let m = Named::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("-a"),
+        OsString::from_vec(vec![0xe9]),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn invalid_utf8_strict_option_short_equals() {
+    let m = Named::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x61, 0x3d, 0xe9]),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn invalid_utf8_strict_option_short_no_space() {
+    let m = Named::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x61, 0xe9]),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn invalid_utf8_strict_option_long_space() {
+    let m = Named::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("--arg"),
+        OsString::from_vec(vec![0xe9]),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn invalid_utf8_strict_option_long_equals() {
+    let m = Named::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x2d, 0x61, 0x72, 0x67, 0x3d, 0xe9]),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[derive(Clap, Debug, PartialEq, Eq)]
+struct PositionalOs {
+    #[clap(parse(from_os_str))]
+    arg: OsString,
+}
+
+#[derive(Clap, Debug, PartialEq, Eq)]
+struct NamedOs {
+    #[clap(short, long, parse(from_os_str))]
+    arg: OsString,
+}
+
+#[test]
+fn invalid_utf8_positional() {
+    let r = PositionalOs::try_parse_from(vec![OsString::from(""), OsString::from_vec(vec![0xe9])]);
+    assert_eq!(
+        r.unwrap(),
+        PositionalOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[test]
+fn invalid_utf8_option_short_space() {
+    let r = NamedOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("-a"),
+        OsString::from_vec(vec![0xe9]),
+    ]);
+    assert_eq!(
+        r.unwrap(),
+        NamedOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[test]
+fn invalid_utf8_option_short_equals() {
+    let r = NamedOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x61, 0x3d, 0xe9]),
+    ]);
+    assert_eq!(
+        r.unwrap(),
+        NamedOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[test]
+fn invalid_utf8_option_short_no_space() {
+    let r = NamedOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x61, 0xe9]),
+    ]);
+    assert_eq!(
+        r.unwrap(),
+        NamedOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[test]
+fn invalid_utf8_option_long_space() {
+    let r = NamedOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("--arg"),
+        OsString::from_vec(vec![0xe9]),
+    ]);
+    assert_eq!(
+        r.unwrap(),
+        NamedOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[test]
+fn invalid_utf8_option_long_equals() {
+    let r = NamedOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0x2d, 0x2d, 0x61, 0x72, 0x67, 0x3d, 0xe9]),
+    ]);
+    assert_eq!(
+        r.unwrap(),
+        NamedOs {
+            arg: OsString::from_vec(vec![0xe9])
+        }
+    );
+}
+
+#[derive(Debug, PartialEq, Clap)]
+enum External {
+    #[clap(external_subcommand)]
+    Other(Vec<String>),
+}
+
+#[test]
+fn refuse_invalid_utf8_subcommand_with_allow_external_subcommands() {
+    let m = External::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from_vec(vec![0xe9]),
+        OsString::from("normal"),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[test]
+fn refuse_invalid_utf8_subcommand_args_with_allow_external_subcommands() {
+    let m = External::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("subcommand"),
+        OsString::from("normal"),
+        OsString::from_vec(vec![0xe9]),
+        OsString::from("--another_normal"),
+    ]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::InvalidUtf8);
+}
+
+#[derive(Debug, PartialEq, Clap)]
+enum ExternalOs {
+    #[clap(external_subcommand)]
+    Other(Vec<OsString>),
+}
+
+#[test]
+fn allow_invalid_utf8_subcommand_args_with_allow_external_subcommands() {
+    let m = ExternalOs::try_parse_from(vec![
+        OsString::from(""),
+        OsString::from("subcommand"),
+        OsString::from("normal"),
+        OsString::from_vec(vec![0xe9]),
+        OsString::from("--another_normal"),
+    ]);
+    assert_eq!(
+        m.unwrap(),
+        ExternalOs::Other(vec![
+            OsString::from("subcommand"),
+            OsString::from("normal"),
+            OsString::from_vec(vec![0xe9]),
+            OsString::from("--another_normal"),
+        ])
+    );
+}
diff --git a/src/build/app/debug_asserts.rs b/src/build/app/debug_asserts.rs
@@ -260,6 +260,7 @@ pub(crate) fn assert_app(app: &App) {
     detect_duplicate_flags(&short_flags, "short");
 
     app._panic_on_missing_help(app.g_settings.is_set(AppSettings::HelpRequired));
+    assert_app_flags(app);
 }
 
 fn detect_duplicate_flags(flags: &[Flag], short_or_long: &str) {
@@ -301,3 +302,27 @@ fn find_duplicates<T: PartialEq>(slice: &[T]) -> impl Iterator<Item = (&T, &T)>
         }
     })
 }
+
+fn assert_app_flags(app: &App) {
+    use AppSettings::*;
+
+    macro_rules! checker {
+        ($a:ident requires $($b:ident)|+) => {
+            if app.is_set($a) {
+                let mut s = String::new();
+
+                $(
+                    if !app.is_set($b) {
+                        s.push_str(&format!("\nAppSettings::{} is required when AppSettings::{} is set.\n", std::stringify!($b), std::stringify!($a)));
+                    }
+                )+
+
+                if !s.is_empty() {
+                    panic!("{}", s)
+                }
+            }
+        }
+    }
+
+    checker!(AllowInvalidUtf8ForExternalSubcommands requires AllowExternalSubcommands);
+}