ok will test this weekend

At face value I'm -1 on this because it changes the data to be pulled
explicitly from a given global to the $_REQUEST global. In at least the
login method it seems preferable to use an explicit global and check the
request method because certain request types won't support login anyway
(why should it work with a DELETE /login request?).

On Friday, July 1, 2016, Demis Palma ツ notifications@github.com wrote:

Preamble

HTTP methods includes HEAD, PUT and DELETE in addition to the classic GET
and POST.
Among them, the HEAD method is largely used by uptime monitor programs or
services that check whether our website is still alive.
Problem description

The HEAD method causes the crash with a fatal error in two components:
com_users and com_mailto.
The reason is the attempt to explicit access a property of JInput that
depends on the HTTP method used.
$input->$method->get(...);
In details,
when the HTTP method is GET, it becomes $input->get->get(...);
when the HTTP method is POST, it becomes $input->post->get(...);
when the HTTP method is HEAD, it becomes $input->head->get(...); which
does not exist and causes a fatal error.
Summary of Changes

I've replaced the explicit method call $input->$method->get(...); in
favour of implicit method call $input->get(...); letting JInput choose
the appropriate input channel (GET or POST) that is one of the task that
JInput has designed to do, and it does it well.
Testing Instructions

1. telnet localhost 80

HEAD /index.php?option=com_users HTTP/1.1 (Hit ENTER)
host: localhost (Hit ENTER twice)

PHP Fatal error: Call to a member function get() on null in
[...]/components/com_users/models/login.php on line 62

1. telnet localhost 80

HEAD /index.php?option=com_mailto HTTP/1.1 (Hit ENTER)
host: localhost (Hit ENTER twice)

PHP Fatal error: Call to a member function get() on null in

[...]/components/com_mailto/views/mailto/view.html.php on line 57

You can view, comment on, or merge this pull request online at:

#10988
Commit Summary

• Fixed JInput calls

File Changes

• M components/com_mailto/views/mailto/view.html.php https://github.com/joomla/joomla-cms/pull/10988/files#diff-0 (3)
• M components/com_users/models/login.php https://github.com/joomla/joomla-cms/pull/10988/files#diff-1 (3)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/10988.patch
• https://github.com/joomla/joomla-cms/pull/10988.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#10988, or mute the thread
https://github.com/notifications/unsubscribe/AAWfoYKSedYlLW_cbgV5sGIFiFydZzkmks5qRUk8gaJpZM4JDUlg
.

@mbabker, I'm not sure to have understood your objection. Could you please provide a simple example and explain what can go wrong with that exactly?

$input->get() pulls data from the $_REQUEST superglobal, whereas
$input->$method->get() goes for a more specific source. That's part 1.

