HTTP method outside whitelist causes 500

[iansltx]

(picking up from the Slack discussion earlier)

If an incoming HTTP request has a verb other than something in Request::$validMethods (e.g. LINK/UNLINK...or CHECKIN...or CHICKEN), Request::filterMethod() throws an InvalidArgumentException on request create from env (which happens on App::run()). This isn't caught anywhere, so it bubbles up to a 500. This happens both with and without a route declared for that URL, that method (yep, the router will let you match on CHICKEN), or neither/both.

By contrast, a verb in $validMethods, but unassigned for a route, will call the notAllowedHandler, which seems like the more proper way of handling the above. Likewise, if you don't have a route for that verb, probably better to call notFoundHandler.

There are a couple catches here:

1. notAllowedHandler assumes you've matched with a route before calling. Which needs a request. The only way I can think of to handle this properly is to set up an exception that builds the request anyway, then throws something that extends \InvalidArgumentException and includes the request along with it. That gets thrown into the router, then we decide how to treat the error (we'll always be erroring out) based on what the router gives us back. A little convoluted, but should give us the behavior we're looking for.
2. The router doesn't enforce the allowed methods array at all, so if we take the above fix then we'll end up with an even more confusing error message (LINK not allowed, please use one of [GET, PUT, POST, LINK]) if someone matches to an unsupported verb. So we'd want to tweak the router to throw an InvalidArgumentException (which should throw us into a 500) when someone throws a verb in there that doesn't make sense.

Does the above concern make sense to someone other than me, and do the miticgations I'm suggesting also make sense? Maybe fully supporting this corner case at the cost of N operations per verb per route match() call doesn't make sense perf-wise, but I'd argue that having a really easy way to make a Slim app 500 is odd too.

If everything makes sense, thumbs-up this and I'll work on a PR.

[geggleto]

This is a pretty complicated issue as we need to preserve BC for SemVer and I think a proper solution would break it.

Minimally I am +1 for allowing the modification for the $validMethods on the Request object. Exposing an add method could be done in a new release, and would effectively solve this particular issue. The user base can then add custom XYZ methods by replacing the Pimple Factory in the container prior to App execution ie ($app->run()) with a new factory that returns a Request object augmented with whatever custom HTTP Verbs they require.

In this particular instance the LINK/UNLINK was included in a draft spec of HTTP 1.1. It was not apart of the official HTTP 1.1 spec, however it is possible that there are programs/protocol/etc that use the LINK/UNLINK HTTP verb.

This particular issue, is uncommon... but seems to occur a few times a year.

The current work-around is to extend the Request class and add the required HTTP VERBS to the protected member, then replace the Pimple Factory for the Request object with your modified one.

This is related to: #1927

[iansltx]

Yeah, catch with the valid methods add piece is you have to hook into the from-env factory, since you're calling it and the valid methods array isn't static. Feels like, if we're going to only look at that as the use case, not doing anything and documenting the request container override option makes more sense (3 lines on container setup plus a class definition with the array override).

...and moving createFromEnvironment() into its own factory class doesn't remove the need to declare a new extended class to make this work, so that's no better.

[geggleto]

All while preserving BC :D woo!

[geggleto]

@iansltx I spoke with @akrabat you can PR this if you want ;)

[nickdnk]

Any news on this? :)

[iansltx]

Got hit with a load of work over the past few weeks. May be a bit before it lets up. Still planning on revising, but can't give a timeframe quite yet :(

[nickdnk]

Cool. Just checking. I'd do it myself if I had the slightest clue how to fix it correctly.

[iansltx]

@nickdnk Writing a (currently failing) test may help (aka light a fire under me...and save me some coding time) move this along. If you're game, hop on the slimphp Slack and maybe we can talk those bits through :)

[nickdnk]

Just FYI I have written the test. I argue for 400 and not 405 because 405 indicates the server knows the method but does not allow it on the endpoint.

[akrabat]

SeetestUnsupportedMethodUsedByClient()

[iansltx]

It took long enough, but I've got a 3.x compatible fix (with BC!...unless you were catching an InvalidArgumentException at App::run() outside of the standard error handlers) for the 500s. I haven't touched the routing side of things due to the potential perf impact and the potential of breaking the way folks have overridden this issue in the past (by extending Request and swapping out the container request impl).

See my commit notes for more detail on why I implemented the way I did. PRing in a sec.

[nickdnk]

Hey Ian

Great job fixing this :) I will admit I don't have the knowledge with Slim to go over the code, though.

But what's the argument for 405 and 404 in the tests?

405 indicates that the server knows the method but doesn't support it on the endpoint. Wouldn't it be 400 when using an unsupported method?

I realize that this hardly matters. Just curious.

On 4 Feb 2017, at 05.07, Ian Littman notifications@github.com<mailto:notifications@github.com> wrote:

It took long enough, but I've got a 3.x compatible fix (with BC!...unless you were catching an InvalidArgumentException at App::run() outside of the standard error handlers) for the 500s. I haven't touched the routing side of things due to the potential perf impact and the potential of breaking the way folks have overridden this issue in the past (by extending Request and swapping out the container request impl).

See my commit notes for more detail on why I implemented the way I did. PRing in a sec.

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/slimphp/Slim/issues/2005#issuecomment-277417130, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIBYsmLE-Jhgq42zS8Mbo14oZlkhQ1Xkks5rY_l2gaJpZM4KM53F.

[nickdnk]

Hey Ian

Great job fixing this :) I will admit I don't have the knowledge with Slim to go over the code, though.

But what's the argument for 405 and 404 in the tests?

405 indicates that the server knows the method but doesn't support it on the endpoint. Wouldn't it be 400 when using an unsupported method?

I realize that this hardly matters. Just curious.

On 4 Feb 2017, at 05.07, Ian Littman notifications@github.com<mailto:notifications@github.com> wrote:

It took long enough, but I've got a 3.x compatible fix (with BC!...unless you were catching an InvalidArgumentException at App::run() outside of the standard error handlers) for the 500s. I haven't touched the routing side of things due to the potential perf impact and the potential of breaking the way folks have overridden this issue in the past (by extending Request and swapping out the container request impl).

See my commit notes for more detail on why I implemented the way I did. PRing in a sec.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/slimphp/Slim/issues/2005#issuecomment-277417130, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIBYsmLE-Jhgq42zS8Mbo14oZlkhQ1Xkks5rY_l2gaJpZM4KM53F.

[iansltx]

Fair question.

Main reason I lumped this in with 404/405 is that allows the bad method and not found handlers to be used as declared, which is where you'd expect to handle cases where, respectively, a route doesn't exist or a route doesn't support a particular HTTP verb. Adding in a special "return a 400 if the method isn't on the list" case would either require adding another exception handler logic block (and tweaking the InvalidMethodException a bit from the way I have it now) or hard-coding a 400 response in a way that would be less configurable. So I went with the way that, if you make a CHICKEN call, you'll get back the same response as if you made a TRACE call (since effectively no one ever implements TRACE).

I can go into a little more detail on the above, but I feel like I'm blathering on already :p

Now, re: 400 vs. 404/405 specifically, there are decent cases for remapping the built-in notFoundHandler to return a 400 rather than a 404 anyway if you're working with an API, because the client is giving you a bad request (namely to a route pattern that isn't actually a thing). But semantically the notFoundHandler, which spits out a 404 by default now, is more of a routeDidNotMatchHandler so dumping things into there in this case seems to make sense...and then you can override that to get your 400 :)

[akrabat]

Fixed with #2141.