Bug 17392 - opac/svc/overdrive_proxy is not plack safe
Summary: opac/svc/overdrive_proxy is not plack safe
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 7172
  Show dependency treegraph
 
Reported: 2016-10-03 18:54 UTC by Nick Clemens
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 17392: Make svc/overdrive_proxy plack safe (1.45 KB, patch)
2016-10-04 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17392 - opac/svc/overdrive_proxy is not plack safe (1.44 KB, patch)
2016-10-05 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17392 - opac/svc/overdrive_proxy is not plack safe (1.49 KB, patch)
2016-10-06 08:40 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 17392 - opac/svc/overdrive_proxy is not plack safe (1.62 KB, patch)
2016-10-11 09:39 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 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