Bug 19911 - Passwords displayed to user during self-registration are not HTML-encoded
Summary: Passwords displayed to user during self-registration are not HTML-encoded
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 19918
Blocks:
  Show dependency treegraph
 
Reported: 2018-01-03 18:38 UTC by Arturo
Modified: 2019-10-14 19:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Example of the generated password not displaying properly due to the less-than character treated as opening HTML tag (105.09 KB, image/png)
2018-01-03 18:38 UTC, Arturo
Details
Bug 19911: Escape password value during self-registration confirmation (2.29 KB, patch)
2018-01-03 19:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19911: Do not escape html characters when saving passwords (1.36 KB, patch)
2018-01-04 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19911: Escape password value during self-registration confirmation (2.29 KB, patch)
2018-01-05 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19911: Do not escape html characters when saving passwords (1.40 KB, patch)
2018-01-05 17:23 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 19911: Escape password value during self-registration confirmation (2.33 KB, patch)
2018-01-05 17:23 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 19911: Do not escape html characters when saving passwords (1.46 KB, patch)
2018-01-12 07:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19911: Escape password value during self-registration confirmation (2.38 KB, patch)
2018-01-12 07:28 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arturo 2018-01-03 18:38:51 UTC
Created attachment 70252 [details]
Example of the generated password not displaying properly due to the less-than character treated as opening HTML tag

If self-registration is enabled and the PatronSelfRegistrationPrefillForm system preference is set to "Display and prefill," self-registered users are shown their password upon successfully registering. If the password contains a less-than character, browsers treat this as the beginning of an HTML element, and so the less-than character and anything after it does not display since the password is not HTML-encoded.

If Koha is set to generate passwords automatically during self-registration (i.e., users are not allowed or required to enter a password in the self-registration form), any generated password containing the less-than character will not display correctly. Users who are expected to copy/save their password at this time cannot do so, and there is no way to recover that generated password.

Attached is a screenshot showing what I mean. A solution would to HTML-encode the passwords when they are displayed as part of the self-registration process, regardless of whether the user must verify their e-mail address first (opac-registration-verify.pl).
Comment 1 Jonathan Druart 2018-01-03 19:17:04 UTC
Created attachment 70253 [details] [review]
Bug 19911: Escape password value during self-registration confirmation

The password must be correctly escape, it can contains html character
and break the display.

Test plan:
Apply first patch and confirm that the display is broken
Apply second patch (this one) and confirm that the display is fixed
Comment 2 Jonathan Druart 2018-01-04 14:02:22 UTC
Created attachment 70271 [details] [review]
Bug 19911: Do not escape html characters when saving passwords

When the password is not generated automatically, we should not escape
the html characters. Otherwise it will be changed without any warnings.
Comment 3 Arturo 2018-01-04 16:34:14 UTC
Thank you for the patches, Jonathan! I've tested this out on a sandbox and it works great! There is only one issue that I found -- the <span> tag on line 45 of opac-registration-confirmation.tt is missing a closing </span> tag. Right now both of the tags are opening tags, so it is causing an HTML validation error.

Despite that, I was able to complete the detailed test plan below and found no errors. These patches work both when e-mail verification is required and when it is not. They also work when the user supplies a password and when it is randomly generated by Koha. My full test plan is below.

These are the sample passwords I tested with:
<password>
<%20>
&nbsp;
&lt;password&gt;
<p></p>
<a href="#">link</a>
&#165;

Test plan:
1. Make sure a valid e-mail is stored in KohaAdminEmailAddress.
2. Set OpacPublic to Enable.
3. Set PatronSelfRegistration to Allow.
4. Be sure there is a valid patron category in PatronSelfRegistrationDefaultCategory.
5. Set PatronSelfRegistrationBorrowerMandatoryField to include at least "firstname|surname|email|password" so that these are required fields.
6. Set PatronSelfRegistrationPrefillForm to "Display and prefill" so that you can see the password and have it prefilled.

