Page MenuHomePhorge

Make PhabricatorSearchIntField validate its input
Needs ReviewPublic

Authored by aklapper on Jan 2 2024, 11:51.
Tags
None
Referenced Files
F2186450: D25502.diff
Fri, May 10, 07:28
Unknown Object (File)
Wed, May 8, 13:39
Unknown Object (File)
Tue, May 7, 18:01
Unknown Object (File)
Tue, May 7, 13:05
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23

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)