ring icon indicating copy to clipboard operation
ring copied to clipboard

ring.middleware.multipart-params depends on javax.servlet/servlet-api

Open ikitommi opened this issue 9 years ago • 21 comments

Writing apps that mostly depending just on ring/ring-core, but using the ring.middleware.multipart-params fails for the missing servlet api. At least org.apache.commons.fileupload.FileUploadBase references to Servlet stuff. Could there be a servlet-free version of the multipart handling?

➜  ~ lein try ring/ring-core 1.5.0
nREPL server started on port 62350 on host 127.0.0.1 - nrepl://127.0.0.1:62350
REPL-y 0.3.7, nREPL 0.2.12
Clojure 1.8.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_31-b13
user=> (require '[ring.middleware.multipart-params :as multipart-params])
CompilerException java.lang.NoClassDefFoundError: javax/servlet/http/HttpServletRequest, compiling:(ring/middleware/multipart_params.clj:52:18)

ikitommi avatar Jun 30 '16 21:06 ikitommi

This has been on the cards for a while, but I haven't had the time to do it. Because it's just a case of requiring an additional dependency that ideally we could avoid, this hasn't been high-priority. However I'd certainly accept a PR that fixes this issue.

weavejester avatar Jun 30 '16 22:06 weavejester

Thanks for explaining. Sadly, don't have extra time to do a PR for this. Should this issue be left open (for someone to grap)? I'm ok, if you want to close this.

ikitommi avatar Jul 01 '16 05:07 ikitommi

Probably better to make this a dependency in the short term, because well, ... it's a dependency.

A very common path for setting up a Clojure web app (requiring compojure.handler) puts this issue front and center for a lot of people. And likely a lot of new people ...

bhauman avatar Jul 16 '16 13:07 bhauman

The servlet-api package is a provided dependency, because sometimes the thing running the handler provides its own version.

weavejester avatar Jul 16 '16 14:07 weavejester

I get that, and this, as usual, is a tough one ... but there is a bottom line where a what seems like high percentage of new Clojure users will be welcomed with a rather confusing stacktrace ...

Is this a miss-perception on my part?

bhauman avatar Jul 16 '16 17:07 bhauman

Is it a high percentage? I haven't heard many complaints about this comparatively, and it would only affect Clojure users who were using multipart-params as part of a library, rather than an application, which seems like it would be a fairly rare use-case.

weavejester avatar Jul 16 '16 18:07 weavejester

I certainly could be wrong, but if you follow the compojure readme and then add compojure.handler to your ns... Boom

This happened to me this morning, right off the bat. I recovered quickly and was very thankful for the warning at the top of the ring readme.

But having that warning front and center at the top of the readme is kind of a smell right? An affirmation of the high likelihood of encountering this. Might be why you haven't gotten many comments.

And while the warning is very helpful, the warning isn't at the top of the compojure Readme or at the top of the readme(s) of gosh knows how many libs that transitively depend on ring.

Anyway food for thought....

Sent from my iPhone

On Jul 16, 2016, at 2:09 PM, James Reeves [email protected] wrote:

Is it a high percentage? I haven't heard many complaints about this comparatively, and it would only affect Clojure users who were using multipart-params as part of a library, rather than an application, which seems like it would be a fairly rare use-case.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

bhauman avatar Jul 16 '16 18:07 bhauman

BTW I have mucho gratitude for all your great work.

bhauman avatar Jul 16 '16 18:07 bhauman

I certainly could be wrong, but if you follow the compojure readme and then add compojure.handler to your ns... Boom

For the error to occur, you need to have a project without the servlet-api package. If you have an adapter in your dependencies, then that typically has a transitive dependency on servlet-api. If you're developing a typical Ring application, you typically won't run into this issue, because you'll be running it through an adapter.

Incidentally, the compojure.handler namespace has been deprecated for a while. Prefer the ring-defaults library instead.

But having that warning front and center at the top of the readme is kind of a smell right?

Either way we need a warning message. We'd just be swapping one exception with another. The only way to solve this permanently is to rewrite the multipart middleware to not make use of the Apache FileUpload library.

weavejester avatar Jul 16 '16 21:07 weavejester

Actually I think I missed the real thing here. I think this is a tooling problem. (perhaps fixable in leiningen)

Scope "provided" is offered as a feature on the one hand... but instead of completing the circle and reporting that that deps haven't actually been provided it continues on its merry way.

Checking that "provided" deps are satisfied is rather simple to do before executing project code.

bhauman avatar Jul 17 '16 12:07 bhauman

What would be a way to remove servlet -api as a dependency? Since org.apache.commons.fileupload itself depends on servlet-api, is the only way to replace it by rewriting that library without the dependency?

taeold avatar Aug 14 '16 23:08 taeold

Fixing it effectively means writing our own multipart-handling code.

weavejester avatar Aug 14 '16 23:08 weavejester

Related: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-3092

The MultipartStream class in Apache Commons Fileupload before 1.3.2, as used in Apache Tomcat 7.x before 7.0.70, 8.x before 8.0.36, 8.5.x before 8.5.3, and 9.x before 9.0.0.M7 and other products, allows remote attackers to cause a denial of service (CPU consumption) via a long boundary string.

ikitommi avatar Aug 29 '16 08:08 ikitommi

Yada implements multipart support: https://github.com/juxt/yada/blob/432d1b25f83a4af5d94d1108f37ee253a2a74bce/ext/multipart/src/yada/multipart.clj

It uses Manifold but maybe the parse logic is helpful for replacing commons-fileupload.

Deraen avatar Feb 15 '18 19:02 Deraen

This could be interesting too: https://github.com/synchronoss/nio-multipart

ikitommi avatar Jul 25 '18 16:07 ikitommi

This could be interesting too: https://github.com/synchronoss/nio-multipart

That is interesting. If it's got no dependencies on the servlet libraries, and relatively minimal other dependencies, then it might be a suitable replacement for commons-fileupload.

weavejester avatar Jul 25 '18 16:07 weavejester