Skip to content

Replace deprecated filter FILTER_SANITIZE_STRING #5540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Replace deprecated filter FILTER_SANITIZE_STRING #5540

wants to merge 2 commits into from

Conversation

TimZ99
Copy link
Contributor

@TimZ99 TimZ99 commented Jan 4, 2022

Description
As of PHP8.1 FILTER_SANITIZE_STRING is deprecated. Replaced it with FILTER_SANITIZE_FULL_SPECIAL_CHARS. This is equivalent to calling htmlspecialchars() with ENT_QUOTES set. (PHP.net)

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

TimZ99 added 2 commits January 4, 2022 22:44
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.
@lonnieezell
Copy link
Member

Good catch. Thanks!

@kenjis
Copy link
Member

kenjis commented Jan 5, 2022

See #5005, #5263
esc(strip_tags($arg)) is used in other places.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this PR are not inconsistent.
Please make the same changes in other places.

@TimZ99
Copy link
Contributor Author

TimZ99 commented Jan 5, 2022

@kenjis changing to striptags() requires the underlying functions to be changed and removes flexibility for developers.

@kenjis
Copy link
Member

kenjis commented Jan 5, 2022

Sorry, what do you mean by removes flexibility ?

First of all, input filtering like FILTER_SANITIZE_STRING is a incorrect practice.
htmlspecialchars() should be used just before outputting HTML.

@TimZ99
Copy link
Contributor Author

TimZ99 commented Jan 5, 2022

Developers now have the option to use a different filter. If we force htmlspecialchars() the dev doesn't have that flexibility.

FILTER_SANITIZE_FULL_SPECIAL_CHARS is the equivalent of htmlspecialchars(), but keeps the flexibility.

@kenjis
Copy link
Member

kenjis commented Jan 5, 2022

@TimZ99 Thank you for the explanation. I don't mean to force htmlspecialchars().
I mean if $xssClean is true, the return value should be esc(strip_tags($value)).
Because in other places such a value is returned.

How about get_cookie() returns esc(strip_tags($request->getCookie($prefix . $index)))?

@paulbalandan What do you think?

@paulbalandan
Copy link
Member

esc(strip_tags($value)) was used to replace use of FILTER_SANITIZE_STRING to maintain the current behavior, although this behavior is incorrect (that's why the filter was deprecated by PHP). Thus, the replacement was just for the time being in preparation for PHP 8.1 compatibility.

If this PR's solution is way better and deviates from the current incorrect behavior, I'd approve this. So, I don't mind if the previously replaced places by esc(strip_tags($value)) would revert to using filter_var and this filter value. However, since this is an improvement, this needs to be documented and mentioned in the changelog.

@kenjis
Copy link
Member

kenjis commented Jan 6, 2022

I searched the source code and found only one $xssClean in get_cookie().
It probably comes from CI3.

I would like to deprecate the param. Because it is a bad practice.
So I don't think FILTER_SANITIZE_FULL_SPECIAL_CHARS is better than FILTER_SANITIZE_STRING or esc(strip_tags($value)).

Input filtering like FILTER_SANITIZE_FULL_SPECIAL_CHARS (or FILTER_SANITIZE_STRING) is a bad practice.
It barely makes sense only when the value is used only for HTML output.
But generally, most data is not coupled with the output system.
If you use the filtered data to other system, you must revert it before outputting.
Eg, if you have the value M&M, you get the filtered value M&M. But if you save it in the database,
the filtering does not make sense. So you should revert it to M&M and save it.
It is double effort. Why don't you convert when you output?

And when you output the filtered value as HTML, you don't escape (esc()) the value.
If you escape it, the value will be double-escaped.
We encourage to use esc() in the view. So no escape is against the recommendation.

@kenjis
Copy link
Member

kenjis commented Jan 6, 2022

I don't mind if the previously replaced places by esc(strip_tags($value)) would revert to using filter_var and this filter value.

I think there are three types of FILTER_SANITIZE_STRING usage.

  1. Inside Routing/URI processing
  2. Request::get*() 2nd param filter
  3. get_cookie() 2nd param xssClean

2 or 3 is a bad practice. I don't recommend to use.

2 is to specify filter itself. If a user uses FILTER_SANITIZE_STRING, it is not framework responsibility.
But we have to remove FILTER_SANITIZE_STRING in 3.

1 is a security measure. I'm not sure the way is good practice or not, but some measures must be needed.
If we change it, we must consider it very well.

@kenjis kenjis added breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Jan 6, 2022
@kenjis
Copy link
Member

kenjis commented Jan 6, 2022

My conclusion on this PR.

  • To avoid deprecated error in PHP 8.1, changing from FILTER_SANITIZE_STRING to FILTER_SANITIZE_FULL_SPECIAL_CHARS in get_cookie() is acceptable. Because using the second param $xssClean is not recommended.
  • But it is a breaking change, should be documented. And It is not any improvement.

@kenjis kenjis added the hotfix label Jan 6, 2022
@kenjis
Copy link
Member

kenjis commented Jan 8, 2022

@TimZ99 Thank you for your contribution.
I will prepare for the next release, I create a new PR based on this, and will add the documentation.
So I close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants