So now you just omit escapeInteger calls, making code less secure again?

The 'sort_by' parameter is validated in saveSearchParams(), so there
should be no need for quoting.

What if $sort_by is later used unaware of being unquoted?

So now you just omit escapeInteger calls, making code less secure again?

The entire search profile is serialized upon save, so there should be no need for escaping at this point.

The individual values are still escaped in buildWhereClause().

What if $sort_by is later used unaware of being unquoted?

I'm not sure what the concern is. $sort_by holds the name of a known database column (one of SORT_BY_FIELDS). It should be perfectly safe to be used without quoting.

May be Misc::escapeInteger shall be replaced with Misc::escapeId which returns unsigned int or empty string?

What if $sort_by is later used unaware of being unquoted?

I'm not sure what the concern is. $sort_by holds the name of a known database column (one of SORT_BY_FIELDS). It should be perfectly safe to be used without quoting.

Not sure I understand. Right on the next line below $sort_by is used unescaped in SQL query:
SQL query

I guess this is unsafe. How could that be missed?

Right a few lines below $sort_by is used unescaped in SQL query: SQL query

I guess this is unsafe.

No, it's not, because saveSearchParams() uses a whitelist to make sure that the only possible values are last_action_date, pri_rank, iss_id, sta_rank, iss_summary, or custom_field. (BTW, custom_field is a separate branch without any quoting even before my change).

The call to quoteIdentifier() was introduced on Nov 9 2021 (c732276).
The whitelist was introduced one day later (9119c50), making the quoting redundant.

If you feel it's safer I can also change the regexp to deal with quoted identifiers.

saveSearchParams() method used to save the current search parameters in a cookie, on a user side. Cookies are editable by user no matter what whitelists were used.

Cookies are editable by user no matter what whitelists were used.

No cookies are involved as far as I know.

Search filters are taken from the request ($_GET or $_POST), or the search profile persisted in the database.

So $options['sort_by'] directly comes from GET/POST request? Is that correct?

So $options['sort_by'] directly comes from GET/POST request? Is that correct?

Correct, with validation against SORT_BY_FIELDS.

Sorry I can not find it. Please point me the line where $options['sort_by'] is sanitized or validated.

Sorry I can not find it. Please point me the line where $options['sort_by'] is sanitized or validated.

class.search.php, line 92

Sorry I can not find it. Please point me the line where $options['sort_by'] is sanitized or validated.

class.search.php, line 92

OK, I can see now. Yet this is so inconsistent and might hit once day later. I stop my ranting. Let's wait for @glensc.

Is this ready for merge? please rebase against origin/master. I won't do deep review, but looks ok.

please also modify composer.json to allow php versions you tested. don't don't forget to update composer.lock

please also modify composer.json to allow php versions you tested.

I'm testing with PHP 8.1.14 (Fedora 37) and PHP 8.2.1 (CentOS 7.9). composer.json requires ^8.0, which should cover both.

they say that php engine versions aren't following semver, so ~8.0, ~8.1, ~8.2 would be more accurate to say, so it wouldn't include 8.3

they say that php engine versions aren't following semver, so ~8.0, ~8.1, ~8.2 would be more accurate to say, so it wouldn't include 8.3

OK, makes sense. Done.

Your lock update is wrong, you ran composer update, updating all dependencies, I asked to run composer update --lock.

Doesn't make sense to put changelog and composer.json change to same commit, totally different changes. not atomic commit. now if I want to revert the composer.json commit I will also revert changelog change as it's same commit. that's about atomic commits principle.

• https://www.freshconsulting.com/insights/blog/atomic-commits/

and "update composer.json" is a clickbait commit title. have to git show to see what the change is about :) "Allow PHP 8.0-8.2 PHP versions in composer" would be a more appropriate title.

• https://cbea.ms/git-commit/

Oh, I didn't realize "^8.0" is already there from 2019:

• 6c3c63c

Oh, I didn't realize "^8.0" is already there from 2019

I thought that why you asked to make it more specific. 😁

Want me to drop the commit?

I thought that why you asked to make it more specific. 😁

yes, we wanted and you agreed. so do it. but properly this time :)