Feature #8370

Constants MAX_MULTIPART_LENGTH in cgi/core.rb

Added by Takeyuki FUJIOKA almost 2 years ago. Updated 9 months ago.

[ruby-core:54798]
Status:Closed
Priority:Normal
Assignee:Takeyuki FUJIOKA

Description

Reported by Andreas Kraus via gmail.

hi xibbar,

I try to understand why the MAX_MULTIPART_LENGTH is a Constant and i can't change it.
If i uload a Multipart file which is larger than 128 MB raise an error "too large multipart data.",
but why i can't change this value to upload larger files.

The Constant comes with this Change:
https://github.com/ruby/ruby/commit/10e9b638069d9e40233242693814b86c672e423e#lib/cgi/core.rb

The only sense i see, is that the Author of cgialt uses max 128MB files und build in this Constant ...

I would like to know why this constant is in place and how to change it's behaviour.
My requirement is to upload files larger than the given limit of 128MB.

regards,
Andreas

Associated revisions

Revision 46392
Added by Takeyuki FUJIOKA 9 months ago

  • lib/cgi/core.rb: Provide a mechanism to specify the max_multipart_length of multipart data. [Feature #8370] patch by Leif Eriksen leif.eriksen.au@gmail.com

Revision 46392
Added by Takeyuki FUJIOKA 9 months ago

  • lib/cgi/core.rb: Provide a mechanism to specify the max_multipart_length of multipart data. [Feature #8370] patch by Leif Eriksen leif.eriksen.au@gmail.com

History

#1 Updated by Takeyuki FUJIOKA almost 2 years ago

  • Description updated (diff)

#2 Updated by Nobuyoshi Nakada almost 2 years ago

  • Description updated (diff)

#3 Updated by Hiroshi SHIBATA about 1 year ago

  • Target version changed from 2.1.0 to current: 2.2.0

#4 Updated by Leif Eriksen 9 months ago

From my reading of RFC-1867, this constant/constraint is not required.

If a server detects space constraints for an upload, it can terminate the connection at any time.

Servers can indicate a maximum length via the MAXLENGTH attribute, but clients are not required to limit themselves to it.

Clients are generally required to supply an overall content-length for the upload - servers can act on that at the commencement of transmission, or terminate at a later time according to whatever policy has been set up.

There is no reason, that I can see, to artificially limit the upload size on the client side, the server can accept or reject at any time.

I will raise a git pull request from my commit https://github.com/leriksen/ruby/commit/77f3a6bbb92e3f395851e7998556d7df160f4da1

#5 Updated by Yusuke Endoh 9 months ago

There is no reason, that I can see, to artificially limit the upload size on the client side

I think that MAX_MULTIPART_LENGTH is the limit on the server side.
The limit itself is actually required to prevent shortage of server resource.

I agree that the limit should be configuable by users.
In fact, the author of cgialt, Makoto Kuwata, said in the proposal,

http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-dev/33606

簡単化のため制限値は定数で指定してますが、必要であればクラス変数や
インスタンス変数で指定できるようにしてください。

For simplicity, the limit is specified by a constant,
but if needed, please use class variable or instance variable
to specify it.

So, what we need is not a patch that just removes the check,
but a patch that enables a user to configure it.

Yusuke Endoh mame@ruby-lang.org

#6 Updated by Leif Eriksen 9 months ago

OK, yeah I got the wrong end of the conversation - CGI is all about the server side.....

OK an idea comes to mind

A new option in the options_hash in the constructor - currently only :tag_maker and :accept_charset are defined

The multipart handling is in the private method initialize_query(), which is only referenced in the constructor. So the only opportunity to set this up is from CGI.new.

I propose a new option in the initialize method - :max_multipart_length

It will default to the old value of 128 *1024 *1024 bytes, and can take as a value either a simple integer scalar, or a lambda. A lambda will allow the user to use more complex logic than a simple integer to determine the size of multipart forms to accept.

Would that work ?

#7 Updated by Takeyuki FUJIOKA 9 months ago

Do you want to set the value of MAX_MULTIPART_LENGTH every request ?
I think request length is only one value in web server.

#8 Updated by Leif Eriksen 9 months ago

Well I'm only trying to solve the problem in the CGI library, not the underlying web server, and the only place that will reference MAX_MULTIPART_LENGTH is in the request parsing. That happens only in the CGI constructor. I dont think I should stray too far from that use-case.

If a user does not specify a MAX_MULTIPART_LENGTH in the constructor, it will default to 128 * 1024 *1024 bytes. Otherwise a user can specify a new length, or a lambda that will calculate it at parse time (say there is a need to query the file system, or get a size from a users account etc).

I will add a getter/setter pair, though I cant think of a use-case for them. But Makoto did mention using an instance variable, so might as well complete that part.

I'll go ahead with this plan, and note the commit/pull-request in my next comment.

#9 Updated by Nobuyoshi Nakada 9 months ago

  • Subject changed from Constants MAX_MULTIPART_LENGTH in cgi\core.rb to Constants MAX_MULTIPART_LENGTH in cgi/core.rb
  • Description updated (diff)

#10 Updated by Leif Eriksen 9 months ago

OK done.

Used the pattern for passing accept_charset, which can be read/modified by a class method, and read by an instance method. That seems reasonable, let me know if a setter instance methods is also required, will do as a separate piece.

I updated the multipart tests to also take an options hash, to make constructing the CGI instance under test a little easier.

Pull request - https://github.com/ruby/ruby/pull/632
Commit - https://github.com/leriksen/ruby/commit/edcd051115165a2cafbf8892081418c621201c37

#11 Updated by Nobuyoshi Nakada 9 months ago

  • Tracker changed from Bug to Feature

Adding new methods is a feature request.

#12 Updated by Leif Eriksen 9 months ago

OK I'll remove the getter/setters and resubmit.

#14 Updated by Nobuyoshi Nakada 9 months ago

I meant "adding new feature".

And you didn't have to resubmit, pushing new changeset should be enough.

#15 Updated by Leif Eriksen 9 months ago

OK noted - just trying to be super polite. Please lt me know if there is anything else I need to complete or consider for this fix.

#16 Updated by Takeyuki FUJIOKA 9 months ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Applied in changeset r46392.


  • lib/cgi/core.rb: Provide a mechanism to specify the max_multipart_length of multipart data. [Feature #8370] patch by Leif Eriksen leif.eriksen.au@gmail.com

Also available in: Atom PDF