[Bug 39882] New: Add phone number masking option
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Bug ID: 39882 Summary: Add phone number masking option Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: lucas@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org It would be nice it we should add a mask for phone number fields. For certain things, like Twilio, it is important that a phone number gets set in an exact format. We have Maskito for date entry, we might be able to leverage that for phone numbers: https://maskito.dev/recipes/phone -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Danielle <danielle.meininger@tillamookcounty.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |danielle.meininger@tillamoo | |kcounty.gov --- Comment #1 from Danielle <danielle.meininger@tillamookcounty.gov> --- I like that idea too. Would this be something we could setup individually or a global system preference with a few options to choose from? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Brandon <brandon@wwcrld.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |brandon@wwcrld.org --- Comment #2 from Brandon <brandon@wwcrld.org> --- +1 for this feature. Phone number fields should not be plain text inputs. Just a recipe for bad data. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23817 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #3 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- (In reply to Brandon from comment #2)
+1 for this feature. Phone number fields should not be plain text inputs. Just a recipe for bad data.
The correct way to do this is probably Bug 23817, normalize all phone numbers in patron search and DB. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #4 from David Cook <dcook@prosentient.com.au> --- (In reply to Lucas Gass (lukeg) from comment #3)
(In reply to Brandon from comment #2)
+1 for this feature. Phone number fields should not be plain text inputs. Just a recipe for bad data.
The correct way to do this is probably Bug 23817, normalize all phone numbers in patron search and DB.
Funny enough I was just talking last night about how I should resurrect this for main. I've been using it locally for the past 2 years and it's nice. That said, formatting phone numbers for display purposes at least could be cool. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #4)
Funny enough I was just talking last night about how I should resurrect this for main. I've been using it locally for the past 2 years and it's nice.
Let me know if you'd like me to post some updated patches. I could certainly make some time for that, if you're interested in it. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Laura O'Neil <laura@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |laura@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Jessie Zairo <jzairo@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jzairo@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |DUPLICATE Status|NEW |RESOLVED --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- David has a much better way of doing this in Bug 23817. *** This bug has been marked as a duplicate of bug 23817 *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|DUPLICATE |--- Status|RESOLVED |REOPENED --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- I'm inclined to keep this open because libraries might want a way to keep phone numbers consistently formatted for display. I think it also improves the user experience of entering the phone number. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Michael Adamyk <michael.adamyk@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michael.adamyk@bywatersolut | |ions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |eric@bywatersolutions.com --- Comment #8 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- *** Bug 41790 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |lucas@bywatersolutions.com |ity.org | Status|REOPENED |ASSIGNED -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23817 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23817 [Bug 23817] Normalize phone number when searching patrons -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #9 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 196897 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196897&action=edit Bug 39882: Add new system preferences Patch from commit 8e64c59 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #10 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 196898 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196898&action=edit Bug 39882: Add ability to mask phone numbers on memberentry.pl To test: 1. APPLY PATCH, restart_all, updatedatabase 2. Seach for the 2 new system preferences PhoneMaskPattern and PhoneMaskFields 3. The 2 prefs should be linked to each other, searching for one should bring up the other. 4. Look at the "Examples" in the system pref description, make sure they make sense. 5. Pick one of the pattern examples, or come up with your own. Add it to PhoneMaskPattern. 6. For PhoneMaskFields, start by selecting all and saving. 7. Now go to Patrons > New patron 8. In each of the phone number fields try entering a phone number, make sure the mask is working correctly. 9. Save the patron and edit them, make sure the phone masking works well on each field when you are editing it. 10. Go back to PhoneMaskFields and turn some of the fields off. 11. Go back to patron editing and make sure that those field from step 10 do NOT apply a mask. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #11 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 196899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196899&action=edit Bug 39882: Corrections to DB update Patch from commit 8bff147 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196897|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196898|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196899|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #12 from David Nind <david@davidnind.com> --- Created attachment 196900 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196900&action=edit Bug 39882: Add new system preferences Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #13 from David Nind <david@davidnind.com> --- Created attachment 196901 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196901&action=edit Bug 39882: Add ability to mask phone numbers on memberentry.pl To test: 1. APPLY PATCH, restart_all, updatedatabase 2. Seach for the 2 new system preferences PhoneMaskPattern and PhoneMaskFields 3. The 2 prefs should be linked to each other, searching for one should bring up the other. 4. Look at the "Examples" in the system pref description, make sure they make sense. 5. Pick one of the pattern examples, or come up with your own. Add it to PhoneMaskPattern. 6. For PhoneMaskFields, start by selecting all and saving. 7. Now go to Patrons > New patron 8. In each of the phone number fields try entering a phone number, make sure the mask is working correctly. 9. Save the patron and edit them, make sure the phone masking works well on each field when you are editing it. 10. Go back to PhoneMaskFields and turn some of the fields off. 11. Go back to patron editing and make sure that those field from step 10 do NOT apply a mask. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #14 from David Nind <david@davidnind.com> --- Created attachment 196902 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=196902&action=edit Bug 39882: Corrections to DB update Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #15 from David Nind <david@davidnind.com> --- I've signed off, but a couple of comments: 1. The options for PhoneMaskFields don't match what is on the patron add and edit form: - Labels on the form: . Contact information: . Primary phone . Secondary phone . Other phone . Alternate address: . Phone . Alternate contact: . Phone . Patron messaging preferences: . SMS number (has hint: Please enter numbers only. Prefix the number with + or 00 if including the country code.) - Options for PhoneMaskFields: . [Select all] . Alternate address phone (can match) . Alternate contact phone (can match) . Mobile* . Phone* . SMS number (can match) . Work phone* 2. I personally found it confusing that there is no indicator or anything on the field to indicate that you should enter it a particular way. 3. Accessibility and other issues: - Is there any reason we don't use the "tel" input type? https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input/t... - I'm not up with the play about creating accessible telephone number input fields, but a quick search resulted in: . Use autocomplete: autocomplete="tel" (may not be relevant here as patrons aren't completing) . Use placeholder/help text - UK design system information for phone numbers: https://design-system.service.gov.uk/patterns/phone-numbers/ - In the absence of a Koha design system, should we just follow the UK or Canadian design system guidelines? (A bigger issue than this on though) 4. I know that this or related issues have been debated for a while now (including the related bugs), so don't want to hold an improvement up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #16 from Michael Adamyk <michael.adamyk@bywatersolutions.com> --- David, would an enhancement like bug 41636 address your point in #2, about the indication of how to enter numbers in a particular way? Though it's specifically about the SMS number text. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41636 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #17 from David Nind <david@davidnind.com> --- (In reply to Michael Adamyk from comment #16)
David, would an enhancement like bug 41636 address your point in #2, about the indication of how to enter numbers in a particular way? Though it's specifically about the SMS number text.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41636
Having some way to add hint text so you know what is required would be good. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #196900|0 |1 is obsolete| | Attachment #196901|0 |1 is obsolete| | Attachment #196902|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #18 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202272&action=edit Bug 39882: Add new system preferences Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #19 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202273 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202273&action=edit Bug 39882: Add ability to mask phone numbers on memberentry.pl To test: 1. APPLY PATCH, restart_all, updatedatabase 2. Seach for the 2 new system preferences PhoneMaskPattern and PhoneMaskFields 3. The 2 prefs should be linked to each other, searching for one should bring up the other. 4. Look at the "Examples" in the system pref description, make sure they make sense. 5. Pick one of the pattern examples, or come up with your own. Add it to PhoneMaskPattern. 6. For PhoneMaskFields, start by selecting all and saving. 7. Now go to Patrons > New patron 8. In each of the phone number fields try entering a phone number, make sure the mask is working correctly. 9. Save the patron and edit them, make sure the phone masking works well on each field when you are editing it. 10. Go back to PhoneMaskFields and turn some of the fields off. 11. Go back to patron editing and make sure that those field from step 10 do NOT apply a mask. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #20 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 202274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202274&action=edit Bug 39882: Corrections to DB update Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |andrew@bywatersolutions.com |y.org | Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202272|0 |1 is obsolete| | Attachment #202273|0 |1 is obsolete| | Attachment #202274|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #21 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 204564 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204564&action=edit Bug 39882: Add new system preferences Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #22 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 204565 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204565&action=edit Bug 39882: Add ability to mask phone numbers on memberentry.pl To test: 1. APPLY PATCH, restart_all, updatedatabase 2. Seach for the 2 new system preferences PhoneMaskPattern and PhoneMaskFields 3. The 2 prefs should be linked to each other, searching for one should bring up the other. 4. Look at the "Examples" in the system pref description, make sure they make sense. 5. Pick one of the pattern examples, or come up with your own. Add it to PhoneMaskPattern. 6. For PhoneMaskFields, start by selecting all and saving. 7. Now go to Patrons > New patron 8. In each of the phone number fields try entering a phone number, make sure the mask is working correctly. 9. Save the patron and edit them, make sure the phone masking works well on each field when you are editing it. 10. Go back to PhoneMaskFields and turn some of the fields off. 11. Go back to patron editing and make sure that those field from step 10 do NOT apply a mask. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #23 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 204566 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204566&action=edit Bug 39882: Corrections to DB update Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #24 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 204616 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204616&action=edit Bug 39882: (follow-up) Click to populate from examples -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #25 from Owen Leonard <oleonard@myacpl.org> --- I don't want to hold this up but I thought my follow-up might be useful. I can submit another bug report if that's better. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #26 from David Cook <dcook@prosentient.com.au> --- Sorry folks but there's a XSS vulnerability in here. I'll comment in-line. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 204565 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204565 Bug 39882: Add ability to mask phone numbers on memberentry.pl Review of attachment 204565: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=39882&attachment=204565) ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ +1648,5 @@
+ const pattern = "[% Koha.Preference('PhoneMaskPattern') | $raw %]"; + const fields = "[% Koha.Preference('PhoneMaskFields') | $raw %]"; + + if (pattern && fields) { + const mask = [[% Koha.Preference('PhoneMaskPattern') | $raw %]];
All these "[% Koha.Preference('PhoneMaskFields') | $raw %]" and "[% Koha.Preference('PhoneMaskPattern') | $raw %]"lines are XSS vulnerabilities that could lead to account takeover. Even if you were using CSP, the Javascript being generated is "trusted", so a malicious payload would still execute. While technically this code is meeting the JS19 coding guideline ( https://wiki.koha-community.org/wiki/Coding_Guidelines#JS19:_Avoid_Template:... ), that guideline is currently incomplete, and changes will be coming in the future. I'll have to take a look at the proposed patterns to see what would be the appropriate filter instead of $raw... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #28 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #27)
Comment on attachment 204565 [details] [review] Bug 39882: Add ability to mask phone numbers on memberentry.pl
Review of attachment 204565 [details] [review]: -----------------------------------------------------------------
::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ +1648,5 @@
+ const pattern = "[% Koha.Preference('PhoneMaskPattern') | $raw %]"; + const fields = "[% Koha.Preference('PhoneMaskFields') | $raw %]"; + + if (pattern && fields) { + const mask = [[% Koha.Preference('PhoneMaskPattern') | $raw %]];
All these "[% Koha.Preference('PhoneMaskFields') | $raw %]" and "[% Koha.Preference('PhoneMaskPattern') | $raw %]"lines are XSS vulnerabilities that could lead to account takeover. Even if you were using CSP, the Javascript being generated is "trusted", so a malicious payload would still execute.
While technically this code is meeting the JS19 coding guideline ( https://wiki.koha-community.org/wiki/Coding_Guidelines#JS19:_Avoid_Template:: Toolkit_tags_in_script_tags ), that guideline is currently incomplete, and changes will be coming in the future.
I'll have to take a look at the proposed patterns to see what would be the appropriate filter instead of $raw...
At a glance, I think you should be able to switch to using the "html" filter instead of $raw, and the double quote enclosed preferences should be OK from there, but that last one that doesn't have quotes is a problem. If that one needs to contain a JSON data structure, then it needs to first be parsed as JSON, escaped using a "json" filter, and then subbed into that line. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #29 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- (In reply to David Cook from comment #28)
(In reply to David Cook from comment #27)
Comment on attachment 204565 [details] [review] [review] Bug 39882: Add ability to mask phone numbers on memberentry.pl
Review of attachment 204565 [details] [review] [review]: -----------------------------------------------------------------
::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ +1648,5 @@
+ const pattern = "[% Koha.Preference('PhoneMaskPattern') | $raw %]"; + const fields = "[% Koha.Preference('PhoneMaskFields') | $raw %]"; + + if (pattern && fields) { + const mask = [[% Koha.Preference('PhoneMaskPattern') | $raw %]];
All these "[% Koha.Preference('PhoneMaskFields') | $raw %]" and "[% Koha.Preference('PhoneMaskPattern') | $raw %]"lines are XSS vulnerabilities that could lead to account takeover. Even if you were using CSP, the Javascript being generated is "trusted", so a malicious payload would still execute.
While technically this code is meeting the JS19 coding guideline ( https://wiki.koha-community.org/wiki/Coding_Guidelines#JS19:_Avoid_Template:: Toolkit_tags_in_script_tags ), that guideline is currently incomplete, and changes will be coming in the future.
I'll have to take a look at the proposed patterns to see what would be the appropriate filter instead of $raw...
At a glance, I think you should be able to switch to using the "html" filter instead of $raw, and the double quote enclosed preferences should be OK from there, but that last one that doesn't have quotes is a problem. If that one needs to contain a JSON data structure, then it needs to first be parsed as JSON, escaped using a "json" filter, and then subbed into that line.
Nice catch. I think this actually helps lead me a better overall solution. New patches incoming. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 --- Comment #30 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 204748 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204748&action=edit Bug 39882: Fix potential XSS problem, switch to x as placeholder Patch from commit ad78391 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|andrew@bywatersolutions.com |testopia@bugs.koha-communit | |y.org --- Comment #31 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- David, could you please take QA here, since you spotted and better understand the security concern? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39882 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette@bywatersolutions.co | |m QA Contact|testopia@bugs.koha-communit |dcook@prosentient.com.au |y.org | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org