Use `htmlentities()` instead of `htmlspecialcharacters()` on Custom Field Listbox Values
Description
On the tin, this seems to allow the list box to function as expected, fixing an issue reported by a user. If there's anything you'd like me to test specifically I'm more than happy to do so, but from what I can tell this should resolve the issue.
https://www.php.net/manual/en/function.htmlentities.php
Type of change
- [x] Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Checked to make sure values in listboxes are still coming through and matching correctly to their DB values.
Test Configuration:
- PHP version: 8.1
- MySQL version: 8.2
PR Summary
-
Enhanced Listbox Element Generation in
custom_fields_form.blade.phpA transformation has been made in the code responsible for generating a listbox element. This modification ensures smoother interaction with the associated element. -
Added Special Characters Escaping in
custom_fields_form.blade.phpThehtmlentities()function has been integrated, which will aid in escaping special characters in the selected value of the listbox. This new feature improves security by preventing potential cross-site scripting (XSS) attacks. -
Listbox Elements Improvement in
custom_fields_form_bulk_edit.blade.phpThe code used for generating a listbox element in this file has also been transformed, mirroring the enhancement done incustom_fields_form.blade.php. -
Extended Special Characters Escaping to
custom_fields_form_bulk_edit.blade.phpSimilarly, the aforementionedhtmlentities()function has been extended to this file as well, ensuring uniform cross-file security measures against XSS attacks. -
Added Uniqueness Condition in
custom_fields_form_bulk_edit.blade.phpAn additional condition has been integrated to verify the uniqueness of a field. This adjustment will prevent accidental duplication of fields, enhancing the efficiency of the system.
I'd love to see some tests here, and also some proofs that it's been tested for XSS vulns. The reason we were using the old method is that we need 13" to actually be 13" in the database, not 13", since that won't match up. I feel like we might need to do something on the backend, like:
- encode that in HTML on the front end to prevent XSS and also to not break HTML
- decode it before storing on the backend
Also looks like we have 8.1.1 failing test?
Also looks like we have 8.1.1 failing test?
Looks like flakey test, re-ran and it passed.
I'd love to see some tests here, and also some proofs that it's been tested for XSS vulns. The reason we were using the old method is that we need
13"to actually be13"in the database, not13", since that won't match up. I feel like we might need to do something on the backend, like:
- encode that in HTML on the front end to prevent XSS and also to not break HTML
- decode it before storing on the backend
Looking into this.
@snipe So, turns out that the actual problem here is that on the initial store of a custom field the value isn't escaped - but it is once it's updated, and then the output escaping starts working. So, pivoting this PR to just escape the initial saved value.
Probably need to do some more testing on this and come up with a way to fix existing data, but that's the update for right now.
Alright, so, fixed the problem with the escaping on update, so we now save unescaped raw input on both store and update.
Then realized that blade's echo helper already does the escaping for us, so htmlspecialcharacters() is redundant (and causes some weird behavior), so that's removed now.
Attaching a couple screenshots of XSS testing.
@snipe friendly poke
@snipe friendly poke