Bug 22781 - Fields on patron search results should be html/json filtered
Summary: Fields on patron search results should be html/json filtered
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords: rel_19_05_candidate
: 22484 (view as bug list)
Depends on: 22715
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-26 10:22 UTC by Nick Clemens
Modified: 2019-05-30 07:20 UTC (History)
6 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
Bug 22781: Add tests (4.46 KB, patch)
2019-05-03 01:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry (2.72 KB, patch)
2019-05-03 01:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22781: Test patron's info filtering (4.27 KB, patch)
2019-05-03 01:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22781: Does not double filter address (3.44 KB, patch)
2019-05-03 01:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22781: Correctly filter the emails (4.10 KB, patch)
2019-05-03 01:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22781: Add tests (4.46 KB, patch)
2019-05-06 19:31 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry (2.76 KB, patch)
2019-05-06 19:32 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22781: Test patron's info filtering (4.31 KB, patch)
2019-05-06 19:32 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22781: Does not double filter address (3.48 KB, patch)
2019-05-06 19:32 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22781: Correctly filter the emails (4.15 KB, patch)
2019-05-06 19:32 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22781: Add tests (4.51 KB, patch)
2019-05-11 09:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry (2.82 KB, patch)
2019-05-11 09:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22781: Test patron's info filtering (4.37 KB, patch)
2019-05-11 09:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22781: Does not double filter address (3.54 KB, patch)
2019-05-11 09:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22781: Correctly filter the emails (4.20 KB, patch)
2019-05-11 09:28 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 2019-04-26 10:22:38 UTC
On bug 22715 we added a filter for circulation notes - this should probably be applied to all the fields on the page for consistency/safety
Comment 1 Jonathan Druart 2019-05-03 00:44:21 UTC
*** Bug 22484 has been marked as a duplicate of this bug. ***
Comment 2 Jonathan Druart 2019-05-03 01:34:23 UTC
Created attachment 89267 [details] [review]
Bug 22781: Add tests

Test plan:
Apply only this patch
=> The tests will fail, the library's name is not correctly escaped.

Note:
This applies for the whole patchset, when the tests fail you will need
to remove the patrons added by the tests with the following SQL query:
  delete from borrowers where surname like "test_patron_%";
We are expecting END to be called even if something goes wrong, but
DESTROY must be used instead. This will be fixed separately, on its own
bug report.
Comment 3 Jonathan Druart 2019-05-03 01:34:26 UTC
Created attachment 89268 [details] [review]
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry

This will fix the previous failure. Note that other fields like
borrowernumber, Price escaped values, integers, etc. could be escaped
the same way but will be useless (save polar bears).
Comment 4 Jonathan Druart 2019-05-03 01:34:29 UTC
Created attachment 89269 [details] [review]
Bug 22781: Test patron's info filtering

The patron's data was not correctly escaped actually.

Test plan:
This is a good value for the whole patchset: <strong>fir's"tname</strong> \123 ❤
use it for a patron's firstname, then do a search
Comment 5 Jonathan Druart 2019-05-03 01:34:32 UTC
Created attachment 89270 [details] [review]
Bug 22781: Does not double filter address

Ho, a bug!
Some fields in the address where double filtered.

Test plan:
Use the evil strings in the address field and do a search, you will see
double html encoded characters without this patch.
Comment 6 Jonathan Druart 2019-05-03 01:34:35 UTC
Created attachment 89271 [details] [review]
Bug 22781: Correctly filter the emails

Ho, another bug!

The email was not correctly filtered.

Test plan:
Use the evil string for an email, do a search.
=> Notice that without this patch the results never show up
Comment 7 Liz Rea 2019-05-06 19:27:09 UTC
Just to point out, the interface won't actually allow saving the "bad string email," so you've got to force it in there in the DB. It's probably still good to make sure this is escaped though, because who knows what can happen in migration.
Comment 8 Liz Rea 2019-05-06 19:31:56 UTC
Created attachment 89395 [details] [review]
Bug 22781: Add tests

Test plan:
Apply only this patch
=> The tests will fail, the library's name is not correctly escaped.

Note:
This applies for the whole patchset, when the tests fail you will need
to remove the patrons added by the tests with the following SQL query:
  delete from borrowers where surname like "test_patron_%";
We are expecting END to be called even if something goes wrong, but
DESTROY must be used instead. This will be fixed separately, on its own
bug report.

NOTE: I (Liz) did not test this first patch because my selenium was not working, that is why my SO line is not on the patch. If someone could do that quickly I'd be really grateful.
Comment 9 Liz Rea 2019-05-06 19:32:05 UTC
Created attachment 89396 [details] [review]
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry

This will fix the previous failure. Note that other fields like
borrowernumber, Price escaped values, integers, etc. could be escaped
the same way but will be useless (save polar bears).

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 10 Liz Rea 2019-05-06 19:32:08 UTC
Created attachment 89397 [details] [review]
Bug 22781: Test patron's info filtering

The patron's data was not correctly escaped actually.

Test plan:
This is a good value for the whole patchset: <strong>fir's"tname</strong> \123 ❤
use it for a patron's firstname, then do a search

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 11 Liz Rea 2019-05-06 19:32:11 UTC
Created attachment 89398 [details] [review]
Bug 22781: Does not double filter address

Ho, a bug!
Some fields in the address where double filtered.

Test plan:
Use the evil strings in the address field and do a search, you will see
double html encoded characters without this patch.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 12 Liz Rea 2019-05-06 19:32:14 UTC
Created attachment 89399 [details] [review]
Bug 22781: Correctly filter the emails

Ho, another bug!

The email was not correctly filtered.

Test plan:
Use the evil string for an email, do a search.
=> Notice that without this patch the results never show up

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 13 Katrin Fischer 2019-05-11 09:28:43 UTC
Created attachment 89582 [details] [review]
Bug 22781: Add tests

Test plan:
Apply only this patch
=> The tests will fail, the library's name is not correctly escaped.

Note:
This applies for the whole patchset, when the tests fail you will need
to remove the patrons added by the tests with the following SQL query:
  delete from borrowers where surname like "test_patron_%";
We are expecting END to be called even if something goes wrong, but
DESTROY must be used instead. This will be fixed separately, on its own
bug report.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2019-05-11 09:28:48 UTC
Created attachment 89583 [details] [review]
Bug 22781: Escape cardnumber, category's description, library's name and dateexpiry

This will fix the previous failure. Note that other fields like
borrowernumber, Price escaped values, integers, etc. could be escaped
the same way but will be useless (save polar bears).

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Katrin Fischer 2019-05-11 09:28:52 UTC
Created attachment 89584 [details] [review]
Bug 22781: Test patron's info filtering

The patron's data was not correctly escaped actually.

Test plan:
This is a good value for the whole patchset: <strong>fir's"tname</strong> \123 ❤
use it for a patron's firstname, then do a search

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2019-05-11 09:28:56 UTC
Created attachment 89585 [details] [review]
Bug 22781: Does not double filter address

Ho, a bug!
Some fields in the address where double filtered.

Test plan:
Use the evil strings in the address field and do a search, you will see
double html encoded characters without this patch.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2019-05-11 09:28:59 UTC
Created attachment 89586 [details] [review]
Bug 22781: Correctly filter the emails

Ho, another bug!

The email was not correctly filtered.

Test plan:
Use the evil string for an email, do a search.
=> Notice that without this patch the results never show up

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2019-05-11 09:29:48 UTC
NOTE FOR RM: I still have trouble running Selenium tests (server won't start right now). So I couldn't test them :(
Comment 19 Martin Renvoize 2019-05-12 08:23:19 UTC
Selenium Tests all pass :), Katrin gave me a heads up on IRC to run them for her.
Comment 20 Nick Clemens 2019-05-15 12:14:44 UTC
Awesome work all!

Pushed to master for 19.05
Comment 21 Martin Renvoize 2019-05-30 07:20:40 UTC
Pushed to 18.11.x for 18.11.06