To test when e-mail verification is NOT required:
1. Set PatronSelfRegistrationVerifyByEmail to "Don't require".
2. Go to the OPAC and fill out the self-registration form. Supply a password that contains the less-than character.
3. Confirm that upon account creation, your password is correctly displayed on the confirmation page.
4. Also confirm that you can log in to your account.

To test when e-mail verification IS required:
1. Be sure that OPACBaseUrl has a value since it is called by the OPAC_REG_VERIFY e-mail template.
2. Set PatronSelfRegistrationVerifyByEmail to "Require."
3. Go to the OPAC and fill out the self-registration form. Supply a password that contains the less-than character.
4. Follow the e-mail verification link created by Koha.
5. Confirm that upon account creation, your password is correctly displayed on the confirmation page.
6. Also confirm that you can log in to your account.

To test when a password is generated randomly:
1. Remove "password" from the list of fields in PatronSelfRegistrationBorrowerMandatoryField and repeat the two blocks of steps above. Be sure that the randomly generated password contains a less-than character and that it displays properly. Since these are generated randomly, you may need to self-register multiple times until your generated password contains this character.
Comment 4 Jonathan Druart 2018-01-05 14:13:17 UTC
Created attachment 70289 [details] [review]
Bug 19911: Escape password value during self-registration confirmation

The password must be correctly escape, it can contains html character
and break the display.

Test plan:
Apply first patch and confirm that the display is broken
Apply second patch (this one) and confirm that the display is fixed
Comment 5 Jonathan Druart 2018-01-05 14:14:25 UTC
(In reply to Arturo from comment #3)
> Thank you for the patches, Jonathan! I've tested this out on a sandbox and
> it works great! There is only one issue that I found -- the <span> tag on
> line 45 of opac-registration-confirmation.tt is missing a closing </span>
> tag. Right now both of the tags are opening tags, so it is causing an HTML
> validation error.

Well spotted!
I have opened, filled and pushed bug 19918 to fix that.
And rebased the patch on top.
Comment 6 Biblibre Sandboxes 2018-01-05 17:23:10 UTC
Patch tested with a sandbox, by Arturo <alongoria@sll.texas.gov>
Comment 7 Biblibre Sandboxes 2018-01-05 17:23:43 UTC
Created attachment 70308 [details] [review]
Bug 19911: Do not escape html characters when saving passwords

When the password is not generated automatically, we should not escape
the html characters. Otherwise it will be changed without any warnings.

Signed-off-by: Arturo <alongoria@sll.texas.gov>
Comment 8 Biblibre Sandboxes 2018-01-05 17:23:46 UTC
Created attachment 70309 [details] [review]
Bug 19911: Escape password value during self-registration confirmation

The password must be correctly escape, it can contains html character
and break the display.

Test plan:
Apply first patch and confirm that the display is broken
Apply second patch (this one) and confirm that the display is fixed

Signed-off-by: Arturo <alongoria@sll.texas.gov>
Comment 9 Arturo 2018-01-05 17:26:29 UTC
Just tested again and it looks great to me. Thank you for your work on this, Jonathan!
Comment 10 Katrin Fischer 2018-01-12 07:28:29 UTC
Created attachment 70445 [details] [review]
Bug 19911: Do not escape html characters when saving passwords

When the password is not generated automatically, we should not escape
the html characters. Otherwise it will be changed without any warnings.

Signed-off-by: Arturo <alongoria@sll.texas.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2018-01-12 07:28:33 UTC
Created attachment 70446 [details] [review]
Bug 19911: Escape password value during self-registration confirmation

The password must be correctly escape, it can contains html character
and break the display.

Test plan:
Apply first patch and confirm that the display is broken
Apply second patch (this one) and confirm that the display is fixed

Signed-off-by: Arturo <alongoria@sll.texas.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2018-01-12 07:29:22 UTC
Thx, Arturo, for documenting your tests and the sign-off!
Comment 13 Jonathan Druart 2018-01-15 15:25:42 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2018-01-23 04:08:02 UTC
Awesome work all!

Pushed to Stable for 17.11.02
Comment 15 Fridolin Somers 2018-01-30 08:13:12 UTC
Pushed to 17.05.x for v17.05.09