Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form: type? raises exception for unexpected Ruby type #135

Closed
jodosha opened this issue May 3, 2016 · 4 comments
Closed

Form: type? raises exception for unexpected Ruby type #135

jodosha opened this issue May 3, 2016 · 4 comments

Comments

@jodosha
Copy link
Member

jodosha commented May 3, 2016

This is related to: #132

require 'bundler/setup'
require 'dry-validation'

form = Dry::Validation.Form do
  required(:foo).filled(:int?)
end

result = form.call('foo' => ['x'])
puts result.success?
puts result.messages[:foo].inspect

__END__

Expected:
  false
  ["must be Integer"]

Actual:
  /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/coercions/form.rb:25:in `to_int': undefined method `to_i' for ["x"]:Array (NoMethodError)
  Did you mean?  to_s
                 to_a
                 to_h
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/constructor.rb:28:in `[]'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/constructor.rb:28:in `call'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/hash/schema.rb:56:in `block in call'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/hash/schema.rb:49:in `each'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/hash/schema.rb:49:in `each_with_object'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/hash/schema.rb:49:in `call'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-types-394fa8b62966/lib/dry/types/safe.rb:11:in `call'
          from /Users/luca/.gem/ruby/2.3.1/bundler/gems/dry-validation-59fb0a09c499/lib/dry/validation/schema.rb:184:in `call'
          from /Users/luca/Code/hanami/validations/test.rb:8:in `<main>'

Please note that Schema works fine.

@timriley
Copy link
Member

timriley commented May 3, 2016

Hi Luca – the issue here is that form schemas are intended to receive input from (rack-processed) form posts only. So this means the data should be: a top-level hash with string keys whose values are either strings, similarly structured hashes, or arrays of strings or hashes.

The input you're passing here (a naked ['x'] array) doesn't conform to the type of data that form schemas expect. In your example schema, you're saying that :foo should be an int?, which (as you no doubt saw from that stacktrace) will run through this form-specific "int" type constructor, which presumes the stringy input I described above.

Since it's an unsupported use case, I think this is a case you probably don't need to include in your suite of validation tests?

@jodosha
Copy link
Member Author

jodosha commented May 4, 2016

@timriley I'm simulating form hijacking with this test. Please let me explain with an example.

We have this component on the server side:

      form = Dry::Validation.Form do
        required(:book).schema do
          required(:price).filled(:int?)
        end
      end

A malicious user forged HTML on their browser: price is NO longer a text field, but a couple of check boxes. Now it looks like this:

<form action="/books" method="POST" accept-charset="utf-8" id="book-form">
  <div>
    <input type="checkbox" name="book[price][]" value="1">
    <input type="checkbox" name="book[price][]" value="2">
  </div>

  <button type="submit">Create</button>
</form>

If they select one or more of the checkboxes and submit the form, we'll receive the following data:

{"book"=>{"price"=>["1", "2"]}}

We expected price to be a string, but now it's an array. Please note that this is similar to the test I was running with {"foo" => ["x"]}.

This scenario explode with the error reported above: NoMethodError. IMO type checking should guarantee to never explode, but to safely fail if it's not able to do the type conversion.

@solnic
Copy link
Member

solnic commented May 7, 2016

This is a bug in dry-types coercion! We blindly call to_i on a non-empty-string value. See here.

@solnic
Copy link
Member

solnic commented May 7, 2016

I'm gonna close this in favor of dry-rb/dry-types#86

@solnic solnic closed this as completed May 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants