Replace deprecated filter `FILTER_SANITIZE_STRING` by TimZ99 · Pull Request #5540 · codeigniter4/CodeIgniter4

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 · 2022-01-04T22:09:15Z

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

Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell · 2022-01-05T03:57:21Z

Good catch. Thanks!

kenjis · 2022-01-05T05:27:27Z

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

kenjis

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

TimZ99 · 2022-01-05T06:00:57Z

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

kenjis · 2022-01-05T06:27:49Z

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 · 2022-01-05T07:40:31Z

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 · 2022-01-05T08:03:28Z

@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 · 2022-01-05T13:31:09Z

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 · 2022-01-06T05:15:57Z

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 · 2022-01-06T05:40:36Z

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.

Inside Routing/URI processing
Request::get*() 2nd param filter
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 · 2022-01-06T08:26:38Z

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 · 2022-01-08T00:59:45Z

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

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

TimZ99 added 2 commits January 4, 2022 22:44

Replaced deprecated FILTER_SANITIZE_STRING
7f544bd
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

Replaced deprecated FILTER_SANITIZE_STRING
2f8466d
Replaced with FILTER_SANITIZE_FULL_SPECIAL_CHARS. Equivalent to calling htmlspecialchars() with ENT_QUOTES set.

lonnieezell approved these changes Jan 5, 2022
View reviewed changes

kenjis requested changes Jan 5, 2022
View reviewed changes

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 added the hotfix label Jan 6, 2022

kenjis mentioned this pull request Jan 8, 2022
fix: replace deprecated FILTER_SANITIZE_STRING #5555
Merged
4 tasks

kenjis closed this Jan 8, 2022

kenjis removed the hotfix label Jan 8, 2022

Uh oh!

Conversation

TimZ99 commented Jan 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonnieezell commented Jan 5, 2022

Uh oh!

kenjis commented Jan 5, 2022

Uh oh!

kenjis left a comment

Choose a reason for hiding this comment

Uh oh!

TimZ99 commented Jan 5, 2022

Uh oh!

kenjis commented Jan 5, 2022

Uh oh!

TimZ99 commented Jan 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Jan 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulbalandan commented Jan 5, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

TimZ99 commented Jan 4, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lonnieezell commented Jan 5, 2022

Uh oh!

kenjis commented Jan 5, 2022

Uh oh!

kenjis left a comment

Choose a reason for hiding this comment

Uh oh!

TimZ99 commented Jan 5, 2022

Uh oh!

kenjis commented Jan 5, 2022

Uh oh!

TimZ99 commented Jan 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Jan 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulbalandan commented Jan 5, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 6, 2022

Uh oh!

kenjis commented Jan 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TimZ99 commented Jan 4, 2022•
edited
Loading

TimZ99 commented Jan 5, 2022•
edited
Loading

kenjis commented Jan 5, 2022•
edited
Loading

TimZ99 commented Jan 4, 2022•
edited
Loading

TimZ99 commented Jan 5, 2022•
edited
Loading

kenjis commented Jan 5, 2022•
edited
Loading

TimZ99 commented Jan 4, 2022•
edited
Loading

TimZ99 commented Jan 5, 2022•
edited
Loading

kenjis commented Jan 5, 2022•
edited
Loading

TimZ99 commented Jan 4, 2022•
edited
Loading