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

`handle_empty_content!': EOFError (EOFError) #1603

Closed
atsushi-zto opened this issue Feb 14, 2020 · 8 comments · Fixed by #1688
Closed

`handle_empty_content!': EOFError (EOFError) #1603

atsushi-zto opened this issue Feb 14, 2020 · 8 comments · Fixed by #1688
Assignees
Milestone

Comments

@atsushi-zto
Copy link

atsushi-zto commented Feb 14, 2020

Hi,

I updated rack@2.1.x to rack@2.2.2.
Since then, rack raises EOFError when send the following request.

curl -X POST -H 'Content-Type: multipart/form-data; boundary=dummy' http://localhost:3000/post-end-point
/usr/local/bundle/gems/rack-2.2.2/lib/rack/multipart/parser.rb:359:in `handle_empty_content!': EOFError (EOFError)

I think this error related PR#1536.
Is this the intended behavior?

best, regards.

@jeremyevans
Copy link
Contributor

I don't think it's related to #1536. It's related to #1572, a bugfix for #761.

It appears the request you are submitting is malformed, specifying a multipart body when no body is provided. Rack used to swallow this error, but we think that was a mistake. Is there a reason you think Rack should silently ignore a malformed request in this case?

@atsushi-zto
Copy link
Author

atsushi-zto commented Feb 15, 2020

@jeremyevans

Thanks for a reply.

I got it, This issue related #1572.


That's right, It's a illegal HTTP request.
Attacker sent illegal request and 5XX was recorded in large quantities, and my holiday is over.... 😭

I want the server to return a 400 (bad request).
I think it's the rack's duty to validate request header (Content-Type) and request body. (not application)

What do you think?

@ioquatix
Copy link
Member

What about other kinds of errors e.g. invalid headers, other kinds of invalid body etc.

@atsushi-zto
Copy link
Author

I misunderstood, Rack :: Request # params was called by framework.

But, EOFError is hard to catch, so I want to change to another error.
eg) Rack::Multipart::EmptyContentError

@ioquatix
Copy link
Member

I think this would be okay.

@ioquatix ioquatix added this to the v2.2.3 milestone Feb 16, 2020
@ioquatix ioquatix self-assigned this Feb 16, 2020
@jeremyevans jeremyevans modified the milestones: v2.2.3, v2.3.0 Feb 24, 2020
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
This makes it simpler to catch this specific error.

Fixes rack#1603
jeremyevans added a commit to jeremyevans/rack that referenced this issue Jul 14, 2020
…ing empty body

This makes it simpler to catch this specific error.

Fixes rack#1603
@Arie
Copy link

Arie commented Aug 28, 2020

The handle_empty_content! / EOFError change also breaks Steam's OpenID-based login when used with the Steam Game Overlay web browser.

Apparently that integrated web overlay sends the multipart/form-data Content-Type but includes no body, just a lot of query parameters for OpenID.

Here's an anonymized curl call of a request failing in rack 2.2.x that worked in 2.1.x:

curl \
 --compressed \
 -H "Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9" \
 -H "Accept-Encoding: gzip" \
 -H "Accept-Language: ru-RU,ru;q=0.9,en-US;q=0.8,en;q=0.7" \
 -H "Cache-Control: max-age=0" \
 -H "Connection: close" \
 -H "Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryvAQ67HmQgn07512v" \
 -H "Host: rack-app-here" \
 -H "Origin: https://steamcommunity.com" \
 -H "Referer: [Filtered]" \
 -H "Sec-Fetch-Mode: navigate" \
 -H "Sec-Fetch-Site: cross-site" \
 -H "Sec-Fetch-User: ?1" \
 -H "Upgrade-Insecure-Requests: 1" \
 -H "User-Agent: Mozilla/5.0 (Windows; U; Windows NT 10.0; en-US; Valve Steam GameOverlay/1598490486; ) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.117 Safari/537.36" \
 -H "Version: HTTP/1.0" \
 "http//rack-app-here?_method=post&openid.assoc_handle=1234567890&openid.claimed_id=https%3A%2F%2Fsteamcommunity.com%2Fopenid%2Fid%2F76561198400000000&openid.identity=https%3A%2F%2Fsteamcommunity.com%2Fopenid%2Fid%2F76561198400000000&openid.mode=id_res&openid.ns=%5BFiltered%5D&openid.op_endpoint=https%3A%2F%2Fsteamcommunity.com%2Fopenid%2Flogin&openid.response_nonce=nonce-here%3D&openid.return_to=%5BFiltered%5D&openid.sig=sig-here%3D&openid.signed=signed%2Cop_endpoint%2Cclaimed_id%2Cidentity%2Creturn_to%2Cresponse_nonce%2Cassoc_handle"

@jeremyevans
Copy link
Contributor

Interesting that something that popular could be so broken. RFC2046 makes it clear that multipart content needs a body (https://tools.ietf.org/html/rfc2046#section-5.1):

   The body must then contain
   one or more body parts, each preceded by a boundary delimiter line,
   and the last one followed by a closing boundary delimiter line.

I'm still open to using a separate class for this, as my pull request (#1688) does. However, it has not been reviewed yet.

@ioquatix
Copy link
Member

ioquatix commented Sep 6, 2020

I don't have a strong opinion about this, but I do concern that we make system more complex to solve the problem. It doesn't bother me a huge amount but handling many edge cases becomes maintenance burden.

rx added a commit to rx/presenters that referenced this issue Jun 16, 2021
The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
rx added a commit to rx/presenters that referenced this issue Jun 16, 2021
The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
rx added a commit to rx/presenters that referenced this issue Jun 16, 2021
If a event chain is halted, it fires the 'V:eventsHalted' event.
That can be handled so that the default error handling does not kick in and you can handle it yourself.

The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
rx added a commit to rx/presenters that referenced this issue Jun 18, 2021
The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
rx added a commit to rx/presenters that referenced this issue Jun 18, 2021
The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
rx added a commit to rx/presenters that referenced this issue Jun 18, 2021
If a event chain is halted, it fires the 'V:eventsHalted' event.
That can be handled so that the default error handling does not kick in and you can handle it yourself.

The post was issuing a multi-part post with body. That is a malformed request and starting with Rack 2.2.2 will generate an exception. rack/rack#1603
This fixes that issue.
catatsuy added a commit to catatsuy/private-isu that referenced this issue Jan 15, 2022
Go's http.client's automatic redirection uses the same Content-Type for
GET. This request is invalid in rack 2.2.
I don't know if the Go implementation is wrong or the rack
implementation is wrong. I will use the old rack for now.
refs: rack/rack#1603
refs: #204
catatsuy added a commit to catatsuy/private-isu that referenced this issue Jan 15, 2022
Go's http.client's automatic redirection uses the same Content-Type for
GET. This request is invalid in rack 2.2.
I don't know if the Go implementation is wrong or the rack
implementation is wrong. I will use the old rack for now.
refs: rack/rack#1603
refs: #204
6rawn added a commit to 6rawn/private-isu that referenced this issue May 18, 2023
Go's http.client's automatic redirection uses the same Content-Type for
GET. This request is invalid in rack 2.2.
I don't know if the Go implementation is wrong or the rack
implementation is wrong. I will use the old rack for now.
refs: rack/rack#1603
refs: #204
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

Successfully merging a pull request may close this issue.

4 participants