? ? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
23 Dec 2021

Code review

Actually a security issue fix too as the filters were not being applied.

avatar PhilETaylor PhilETaylor - open - 23 Dec 2021
avatar PhilETaylor PhilETaylor - change - 23 Dec 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Dec 2021
Category Unit Tests Repository Administration com_admin SQL Postgresql com_config com_contact com_content com_finder com_installer com_media NPM Change JavaScript
avatar PhilETaylor PhilETaylor - change - 23 Dec 2021
Labels Added: ? ? NPM Resource Changed
avatar joomla-cms-bot joomla-cms-bot - change - 23 Dec 2021
Category Unit Tests Repository Administration com_admin SQL Postgresql com_config com_contact com_content com_finder com_installer com_media NPM Change JavaScript
avatar PhilETaylor PhilETaylor - change - 23 Dec 2021
Labels Added: ?
Removed: ? NPM Resource Changed
avatar PhilETaylor PhilETaylor - change - 23 Dec 2021
Title
[4.1] Method call is provided 3 parameters, but the method signature uses 2 parameters
[4.1] Security. Input filters not being applied.
avatar PhilETaylor PhilETaylor - edited - 23 Dec 2021
avatar PhilETaylor PhilETaylor - change - 23 Dec 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 23 Dec 2021
avatar sandewt
sandewt - comment - 25 Dec 2021

See line 50:
$requestBool = $this->input->get('core', $this->input->get->get('core'));

Why not change this?

$requestBool = $this->input->get('core', $this->input->get->get('core', null, 'BOOLEAN');

null = ? => false or true ??

avatar sandewt
sandewt - comment - 25 Dec 2021

Actually a security issue fix too as the filters were not being applied.

Note: If you don't specify this parameter then a default of "cmd" is used. See https://docs.joomla.org/Retrieving_request_data_using_JInput

avatar PhilETaylor
PhilETaylor - comment - 25 Dec 2021

You fail to understand that the code as is currently is wrong with the method params in the wrong place completely - this is what this PR fixes

line 50 is out of scope of this PR as it is not suffering from the problem this PR seeks to resolve.

avatar sandewt
sandewt - comment - 25 Dec 2021

Really missed me.

avatar Fedik Fedik - test_item - 26 Dec 2021 - Tested successfully
avatar Fedik
Fedik - comment - 26 Dec 2021

I have tested this item successfully on 4e0f69a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36376.

avatar Fedik
Fedik - comment - 26 Dec 2021

Not much about security, a default filter is cmd.
So even with such typo it still was safe

avatar PhilETaylor
PhilETaylor - comment - 26 Dec 2021

Well you believe what you want then. Applying the wrong filter, assuming that you are applying a different filter, to unsanitized user supplied input, is a security issue, regardless if it can be exploited or not.

Input filtering is not about type casting - it’s about sanitising user input. That has everything to do with security.

It’s just another example of shocking code quality that that has apparently had two human tests and a maintainer review before being merged. Any IDE would point out that it was wrong.

avatar Quy Quy - test_item - 27 Jan 2022 - Tested successfully
avatar Quy
Quy - comment - 27 Jan 2022

I have tested this item successfully on 446c353


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36376.

avatar Quy Quy - alter_testresult - 27 Jan 2022 - Fedik: Tested successfully
avatar Quy Quy - change - 27 Jan 2022
Status Pending Ready to Commit
avatar Quy Quy - edited - 27 Jan 2022
avatar Quy
Quy - comment - 27 Jan 2022

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36376.

avatar bembelimen bembelimen - close - 31 Jan 2022
avatar bembelimen bembelimen - merge - 31 Jan 2022
avatar bembelimen bembelimen - change - 31 Jan 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-01-31 17:34:42
Closed_By bembelimen
Labels Added: ?
Removed: ?
avatar bembelimen
bembelimen - comment - 31 Jan 2022

Thx

Add a Comment

Login with GitHub to post a comment