Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client.
Summary: Too many search cursor cookies overflow HTTP-header size, when making multipl...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 10404
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-10 17:00 UTC by Olli-Antti Kivilahti
Modified: 2015-12-03 22:03 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11369 - Updating the jquery.cookie.js-plugin (4.53 KB, patch)
2013-12-11 13:22 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client. (3.49 KB, patch)
2013-12-11 13:22 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 11369 - Updating the jquery.cookie.js-plugin (3.80 KB, patch)
2014-02-26 15:53 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client. (3.54 KB, patch)
2014-02-26 15:53 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 11369 - Correct usage of me.searchid in browser.js (3.75 KB, patch)
2014-02-26 16:09 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 11369 - General searchid in browser.js (2.52 KB, patch)
2014-02-26 16:25 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 11369 - Updating the jquery.cookie.js-plugin (3.84 KB, patch)
2014-02-28 16:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client. (3.60 KB, patch)
2014-02-28 16:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11369 - Correct usage of me.searchid in browser.js (3.81 KB, patch)
2014-02-28 16:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11369 - General searchid in browser.js (2.59 KB, patch)
2014-02-28 16:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11369 - Updating the jquery.cookie.js-plugin (3.93 KB, patch)
2014-03-21 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client. (3.67 KB, patch)
2014-03-21 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11369 - Correct usage of me.searchid in browser.js (3.87 KB, patch)
2014-03-21 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11369 - General searchid in browser.js (2.95 KB, patch)
2014-03-21 07:35 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 Olli-Antti Kivilahti 2013-12-10 17:00:24 UTC
When making a new search or navigating the search result pages, catalogue/search.pl->browser.js generates a search cursor cookie for every page change or search. When 30+ search/page change -actions are done, browser has collected 30+ search cursor cookies.

This causes the HTTP-header to bloat and get rejected by apache2. User needs to reset cookies to get rid of this issue and be able to continue using Koha.

------------
- SOLUTION -
------------

Name searchCookies (search cursor cookies) as "scs_<timestamp>" and remove the 3 oldest cookies if more than 15 searchCookies are present.
Comment 1 Olli-Antti Kivilahti 2013-12-10 17:02:01 UTC
Sponsored by the Joensuu Regional Library
Comment 2 Olli-Antti Kivilahti 2013-12-11 13:22:07 UTC Comment hidden (obsolete)
Comment 3 Olli-Antti Kivilahti 2013-12-11 13:22:38 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2013-12-11 13:25:51 UTC
Sponsored by Joensuu Regional Library
Comment 5 Jonathan Druart 2014-01-15 13:59:06 UTC
I did not test this patch but the issue makes me think to bug 10952.
Julian fixed the issue storing the values in session instead of cookies.
Comment 6 Fridolin SOMERS 2014-02-26 15:53:07 UTC Comment hidden (obsolete)
Comment 7 Fridolin SOMERS 2014-02-26 15:53:32 UTC Comment hidden (obsolete)
Comment 8 Fridolin SOMERS 2014-02-26 16:00:29 UTC
I've signed with a few trivial changes : 
  - I've set : scsCookieKeys.length >= 10 instead of > to maintain 10 cookies max.
  - I've removed '<<' from '//EO Bug_11369' because it is better not using several '<' like in patch conflicts (<<<<<<).

@Jonathan indeed, the could use session but it seems that it tries to use maximum javascript only.
For me best of all would be using the HTML5 SessionStorage feature.
Comment 9 Fridolin SOMERS 2014-02-26 16:09:41 UTC Comment hidden (obsolete)
Comment 10 Fridolin SOMERS 2014-02-26 16:25:00 UTC Comment hidden (obsolete)
Comment 11 Fridolin SOMERS 2014-02-26 16:26:15 UTC
Two new patches need signoff :
Bug 11369 - Correct usage of me.searchid in browser.js
Bug 11369 - General searchid in browser.js
Comment 12 Kyle M Hall 2014-02-28 16:46:06 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-02-28 16:46:27 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-02-28 16:46:31 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2014-02-28 16:46:34 UTC Comment hidden (obsolete)
Comment 16 Fridolin SOMERS 2014-03-07 13:45:17 UTC
In last patch, "General searchid in browser.js", i mean "Generate ...".
:D
Comment 17 Katrin Fischer 2014-03-21 07:31:39 UTC
I think this patch can cause some problems, when you still have an old cookie in your browser. I had this error showing when testing batch modifying items:

