dry-rb / dry-validation

Validation library with type-safe schemas and rules
https://dry-rb.org/gems/dry-validation
MIT License
1.33k stars 187 forks source link

Form: whitelisting fails with non string keys/values #143

Closed jodosha closed 8 years ago

jodosha commented 8 years ago
form = Dry::Validation.Form { required(:id).maybe }
puts form.call('id' => '23', 'foo' => '1').output # => {:id=>"23"} # SUCCESS
puts form.call('id' => 23, 'foo' => 1).output     # => {"id"=>23, "foo"=>1} # FAILURE - returned keys are strings, it didn't filtered "foo"
puts form.call(id: 23, foo: 1).output             # => {:id=>23, :foo=>1} # FAILURE - it didn't filtered "foo"

By talking with @AMHOL in chat, I've got the impression that Form only expects strings as keys and values because of Rack protocol. However there are cases where accepting different types as input is helpful.

Router Params

For a given route GET /books/:id, a router can return a set of params as a symbolized hash. This is the case of hanami-router, for the route above it would return {:id => "23"}.

Rack Middleware

A Rack middleware can set a non-string value (eg. a Time instance), this would cause a whitelisting failure as well.

AMHOL commented 8 years ago

@jodosha turns out I was wrong on this, form schemas were updated to accept symbol keys a while back, and it works if you specify a type coercion i.e.

form = Dry::Validation.Form { required(:id).maybe(:int?) }
form.call('id' => '23', 'foo' => '1').output
# => {:id=>23}
form.call('id' => 23, 'foo' => 1).output
# => {:id=>23}
form.call(id: 23, foo: 1).output
# => {:id=>23}
jodosha commented 8 years ago

Why does it works only if you specify a type?

http://lucaguidi.com

On 06/mag/2016, at 16:26, Andy Holland notifications@github.com wrote:

@jodosha turns out I was wrong on this, form schemas were updated to accept symbol keys a while back, and it works if you specify a type coercion i.e.

form = Dry::Validation.Form { required(:id).maybe(:int?) } form.call('id' => '23', 'foo' => '1').output

=> {:id=>23}

form.call('id' => 23, 'foo' => 1).output

=> {:id=>23}

form.call(id: 23, foo: 1).output

=> {:id=>23}

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

AMHOL commented 8 years ago

I think the coercion is inferred from the value rules, not 100% sure on this, but Solnic will clear it up when he's back

solnic commented 8 years ago

We rely on type expectations in order to set specific type for dry-type hash schema, we could default to string in Form though. It's somewhat related to #145 where I suggested adding an explicit interface for type expectations. I think we could get rid of rather complex input-processor compiler and simply gather info about the keys + potential types and just use that for inferring input processors. So ie required(:foo).type(:int) in a form schema would simply configure { foo: Dry::Types['form.int'] } as the schema type map and that's all. We can simply aggregate this under @type_map in value DSL objects and when configuring a schema class we could set it for each schema class. For nested schemas we'd simply merge existing type maps and call it a day.

This would solve a couple of issues - first of all people won't be confused about implicit behavior of inferring coercions from predicates that check types, it'd be an explicit interface for setting type and its predicate, it's important to make it the very first check that is applied and this interface would guarantee that. Secondly, we could completely remove input processor compilers, which is already quite a bit of LoC.

jodosha commented 8 years ago

@solnic Thanks for your reply. I really appreciate you took the time to respond during your OSS vacation.

My question is still open and you folks maybe share some knowledge I'm missing 😉 , which is good, I think. So the question is: what whitelisting has to do with types?

For me whitelisting is logic focused on keys: do a user wants this key to pass or not? On the other hand, type coercion is related to values: do a user defined a Ruby type?

Hence my confusion 😄


Speaking of whitelisting and key related logic, why the following syntax doesn't work?

form = Dry::Validation.Form { required(:id) }
puts form.call('id' => '23').output

# Actual: {}
# Expected: {:id => "23"}

This is confusing as well: I define a key, but it isn't included in the output. Why do I need at least to modify with maybe to make it work? Again, maybe is related to values, not keys. Shall we make this syntax to work as well?

/cc @AMHOL

solnic commented 8 years ago

We need types because coercion hash schemas are used for input processors and they need type specs. We can have plain whitelisting though but I gotta say I prefer to have types always specified. Enforcing types is one of the core aspects of dry-validation but I could be missing some use cases where not having types defined is useful (who knows?).

ifokeev commented 8 years ago

1

I'm really confused. As you can see, different output. I have no idea how to get correct output :(

Also form = Dry::Validation.Form { required(:id) } raises NoMethodError

solnic commented 8 years ago

@ifokeev paste your schema pls and remember that required(:id) is not enough, you need to specify rules for the value (right-side)

solnic commented 8 years ago

FYI this is fixed on master, here's the output:

{:id=>"23"}
{:id=>23}
{:id=>23}