See bug 22478 comments 44 and 45. The tests were added originally to catch XSS vulnerabilities when pagination was used (shelves, reviews, authorities searches, etc.). With one of the QA followup (Handle category in opac-shelves like a boolean) we did not trust the escape by resetting the "category" if not set to 1 or 2. We should rely on the correct filtering instead. However, if one really wants to use this change, we should adapt the tests to catch the correct filtered values (and so do not use unlike), in another area (i.e. not shelves, where we are handling the invalid values differently). I am suggestion to revert those patches, as it is the easiest solution.
Created attachment 89264 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5.
Created attachment 89265 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4.
Created attachment 89266 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing.
(In reply to Jonathan Druart from comment #2) > Created attachment 89265 [details] [review] [review] > Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a > boolean" > > This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. I don't see the sense of removing these lines.
Hello Marcel, I have tried to explain it in the comment of this bug report. Which part does not make sense to you?
(In reply to Jonathan Druart from comment #5) > Hello Marcel, > I have tried to explain it in the comment of this bug report. Which part > does not make sense to you? Improve filtering is great. But we do not need to remove solid code.
(In reply to Marcel de Rooy from comment #6) > (In reply to Jonathan Druart from comment #5) > > Hello Marcel, > > I have tried to explain it in the comment of this bug report. Which part > > does not make sense to you? > > Improve filtering is great. But we do not need to remove solid code. Then we need to write more tests. I *strongly* think that we must remove this code.
IMO It's not solid code, it's code which leads us to believe that the values are not correctly filtered.
I've come to the conclusion that Jonathan is correct here. 1) We need to have a generic test for missing filters in pagination 2) The 'noise' created by not cleaning up the lines directly is inconsequential (in so much as it'll only ever appear if someone is attempting to XSS us AND the end user will never see it unless they're inspecting the html). Signing off.
Created attachment 89677 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89678 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89679 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89710 [details] [review] Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89711 [details] [review] Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4. https://bugs.koha-community.org/show_bug.cgi?id=22836 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89712 [details] [review] Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name One occurrence was missing. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
I am not sure about the commit messages - the first 2 are not referring to this bug. Leaving that to Nick ;)
(In reply to Katrin Fischer from comment #16) > I am not sure about the commit messages - the first 2 are not referring to > this bug. Leaving that to Nick ;) They are ok, as they are reverting commits I think we should keep this syntax.
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.06