Better handling of multipart parsing when request is missing body. by ioquatix · Pull Request #1997 · rack/rack

ioquatix · 2022-12-16T02:06:09Z

Introduce module Rack::BadRequest for unified exception handling.
Make env['rack.input'] optional.

rack.input feels like a hangover from the CGI days when $stdin was always present and open. But in modern servers, we know that there is no entity body in most cases where it's relevant. However, we still need to allocate something which fails on the first call to read (EOFError, IOError or something else). There is actually no way to detect that there is no entity body. So, I'd like to fix that, as it makes handling the multipart error more straight forward.

If you think there are any other Rack error classes that should include BadRequest, please feel free to suggest it.

Fixes #1994. Fixes #1995. Fixes #1996.

ioquatix · 2022-12-16T02:35:01Z

@MSP-Greg @dentarg @nateberkopec can I please get your feedback on this change as you would be expected to add rescue Rack::BadRequest and map that to a 400 error.

begin
  status, headers, body = @app.call(env)
rescue Rack::BadRequest => error
  status = 400
  headers = {'content-type' => 'text/plain'}
  body = [error.to_s]
end

It also means you should not provide rack.input if the request doesn't contain an entity body (we might need to define how to detect this for HTTP/1.x and 2+ because the semantics aren't identical).

MSP-Greg · 2022-12-16T03:22:13Z

LGTM

If you think there are any other Rack error classes that should include BadRequest, please feel free to suggest it.

If that's a possibility, might Rack::BadRequest have some way of providing the status, headers & body? Maybe have it respond to call, just like an app?

jeremyevans · 2022-12-16T04:03:35Z

@MSP-Greg @dentarg @nateberkopec can I please get your feedback on this change as you would be expected to add rescue Rack::BadRequest and map that to a 400 error.

I think would be expected to is better phrased as can. Neither server, middleware, frameworks, or applications should have to require rack the library to be rack compatible.

Personally, I don't think servers should directly be rescuing the exception. We should probably ship a middleware that does the handling (maybe Rack::BadRequest::Middleware), which servers/frameworks/applications that want to handle this such exceptions automatically can use.

If that's a possibility, might Rack::BadRequest have some way of providing the status, headers & body? Maybe have it respond to call, just like an app?

I don't think we should do that. Anything that could provide the status, headers, and body should just return the expected rack response array instead of raising an exception and relying on something else to do the translation.

ioquatix · 2022-12-16T04:16:56Z

@jeremyevans I don't disagree with your points. I agree servers should be able to deal with Rack as an interface without needing rack the gem. But I don't know what a better alternative is.

Rack as an interface is different from all the other stuff like Rack::Request and Rack::Response. The only thing which constitutes "rack the interface" is the spec. But we are talking about Rack::Request and Rack::Multipart which I don't consider part of "Rack the interface". In that regard, I agree that a server should not need to know about Rack::BadRequest.

Neither server, middleware, frameworks, or applications should have to require rack the library to be rack compatible.

Sure, and they can just blow up with a 500 error if that's their preference because there is no way to handle this case generally without having some class of exceptions that are "The request did not meet the requirements" vs "Something unexpected happened".

Personally, I don't think servers should directly be rescuing the exception.

How, in your opinion, should we turn this error into a 400 Bad Request error?

The only other option is some kind of middleware, but is that really good enough? As it stands, it feels like some parts of Rack::Request would need to signal "This input environment is bad/invalid" and we'd want to communicate that with a 400-status rather than a 500-status.

jeremyevans · 2022-12-16T04:39:50Z

Personally, I don't think servers should directly be rescuing the exception.

How, in your opinion, should we turn this error into a 400 Bad Request error?

The only other option is some kind of middleware, but is that really good enough?

Directly after the sentence you quoted, I wrote We should probably ship a middleware that does the handling, which indicates I considered it good enough.

As it stands, it feels like some parts of Rack::Request would need to signal "This input environment is bad/invalid" and we'd want to communicate that with a 400-status rather than a 500-status.

Code that uses Rack::Multipart for parsing (or other parts of Rack where this exception could be used) should have no issues using Rack::BadRequest::Middleware to handle it at the middleware level.

ioquatix · 2022-12-16T04:47:18Z

Directly after the sentence you quoted, I wrote We should probably ship a middleware that does the handling, which indicates I considered it good enough.

