Page MenuHomePhorge

Make PhabricatorSearchIntField validate its input
Needs ReviewPublic

Authored by aklapper on Jan 2 2024, 11:51.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 17, 02:44
Unknown Object (File)
Fri, Jul 12, 15:55
Unknown Object (File)
Wed, Jul 10, 08:09
Unknown Object (File)
Wed, Jul 10, 07:59
Unknown Object (File)
Tue, Jul 9, 04:33
Unknown Object (File)
Thu, Jul 4, 18:54
Unknown Object (File)
Wed, Jun 26, 06:39
Unknown Object (File)
Jun 18 2024, 09:56

Details

Reviewers
None
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15700: An "Integer" search field
Summary

Allow only positive and negative integer values as input in a PhabricatorSearchIntField.

Use getStr() instead of getInt() to allow differentiation in validateControlValue() between a literal value 0 versus 0 only returned by PHP casting a random string value to an int.
The validateControlValue() function does not use is_numeric($value) as this would also accept float values, and does not use is_int($value) as that is impossible due to the previous getStr().
Generally, the code is similar to PhabricatorSearchDateField.php.

See T15700

Test Plan
  • Make sure that rPfbe07fbeefcadea3abdb4dc3e4d4558c2b91ada9 is included
  • Go to /maniphest/query/advanced/ and enter example values into the Page Size field and click the Search button - get Integer value for "Page Size" can not be parsed. now for invalid example values.
  • In /src/applications/project/query/PhabricatorProjectSearchEngine.php, change the two only uses of PhabricatorSearchIntField to ->setIsHidden(false). Then go to /project/query/advanced/ and enter example values into the Maximum Depth and Maximum Depth fields and click the Search button.
  • Example values: 13, 0, -13, foobar, foo13bar, -foo1313, 13.5, -13.5, 13,5, -13,5, -, -0 (the latter is still accepted as 0).
  • API call /conduit/method/project.query/ does not offer passing depth values to test.
  • API call for /conduit/method/maniphest.search/ and entering a non-integer value as limit triggers an error but this code path does not call PhabricatorSearchIntField.php at all, so this is unrelated and a separate issue.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25502
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1216
Build 1216: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.Jan 2 2024, 11:51

Replace minus dash by its UTF8 representation to pass lint

Would anyone fancy reviewing this? :)

For the records, adding public function getValueForQuery($value) is needed because otherwise Expected a numeric scalar or null for %d conversion. Query: project.projectDepth >= %d

Is it possible to check if anything is relying on this not working with negative numbers?

src/applications/search/field/PhabricatorSearchIntField.php
17

None of the returns in validateControlValue return anything.

In D25502#15384, @speck wrote:

Is it possible to check if anything is relying on this not working with negative numbers?

In the current codebase, there are only two uses of PhabricatorSearchIntField in /src/applications/project/query/PhabricatorProjectSearchEngine.php and both make no sense for negative integers. Same for my intended third use in T15723.
If there was a future use case to allow negative integers, this check code be adjusted accordingly.

None of the returns in validateControlValue return anything.

Eh, good catch, thank you! I should remove the very last return, indeed.

It is supposed to fail only if "wrong" data is found by reaching the addError line, otherwise the check passes (via returning nothing). I basically copied pre-existing https://we.phorge.it/source/phorge/browse/master/src/applications/search/field/PhabricatorSearchDateField.php here.

Remove unreached return call as pointed out by speck

Would anyone give this another review? Might make sense to apply the one-liner in D25527 to have a simple test case on the Maniphest Advanced Search page

I'm pretty sure getValueForQuery() should return some value?

Maybe adding the use-case would be good.

src/applications/search/field/PhabricatorSearchIntField.php
24–30

Can't this just be is_numeric() ?

src/applications/search/field/PhabricatorSearchIntField.php
24–30

@avivey: Makes a difference for float values:
is_numeric((string)$value) allows entering e.g. the value 13.7 (resulting in 13 task results).
Current ctype_digit((string)$value) throws Query Errors - Integer value for "Page Size" can not be parsed.

Question is how lenient we want to be?

Add return value (though it seems to make no difference)

src/applications/search/field/PhabricatorSearchIntField.php
24

Stupid question:

In any case, I don't understand here, how is this able to store negative numbers, if we strip the sign. Thanks for the clarification

26

I don't know what is \xe2\x88\x92 but substr("\xe2\x88\x92123", 1) will not do what we want here.

Evaluate str_replace() instead.

E.g. this, even without if:

// Handle negative number strings by stripping the minus
$value = str_replace(array("\xe2\x88\x92", '-'), '', "\xe2\x88\x921238768");
28

This utility gives better error messages with incoming aliens

src/applications/search/field/PhabricatorSearchIntField.php
28

Edited: if a cast is ever needed, in any way, we should probably cast this earlier, since the whole method seems to assume to receive wild things and work on strings internally.

I still do not understand if this method is supposed to return a string or the original type, anyway.