Slim3: XML _METHOD override throws InvalidArgumentException

[rbairwell]

Given the test XML:

<test><person><name>Josh</name></person><_METHOD>HEAD</_METHOD></test>

I would expect Slim's method override (in Slim\Http\Request) to change the request type from the original to "HEAD". However, passing that string in as the request body (with an application/xml;charset=utf8 content-type) results in an error:

InvalidArgumentException: Unsupported HTTP method; must be a string, received SimpleXMLElement

/vagrant/..../vendor/slim/slim/Slim/Http/Request.php:308
/vagrant/..../vendor/slim/slim/Slim/Http/Request.php:243

A "quick fix" for this seems to be changing \Slim\Http\Request::getMethod

$this->method = $this->filterMethod($body->_METHOD);

to

$this->method = $this->filterMethod((string)$body->_METHOD);

I have not provided a PR as I'm not 100% confident that is how you are expecting people to override the request method in XML and I'm not confident the fix is the best solution (I dislike typecasting unless really really necessary) [If I get time at the weekend, I'll see if I can do something though]

[PHP7, composer installed]

[geggleto]

I think the above solution is fine. This was just an oversight on our part. I don't think I have seen anyone use the XML portion yet. This could also be somewhat related to the XXE vuln that Akrabat patched,

If you could please PR this and add a Unit Test for this as well that would be wonderful. The unit tests obviously did not cover this test case or we would see the build failing.

Good catch! Thank you!

[codeguy]

:+1:

[codeguy]

I would just make sure that we agree on where the method override element and/or attribute exists in the request body XML. Should be a child element named "method" beneath the root element? Should it be in an attribute on the root element? Or both?

[codeguy]

Or should we just say it's better to rely on a request header for this?

[geggleto]

Both imo... I actually created a middleware to handle it, it felt better than having it in the slim source.

[rbairwell]

I actually got half way througg writing my own middleware too: but keyed off slightly different fields: then found out if _METHOD was set in the body it would always override my call to setMethod.

I would personally prefer this sort of "magic" to be non-core On 1 Dec 2015 16:39, "Glenn Eggleton" notifications@github.com wrote:

Both imo... I actually created a middleware to handle it, it felt better than having it in the slim source.

— Reply to this email directly or view it on GitHub https://github.com/slimphp/Slim/issues/1637#issuecomment-161025334.

[akrabat]

Simplest solution possible at this stage.

$this->method = $this->filterMethod((string)$body->_METHOD);

is the change we need.

[akrabat]

In an ideal world though, we wouldn't support _METHOD at all!

[geggleto]

I am okay with removing support for _METHOD, in favour of a slim middleware

[codeguy]

If we remove this to middleware, it should be a first-class middleware and available as soon as 3.0 launches.

[geggleto]

@codeguy wanna review this? https://github.com/geggleto/method-override

[codeguy]

@geggleto Should also support the X-Http-Method-Override header.

[geggleto]

Will update that tonight if I get a chance.

[geggleto]

@akrabat @codeguy https://github.com/geggleto/method-override/releases/tag/0.1.0 updated to include the header override

[akrabat]

So we're going to remove _METHOD support in Slim 3 and document using the middleware? If so, are we going to adopt the middleware to the slimphp organisation?

[geggleto]

I leave it up to you guys ;) matters not to me :D

[codeguy]

Moving this to optional middleware is fine, imo. It'll have a marginal speed increase, and not everyone needs this functionality. But it should be moved to the @slimphp organization if it's going to be extracted. @geggleto I'll add you to the @slimphp/maintainers team if this sounds good to you.

[designermonkey]

Personally, I would leave this until a 3.1.0 level release. Release Candidates are complete functionality, that only expect last minute bug fixes. The proposed change here goes against this as Slim 3 is in an RC stage.

Please reconsider this for a minor version update to keep with semantic versioning.

(Sorry to be a stickler on this for a second time).

[akrabat]

Tricky isn't it? If we don't remove it now, then we're keeping it until 4.0.

[rbairwell]

All I can say is sorry guys for starting this tricky conversation... My primary preference is to rip out the "magic" and replace it with middleware, my second preference is to remove the "Force method override recalculation" setting in Request::withMethod (so Slim always uses what is "forceabiliy set/set by other code") - the second option allows "patching" of the existing code, but a way for devs to "bypass" it on preference (which isn't possible atm).

[akrabat]

my second preference is to remove the "Force method override recalculation" setting in Request::withMethod (so Slim always uses what is "forceabiliy set/set by other code")

I am leaning towards this change on the grounds that if you call withMethod('POST'), it's reasonable to assume that getMethod() will return 'POST'!

[akrabat]

PR #1655 to fix the string issue with XML _METHOD and also to ensure that withMethod() does what you expect.

[akrabat]

I also don't mind removing the _METHOD detection code altogether, but I don't have enough strong feelings either way as long as there's a way to turn it off and $request = $request->withMethod($request->getOriginalMethod()); will now do so.

[geggleto]

@codeguy sounds fine to me. @akrabat I would think that 3.1 might be a more proper place to put this. Is there a way to can mark it as going to be deprecated in 3.1 ?

[akrabat]

The way to put it into 3.1 is if we add the middleware automatically and have a setting to disable it. i.e. an application that uses _METHOD parameters must continue to work with no code changes in 3.1 regardless of whether it is marked deprecated.

(We would also have to be careful to ensure that if we were adding middleware automatically, we didn't break someone using $request = $request->withMethod($request->getOriginalMethod()); in their own middleware to turn off the current system.)

[geggleto]

I don't know ... that seems like replacing magic with more magic ?

[akrabat]

shrugs This is how SemVer works.

[akrabat]

Closing as can't remove until 4.0.