Sure, I understand your position, but I don't consider it good enough from a developer UX POV. Because it won't work by default, it's another overhead users need to deal with, and it's version specific (although everything here is version specific, at least servers can hide the complexity and application users don't need to deal with it).

MSP-Greg · 2022-12-16T04:57:50Z

From RFC 9110 15.5 "Client Error 4xx":

"the server SHOULD send a representation containing an explanation of the error situation, and whether it is a temporary or permanent condition. These status codes are applicable to any request method. User agents SHOULD display any included representation to the user."

Maybe it's best left to the app, rather than Rack?

nateberkopec · 2022-12-16T05:22:40Z

"No one has to require rack" feels like something that's too useful to lose.

ioquatix · 2022-12-16T05:55:17Z

Well, I'm also fine with this just being a 500 error. But that won't solve the original problem.

dentarg · 2022-12-16T12:49:32Z

I think it is okay if Rack just raises some error that web frameworks can rescue and return whatever status code they want for it. It is of course nice if there's a middleware included with Rack that can handle them automatically if you are fine with the responses that middleware makes.

I'll comment on the suggestion in #1996 where BadRequest has an attribute status. We got this bug report in Sinatra sinatra/sinatra#1518 and indeed the bug was with Sinatra (only checking if certain method existed), but seeing that bug I would be a bit wary of introducing error objects with such common method names. Maybe there are other code bases making the same mistake as Sinatra.

catatsuy · 2022-12-18T01:32:07Z

I think that other language implementations would often ignore this.
But if you don't want them to be processed, a new implementation would be more appropriate than the current one

ioquatix · 2023-01-12T07:19:16Z

Maybe it's best left to the app, rather than Rack?

The problem with this, is that Rack is the one actually generating the errors, because Rack::Request is a library level implementation, not part of "rack the interface". We are literally providing application level constructs that are independent of the server interface, and those can signal failures due to bad request data, and we need some way to signal this so that applications can handle it correctly.

ioquatix · 2023-01-12T07:24:29Z

@jeremyevans what do you think of the proposed change?

The middleware Rack::BadRequest::Middleware can help for people who want to adopt it.

The only issue is now:

    class EmptyContentError < ::EOFError
      include BadRequest
      # "Middleware" as a constant is defined here?
    end

Is it acceptable or should we reorganise the modules, e.g.

module Rack::BadRequest... etc

class Rack::ErrorHandler # (or ExceptionHandler?)
  def call(env)
    @app.call(env)
  rescue Rack::BadRequest
    [400 ...
  rescue
    [500 ...
  end
end

etc

jeremyevans

I think this PR should be split. There should be a separate PRs for the spec change from must to may, for BadRequest, and for ErrorHandler.

jeremyevans

Please split this into 3 separate pull requests, one for rack.input, one for BadRequest, and one for ErrorHandler.

jeremyevans · 2023-01-13T15:56:18Z

+    end
+
+    Error = BoundaryTooLongError
+    deprecate_constant :Error


Deprecation at this point is too aggressive. Otherwise, how do you write code that rescues this error and works in both Rack 3.0 and 3.1 (and ideally, Rack 2)?

This error AFAIK doesn't exist in Rack 2.x?

It was introduced here: d866dc4

Deprecations only show up with warnings enabled. We can keep it for one more minor version if you think that makes sense. How would you like to deprecate this?

I think going forward, people should probably rescue Rack::BadRequest rather than specific multipart/query_parser errors.

I think we should keep it until Rack 4.

My question was when should we deprecate it? I believe it can be removed in Rack 4, are you suggesting we should deprecate it it in Rack 4?

jeremyevans · 2023-01-13T15:58:39Z

-    Rack::MockRequest.env_for("/", *args)
+  def env(options = {})
+    unless options.key?(:input)
+      options[:input] = String.new


This seems like a smell. If we are changing rack.input to not be required, we shouldn't have the tests default to using it with an empty string.

Lot's of tests fail without it because they assume that it's an empty string. We can rewrite those tests. However if we change that, it will make backporting (semi-unrelated changes) harder if the tests overlap since they will have diverged. This is about changing as few tests as possible - we can always rewrite the tests later once backporting is no longer expected.

That should give you a rough indication of how much code this change will break. If we aren't willing to modify rack's own tests to fully implement the change, it's hard to argue the benefits of the change are worth the cost.

If we aren't willing to modify rack's own tests to fully implement the change

Well, we did modify rack's own tests, and it was just in one place since the test fixture uses this and these tests assume an empty string. That shows minimal changes were required in this specific case.

jeremyevans

I would still like this split into 3 pull requests, as previously indicated.

Also, please do not mark conversations as resolved. The reviewer should decide when a conversation has been resolved, not the person proposing the change.

jeremyevans · 2023-01-13T21:44:56Z

-    Rack::MockRequest.env_for("/", *args)
+  def env(options = {})
+    unless options.key?(:input)
+      options[:input] = String.new


That should give you a rough indication of how much code this change will break. If we aren't willing to modify rack's own tests to fully implement the change, it's hard to argue the benefits of the change are worth the cost.

jeremyevans · 2023-01-13T21:51:34Z

+    end
+
+    Error = BoundaryTooLongError
+    deprecate_constant :Error


I think we should keep it until Rack 4.

ioquatix · 2023-01-16T02:13:38Z

      "ActionDispatch::Http::Parameters::ParseError"       => :bad_request,
      "ActionController::BadRequest"                       => :bad_request,
      "ActionController::ParameterMissing"                 => :bad_request,
      "Rack::QueryParser::ParameterTypeError"              => :bad_request,
      "Rack::QueryParser::InvalidParameterError"           => :bad_request

Rails is already doing this basically, but in a worse way.

- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional. # Conflicts: # lib/rack/request.rb

- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional.

tenderlove

I like the idea of having a BadRequest exception that people can rescue, but I agree with @jeremyevans that this PR should be split. I am having a hard time reasoning about the changes with such a big diff.

ioquatix · 2023-01-19T01:49:06Z

Okay, I will split it.

ioquatix · 2023-01-19T02:04:07Z

See #2018 and #2019 (depends on 2018).

ioquatix requested review from jeremyevans and tenderlove December 16, 2022 02:07

ioquatix commented Dec 16, 2022

View reviewed changes

Comment thread lib/rack/request.rb

ioquatix commented Dec 16, 2022

View reviewed changes

Comment thread test/spec_mock_request.rb

ioquatix force-pushed the invalid-request branch from 2a20f08 to e403966 Compare December 16, 2022 02:15

catatsuy reviewed Dec 18, 2022

View reviewed changes

Comment thread lib/rack/multipart/parser.rb Outdated

ioquatix force-pushed the invalid-request branch from 8caf16a to 5878656 Compare January 12, 2023 07:16

ioquatix force-pushed the invalid-request branch from 1edc104 to 46047b5 Compare January 12, 2023 07:43

jeremyevans requested changes Jan 12, 2023

View reviewed changes

ioquatix requested review from catatsuy and jeremyevans and removed request for catatsuy January 13, 2023 06:31

ioquatix force-pushed the invalid-request branch 5 times, most recently from 555e95a to 65782e6 Compare January 13, 2023 07:53

ioquatix force-pushed the invalid-request branch 3 times, most recently from 9b56e6b to d90c916 Compare January 13, 2023 08:36

jeremyevans requested changes Jan 13, 2023

View reviewed changes

ioquatix force-pushed the invalid-request branch from 81108f4 to c3812e3 Compare January 13, 2023 21:50

jeremyevans requested changes Jan 13, 2023

View reviewed changes

ioquatix force-pushed the invalid-request branch from c3812e3 to f11e933 Compare January 14, 2023 04:28

Better handling of multipart parsing when request is missing body.

25de02a

- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional. # Conflicts: # lib/rack/request.rb

ioquatix force-pushed the invalid-request branch from f11e933 to 25de02a Compare January 17, 2023 06:55

Better handling of multipart parsing when request is missing body.

0c10e7a

- Introduce `module Rack::BadRequest` for unified exception handling. - Add `BadRequest` to query parser too. - Make `env['rack.input']` optional.

ioquatix force-pushed the invalid-request branch from ecabf18 to 0c10e7a Compare January 17, 2023 07:13

tenderlove requested changes Jan 19, 2023

View reviewed changes

ioquatix closed this Jan 19, 2023

ioquatix deleted the invalid-request branch January 19, 2023 01:49

Fjan mentioned this pull request Jun 14, 2024

Some Rails integration tests break with rack 3.1 rails/rails#52125

Closed

Conversation

ioquatix commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ioquatix commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSP-Greg commented Dec 16, 2022

Uh oh!

jeremyevans commented Dec 16, 2022

Uh oh!

ioquatix commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremyevans commented Dec 16, 2022

Uh oh!

ioquatix commented Dec 16, 2022

Uh oh!

MSP-Greg commented Dec 16, 2022

Uh oh!

nateberkopec commented Dec 16, 2022

Uh oh!

ioquatix commented Dec 16, 2022

Uh oh!

dentarg commented Dec 16, 2022

Uh oh!

Uh oh!

catatsuy commented Dec 18, 2022

Uh oh!

ioquatix commented Jan 12, 2023

Uh oh!

ioquatix commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremyevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremyevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremyevans Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

ioquatix Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyevans Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

ioquatix Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyevans Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

ioquatix Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyevans Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

ioquatix Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ioquatix commented Dec 16, 2022 •

edited

Loading

ioquatix commented Dec 16, 2022 •

edited

Loading

ioquatix commented Dec 16, 2022 •

edited

Loading

ioquatix commented Jan 12, 2023 •

edited

Loading

ioquatix Jan 13, 2023 •

edited

Loading

ioquatix Jan 13, 2023 •

edited

Loading

ioquatix Jan 13, 2023 •

edited

Loading

ioquatix Jan 13, 2023 •

edited

Loading