Error: Syntax error, unrecognized expression: :nth-child
...{href:function(a){return a.getAttribute("href")},type:function(a){return a.getAt...

After restarting the browser, it's no problem. 

... just noting - no blocker.
Comment 18 Katrin Fischer 2014-03-21 07:34:49 UTC
Created attachment 26501 [details] [review]
[PASSED QA] Bug 11369 - Updating the jquery.cookie.js-plugin

The current jquery.cookie-plugin crashes when trying to fetch
all cookies using $.cookie();
Downloaded the newest plugin version and minified it.
Now works as intended.

Encountered an issue with the plugin now returning null when
no cookies are found, and applied a fix in browser.js.

-------------
- Test plan -
-------------
Plugin is used in browser.js and batchMod.js so testing both

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 19 Katrin Fischer 2014-03-21 07:34:53 UTC
Created attachment 26502 [details] [review]
[PASSED QA] Bug 11369 - Too many search cursor cookies overflow HTTP-header size, when making multiple searches in the staff client.

To replicate this issue, make multiple searches in catalogue/search.pl.
50+ Should be enough to cause the HTTP-request header to overgrow.
One can verify this issue by observing the searchCookie growth in
browser's stored cookies.

-------------
- TEST PLAN -
-------------

Keep making searches.
One should never have more than 10 searchCookies. Browser might display only 9,
because for some reason the newest js-generated cookie is not included in
Firefox's cookies listing.

------------
- DRAWBACK -
------------

Removing these cookies disables the search cursor for traversing search
results (next/previous) for the removed cookie. This maybe be problematic
in some cases,
(for ex when multiple search tabs need to be open and they need to be traversed)
One easy solution is to grow the amount of stored searchCookies from 10 to 20,
but 10 is chosen so there will be plenty of room for other cookies as well.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 20 Katrin Fischer 2014-03-21 07:34:57 UTC
Created attachment 26503 [details] [review]
[PASSED QA] Bug 11369 - Correct usage of me.searchid in browser.js

In browser.js, at creation of browser, the searchid is transmited to
JS object into me.searchid.
To be consistant, me.searchid should always be use, never searchid alone.
In browseRecords function, setting searchid as parameter is useless
because it is defined in me.searchid.

Test with test plan of Bug 10404

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 21 Katrin Fischer 2014-03-21 07:35:00 UTC
Created attachment 26504 [details] [review]
[PASSED QA] Bug 11369 - General searchid in browser.js

The search browser feature uses nearly only the browser.js file.
That is why I propose to move the searchid generation from search.pl
to browser.js.
We then use Date.getTime() to use current timestamp as searchid,
prefixed by 'scs_' like before.

Test by using test plan of main patch :
Too many search cursor cookies overflow HTTP-header size, when
making multiple searches in the staff client

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes tests and QA script.
Tested the browse functionality in staff by using the different
options (back to results, next, previous, etc) and the batch
modifications for items. An old cookie can cause a Javascript error,
but after restarting the browser/deleting cookies it all works
correctly.
Comment 22 Katrin Fischer 2014-03-21 07:41:31 UTC
This works as described, at least if I haven't missed something.

Like some of the other commenters I would feel a bit better about this if we were not losing history here at some point by limiting the number of cookies, but stored the data in another place - sessions or using the HTML5 feature.
Comment 23 Galen Charlton 2014-05-02 23:27:37 UTC
Pushed to master.  Thanks, Olli and Fridolin!

In retrospect, the OPAC's version of results browsing that doesn't depend on cookies or retaining state seems preferable.
Comment 24 Fridolin SOMERS 2014-05-07 16:36:04 UTC
Pushed to 3.14.x, will be in 3.14.07
Comment 25 David Cook 2014-07-16 04:17:30 UTC
Sweet! I've encountered this problem a few times recently so it's nice to see it get remedied (in some way).
Comment 26 David Cook 2014-12-01 05:29:24 UTC
(In reply to Katrin Fischer from comment #22)
> This works as described, at least if I haven't missed something.
> 
> Like some of the other commenters I would feel a bit better about this if we
> were not losing history here at some point by limiting the number of
> cookies, but stored the data in another place - sessions or using the HTML5
> feature.

I think using sessions is really the way to go.

I'm going to backport this patch to our local Koha, but I still think it's less than ideal.

I think the best way to do this is to keep using unique IDs for the search, and to store them and the relevant data in the active session in the database. 

In this way, we can still have multiple tabs open with unique searches and still use the record browser without the limits imposed by using cookies.

At the moment, we store the OPAC record browser info in the active session in the database, but the limitation is that the key isn't unique (ie the key is "busc"). That means if you have multiple tabs open, the browser for all of them will return the results for the latest search... definitely not a good thing!
Comment 27 David Cook 2014-12-22 01:10:06 UTC
Adding reference to http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12295 for anyone adding patches manually...