Description
Greg
2013-05-02 20:01:37 UTC
Verifying this is still an issue in current master This bug is similar to Bug 13441 and should be fixed the same way as Bug 13441 (prevent new passwords with whitespace characters, but do not touch functionalliy for existing passwords that already could contain spaces characters). Maybe something for the Catalyst academy (starting today) Created attachment 35265 [details] [review] Bug 10177 - whitespace is a valid password causing pernicious perturbations. Test plan: Go to your patron, go to change username and/or password. change password to whitespace, notice it is accepted, apply this patch, change password again, to whitespace, notice password is not valid. I did not test, but I compared with Bug 13441 and I think this patch should behave the same way resulting in a message like "The password entered contains whitespace characters. Please remove any whitespace characters." With other words: There should be no whitespace characters accepted at all (or at least no leading and/or trailing whitespace characters). Test in Bug 13441 is: match(/\s/) Test in this patch is: match(/^\s*$/) (In reply to Marc Véron from comment #5) > I did not test, but I compared with Bug 13441 and I think this patch should > behave the same way resulting in a message like "The password entered > contains whitespace characters. Please remove any whitespace characters." > > With other words: There should be no whitespace characters accepted at all > (or at least no leading and/or trailing whitespace characters). > > Test in Bug 13441 is: > match(/\s/) > > Test in this patch is: > match(/^\s*$/) I disagree, whitespace in passwords is totally valid, a strong password is a phrase. However having a password that is entirely whitespace (which is what we are testing) is not a strong password. This patch will allow you to have whitespace in your password, but you must have other characters also. Hi Chris, I agree with whitespace inside of passwords is OK. What about leading / trailing whitespace? See comment #1: --- 2. gmcharlt suggests that patches submitted in the future may strip trailing whitespace characters in form input, causing pernicious perturbations. --- I'de vote toward allowing whitespace within password, but not allowing it to be leading or trailing.. that allows for strong passphrases without giving ourselves headaches in the future. I agree with Martin. Created attachment 35320 [details] [review] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. Test plan: 1: Go to your patron 2: Go to change username and/or password. 3: Change password to something with spaces in the middle. Notice it is invalid. 4: Apply this patch. 5: Change password again using spaces in the middle. Notice it is accepted. 6: Change password with leading and/or trailing spaces, notice they are not accepted. Patch #35320 does not apply, it seems that it needs obsoleted #35265 first Created attachment 35343 [details] [review] [Signede-off] Bug 10177 - whitespace is a valid password causing pernicious perturbations. Test plan: Go to your patron, go to change username and/or password. change password to whitespace, notice it is accepted, apply this patch, change password again, to whitespace, notice password is not valid. Followed test plan. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 35344 [details] [review] [Signed-off] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. Test plan: 1: Go to your patron 2: Go to change username and/or password. 3: Change password to something with spaces in the middle. Notice it is invalid. 4: Apply this patch. 5: Change password again using spaces in the middle. Notice it is accepted. 6: Change password with leading and/or trailing spaces, notice they are not accepted. Applied bothe patches. Followed test plan. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 35356 [details] [review] [PASSED QA] Bug 10177 - whitespace is a valid password causing pernicious perturbations. Test plan: Go to your patron, go to change username and/or password. change password to whitespace, notice it is accepted, apply this patch, change password again, to whitespace, notice password is not valid. Followed test plan. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 35357 [details] [review] [PASSED QA] Bug 10177 - Whitespace is accepted within the password but leading and trailing spaces are not. Test plan: 1: Go to your patron 2: Go to change username and/or password. 3: Change password to something with spaces in the middle. Notice it is invalid. 4: Apply this patch. 5: Change password again using spaces in the middle. Notice it is accepted. 6: Change password with leading and/or trailing spaces, notice they are not accepted. Applied bothe patches. Followed test plan. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Hi Brandon, thx for your patch, works great! I suggest to leave this bug open after pushing the patch as there are 2 more pages where you can chane the password: the patron edit screen and in the OPAC. I think adding the check to the OPAC too would be good. Pushing this to the authentication module maintainers branch. https://github.com/mrenvoize/Koha/tree/authentication Cheers Brandon :) Patches pushed to master. Thanks Brandon! This change is pushed to master, but only covers part of the problem - you can still set your password to whitespace in the OPAC and from the full patron edit screen without any error. Pushed to 3.18.x is in 3.18.12 |