Issues with hidden field + file field #301

bjeanes · 2022-05-26T08:35:05Z

Hi Jeremy, hope you're well.

Today I noticed a discrepancy between rack-test (1.1.0) and a real browser, which led me to #278 as possibly related, but trying latest GitHub version causes a separate error. I'll describe both and then we can see if they should be split up.

The primary issue is the combination of a hidden field and a file field of the same param name. In a real browser, if the file field (which is after the hidden field in the DOM) is left blank, the hidden field string content is posted under that parameter name. If the file field is filled out, the hidden field param value is absent and a multipart part for the file is submitted with that param name.

In rack-test however, a hidden field and a subsequent empty file field which share a name results in an empty param for that name.

This led me to #278 as possibly related, so I upgraded to 2988e7e. Unfortunately, this version raises the following error for any unfilled file field:

     Failure/Error: click_button 'Submit'

     NoMethodError:
       undefined method `set_encoding' for nil:NilClass

               tempfile.public_send(method_name, *args, &block)
                       ^^^^^^^^^^^^
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/uploaded_file.rb:49:in `public_send'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/uploaded_file.rb:49:in `method_missing'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:129:in `build_file_part'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:105:in `block (2 levels) in get_parts'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:103:in `map'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:103:in `block in get_parts'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:95:in `each'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:95:in `map'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:95:in `get_parts'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:90:in `build_parts'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test/utils.rb:51:in `build_multipart'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test.rb:304:in `env_for'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test.rb:156:in `custom_request'
     # /bundle/ruby/3.1.0/bundler/gems/rack-test-2988e7edfbdb/lib/rack/test.rb:106:in `post'

In 1.1.0, an empty field is not part of the params at all. However, it also seems to erase a non-empty hidden field of the same name.

The text was updated successfully, but these errors were encountered:

jeremyevans · 2022-05-26T14:14:02Z

Thanks for the report. I think the check before set_encoding was removed because there was no test for it, and it appeared to be related to Ruby 1.8 support, checking if the object responded to set_encoding. I'll add an explicit check for nil.

jeremyevans · 2022-05-26T14:49:03Z

I looked into this and I'm not sure how it happens. The error message and failing line shows that tempfile is nil, but tempfile is set in the initializer to a StringIO or Tempfile instance, and never set to nil. So I have no idea how it becomes nil.

I'll keep looking, but this would be much easier if you could provide the arguments you passed to post.

jeremyevans · 2022-05-26T15:25:31Z

I'm guessing this is using Capybara, so I tried to build a simple app that uses capybara. I couldn't replicate the set_encoding failure are seeing:

require 'capybara'
require 'capybara/dsl'
require 'rack/test'
require 'minitest/autorun'

Capybara.app = lambda do |env|
  [200, {}, [env['REQUEST_METHOD'] == 'POST' ? env['rack.input'].read : (<<END)]]
<!DOCTYPE html>
<html>
<body>
<form method="POST">
<input type="hidden" name="file" value="none" />
<input type="file" name="file" />
<input type="submit" value="Submit" />
</form>
</body>
</html>
END
end

class Minitest::Spec
  include Rack::Test::Methods
  include Capybara::DSL

  def teardown
    Capybara.reset_sessions!
    Capybara.use_default_driver
  end

  def test_works
    visit '/'
    click_button 'Submit'
    assert_equal 'file=', page.html
  end
end

I do see that the empty file input takes precedence over the previous hidden input, and that may differ from browser behavior, but I'm guessing that is a capybara issue, not a rack-test issue, as rack-test does no DOM parsing, it just uses the arguments you pass to the get and post methods. Most likely, the issue is the arguments that capybara passes to rack-test that is causing the issue, and not anything in rack-test itself. I checked that by printing the arguments passed to post:

["http://www.example.com/", {"file"=>""}, {"HTTP_REFERER"=>"http://www.example.com/"}]

So that issue does appear to be confirmed as a capybara issue and not a rack-test issue.

If you can provide a reproducer for the set_encoding issue, I would greatly appreciate it.

jeremyevans · 2022-05-26T15:28:02Z

Looks like I spoke to soon. Needed to add enctype="multipart/form-data" to the example, then I was able to reproduce. A lot of this time could have been saved if a reproducible example was provided up front. Anyway, now that I'm able to reproduce, I should be able to fix it.

jeremyevans · 2022-05-26T15:30:55Z

Actually, this is a bug in Capybara, as I can see from the arguments it is passing to post:

