snipe-it icon indicating copy to clipboard operation
snipe-it copied to clipboard

Use `htmlentities()` instead of `htmlspecialcharacters()` on Custom Field Listbox Values

Open spencerrlongg opened this issue 2 years ago • 7 comments

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

spencerrlongg avatar Jan 03 '24 18:01 spencerrlongg

PR Summary

  • Enhanced Listbox Element Generation in custom_fields_form.blade.php A 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.php The htmlentities() 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.php The code used for generating a listbox element in this file has also been transformed, mirroring the enhancement done in custom_fields_form.blade.php.
  • Extended Special Characters Escaping to custom_fields_form_bulk_edit.blade.php Similarly, the aforementioned htmlentities() 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.php An 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.

what-the-diff[bot] avatar Jan 03 '24 18:01 what-the-diff[bot]

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:

  1. encode that in HTML on the front end to prevent XSS and also to not break HTML
  2. decode it before storing on the backend

snipe avatar Jan 05 '24 11:01 snipe

Also looks like we have 8.1.1 failing test?

snipe avatar Jan 05 '24 11:01 snipe

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 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:

  1. encode that in HTML on the front end to prevent XSS and also to not break HTML
  2. decode it before storing on the backend

Looking into this.

spencerrlongg avatar Jan 08 '24 15:01 spencerrlongg

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

spencerrlongg avatar Jan 26 '24 17:01 spencerrlongg

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

spencerrlongg avatar Feb 06 '24 18:02 spencerrlongg

@snipe friendly poke

spencerrlongg avatar Feb 15 '24 19:02 spencerrlongg

@snipe friendly poke

spencerrlongg avatar Mar 18 '24 15:03 spencerrlongg