https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25382 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Created attachment 104389 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104389&action=edit Bug 25382: validate email addresses for opac-sendbasket.pl and opac-sendshelf.pl This patch adds email validation to these two OPAC scripts using Email::Valid Test plan: 0) Don't apply patch yet 1) Send a cart to "blah@blah.blah blah@blah.blah" 2) Note the cart says it has sent to both email addresses 3) Apply patch 4) Send a cart to "blah@blah.blah blah@blah.blah" 5) Note the cart says it has an error 6) Send a cart to "blah@blah.blah" 7) Note the cart says email was sent successfully 8) Repeat above process for OPAC List functionality -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25382 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Instead of "blah@blah.blah blah@blah.blah" you could do "sagsadsadg blah@blah.blah" for your bad email input too. That might make it more obvious. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25382 David Roberts <david.roberts@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david.roberts@ptfs-europe.c | |om 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=25382 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to Katrin Fischer from comment #4)

Hi David,

while this works, there are a few catches:

1) I feel like the unconditional carp here is not necessary. We usually don't ouput erros like this in the logs.

I was just copying the style used elsewhere in those same scripts. Happy not to include it if you don't want it.

2) You pass parameters to the template - why not have a specific error message? At the moment we always have: There was an error sending the cart. For the user that could read like there is a server issue, not an issue with the entered information.

I was just copying the style used elsewhere in those same scripts. I didn't think this was a good opportunity to change the error handling for the whole script.

3) I think we should also add additional client side validation like we have on the email fields on the staff client or in the patron details form in OPAC. This way we can give even more specific feedback in a standard way.

That's a good idea. My patch is mostly to address security issues, but a better user experience is a great idea.

Failing for 1, but it would be great if you could consider 2 and 3 as well.

I'm not sure these points deserve the patch to be failed, but I'd be willing to remove the carp to get it moving again. If points 2 and 3 are necessary, I'll just leave this patch for now. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25382 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #6)

1. carp is not needed indeed. 2. var you pass to the template need to be useful, otherwise what is the purpose?

As I said above, I was just copying the existing patterns of these scripts.

There is plenty of places where we push @messages, { error => $error_code }; then loop on the messages in the template.

Not for these scripts we don't.

That's mandatory here IMO.

Rewriting these scripts and templates seems out of scope to me, but if that's mandatory that's OK, as they're very reasonable changes to make. It's more work than I'm willing to do at this point, so I'll put it on my TODO list for the future. This seemed like a faster/easier win than validating the input to Koha::Email for all scripts, but if that's not the case I'll just maintain the changes locally for now, and get back to it when I have more time. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25382 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=41025 -- You are receiving this mail because: You are watching all bug changes.