Bug 22836 - Tests catching XSS vulnerabilities in pagination are not correct
Summary: Tests catching XSS vulnerabilities in pagination are not correct
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords: rel_19_05_candidate
Depends on: 22478
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-03 01:04 UTC by Jonathan Druart
Modified: 2019-05-30 07:06 UTC (History)
4 users (show)

See Also:
Change 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:
19.05.00, 18.11.06


Attachments
Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" (1.91 KB, patch)
2019-05-03 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" (1.47 KB, patch)
2019-05-03 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name (1.36 KB, patch)
2019-05-03 01:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" (1.97 KB, patch)
2019-05-13 19:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" (1.53 KB, patch)
2019-05-13 19:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name (1.43 KB, patch)
2019-05-13 19:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags" (2.03 KB, patch)
2019-05-13 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean" (1.59 KB, patch)
2019-05-13 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name (1.49 KB, patch)
2019-05-13 20:51 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 Jonathan Druart 2019-05-03 01:04:13 UTC
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.
Comment 1 Jonathan Druart 2019-05-03 01:06:21 UTC
Created attachment 89264 [details] [review]
Revert "Bug 22478: (QA follow-up) Update tests to check for any script tags"

This reverts commit d9b3b90849fd95d296b73b330c4e32ff018c81a5.
Comment 2 Jonathan Druart 2019-05-03 01:06:23 UTC
Created attachment 89265 [details] [review]
Revert "Bug 22478: (QA follow-up) Handle category in opac-shelves like a boolean"

This reverts commit 375dd35d5354d46c4c16f534494b123ce370c3f4.
Comment 3 Jonathan Druart 2019-05-03 01:06:26 UTC
Created attachment 89266 [details] [review]
Bug 22836: (follow-up) Bug 22478: (QA follow-up) Make test consistent with variable name

One occurrence was missing.
Comment 4 Marcel de Rooy 2019-05-03 05:41:39 UTC
(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.
Comment 5 Jonathan Druart 2019-05-03 13:08:38 UTC
Hello Marcel,
I have tried to explain it in the comment of this bug report. Which part does not make sense to you?
Comment 6 Marcel de Rooy 2019-05-06 05:30:59 UTC
(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.
Comment 7 Jonathan Druart 2019-05-06 15:24:54 UTC
(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.
Comment 8 Jonathan Druart 2019-05-06 15:28:57 UTC
IMO It's not solid code, it's code which leads us to believe that the values are not correctly filtered.
Comment 9 Martin Renvoize 2019-05-13 13:28:53 UTC
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.
Comment 10 Martin Renvoize 2019-05-13 19:00:39 UTC
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>
Comment 11 Martin Renvoize 2019-05-13 19:00:41 UTC
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>
Comment 12 Martin Renvoize 2019-05-13 19:00:44 UTC
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>
Comment 13 Katrin Fischer 2019-05-13 20:51:32 UTC
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>
Comment 14 Katrin Fischer 2019-05-13 20:51:35 UTC
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>
Comment 15 Katrin Fischer 2019-05-13 20:51:39 UTC
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>
Comment 16 Katrin Fischer 2019-05-13 20:52:12 UTC
I am not sure about the commit messages - the first 2 are not referring to this bug. Leaving that to Nick ;)
Comment 17 Jonathan Druart 2019-05-13 23:00:58 UTC
(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.
Comment 18 Nick Clemens 2019-05-15 12:16:05 UTC
Awesome work all!

Pushed to master for 19.05
Comment 19 Martin Renvoize 2019-05-30 07:06:24 UTC
Pushed to 18.11.x for 18.11.06