sorry messed with the suggestion feature please see my new suggestion.

It should be noted that the Itemid is always an integer.

Do you have a special page in mind where we should add such notice?

This looks like a bad hack in my opinion

I still think an Exception and aborting is the appropriate behavior here. This still accepts invalid input data, the array just needs to be flat out rejected.

I think throwing an exception should be done on J4. In J3 I would be a bit more tolerant :-)

Do you have a special page in mind where we should add such notice?

Do we have a JDoc which covers the Itemid?

I think throwing an exception should be done on J4. In J3 I would be a bit more tolerant :-)

Why? Where else in Joomla is invalid input being manipulated like this? Is it not standard behavior to reject actions with invalid input data?

https://www.joomla.org/index.php?option[]=com_content&option[]=com_contact&Itemid[]=101&Itemid[]=435&format=feed&type[]=rss&type[]=atom&lang[]=en-GB&lang[]=en-US is clearly an invalid URL. Your idea to take the first element of the array as the input, while tolerant, is allowing the system to run with completely invalid data and unexpected results. Just from this URL which converts several system request variables to arrays:

• What component is expected to be executed?
• What menu item ID is expected to be used?
• What feed renderer is expected to be used?
• What language is expected to be used?

Similar to a POST request rejecting invalid data (either because of invalid format or failing validation checks), Joomla should reject requests which are clearly trying to mangle the system input variables in a way the platform does not expect them to be. I would hard abort instead of giving any grace to what basically looks like a hack attempt.

You are right though that initialiseApp is the right spot to do this type of check in, if not even before that method runs (because the lang variable should be checked too for an array). Checking these system variables at this stage of the request cycle won't conflict with variables that aren't present (i.e. if SEF URLs are in use as those will be defined by the router later).

// Inside `Joomla\CMS\Application\CMSApplication`
public function execute()
{
    foreach (['option', 'view', 'format', 'lang', 'Itemid'] as $systemVariable) {
        if ($this->input->exists($systemVariable) && is_array($this->input->getRaw($systemVariable)) {
            throw new \RuntimeException('Invalid input, aborting application.');
        }
    }

    $this->doExecute();

    // ...
}

I have to agree with @mbabker it is clearly an invalid request that should be rejected. including the lang param as suggested.

Right now that invalid data would result into an issue down the road anyway.

Do we have a JDoc which covers the Itemid?

We might should prepare a doc page than to be released in the FAQ for the release that includes that change.

here is a suggestion for the docs page to be published with the release this change is shipping.

<!-- Start with an intro below this line -->

==Errors reported== <!-- Fill errors below -->

Joomla exits with the error message `Invalid input, aborting application.` after the update to 3.9.xx on special requests.

==Versions affected== <!--refers to and other information below -->
{{tip|This pertains only to Joomla! version(s): ''' >=3.9.xx'''|title=General Information}} <!-- add the version(s) you need -->

==What is the cause== <!-- Cause if known -->

With Joomla 3.9.xx we added checks to make sure the itemid and lang URL parameter can never be an array.

==How to fix== <!-- How to fix it if known -->

As this is an expected behavior and the old behavior cloud have triggerd PHP warnings that could possibly result into an [https://owasp.org/www-community/attacks/Full_Path_Disclosure full path disclosure]. The only valid patch is to not try to pass the item ID or lang query parameter as array. Please only pass one integer als itemid and one language to the lang parameter.

__NOTOC__
<!-- Change if needed -->

cc @SniperSister

Based on https://volunteers.joomla.org/departments/production/reports/1226-production-dept-meeting-april-21-2020 I need to say something before people go and completely over-engineer something.

First off, it is not some "magical" behavior and a little respect for the developers who made the class consistently handle arrays would be welcome (as prior to that change if you gave the class an array and an explicit filter that it supported then it would only clean and return a single item, whereas an unknown filter would clean and return the full array)

Secondly, InputFilter::clean() should NOT have any additional parameters added to it. The concept is pretty simple behind the behavior. If you give it an array, it will clean all items in the array with the given filter. If you give it a single value (typically a string), it will clean that single value. This class should NOT be wired up in a way where if you give it a single item it aborts because you are looking for an array, or vice-versa; that is a major violation of SRP and the method's design which is to clean the provided input value.

As for a flag on Input::get(), I personally would not encourage this. I think it should honestly just hard throw, there is already a getArray() method (which, in all honesty, comes across as a bit busted for retrieving a single key as an array) and that method should be encouraged over a flag. For the record, symfony/symfony#34363 tries to deal with this type of behavior for Symfony's Request object by deprecating array support in the single get() method in favor of a dedicated method for fetching a key's array values.

In truth I would actually suggest deprecating joomla/input and anything inheriting from it. I've found enough design flaws with the API (hint, try doing $input->get->post->server->get->request->env->post->get('foo'); and analyze every object in the result chain) that I actually started writing a Joomla style Request object (think Symfony or Laravel for these) as a proper abstraction to replace it, but never got around to completing it because time and effort are a lot to do it right.

Thanks for the feedback Michael. Will make sure it is beeing looked into! Thanks

Is the wip code up somewhere for that replacement so i could take a look?

https://gist.github.com/mbabker/33a92012383ed733d1bcf2642cc41d3d has a bulk of the initialization logic for everything but the files store and parsing the raw request body to more easily support request methods with data beyond POST (i.e. PUT). There are probably other helper methods that could be added to it (i.e. Request::getMethod() to map directly to Input::getMethod()), but the point that I had gotten to hadn't really looked at use of it as a consumer as of yet. The initialization logic and the registry design isn't too different from Symfony's Request object, it just adds a compulsory InputFilter to the constructor to allow Request::filter() to work correctly.

Awesome will take a closer look into that gist in the comming days. Would you mind uploading it as repo so you can get PRs for that?

https://github.com/mbabker/jfw-app-request provided as is

Thanks will take a closer look into it.

So as for 3.x imo we should go with that PR here. And for 4.x we should try to find a better aproach for that stuff. Do you agree?

Cc @SniperSister @HLeithner @mbabker ?

So as for 3.x imo we should go with that PR here. And for 4.x we should try to find a better aproach for that stuff. Do you agree?

Agreed

So as for 3.x imo we should go with that PR here. And for 4.x we should try to find a better aproach for that stuff. Do you agree?

Better then nothing even if I don't like it. We really should remove the array (auto detect) in inputfilter.

There is nothing to remove from InputFilter!!!

Give it an array, it cleans and returns an array.

Give it a single scalar (string or int usually) it cleans and returns a single scalar.

The only reason people seem to think this is a problem is because the most common use case is that API cleaning request data and depending on how you mangle a request you can cause something to be an array that shouldn’t be. That shouldn’t mean you remove array support from the cleaning API, that means you as a developer need to validate your input. Just because input has been cleaned does not automatically mean it is valid for use, and the Input API has NEVER made a guarantee of valid input, only cleaning (sanitization).

I run $input->get('id', 1234, 'int'); I expect an integer in any case and never an array that's a design flaw of the API.

The function it calls is called FILTER so I expect it's filtering and doesn't do magic array detection, which basically could lead to a security problem.

I know this and my main intention is to remove array auto detection support from InputFilter::get(). That doesn't mean that it should be removed completely because of the reason you mentioned we need support to get arrays from the super globals. For this a parameter for get is a possibility (which as already been rejected by production) or to add a new parameter to jinput like InputFilter::getArray() (I know this function already exists and does also something you wouldn't expect) so use InputFilter::getArrayFiltered('id', [], 'int');

In both cases input or InputFilter::clean() can throw an exception (or does something similar) if the data has the wrong format.

That's what I would expect from an dedicated framework helping me to get user input.

Please be clear on what classes you’re talking about. There is no Joomla\Filter\InputFilter::get(), are you meaning Joomla\Input\Input::get()? I know this seems like a nitpick but it’s two very different classes with two very different purposes (even if one relies on the other).

I still think a validation step in those two classes is the wrong API design. The Filter API absolutely shouldn’t validate, and if it were to do so it needs to be a method outside of the existing clean() method with more logic than “reject array of wanting single item or reject single item if wanting array”. The Input API, while it can be argued could support validation, I would suggest just needs flat out replacement (as I already said) instead of trying to monkey patch validation logic into it with the API B/C breaks that would create. TBH that’s actually the least of my concerns with the Input API (whether it validates or not), I still think there are enough fundamental issues with the API that it needs a full-on replacement and if people are going to take that part seriously then I think everyone’s better off documenting the existing code’s behaviors and adjusting their code to match (add is_array() checks where necessary).

Sorry for the confusing for the different classes.

You mentioned in your comment that symfony does exactly the same 16 days ago, they remove array support form the get function (if I understood the PR correctly)

For the record, symfony/symfony#34363 tries to deal with this type of behavior for Symfony's Request object by deprecating array support in the single get() method in favor of a dedicated method for fetching a key's array values.

and added an alternative way to get arrays.

So why shouldn't we do the same?

For me Joomla\Input\Input::get() is only a proxy to Joomla\Filter\InputFilter::get() and InputFilter has the name Input (which is most likely user provided content in our case) and Filter so it should filter invalid input (by removing it or throw an exception or what ever) and if you call Joomla\Filter\InputFilter::get('id', 0, 'int') or Joomla\Input\Input::get('id', 0, 'int') doesn't really matter you (ok not you because you know it) or at least an inexperienced developer starting with Joomla would never expect an array of INT.

If you think we should thru Joomla\Input\Input::get() and Joomla\Input\InputFilter::get() away and replace it with symfony or any other framework that doesn't give you a wrong type back is also a possible way.

And this PR is result of this design flaw, we need a global check before we use our own input filter mechanism because even core developers didn't get right to use Joomla\Input\Input::get() correctly.

I'm not saying that Joomla\Filter\InputFilter is bad. It works for exactly what it does.

Joomla\Filter\InputFilter::clean() (there is no get() method in that class) should NOT have any type of validation logic added to it. The method name explains exactly what it is doing, it is cleaning the input. The logic behind cleaning is based on the class configuration through the constructor (mainly impacting HTML cleaning) and the supplied filter. To me, based on this API design, this is the wrong place to introduce any form of validation step.

Joomla\Input\Input::get() is a getter method, which reads data out of an input source (the array that is set in the class' constructor, typically one of the superglobals but it can work with any user supplied array, such as the data source for $app->input->json being to read file_get_contents('php://input') then convert it into an array). Getters by design are not meant to do any validation, this is a SRP violation.

Adding flags to method signatures generally indicates that a method has too many separate responsibilities, so any type of flag to Joomla\Input\Input::get() is to me a red flag (no pun intended) that something was designed incorrectly. Also, just to re-iterate, Joomla\Input\Input and its subclasses can operate on any user provided array, it's really not much more than a single level key/value store except it has default and magic behaviors which operate primarily against the request data (superglobals). So even though the class' primary design is to work with the superglobals, it can't be assumed that that is its only behavior.

So to me, that means you need a validator in front of $app->input->get() if you are going to be insistent that the application provide something to arbitrarily reject arrays by default and make developers bend over backwards to use array inputs where intended, maybe an $app->filter(string $key, $default, string $filter, bool $arrayAllowed) method gets introduced to the web application chain (this is unnecessary as of 4.0 for CLI context, Symfony's console input classes are used, and to be frank CLI input should not be handled the same way as HTTP input and trying to do both in the same interface will eventually cause problems, this is another Joomla\Input\Input flaw that naturally goes away).

As for how Symfony is changing their Request data stores to deprecate array support, in some ways it works because it means a developer has to go to the explicit store the data is in and request it as an array. My Request::get() is similar to Symfony's, so this is an easy point of comparison to discuss. So right now, one could do $request->get('id'); and it'll return either single value or an array by checking the data stores in an explicit order (first for any custom attributes defined while the request is executing, which avoids pollution of the original data sources (superglobals), then the GET data, then the POST data). For comparison, $app->input->get('id') by default reads from $_REQUEST with no questions asked, so your data source could possibly be manipulated by how PHP seeds that superglobal. Their deprecation means that this won't work anymore, and you would have to explicitly call $request->query->all('id') to get the array of IDs passed as part of a GET request (loosely similar to $app->input->get->get('id', [], 'array'), but from what I remember this just returns the input as an array with no cleaning, at least $app->input->get->getUint('id', []) would clean all the array values).

This has both positives and negatives, but it does force the developer to think about their input validation a little bit more (which is kind of what I've been arguing for since the beginning, you can't rely on something that was sanitized to be valid). So, my Request::filter() would work just fine on single values with this API design in mind, but everything about my request registries (which are somewhat based on Symfony's ParameterBag with the use of Joomla\Filter\InputFilter for filtering instead of filter_var(), obviously prior to the PR we're discussing) would need re-thinking if you're going to force get() to only be allowed to return a single value (as the filter() method would also need to be aware of this design and be able to act appropriately). And for the record, yes filter() and get() should co-exist with the former being the preferred method, knowing full well that the latter reads raw request data (similar to $app->input->getRaw()), but there are indeed legitimate use cases where one would need to read the data out of the request and then filter it in a way not supported by the Joomla\Filter\InputFilter class (i.e. HTML from a textarea or WYSIWYG based editor which generally goes through Joomla\CMS\Component\ComponentHelper::filterText() to do configuration aware filtering based on the current user's groups and the filtering global config).

Ok so in the end we want more or less the same, I think the main difference is that you don't want to make a difference between string and array of string.

2 questions about your implementation of Request

1. Why do you thing that a string and a array of string is the same (ok that has not much todo with the implementation but uses the same pattern like Joomla\Input\Input::get())
2. Do you think a generic Request::Get() function with a mixed return type is a good idea going more the way into strict typing, especially for base variable types like string|int|float...

I'm a fan of declare(strict=1); and strict typing as much as possible, having an input filter function returning directly this typed properties would be a win (at least for the possible one).

Generic Request::get() still has value. Likewise, you can’t type a Request::filter() that returns the value from InputFilter::clean() since that has a legitimate mixed return (PHP 8 union type would be array|bool|float|int|string, no objects and it’s not nullable as you can get an empty string in some HTML filter cases). Also consider that everything in the superglobals is a string and it’s not until you start processing it that you get a typecasted value. I don’t know if I’d explicitly add typed helper methods as you end up with 10 or so extra methods that are just calling a named filter, double that if you have explicit single or array methods, or you’d still have union returns with something like function getInt(string $key, array|int $default = 0): array|int.

I don’t really think it’s the request helper’s role to do array or single validation (be it existing Input API or a new Request object), I really do think that is the dev’s role to validate after retrieving the sanitized data. In part because the Joomla API doesn’t make use of inbuilt PHP filtering through filter_var() (which does do basic level sanitization AND validation), and because the existing InputFilter class was not designed to support validation, only sanitization. If the APIs are designed to support both then it becomes easier to build out in a way where you tell developers “do this if you want an array value from POST” (which, you have to do in core anyway to handle form submissions since everything is under the "jform" key by default) and “do this if you are expecting a single value which will cause a XException if it fails this validation check”.

Maybe it’s time to revisit the Filter package’s use as a whole in Joomla. Is it really more reliable than core PHP filter_var() for the basic scalar filtering? What about more unique cases like the path or base64 filters, how does that get handled? And for the HTML cleaning, what about other third party libraries instead of what core has now, can they function with Joomla’s existing filter configs to sanitize adequately? If you can start offloading the custom class handling, maybe you can come up with a way to introduce validation behaviors alongside the sanitization behaviors in a way that doesn’t force an API designed to only support sanitization to not become a clunky mess because validation was introduced.

Either way there are a lot of ways forward, it’s just a matter of finding one that is reliable for the ecosystem.

I guess all of this should be done on J4. I would like to focus on the issue on J3. I'v tested it with throwing an exception. The problem there is that the error page expects an integer as Itemid and it loads also some modules which do crash because of the array as Itemid. So if you want to abort then I guess a die is the only practical solution to get rid of the issue we have now that Itemid is an array.

How about setting Itemid to null, 0 or default menu item's ID before throwing exception?

That would work.

Changed the code to abort the request and unset the invalid variables. Feedback welcome.

template and templateStyle should also be added. At least for site app. And maybe tp? view looks like it should belong in MVC layer, not all components use core MVC and thus view input variable. Otherwise might as well add task.

Added the variables from @SharkyKZ.

@HLeithner how you want to proceed here?

Still not happy doing this in a bugfix release (people doing stupid things), also you should decide if we filter or throw an exception. I don't think we need to set the variables if we throw an exception (which can not be catched by a 3rd party dev).

If we don't set the variables, then you will get errors on the error pages. I would also prefer not to set them. Test it and then you will see that the modules on the error page will use Itemid for example as well.

@zero-24 are you ok to add it for 3.10? As I share the concern of @HLeithner to ship it with a patch release.

Sure feel free to send it against 3.10

Rebased to 3.10-dev.

Merging for now thanks @laoneo ?

This has been documented here: https://docs.joomla.org/J3.10:Invalid_input_aborting_application