["http://www.example.com/", {"file"=>#<Capybara::RackTest::Form::NilUploadedFile:0x00000450288e8858 @empty_file=#<Tempfile:/tmp/nil_uploaded_file20220526-31449-9h5mdj (closed)>>}, {"HTTP_REFERER"=>"http://www.example.com/"}]

Capybara is passing an instance of it's own class that does not mirror the UploadedFile API. That's a horrible idea. However, since one of the goals of rack-test is to remain compatible with Capybara, I will add back the related code checking for set_encoding.

…File Capybara subclasses Rack::Test::UploadedFile, overrides initialize and redefines a bunch of methods. This is a terrible approach, but as one of the goals is to keep capybara working with rack-test, this hack is needed for compatibility. Note that Capybara master already defines set_encoding and append_to, so this is only needed for Capybara 3.37.1 and older. Fixes rack#301

jeremyevans · 2022-05-26T16:15:42Z

I've added a pull request to fix this, so that rack-test 2.0.0 will work with older versions of Capybara (fixing the set_encoding issue). Note that Capybara master already had related fixes: teamcapybara/capybara@82138ab

You'll still need to contact Capybara if you want them to make it so unfilled file inputs are ignored, so previous hidden inputs can take precedence. However, there must be a reason they are using Capybara::RackTest::Form::NilUploadedFile and not ignoring file inputs without attached files, so I would not anticipate a positive reception.

bjeanes · 2022-05-26T22:08:20Z

A lot of this time could have been saved if a reproducible example was provided up front.

Apologies. It was my intent to prepare one if needed but didn't do so preemptively as it was well into the evening for me.

You'll still need to contact Capybara if you want them to make it so unfilled file inputs are ignored, so previous hidden inputs can take precedence. However, there must be a reason they are using Capybara::RackTest::Form::NilUploadedFile and not ignoring file inputs without attached files, so I would not anticipate a positive reception.

Thank you. I'll reach out there. Because this works in capybara drivers which execute a real browser I jumped to the conclusion that it might be rack-test. I should have considered it could be the capybara driver later itself for rack-test.

I appreciate the time and attention you gave the issue.

bjeanes · 2022-05-27T00:12:06Z

Wow it looks like this issue is potentially 12 years old -- teamcapybara/capybara@2989829

That commit changed the behaviour from not submitting an unfilled file field to the current state of posting the weird NilUploadFile. The comment on that class (to this day) implies it's working around the way rack-test determines whether or not to build a multipart request.

That seems to be referring to

rack-test/lib/rack/test/utils.rb

Lines 32 to 51 in cf9d2e0

    
           def build_multipart(params, _first = true, multipart = false) 
        
             raise ArgumentError, 'value must be a Hash' unless params.is_a?(Hash) 
        
             unless multipart 
        
               query = lambda { |value| 
        
                 case value 
        
                 when Array 
        
                   value.each(&query) 
        
                 when Hash 
        
                   value.values.each(&query) 
        
                 when UploadedFile 
        
                   multipart = true 
        
                 end 
        
               } 
        
               params.values.each(&query) 
        
               return nil unless multipart 
        
             end 
        
             build_parts(_build_multipart(params, true)) 
        
           end

(specifically L42) which seems to only be called here, which does not supply the multipart value at all:

rack-test/lib/rack/test.rb

Line 304 in cf9d2e0

if data = build_multipart(params)

jeremyevans · 2022-05-27T04:55:08Z

Should be fairly easy to make env_for accept an additional env value :multipart to force a multipart request. That way Capybara could use that instead of the terrible NilUploadFile hack. I've added a pull request for that: #303.

bjeanes · 2022-05-27T04:57:36Z

Yeah that looks pretty straight forward. I plan to chase up an issue with Capybara later today so I'll reference that issue. However, given rack-test hasn't had a release in 3ish years, I think I'll need to find an alternate workaround anyway.

jeremyevans · 2022-05-27T04:59:55Z

Release date for rack-test should be pretty soon. Looks like capybara already works with it. With both Capybara and Rails confirmed to be working, a release can happen shortly, maybe as early as next week.

bjeanes · 2022-05-27T05:03:02Z

Good to know. I'll see about making a case for fixing this hidden field issue as part of any potential Capybara release then, but that would probably require them to depend on the new rack-test version as a minimum.

bjeanes · 2022-05-27T05:23:42Z

I have opened teamcapybara/capybara#2556

jeremyevans closed this as completed in cf9d2e0 May 26, 2022

bjeanes mentioned this issue May 27, 2022

With rack test driver, unfilled file fields clobber hidden fields of same name, unlike real browser teamcapybara/capybara#2556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues with hidden field + file field #301

Issues with hidden field + file field #301

bjeanes commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

bjeanes commented May 26, 2022

bjeanes commented May 27, 2022

jeremyevans commented May 27, 2022

bjeanes commented May 27, 2022

jeremyevans commented May 27, 2022

bjeanes commented May 27, 2022

bjeanes commented May 27, 2022

Issues with hidden field + file field #301

Issues with hidden field + file field #301

Comments

bjeanes commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

jeremyevans commented May 26, 2022

bjeanes commented May 26, 2022

bjeanes commented May 27, 2022

jeremyevans commented May 27, 2022

bjeanes commented May 27, 2022

jeremyevans commented May 27, 2022

bjeanes commented May 27, 2022

bjeanes commented May 27, 2022