Return status 400 on invalid file names

[expez]

The multipart middleware would throw an exception when it encountered an
invalid filename.

For most apps that exception would be caught by the default exception
handler and result in a status code of 500 instead of 400.

This commit catches this specific exception and returns a 400 response
from the server instead.

[expez]

This PR would close #342.

This is the simplest possible thing that would do what I need.

If you want something more elaborate, please let me know!

[expez]

@weavejester Is this a satisfactory solution, or would you like something else?

[weavejester]

Sorry for the late response. There are a few issues:

1. It needs some tests
2. The async version is broken
3. The indentation is off

[expez]

Thanks for taking a look!

1 and 2 addressed.

I tried to find examples of other error handling for the async model in the code base, but couldn't find any. I opted to respond with a 400 instead of using raise on the exception. Let me know if I misunderstood how the async error handling should work and I'll update the code and tests.

As to 3) I'm not sure what you mean. I went to some lengths to avoid making stylistic changes to the indentation.
I even opened vim to prevent the default emacs indendation when adding the import. Before submitting the latest patch I ran cljfmt on the file but that didn't result in any changes.

edit: I didn't squash these (yet) to make review slightly easier.

[weavejester]

This should mirror the functionality of the 1-arity form, so:

(catch InvalidFileNameException e
  (respond (response/bad-request (.getMessage e))))

[weavejester]

We should probably make this customizeable, so perhaps something like:

