Bug 25382 - opac-sendbasket.pl and opac-sendshelf.pl don't validate email addresses
Summary: opac-sendbasket.pl and opac-sendshelf.pl don't validate email addresses
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-06 00:00 UTC by David Cook
Modified: 2020-05-11 23:22 UTC (History)
2 users (show)

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


Attachments
Bug 25382: validate email addresses for opac-sendbasket.pl and opac-sendshelf.pl (2.35 KB, patch)
2020-05-06 00:22 UTC, David Cook
Details | Diff | Splinter Review
Bug 25382: validate email addresses for opac-sendbasket.pl and opac-sendshelf.pl (2.41 KB, patch)
2020-05-09 15:32 UTC, David Roberts
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-05-06 00:00:20 UTC
opac-sendbasket.pl and opac-sendshelf.pl should validate email addresses to prevent abuse
Comment 1 David Cook 2020-05-06 00:22:44 UTC
Created attachment 104389 [details] [review]
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
Comment 2 David Cook 2020-05-06 00:26:24 UTC
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.
Comment 3 David Roberts 2020-05-09 15:32:53 UTC
Created attachment 104626 [details] [review]
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

Signed-off-by: David Roberts <david@koha-ptfs.co.uk>
Comment 4 Katrin Fischer 2020-05-10 22:15:23 UTC
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.

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.

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.

Failing for 1, but it would be great if you could consider 2 and 3 as well.
Comment 5 David Cook 2020-05-10 23:16:15 UTC
(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.
Comment 6 Jonathan Druart 2020-05-11 13:34:26 UTC
1. carp is not needed indeed.
2. var you pass to the template need to be useful, otherwise what is the purpose?
There is plenty of places where we push @messages, { error => $error_code }; then loop on the messages in the template.
That's mandatory here IMO.
Comment 7 David Cook 2020-05-11 23:22:14 UTC
(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.