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

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

Closed
bjeanes opened this issue May 27, 2022 · 3 comments

Comments

@bjeanes
Copy link
Contributor

bjeanes commented May 27, 2022

Hello :)

I have noticed a bug with rack test driver which is a significant departure from real browser behaviour when it comes to form submissions. At first, I thought this was an issue rack-test and opened rack/rack-test#301 but further digging has shown the issue to be on the Capybara side.

The bug is that when there is a hidden field named foo with value bar followed by a file field also named foo, a form submitted without attaching a file does not submit foo=bar as a parameter.

This appears to be due to NilUploadedFile, first introduced in 2989829, 12 years ago.

As best I can tell, this was to work around the fact that rack-test historically had no way to declare that the params should be encoded as multipart other than by sniffing for a Rack::Test::UploadedFile (from which NilUploadedFile descends).

rack/rack-test#303 has been opened to support a multipart key in the env to force encoding the params as multipart. Given this, I'd hope that a future release of Capybara which depends on a rack-test release with this change included would be able to drop NilUploadedFile, set :multipart according to the <form>'s enctype, and avoid the historical workaround.

Barring that, I wonder if a special-case to avoid clobbering existing keys in the params in this code path is merited? It's trading one incorrect browser behaviour (posting the wrong param values) for another (sometimes incorrectly setting the content-type as application/x-www-form-urlencoded -- though only when a hidden field is also present).

The context for why I have a field that is both a file and hidden is a submitted form which did not pass validation stores the ActiveRecord::Blob#signed_id in a hidden field to allow the user to avoid re-uploading the file to re-submit. However, I keep the file field on the page (but non-required) so they may still choose to change its value.

Prior to 2989829, it seems as though value-less file fields were omitted for the params, which is more representative of browser behaviour (other than I presume they would encode the params as multipart anyway).

Meta

Capybara Version: 3.37.1 (and probably every version prior for at least a decade)
Driver Information (and browser if relevant): rack-test

Expected Behavior

Submitting the following form fields:

<input type="hidden" name="foo" value="bar" />
<input type="file" name="foo" />

should produce params foo=bar, ideally preserving the multipart content-type---but that may be difficult without depending on a new rack-test or attaching a dummy NilUploadedFile with a non-clashing name.

Actual Behavior

foo is not present or nil.

Steps to reproduce

Submit following form fields:

<input type="hidden" name="foo" value="bar" />
<input type="file" name="foo" />
@twalpole
Copy link
Member

Not going to be special casing any parameter behavior here. If/when rack-test releases a version that doesn't require us to send the NilUploadedFile to trigger multipart submission then we will change the Capybara behavior when using that version of higher. I would suggest naming the hidden field differently than the file field - doesn't really make sense to have one field be two different types

@bjeanes
Copy link
Contributor Author

bjeanes commented May 28, 2022

doesn't really make sense to have one field be two different types

This was done in response to how Rails internally receives these params but I will workaround with a different param name.

I just wanted to raise this issue as I noticed it and rack-test driver will be improved once fixed.

@twalpole
Copy link
Member

Should be fixed by a3ff551 when using rack-test 2+ (currently the master branch of rack-test)

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

2 participants