Part 2 is why are we allowing our login method to work with all HTTP
requests? Why not make some sanity check and only allow certain methods
with a sane response for unsupported methods. Unless someone can
demonstrate valid cases, I don't see login ever needing to support PUT,
DELETE, or PATCH (I'd say HEAD too but I'm honestly not as familiar with
what that request type's purpose is).

On Friday, July 1, 2016, Demis Palma ツ notifications@github.com wrote:

@mbabker https://github.com/mbabker, I'm not sure to have understood
your objection. Could you please provide a simple example and explain what
can go wrong with that exactly?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10988 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAWfob_BHZe7ix0MnDJa57Bkk_K9jKgKks5qRZ6_gaJpZM4JDUlg
.

The HEAD method is identical to GET except that the server MUST NOT
send a message body in the response (i.e., the response terminates at
the end of the header section). The server SHOULD send the same
header fields in response to a HEAD request as it would have sent if
the request had been a GET, except that the payload header fields
(Section 3.3) MAY be omitted. This method can be used for obtaining
metadata about the selected representation without transferring the
representation data and is often used for testing hypertext links for
validity, accessibility, and recent modification.

Source: https://tools.ietf.org/html/rfc7231#section-4.3.2

Michael,
to me, the responsibility of dealing with low level, input related details (such as the HTTP protocol currently in use), should be delegated to JInput.

Being aware of low level details at component level is a bad design pattern and inevitably causes duplicated code.
There is nothing bad with duplicated code per se, except that it often causes bugs. A fresh example

Take a look to the files changes in this PR. Do you consider normal having duplicated code (with the same bug) among a view and a model? And I stress the difference between a view and a model.
Not to mention at least another 4 occurrences that I haven't fixed because, thanks to a series of lucky coincidences, the values assumed at run-time by the data does not raise a PHP fatal error, unlike com_user and com_mailto do.

In conclusion this PR is limited to fix the problem itself (the PHP fatal error), without any pretence of improving JInput, but I'm ready to test and confirm an alternative fix made by you, just to close this bug.

As a side note, I am fixing other PHP notices, warnings and errors that I'd like to submit as pull requests.
In this regard, for my own information, have you already planned when you'll be on vacation? ?

to me, the responsibility of dealing with low level, input related details (such as the HTTP protocol currently in use), should be delegated to JInput.

That's where you and I (and others from past discussions) differ. Controllers should not be environment agnostic; if anything they should have the most awareness of the environment (i.e. JInput in the case of Joomla) of anything at the component level and the models and views should be blissfully ignorant to it. So it would make perfect sense to me for the controller to check this detail and decide that the request type is unsupported and give back an appropriate response (in Symfony this would be a MethodNotAllowedHttpException issuing a 405 response to the requestor thrown either by the controller or the router if it is configured with supported methods).

Granted, the code in question is a model (in the case of the login change), but the fact it's being executed tells me that we still have issues with tight coupling of the model to the request/environment and that a controller is failing to do its job. So in that regard while the change is somewhat correct, it still sucks being completely blunt about it. It's why I'd rather see it changed "correctly" (YMMV) versus a hotfix to deal with an immediate issue that shouldn't be an issue at all.

In this regard, for my own information, have you already planned when you'll be on vacation? ?

Actually, yes. I'm going to try and take a week off in September for an international trip. Odds are I will probably be working during it though because the only way I know how to truly vacation and disconnect is to go out in the mountains and hike 40 miles with nothing but a camera and a GPS from a technology perspective (has it really been 4 years since my last "vacation"!?).

So what about using $input->post->get(...); instead of $input->$method->get(...);?
If you want to be specific, the first is more specific that the latter.
Nevertheless, the login always expects the credentials to be in $_POST, isn't it?

Presumably login could also support GET requests (though that'd be less favorable for obvious reasons). And we might run a B/C risk with trying to lock it down to only POST (then again the same argument can be made for any lock down in general). I'd say supporting GET/POST is reasonable enough to leave the $input->$method->get(...); structure in place if we're only supporting those two methods. If we don't do that then honestly the only choices are either if conditionals based on the method or just always pulling data from $_REQUEST and hoping no issues come from that.

so @demis-palma is this for test or not?

Setting to information required awaiting a reply from @demis-palma

Otherwise this will be closed in a few weeks.

After further analysis, I confirm that this is the preferable patch to me.
Thanks for asking, and for waiting.

About the usefulness

I just want to add that the HEAD method is also used by client-side cache systems and proxies, in order to understand whether the resource is expired or not.
For that reason, it is a quite common HTTP request, and I have several PHP fatal errors in my apache error log caused by that.

About the code

The "if conditionals based" structure proposed by Michael would produce exactly the same effect than taking the input always by the $_REQUEST superglobal with $input->get():

if method is GET, then take the input from $_GET
else if method is POST, then take the input from $_POST
in any other case, ignore the input, because it hasn't arrived at all due to the HTTP method

The main logic and the effects are the same in both cases, but we can simplify all those instructions in favour of one simple statement: $input->get()

That's why I confirm the PR as is.

@mbabker decision time

I'm still not a fan because by the arguments given here we should remove support for the GET and POST superglobals and always just read from REQUEST. To me there is a reason these (and other methods) prefer to not read data from both globals and our code should be updated to accommodate this, not move in a direction where it can work in undesired ways.

With our existing code structure, there is not a valid reason to support a GET request for logging in. any controller using that model should be 405'ing before that model can read the request. Instead of addressing the issue of invalid methods making a login request, this change allows me to issue a DELETE request to the login route and I'd be logged in no problem.

If someone else wants to merge it, I won't stop them. But I absolutely do not support this methodology of fixing this particular issue.

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/10988

closed as stated above. It can always reopened if needed.