Disallow defaults in strict forms.

Partially resolves #1536.
This commit is contained in:
Sergio Benitez 2021-03-11 02:03:13 -08:00
parent ef7b7a953e
commit e532f4e2b3
8 changed files with 104 additions and 80 deletions

View File

@ -247,11 +247,13 @@ pub fn derive_from_form(input: proc_macro::TokenStream) -> TokenStream {
let _err = _Err; let _err = _Err;
Ok(quote_spanned! { ty.span() => { Ok(quote_spanned! { ty.span() => {
let _name = #name_view; let _name = #name_view;
let _opts = __c.__opts;
__c.#ident __c.#ident
.map(<#ty as #_form::FromForm<'__f>>::finalize) .map(<#ty as #_form::FromForm<'__f>>::finalize)
.unwrap_or_else(|| <#ty as #_form::FromForm<'__f>>::default() .unwrap_or_else(|| {
<#ty as #_form::FromForm<'__f>>::default(_opts)
.ok_or_else(|| #_form::ErrorKind::Missing.into()) .ok_or_else(|| #_form::ErrorKind::Missing.into())
) })
.and_then(|#ident| { .and_then(|#ident| {
let mut _es = #_form::Errors::new(); let mut _es = #_form::Errors::new();
#(if let #_err(_e) = #validator { _es.extend(_e); })* #(if let #_err(_e) = #validator { _es.extend(_e); })*

View File

@ -1,6 +1,4 @@
#[macro_use]extern crate rocket; use rocket::form::{Form, Strict, FromForm, FromFormField, Errors};
use rocket::form::{Form, Strict, FromForm, Errors};
fn strict<'f, T: FromForm<'f>>(string: &'f str) -> Result<T, Errors<'f>> { fn strict<'f, T: FromForm<'f>>(string: &'f str) -> Result<T, Errors<'f>> {
Form::<Strict<T>>::parse(string).map(|s| s.into_inner()) Form::<Strict<T>>::parse(string).map(|s| s.into_inner())
@ -135,13 +133,13 @@ fn base_conditions() {
})); }));
// Check that a `bool` value that isn't in the form is marked as `false`. // Check that a `bool` value that isn't in the form is marked as `false`.
let manual: Option<UnpresentCheckbox> = strict("").ok(); let manual: Option<UnpresentCheckbox> = lenient("").ok();
assert_eq!(manual, Some(UnpresentCheckbox { assert_eq!(manual, Some(UnpresentCheckbox {
checkbox: false checkbox: false
})); }));
// Check that a `bool` value that isn't in the form is marked as `false`. // Check that a `bool` value that isn't in the form is marked as `false`.
let manual: Option<UnpresentCheckboxTwo<'_>> = strict("something=hello").ok(); let manual: Option<UnpresentCheckboxTwo<'_>> = lenient("something=hello").ok();
assert_eq!(manual, Some(UnpresentCheckboxTwo { assert_eq!(manual, Some(UnpresentCheckboxTwo {
checkbox: false, checkbox: false,
something: "hello".into() something: "hello".into()
@ -152,7 +150,6 @@ fn base_conditions() {
assert_eq!(manual, Some(FieldNamedV { assert_eq!(manual, Some(FieldNamedV {
v: "abc".into() v: "abc".into()
})); }));
} }
#[test] #[test]
@ -343,63 +340,65 @@ fn form_errors() {
let errors = strict::<WhoopsForm>("complete=true&other=unknown").unwrap_err(); let errors = strict::<WhoopsForm>("complete=true&other=unknown").unwrap_err();
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
"other" == e.name.as_ref().unwrap() e.name.as_ref().unwrap() == "other"
&& Some("unknown") == e.value.as_deref() && e.value.as_deref() == Some("unknown")
&& match e.kind { && matches!(e.kind, ErrorKind::Int(..))
ErrorKind::Int(..) => true,
_ => false
}
})); }));
let errors = strict::<WhoopsForm>("complete=unknown&other=unknown").unwrap_err(); let errors = strict::<WhoopsForm>("complete=unknown&other=unknown").unwrap_err();
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
e.name.as_ref().unwrap() == "complete" "complete" == e.name.as_ref().unwrap()
&& Some("unknown") == e.value.as_deref() && e.value.as_deref() == Some("unknown")
&& match e.kind { && matches!(e.kind, ErrorKind::Bool(..))
ErrorKind::Bool(..) => true,
_ => false
}
})); }));
let errors = strict::<WhoopsForm>("complete=true&other=1&extra=foo").unwrap_err(); let errors = strict::<WhoopsForm>("complete=true&other=1&extra=foo").unwrap_err();
dbg!(&errors);
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
"extra" == e.name.as_ref().unwrap() e.name.as_ref().unwrap() == "extra"
&& Some("foo") == e.value.as_deref() && e.value.as_deref() == Some("foo")
&& match e.kind { && matches!(e.kind, ErrorKind::Unexpected)
ErrorKind::Unexpected => true,
_ => false
}
})); }));
let errors = strict::<WhoopsForm>("complete=unknown&unknown=!").unwrap_err(); let errors = strict::<WhoopsForm>("complete=unknown&unknown=!").unwrap_err();
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
"complete" == e.name.as_ref().unwrap() e.name.as_ref().unwrap() == "complete"
&& Some("unknown") == e.value.as_deref() && e.value.as_deref() == Some("unknown")
&& match e.kind { && matches!(e.kind, ErrorKind::Bool(..))
ErrorKind::Bool(..) => true,
_ => false
}
})); }));
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
"unknown" == e.name.as_ref().unwrap() e.name.as_ref().unwrap() == "unknown"
&& Some("!") == e.value.as_deref() && e.value.as_deref() == Some("!")
&& match e.kind { && matches!(e.kind, ErrorKind::Unexpected)
ErrorKind::Unexpected => true, }));
_ => false
} let errors = strict::<WhoopsForm>("unknown=!").unwrap_err();
assert!(errors.iter().any(|e| {
e.name.as_ref().unwrap() == "unknown"
&& e.value.as_deref() == Some("!")
&& matches!(e.kind, ErrorKind::Unexpected)
}));
assert!(errors.iter().any(|e| {
e.name.as_ref().unwrap() == "complete"
&& e.value.is_none()
&& e.entity == Entity::Field
&& matches!(e.kind, ErrorKind::Missing)
}));
assert!(errors.iter().any(|e| {
e.name.as_ref().unwrap() == "other"
&& e.value.is_none()
&& e.entity == Entity::Field
&& matches!(e.kind, ErrorKind::Missing)
})); }));
let errors = strict::<WhoopsForm>("complete=true").unwrap_err(); let errors = strict::<WhoopsForm>("complete=true").unwrap_err();
assert!(errors.iter().any(|e| { assert!(errors.iter().any(|e| {
"other" == e.name.as_ref().unwrap() e.name.as_ref().unwrap() == "other"
&& e.value.is_none() && e.value.is_none()
&& e.entity == Entity::Field && e.entity == Entity::Field
&& match e.kind { && matches!(e.kind, ErrorKind::Missing)
ErrorKind::Missing => true,
_ => false
}
})); }));
} }

