WARNING: reproducing this bug makes server/computer UNRESPONSIVE. To reproduce the Denial of Service go to http://localhost:8080/cgi-bin/koha/opac-search.pl?advsearch=1&sort_by=relevance&do=Search and using browser's developer console set a new cookie: num_paragraph: 1000000000 After setting the cookie refresh the page and wait for the server to freeze.
I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug.
Created attachment 123407 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307.
Created attachment 123409 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307.
Created attachment 123412 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 123413 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 123414 [details] [review] Bug 28784: (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 123416 [details] [review] Bug 28784: (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 123467 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 123468 [details] [review] Bug 28784: (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
git grep num_para koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc: jQuery.removeCookie("num_paragraph", { path: '/'});
Just looking a little bit more to the +/- buttons ..
Created attachment 123558 [details] [review] Bug 28784: Remove code related to num_paragraph cookie It could lead to server freeze if set to a big value (we are pushing into an array and so RAM is being fulfilled, and CPU is looping). I don't understand the point of this cookie. var numPar = $("#booleansearch fieldset p").size(); if (numPar > [% search_boxes_count | html %]){ jQuery.cookie("num_paragraph", numPar,{ path: '/'}); }else{ jQuery.removeCookie("num_paragraph", { path: '/'}); } But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements. I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before. From this comment: # determine what to display next to the search boxes (ie, boolean option # shouldn't appear on the first one, scan indexes should, adding a new # box should only appear on the last, etc. The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug. Test plan: Read the code, check that the above is correct. Search for regression in this "return to last adv search" feature added by bug 13307. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 123559 [details] [review] Bug 28784: (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 123560 [details] [review] Bug 28784: (QA follow-up) Remove num_paragraph cookie from include Still found in opac-bottom.inc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 123561 [details] [review] Bug 28784: (follow-up) Improve operation of button plus/less Do not remove the op select on the first row. Allow plus/less on all lines. Allow less until we have one line left. Make sure that first op is disabled and hidden with plus/less. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Last follow-up might go on its own?
(In reply to Marcel de Rooy from comment #16) > Last follow-up might go on its own? Yes please, we will have to backport and publish security releases. The fewer changes the better :)
(In reply to Jonathan Druart from comment #17) > (In reply to Marcel de Rooy from comment #16) > > Last follow-up might go on its own? > > Yes please, we will have to backport and publish security releases. The > fewer changes the better :)
Comment on attachment 123561 [details] [review] Bug 28784: (follow-up) Improve operation of button plus/less Follow-up moved to bug 28821
Rebase needed for 20.05.xx > Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes
(In reply to Victor Grousset/tuxayo from comment #20) > Rebase needed for 20.05.xx > > > Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes I guess you need to keep this and drop the second chunk: - [% FOREACH search_box IN search_boxes_loop %] + [% FOREACH search_box IN [ 1, 2, 3] %]
What is a clean way to do this? Git doesn't even apply and conflict. So my plan is to download the patch file, and edit it directly with git am.
This doesn't apply cleanly to 19.11.x
Somehow now git managed to apply & conflict. So I'll be able to try to solve. Maybe I got lucky and my last backports brought a missing piece. Clocking off, will continue tomorrow. (note: don't forget the remove the test cookie, otherwise it will blow up latter when not being ready to kill the process...)
Created attachment 123866 [details] [review] Bug 28784: [20.05.x] (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 123867 [details] [review] Bug 28784: [20.05.x] (follow-up) Always make three search boxes The previous patch removed search_boxes_loop - that's okay, it was always getting the same three values. If we don't do something in the template though, we get no boxes Ultimately this should be a include, and not a hardcoded loop, but keeping changes small for backporting Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
For 20.05.x Bug 28784: (QA follow-up) Remove num_paragraph cookie from include Bug 28784: [20.05.x] (follow-up) Always make three search boxes Bug 28784: Remove code related to num_paragraph cookie And you also need: Bug 28518: Display missing inputs for "Return to the last advanced search"
(In reply to Jonathan Druart from comment #27) > For 20.05.x > > Bug 28784: (QA follow-up) Remove num_paragraph cookie from include > Bug 28784: [20.05.x] (follow-up) Always make three search boxes > Bug 28784: Remove code related to num_paragraph cookie > > And you also need: > Bug 28518: Display missing inputs for "Return to the last advanced search" Same for 19.11.x
On 19.11.x after doing git bz apply 28784 I get: Bug 28784 - DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't require login!) 123558 - Bug 28784: Remove code related to num_paragraph cookie 123559 - Bug 28784: (follow-up) Always make three search boxes 123560 - Bug 28784: (QA follow-up) Remove num_paragraph cookie from include 123867 - Bug 28784: [20.05.x] (follow-up) Always make three search boxes Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28784: Remove code related to num_paragraph cookie Applying: Bug 28784: (follow-up) Always make three search boxes error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt). error: could not build fake ancestor Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-28784-follow-up-Always-make-three-search-boxes-1CY3s7.patch
(In reply to wainuiwitikapark from comment #29) > On 19.11.x after doing git bz apply 28784 I get: > > Bug 28784 - DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't > require login!) > > 123558 - Bug 28784: Remove code related to num_paragraph cookie > 123559 - Bug 28784: (follow-up) Always make three search boxes > 123560 - Bug 28784: (QA follow-up) Remove num_paragraph cookie from include > 123867 - Bug 28784: [20.05.x] (follow-up) Always make three search boxes See comment 27, you don't need the second patch.
Thanks Jonathan, it works. Pushed on the security repo on branch 20.05.x-security
The question came about whether or not should we do a synced release. (non-urgent, just synced) So, does this bug warrants it?
(In reply to Victor Grousset/tuxayo from comment #31) > Thanks Jonathan, it works. Pushed on the security repo on branch > 20.05.x-security I have now also pushed to security repo on 19.11.x-security branch
Pushed to 20.11.x for 20.11.09
Pushed to 21.05.x for 21.05.03 Pushed to 20.11.x for 20.11.09 Pushed to 20.05.x for 20.05.15 Pushed to 19.11.x for 19.11.21
Pushed to master for 21.11.00.