(let [invalid-filename-handler
      (:invalid-filename-handler options default-invalid-filename-handler)]
  (fn
    ([request]
       ...)

[weavejester]

Why have you created a new test file? Just add your tests to the existing file at: ring-core/test/ring/middleware/test/multipart_params.clj

[weavejester]

Don't use with-redefs here. It would be better to pass in a request with an invalid filename.

[weavejester]

Prefer using the full request map in tests, over helper functions like bad-request.

[weavejester]

I tried to find examples of other error handling for the async model in the code base, but couldn't find any. I opted to respond with a 400 instead of using raise on the exception.

That's correct, because it should mirror the 1-arity version.

As to 3) I'm not sure what you mean. I went to some lengths to avoid making stylistic changes to the indentation.

The incorrect indentation was here and here, but that's been fixed in the latest commit.

[expez]

Thanks for your patience and guidance on this @weavejester!

Should be ready for another review now.

[weavejester]

Overall looks good. Just a few minor changes.

[weavejester]

Maybe we should change the formatting so that it's more like:

:encoding
- Character encoding...

With the addition of :invalid-filename-handler, there's just too much whitespace, and the lines go over 80 characters as well.

[weavejester]

The try/catch should only wrap the multipart-params-request call, since we don't want exceptions thrown by the handler to be caught within this block.

[expez]

Done @weavejester. I also I pushed the error handling down one level, to avoid code duplication.

Just let me know if the use of meta, to indicate that the response resulted in an error, is making you see red and I can change it to e.g. using return value with {:result ... :error ...} or something else instead.

[weavejester]

We shouldn't be passing metadata instead of catching an exception. Instead, just have it as you had previously, but narrow the scope of the try/catch. Something like

(defn wrap-multipart-params [handler options]
  (let [invalid-file-name-handler
        (:invalid-filename-handler options default-invalid-filename-handler)]
   (fn
     ([request]
      (let [req-or-ex (try (multipart-params-request request options)
                           (catch InvalidFileNameException ex ex)]
        (if (instance? Throwable req-or-ex)
          (invalid-filename-handler request)
          (handler req-or-ex))))
     ([request respond raise]
      (let [req-or-ex (try (multipart-params-request request options)
                           (catch InvalidFileNameException ex ex)]
        (if (instance? Throwable req-or-ex)
          (invalid-filename-handler request respond raise)
          (handler req-or-ex respond raise))))))

There's potential to simplify this, but I'm not sure whether we should retain the exception or not. If we don't need to, we can just have the catch swallow the exception and return nil.

[expez]

@weavejester That does indeed look better. Updated!

[weavejester]

This let binding can be moved out of the inner function so we don't need to perform a key lookup each request.

[expez]

Done!

[weavejester]

Throwable needs to be InvalidFileNameException.

[weavejester]

This needs to support the asynchronous 3-arity form.

[weavejester]

Exception needs to be InvalidFileNameException.

[expez]

Latest feedback addressed 🙂

[weavejester]

Extra whitespace that needs to be removed.

[expez]

Done

[expez]

Ping @weavejester. Is this in a merge-worthy state now? Do you want me to squash or are you using the github workflow for that?

[weavejester]

Oh, I forgot to commit my last review. Hang on, let me do that now.

[weavejester]

Could you add a simple explanatory message to the default response, so people know what caused the error? For example:

(response/bad-request
 (str "Invalid filename in multipart upload: "
      (::invalid-filename request)))

[expez]

Done, thanks! 👍

[weavejester]

Thanks! That looks good. Can you squash the commits and ensure the commit message adheres to the contributing guidelines?

[expez]

Can you squash the commits and ensure the commit message adheres to the contributing guidelines?

Done :)

[expez]

ping @weavejester, is this in a mergeworthy state?

[weavejester]

Thanks for being patient. I've been thinking about this, as I'm not completely happy with the solution we've ended up with. Other middleware, such as wrap-params and wrap-cookies, ignore invalid values, so the more consistent behavior of wrap-multipart-params would be to ignore parameters with invalid filenames.

Longer term, I'd like to introduce optional exceptions to all those functions. I've been thinking about introducing some middleware for Ring 1.8 that would catch exceptions and produce some manner of response. With clojure.lang.ExceptionInfo, a key like :ring.response/status can be introduced to the ex-data that indicates to Ring how the error should be reported to the client.

In conjunction with that middleware, an extra :raise-exceptions? option can then be added to wrap-params, wrap-multipart-params and wrap-cookies that will raise exceptions on invalid data instead of ignoring it.

However, in the meantime we can adopt the simpler solution of just passing over invalid data. My thought is that we can omit the :filename key when the value can't be parsed. From the purpose of backward compatibility, this would still raise an exception if someone tries to use the filename, but it would also allow people to ignore that part if they don't need it. This should also result in a far simpler change, as all we need to do is wrap (.getName item) in a try/catch.

What are your thoughts?

[expez]

Most projects probably have an exceptions middleware, so if your intention is for the new ring exception middleware to be extensible to the point where it can replace all the home-grown solutions, then I think this is a good idea. All exceptions will then be handled in a uniform manner.

If the new middleware isn't going to be extensible, then I think I'd prefer the current implementation for reasons of code locality and because it has fewer moving parts.

I'm not sure continuing without :filename is a good idea. For most this would be a regression. The old functionality at least triggered their general exception handler, whereas the new one might do something else as nil from a key that was originally assumed present is propagated through the system. The best outcome they can hope for is a new stacktrace caused by a NPE with the same outcome.

I don't think anyone really cares about the exact response given to the Hackerone script kiddie, that handcrafted a request with a malformed filename, beyond avoiding a stacktrace in their logs. We could trim this down, to avoid committing to an approach that is likely to be temporary, and just do a try/catch with status 400. This would still be a step in the right direction while general error handling is being worked on.

[weavejester]

If the new middleware isn't going to be extensible, then I think I'd prefer the current implementation for reasons of code locality and because it has fewer moving parts.

I'd prefer not to have error handlers on every piece of middleware. Both synchronous and asynchronous middleware can raise exceptions, and exceptions are designed to shortcircuit logic. My thought is that a global layer to handle turning exceptions into responses would be a better solution than a piecemeal set of options.

I'm not sure continuing without :filename is a good idea. For most this would be a regression. The old functionality at least triggered their general exception handler, whereas the new one might do something else as nil from a key that was originally assumed present is propagated through the system.

I'm not sure how often :filename is used, but you're right that a missing key would interact unexpectedly with functions like str, for instance.

In which case let's just omit multipart parameters with invalid filenames. This is consistent with how other middleware handle it. In future an option can be introduced that explicitly raises exceptions on invalid data.