-
Notifications
You must be signed in to change notification settings - Fork 517
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
ring.middleware.multipart-params depends on javax.servlet/servlet-api #251
Comments
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. |
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. |
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 ... |
The |
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? |
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. |
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
|
BTW I have mucho gratitude for all your great work. |
* Ring brings in the needed Servlet dependency (which is needed in multipart-handling even if you are not running on servlet container - see ring-clojure/ring#251)
For the error to occur, you need to have a project without the Incidentally, the
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. |
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. |
What would be a way to remove |
Fixing it effectively means writing our own multipart-handling code. |
Related: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-3092
|
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. |
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. |
Hi, |
WORKAROUND: Just for the record, the missing dependency is |
The dependency is 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. |
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. |
I see. |
Writing apps that mostly depending just on
ring/ring-core
, but using thering.middleware.multipart-params
fails for the missing servlet api. At leastorg.apache.commons.fileupload.FileUploadBase
references to Servlet stuff. Could there be a servlet-free version of the multipart handling?The text was updated successfully, but these errors were encountered: