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

Empty multipart form data raises ActionController::BadRequest error since Rails 7.0.5 #51292

Closed
JunichiIto opened this issue Mar 11, 2024 · 9 comments

Comments

@JunichiIto
Copy link
Contributor

Overview

Axios can post empty form data as multipart/form-data:

$(function() {
  $('.hi-button').click(function() {
    // formData contents are dynamically created. Sometimes it can be empty.
    const formData = new FormData();
    axios.post('/hello', formData).then((response) => {
      $('.message').show();
    })
  })
});

The code above will request the following form data:

------WebKitFormBoundaryXGYtA42dcjDzyZOR--

Screenshot 2024-03-11 at 11 00 23

Rails 7.0.4.3 was able to process such data, but Rails 7.0.5 raises ActionController::BadRequest error.

I created a sample app to reproduce this error:
https://github.com/JunichiIto/empty-multipart-sandbox

Steps to reproduce

Please clone my sample app: https://github.com/JunichiIto/empty-multipart-sandbox

The main branch is running on Rails 7.0.4.3 and tests will pass:

$ rails test:all
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 45860

# Running:

2024-03-11 10:39:24 INFO Selenium [:logger_info] Details on how to use and modify Selenium logger:
  https://selenium.dev/documentation/webdriver/troubleshooting/logging

2024-03-11 10:39:24 WARN Selenium [:capabilities] [DEPRECATION] The :capabilities parameter for Selenium::WebDriver::Chrome::Driver is deprecated. Use :options argument with an instance of Selenium::WebDriver::Chrome::Driver instead. 
Capybara starting Puma...
* Version 5.6.8, codename: Birdie's Version
* Min threads: 0, max threads: 4
* Listening on http://127.0.0.1:60990
Bad request content body
.

Finished in 1.322483s, 0.7562 runs/s, 2.2685 assertions/s.
1 runs, 3 assertions, 0 failures, 0 errors, 0 skips

Then, please checkout rails-7-0-5 branch and run tests. They will fail:

$ rails test:all                                                             
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 29194

# Running:

Capybara starting Puma...
* Version 5.6.8, codename: Birdie's Version
* Min threads: 0, max threads: 4
* Listening on http://127.0.0.1:61659
Bad request content body
2024-03-11 10:41:50 +0900 Rack app ("POST /hello" - (127.0.0.1)): #<ActionController::BadRequest: Invalid request parameters: EOFError>
[Screenshot Image]: /Users/jnito/dev/sandbox/empty-multipart-sandbox/tmp/screenshots/failures_test_#create.png
E

