Page MenuHomePhabricator

Insufficient param validation
Open, MediumPublic

Description

LQT has the following declaration:
···
'type' => array (
ApiBase :: PARAM_DFLT => 'newthreads',
ApiBase :: PARAM_TYPE => array( 'replies', 'newthreads' ),
ApiBase :: PARAM_ISMULTI => true,
),
···
With invalid input the value of 'type' is array(). This is unexpected. It should either complain that the value(s) are not one of the allowed values (preferred), or use the default value 'newthreads' (less preferred). PARAM_ISMULTI seems to bypass the regular checks.

This causes exceptions in other code in LiquidhThreads which excepts that the values are sane. I believe this is an issue that should be fixed in core.


Version: 1.20.x
Severity: normal

Details

Reference
bz39830

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:52 AM
bzimport set Reference to bz39830.
bzimport added a subscriber: Unknown Object (MLST).

A wrong value is removed from the values and a empty array is threated like a valid value. For core you can see this with prop=revisions. When giving an empty rvprop= or a rvprop= with wrong value, that results in the same output, because no of the properties are requested. Requesting no properties makes no sense, but is valid at the moment (no need for b/c).

In my opinion is there no problem, because it is not required that a value must be set and than the empty value is ok.

But setting PARAM_REQUIRED = true has no effect. That is the bug, in my opinion.

(In reply to comment #1)

But setting PARAM_REQUIRED = true has no effect. That is the bug, in my
opinion.

Commited gerrit 52987 for this

(In reply to comment #2)
Commited Gerrit change #52987 for this

Abandoned for the moment, does not work as aspected.

Still there is a patch in gerrit that somebody could pick up, hence restoring patch-in-gerrit, keyword.

Anomie set Security to None.
Anomie added subscribers: Aklapper, Anomie.

Still there is a patch in gerrit that somebody could pick up, hence restoring patch-in-gerrit, keyword.

While there was a patch, it's not something someone could "pick up". The approach there turned out to be unfeasible, and while the review there has useful information for someone writing a new patch it would in fact be an entirely new patch.