Page MenuHomePhorge
Differential D25502

Make PhabricatorSearchIntField validate its input
Needs ReviewPublic
Actions

Authored by aklapper on Jan 2 2024, 11:51.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 22:33
Unknown Object (File)
Sun, Apr 21, 18:52
Unknown Object (File)
Wed, Apr 17, 18:19
Unknown Object (File)
Wed, Apr 17, 06:45
Unknown Object (File)
Mon, Apr 8, 22:34
Unknown Object (File)
Mon, Apr 8, 20:20
Unknown Object (File)
Sat, Mar 30, 15:55
Unknown Object (File)
Sat, Mar 30, 15:55
View All 50 Files
Subscribers
avivey
Cigaryno
Matthew
speck
tobiaswiese
valerio.bozzolan

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

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).

It seems that there are no API calls to test - /conduit/method/project.query/ does not offer passing depth values.

Diff Detail

Repository
rP Phorge
Branch
T15700checkInt (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1062
Build 1062: arc lint + arc unit

Event Timeline

aklapper created this revision.Jan 2 2024, 11:51
Owners added a reviewer: O1: Blessed Committers.Jan 2 2024, 11:51
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 2 2024, 11:51
aklapper requested review of this revision.Jan 2 2024, 11:51
Harbormaster completed remote builds in B997: Diff 1605.Jan 2 2024, 11:51
aklapper updated this revision to Diff 1606.Jan 2 2024, 11:58

Replace minus dash by its UTF8 representation to pass lint

Harbormaster completed remote builds in B998: Diff 1606.Jan 2 2024, 11:58
aklapper edited the test plan for this revision. (Show Details)Jan 2 2024, 12:00
aklapper mentioned this in T15723: Convert "Page Size" field in ManiphestTaskSearchEngine.php to PhabricatorSearchIntField.Jan 26 2024, 10:55
aklapper added a comment.Feb 12 2024, 12:27

Would anyone fancy reviewing this? :)

aklapper added a comment.Feb 12 2024, 12:28

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

speck subscribed.Feb 12 2024, 22:56

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.

aklapper added a comment.Feb 13 2024, 15:56
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.

aklapper updated this revision to Diff 1700.Feb 13 2024, 15:58

Remove unreached return call as pointed out by speck

Harbormaster completed remote builds in B1062: Diff 1700.Feb 13 2024, 15:58
aklapper marked an inline comment as done.Mar 13 2024, 14:32
aklapper added a comment.Mar 16 2024, 07:57

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

avivey subscribed.Mar 16 2024, 14:13

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() ?

Revision Contents
Changeset List

PathSizePackages
src/
applications/
search/
field/
24 linesO1: Blessed Committers

Diff 1700

View Options

src/applications/search/field/PhabricatorSearchIntField.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0