View File

@ -156,9 +156,4 @@ impl<'v, T: FromForm<'v>> FromForm<'v> for Contextual<'v, T> {
Ok(Contextual { value, context }) Ok(Contextual { value, context })
} }
fn default() -> Option<Self> {
Self::finalize(Self::init(Options::Lenient)).ok()
}
} }

View File

@ -387,10 +387,10 @@ pub trait FromForm<'r>: Send + Sized {
/// Returns a default value, if any, to use when a value is desired and /// Returns a default value, if any, to use when a value is desired and
/// parsing fails. /// parsing fails.
/// ///
/// The default implementation initializes `Self` with lenient options and /// The default implementation initializes `Self` with `opts` and finalizes
/// finalizes immediately, returning the value if finalization succeeds. /// immediately, returning the value if finalization succeeds.
fn default() -> Option<Self> { fn default(opts: Options) -> Option<Self> {
Self::finalize(Self::init(Options::Lenient)).ok() Self::finalize(Self::init(opts)).ok()
} }
} }

View File

@ -257,9 +257,9 @@ impl<'v, T: FromFormField<'v>> FromFieldContext<'v, T> {
} }
fn push(&mut self, name: NameView<'v>, result: Result<'v, T>) { fn push(&mut self, name: NameView<'v>, result: Result<'v, T>) {
let is_unexpected = |e: &Errors<'_>| e.last().map_or(false, |e| { fn is_unexpected(e: &Errors<'_>) -> bool {
if let ErrorKind::Unexpected = e.kind { true } else { false } matches!(e.last().map(|e| &e.kind), Some(ErrorKind::Unexpected))
}); }
self.field_name = Some(name); self.field_name = Some(name);
match result { match result {
@ -299,12 +299,13 @@ impl<'v, T: FromFormField<'v>> FromForm<'v> for T {
fn finalize(ctxt: Self::Context) -> Result<'v, Self> { fn finalize(ctxt: Self::Context) -> Result<'v, Self> {
let mut errors = match ctxt.value { let mut errors = match ctxt.value {
Some(Ok(val)) if !ctxt.opts.strict || ctxt.pushes <= 1 => return Ok(val), Some(Ok(val)) if !ctxt.opts.strict || ctxt.pushes <= 1 => return Ok(val),
Some(Err(e)) => e,
Some(Ok(_)) => Errors::from(ErrorKind::Duplicate), Some(Ok(_)) => Errors::from(ErrorKind::Duplicate),
None => match <T as FromFormField>::default() { Some(Err(errors)) => errors,
None if !ctxt.opts.strict => match <T as FromFormField>::default() {
Some(default) => return Ok(default), Some(default) => return Ok(default),
None => Errors::from(ErrorKind::Missing) None => Errors::from(ErrorKind::Missing)
} },
None => Errors::from(ErrorKind::Missing),
}; };
if let Some(name) = ctxt.field_name { if let Some(name) = ctxt.field_name {
@ -362,7 +363,9 @@ impl<'v> FromFormField<'v> for Capped<String> {
impl_strict_from_form_field_from_capped!(String); impl_strict_from_form_field_from_capped!(String);
impl<'v> FromFormField<'v> for bool { impl<'v> FromFormField<'v> for bool {
fn default() -> Option<Self> { Some(false) } fn default() -> Option<Self> {
Some(false)
}
fn from_value(field: ValueField<'v>) -> Result<'v, Self> { fn from_value(field: ValueField<'v>) -> Result<'v, Self> {
match field.value.as_uncased() { match field.value.as_uncased() {

View File

@ -128,7 +128,8 @@
//! This implementation is complete except for the following details: //! This implementation is complete except for the following details:
//! //!
//! * not being pseudocode, of course //! * not being pseudocode, of course
//! * checking for duplicate pushes when paring is requested as `strict` //! * checking for duplicate pushes when parsing is `strict`
//! * disallowing defaults when parsing is `strict`
//! * tracking the field's name and value to generate a complete `Error` //! * tracking the field's name and value to generate a complete `Error`
//! //!
//! See [`FromForm`] for full details on push-parsing and a complete example. //! See [`FromForm`] for full details on push-parsing and a complete example.

View File

@ -9,7 +9,8 @@ use crate::http::uri::{Query, FromUriParam};
/// generic parameter to the [`Form`] data guard: `Form<Strict<T>>`, where `T` /// generic parameter to the [`Form`] data guard: `Form<Strict<T>>`, where `T`
/// implements `FromForm`. Unlike using `Form` directly, this type uses a /// implements `FromForm`. Unlike using `Form` directly, this type uses a
/// _strict_ parsing strategy: forms that contains a superset of the expected /// _strict_ parsing strategy: forms that contains a superset of the expected
/// fields (i.e, extra fields) will fail to parse. /// fields (i.e, extra fields) will fail to parse and defaults will not be use
/// for missing fields.
/// ///
/// # Strictness /// # Strictness
/// ///
@ -38,6 +39,20 @@ use crate::http::uri::{Query, FromUriParam};
/// format!("Your value: {}", user_input.value) /// format!("Your value: {}", user_input.value)
/// } /// }
/// ``` /// ```
///
/// `Strict` can also be used to make individual fields strict while keeping the
/// overall structure and remaining fields lenient:
///
/// ```rust
/// # #[macro_use] extern crate rocket;
/// use rocket::form::{Form, Strict};
///
/// #[derive(FromForm)]
/// struct UserInput {
/// required: Strict<bool>,
/// uses_default: bool
/// }
/// ```
#[derive(Debug)] #[derive(Debug)]
pub struct Strict<T>(T); pub struct Strict<T>(T);

View File

@ -716,11 +716,8 @@ parse or is simply invalid, a customizable error is returned. As before, a
forward or failure can be caught by using the `Option` and `Result` types: forward or failure can be caught by using the `Option` and `Result` types:
```rust ```rust
# #[macro_use] extern crate rocket; # use rocket::{post, form::Form};
# fn main() {} # type Task = String;
# use rocket::form::Form;
# #[derive(FromForm)] struct Task { complete: bool }
#[post("/todo", data = "<task>")] #[post("/todo", data = "<task>")]
fn new(task: Option<Form<Task>>) { /* .. */ } fn new(task: Option<Form<Task>>) { /* .. */ }
@ -732,11 +729,12 @@ fn new(task: Option<Form<Task>>) { /* .. */ }
### Strict Parsing ### Strict Parsing
Rocket's `FromForm` parsing is _lenient_ by default: a `Form<T>` will parse Rocket's `FromForm` parsing is _lenient_ by default: a `Form<T>` will parse
successfully from an incoming form even if it contains extra or duplicate successfully from an incoming form even if it contains extra, duplicate, or
fields. The extras or duplicates are ignored -- no validation or parsing of the missing fields. Extras or duplicates are ignored -- no validation or parsing of
fields occurs. To change this behavior and make form parsing _strict_, use the the fields occurs -- and missing fields are filled with defaults when available.
[`Form<Strict<T>>`] data type, which errors if there are any extra, undeclared To change this behavior and make form parsing _strict_, use the
fields. [`Form<Strict<T>>`] data type, which emits errors if there are any extra or
missing fields, irrespective of defaults.
You can use a `Form<Strict<T>>` anywhere you'd use a `Form<T>`. Its generic You can use a `Form<Strict<T>>` anywhere you'd use a `Form<T>`. Its generic
parameter is also required to implement `FromForm`. For instance, we can simply parameter is also required to implement `FromForm`. For instance, we can simply
@ -744,21 +742,32 @@ replace `Form<T>` with `Form<Strict<T>>` above to get strict parsing:
```rust ```rust
# #[macro_use] extern crate rocket; # #[macro_use] extern crate rocket;
# fn main() {}
use rocket::form::{Form, Strict}; use rocket::form::{Form, Strict};
#[derive(FromForm)] # #[derive(FromForm)] struct Task { complete: bool, description: String, }
struct Task {
/* .. */
# complete: bool,
# description: String,
}
#[post("/todo", data = "<task>")] #[post("/todo", data = "<task>")]
fn new(task: Form<Strict<Task>>) { /* .. */ } fn new(task: Form<Strict<Task>>) { /* .. */ }
``` ```
`Strict` can also be used to make individual fields strict while keeping the
overall structure and remaining fields lenient:
```rust
# #[macro_use] extern crate rocket;
# use rocket::form::{Form, Strict};
#[derive(FromForm)]
struct Input {
required: Strict<bool>,
uses_default: bool
}
#[post("/", data = "<input>")]
fn new(input: Form<Input>) { /* .. */ }
```
[`Form<Strict<T>>`]: @api/rocket/form/struct.Strict.html [`Form<Strict<T>>`]: @api/rocket/form/struct.Strict.html
### Field Renaming ### Field Renaming