Error:
HelloTest#test_#create:
ActionController::BadRequest: Invalid request parameters: EOFError
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/multipart/parser.rb:373:in `handle_empty_content!'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/multipart/parser.rb:199:in `on_read'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/multipart/parser.rb:80:in `block in parse'
    <internal:kernel>:187:in `loop'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/multipart/parser.rb:78:in `parse'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/multipart.rb:53:in `extract_multipart'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/request.rb:594:in `parse_multipart'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/request.rb:446:in `POST'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/request.rb:390:in `block (2 levels) in POST'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/parameters.rb:90:in `block in parse_formatted_parameters'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/parameters.rb:90:in `fetch'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/parameters.rb:90:in `parse_formatted_parameters'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/request.rb:389:in `block in POST'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/request.rb:69:in `fetch'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/request.rb:69:in `fetch_header'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/request.rb:388:in `POST'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/parameters.rb:55:in `parameters'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/filter_parameters.rb:28:in `filtered_parameters'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_controller/metal/instrumentation.rb:57:in `process_action'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_controller/metal/params_wrapper.rb:259:in `process_action'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activerecord-7.0.5/lib/active_record/railties/controller_runtime.rb:27:in `process_action'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/abstract_controller/base.rb:151:in `process'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionview-7.0.5/lib/action_view/rendering.rb:39:in `process'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_controller/metal.rb:188:in `dispatch'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_controller/metal.rb:251:in `dispatch'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/routing/route_set.rb:49:in `dispatch'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/routing/route_set.rb:32:in `serve'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/journey/router.rb:50:in `block in serve'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/journey/router.rb:32:in `each'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/journey/router.rb:32:in `serve'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/routing/route_set.rb:852:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/tempfile_reaper.rb:15:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/etag.rb:27:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/conditional_get.rb:40:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/head.rb:12:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/permissions_policy.rb:38:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/http/content_security_policy.rb:36:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/session/abstract/id.rb:266:in `context'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/session/abstract/id.rb:260:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/cookies.rb:704:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/callbacks.rb:27:in `block in call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.0.5/lib/active_support/callbacks.rb:99:in `run_callbacks'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/callbacks.rb:26:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/actionable_exceptions.rb:17:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/debug_exceptions.rb:28:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/show_exceptions.rb:26:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/railties-7.0.5/lib/rails/rack/logger.rb:40:in `call_app'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/railties-7.0.5/lib/rails/rack/logger.rb:25:in `block in call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.0.5/lib/active_support/tagged_logging.rb:99:in `block in tagged'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.0.5/lib/active_support/tagged_logging.rb:37:in `tagged'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.0.5/lib/active_support/tagged_logging.rb:99:in `tagged'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/railties-7.0.5/lib/rails/rack/logger.rb:25:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/remote_ip.rb:93:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/request_id.rb:26:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/method_override.rb:24:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/runtime.rb:22:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/activesupport-7.0.5/lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/executor.rb:14:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/static.rb:23:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/sendfile.rb:110:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/actionpack-7.0.5/lib/action_dispatch/middleware/host_authorization.rb:131:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/railties-7.0.5/lib/rails/engine.rb:530:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/urlmap.rb:74:in `block in call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/urlmap.rb:58:in `each'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/urlmap.rb:58:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/rack-2.2.8.1/lib/rack/builder.rb:244:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/capybara-3.40.0/lib/capybara/server/middleware.rb:60:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/configuration.rb:252:in `call'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/request.rb:77:in `block in handle_request'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/thread_pool.rb:340:in `with_force_shutdown'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/request.rb:76:in `handle_request'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/server.rb:443:in `process_client'
    /Users/jnito/.rbenv/versions/3.3.0/lib/ruby/gems/3.3.0/gems/puma-5.6.8/lib/puma/thread_pool.rb:147:in `block in spawn_thread'


rails test test/system/hello_test.rb:4



Finished in 2.070979s, 0.4829 runs/s, 1.4486 assertions/s.
1 runs, 3 assertions, 0 failures, 1 errors, 0 skips

Expected behavior

The test should pass like Rails 7.0.4.3.

Actual behavior

As mentioned above, it fails.

Investigation

This issue was introduced by #45833

ActionController::BadRequest should be rescued here: https://github.com/rails/rails/blob/v7.0.5/actionpack/lib/action_dispatch/http/parameters.rb#L56

I wrote a sample patch in rails-7-0-5-with-patch branch: https://github.com/JunichiIto/empty-multipart-sandbox/blob/rails-7-0-5-with-patch/config/initializers/actionpack.rb#L10-L15

With the patch, the tests will pass on Rails 7.0.5.

$ rails test:all
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 56255

# Running:

Capybara starting Puma...
* Version 5.6.8, codename: Birdie's Version
* Min threads: 0, max threads: 4
* Listening on http://127.0.0.1:63881
Bad request content body
.

Finished in 1.368708s, 0.7306 runs/s, 2.1918 assertions/s.
1 runs, 3 assertions, 0 failures, 0 errors, 0 skips

System configuration

Rails version: 7.0.5

Ruby version: 3.3.0

@JoeDupuis
Copy link
Contributor

I was trying to build a minimal repro script, but this is a Rack error not Rails.

This behavior is intended.
That being said, there is a difference between the Rack ticket example and the example in this issue. The example in the Rack ticket doesn't pass the ending boundary, which is definitely invalid.

This ticket example does pass the ending boundary. Here's a curl version triggering the error:

curl 'http://localhost:3000/hello' \
  -H 'Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryy8DrMokPrH5xA9X2' \
  --data-raw $'------WebKitFormBoundaryy8DrMokPrH5xA9X2--\r\n'

I feel like this is a different case. It's not passing any fields, but it is passing a body, with the final boundary. I would expect a parser to parse this without crashing. I was trying to look what the spec says about this, but haven't found a quick answer. I'll look into it this week.

@JoeDupuis
Copy link
Contributor

I am not yet 100% sure what the 1* means in BNF notation, but I would assume one or more. If that's the case, an empty request, even with the final boundary, would not be valid.
From https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html

boundary := 0*69<bchars> bcharsnospace 

bchars := bcharsnospace / " " 

bcharsnospace :=    DIGIT / ALPHA / "'" / "(" / ")" / "+"  / 
"_" 
               / "," / "-" / "." / "/" / ":" / "=" / "?" 

Overall, the body of a multipart entity may be specified as follows:
multipart-body := preamble 1*encapsulation 
               close-delimiter epilogue 

encapsulation := delimiter CRLF body-part 

delimiter := CRLF "--" boundary   ; taken from  Content-Type 
field. 
                               ;   when   content-type    is 
multipart 
                             ; There must be no space 
                             ; between "--" and boundary. 

close-delimiter := delimiter "--" ; Again, no  space  before 
"--" 

preamble :=  *text                  ;  to  be  ignored  upon 
receipt. 

epilogue :=  *text                  ;  to  be  ignored  upon 
receipt. 

body-part = <"message" as defined in RFC 822, 
         with all header fields optional, and with the 
         specified delimiter not occurring anywhere in 
         the message body, either on a line by itself 
         or as a substring anywhere.  Note that the 
         semantics of a part differ from the semantics 
         of a message, as described in the text.> 

1*encapsulation suggest one or more encapsulation (where encapsulation is a delimiter + a body part).

If I read that correctly I doubt a PR to change Rack's behavior would be accepted.

I think the fix here is to open an issue or a PR on Axios' repo to stop sending empty multi part as they are not valid in the spec.

@JunichiIto
Copy link
Contributor Author

Thank you for your investigation. I opened an issue on Axios:
axios/axios#6289

@JoeDupuis
Copy link
Contributor

I was able to confirm that the spec does require at least one part.
.

Turns out, the browser implementations of fetch do send this invalid request. It is not an axios issue.

The spec does warn against being too rigid in parsing. It is not this specific section, but it makes me think that this is worth the discussion.
I believe the RFC is too restrictive (see my comment in the linked issue) and/or that it should be the job of the browsers to prevent sending the invalid request on an empty form data:

await (await fetch('https://httpbin.org/post', {method: 'post', body: new FormData()})).json()

But realistically the fix here would be to relax Rack's parsing or to change your application code to account for this. I'll look into it a bit more this week.

@JoeDupuis
Copy link
Contributor

JoeDupuis commented Mar 13, 2024

Hi @JunichiIto, I am not sure how I missed it the first time I looked, but turns out this issue is already fixed In Rack, but the fix wasn't included in the last release.

I asked the Rack maintainers if they could cut a release with the fix, but you don't have to wait.

If you point your Gemfile to the Rack repo, you can use the new version today.

To do so replace (or add) the Rack gem in your Gemfile with either:

gem "rack", github: 'rack/rack', branch: "2-2-stable" (for version 2)

or gem "rack", github: 'rack/rack', branch: "main" ( for version 3)

Then run bundle update rack
That should fix the error.

@JunichiIto
Copy link
Contributor Author

Hi @JoeDupuis , thank you for letting me know. I confirmed the latest "2-2-stable" resolved this issue. I appreciate your support!

@JoeDupuis
Copy link
Contributor

@JunichiIto The maintainers of Rack just released new releases for Rack 2 and 3 containing the fix! You can revert back your gemfile so that it doesn't point to github if desired. I believe Installing gems from github is a bit slower.

@skipkayhil
Copy link
Member

Thanks for getting this sorted @JoeDupuis!

@JunichiIto
Copy link
Contributor Author

@JoeDupuis That's great news! I appreciate your support 😃

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