Bug 17392

Summary: opac/svc/overdrive_proxy is not plack safe
Product: Koha Reporter: Nick Clemens <nick>
Component: OPACAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: brendan, f.demians, jonathan.druart, julian.maurice, nick, srdjan, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7172    
Attachments: Bug 17392: Make svc/overdrive_proxy plack safe
Bug 17392 - opac/svc/overdrive_proxy is not plack safe
Bug 17392 - opac/svc/overdrive_proxy is not plack safe
[PASSED QA] Bug 17392 - opac/svc/overdrive_proxy is not plack safe

Description Nick Clemens 2016-10-03 18:54:44 UTC
When running under plack the parameters in the url passed to overdrive are separated by semi-colons as opposed to ampersands

example, dumping $query->query_string from /opac/svc/overdrive_proxy under plack we have:
'q=howard%20wigglebottom;limit=1;offset=0'

If the page is excluded from plack we get:
'q=howard%20wigglebottom&limit=1&offset=0'

You can test in a rest client, the first will return spurious results, searches in opac will report one number of hits, clicking the link for results will return none or a different number of hits from overdrive

Tried removing the qw(-oldstyle-urls) from the script but it had no effect, though without that when excluded from plack things also don't work

This is verified under master and 3.22 under plack
Comment 1 Jonathan Druart 2016-10-04 10:16:23 UTC
Created attachment 56016 [details] [review]
Bug 17392: Make svc/overdrive_proxy plack safe
Comment 2 Jonathan Druart 2016-10-04 10:17:26 UTC
Just a try, I am not able to test the whole workflow.
Please test and adapt if needed.
I think that the problem comes from the fact that the CGI->new method is overwritten from the psgi file.
Comment 3 Jonathan Druart 2016-10-04 10:18:23 UTC
(In reply to Jonathan Druart from comment #2)
> Just a try, I am not able to test the whole workflow.
> Please test and adapt if needed.
> I think that the problem comes from the fact that the CGI->new method is
> overwritten from the psgi file.

And so the -oldstyle_urls pragma is not taken into account.
Comment 4 Nick Clemens 2016-10-05 13:30:06 UTC
Created attachment 56054 [details] [review]
Bug 17392 - opac/svc/overdrive_proxy is not plack safe

This patch simply replaces the ';'  in the param passed to OverDrive
with '&'

To test:
1 - Enable overdrive (requires an account)
2 - Perform an opac search
3 - Note the number of overdrive results reported
4 - Click the link to view the actualt overdrive results
5 - Note the result numbers don't match
6 - Apply patch
7 - Repeat 1-4 and note results numbers match and results are relevant
8 - Test a search with a ';' to ensure this patch isn't breaking
searches
Comment 5 Chris Cormack 2016-10-06 08:40:11 UTC
Created attachment 56069 [details] [review]
Bug 17392 - opac/svc/overdrive_proxy is not plack safe

This patch simply replaces the ';'  in the param passed to OverDrive
with '&'

To test:
1 - Enable overdrive (requires an account)
2 - Perform an opac search
3 - Note the number of overdrive results reported
4 - Click the link to view the actualt overdrive results
5 - Note the result numbers don't match
6 - Apply patch
7 - Repeat 1-4 and note results numbers match and results are relevant
8 - Test a search with a ';' to ensure this patch isn't breaking
searches

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 6 Katrin Fischer 2016-10-11 09:39:24 UTC
Created attachment 56151 [details] [review]
[PASSED QA] Bug 17392 - opac/svc/overdrive_proxy is not plack safe

This patch simply replaces the ';'  in the param passed to OverDrive
with '&'

To test:
1 - Enable overdrive (requires an account)
2 - Perform an opac search
3 - Note the number of overdrive results reported
4 - Click the link to view the actual overdrive results
5 - Note the result numbers don't match
6 - Apply patch
7 - Repeat 1-4 and note results numbers match and results are relevant
8 - Test a search with a ';' to ensure this patch isn't breaking
searches

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Verified by reading code - couldn't verify using Overdrive.
Comment 7 Brendan Gallagher 2016-10-14 09:32:31 UTC
Pushed to Master - Should be in the November 16.11 Release.  Thanks!
Comment 8 Frédéric Demians 2016-10-22 12:25:23 UTC
Pushed in 16.05. Will be in 16.05.05.
Comment 9 Julian Maurice 2016-10-24 10:00:41 UTC
Pushed to 3.22.x, will be in 3.22.12