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

[ikitommi]

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)

[weavejester]

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.

[ikitommi]

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.

[bhauman]

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 ...

[weavejester]

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

[bhauman]

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?

[weavejester]

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.

[bhauman]

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]

BTW I have mucho gratitude for all your great work.

[weavejester]

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.

[bhauman]

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.

[taeold]

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?

[weavejester]

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

[ikitommi]

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.

[Deraen]

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.

[ikitommi]

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

[weavejester]

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.

[allentiak]

@weavejester

[T]his hasn't been high-priority[.]

Hi,
Any priority change for this?

[allentiak]

WORKAROUND: Just for the record, the missing dependency is ring/ring-jetty-adapter.
(See correction below.)

[weavejester]

WORKAROUND: Just for the record, the missing dependency is ring/ring-jetty-adapter.

The dependency is [javax.servlet/javax.servlet-api "3.1.0"]. For Ring Core, this is a "provided" dependency, i.e. you have to explicitly add it to your project dependencies.

Jetty includes the servlet API as part of its non-provided dependencies, so including the Ring Jetty adapter will add the "missing" dependency. However, you can just include the provided dependency directly, without requiring all of Jetty if you so choose.

[weavejester]

[T]his hasn't been high-priority[.]

Hi, Any priority change for this?

Unfortunately not. While it would save people adding an extra dependency (which would be convenient), it would require either finding a maintained Java library that parses multipart uploads without depending on the servlet API, or writing our own parser just for Ring. The latter is possible, but it feels like a lot of work just to remove an annoying dependency.

[allentiak]

I see.
Thanks for the quick reply, @weavejester !