Bug 23329 - tracklinks.pl accepts any url from a parameter for proxying if not tracking
Summary: tracklinks.pl accepts any url from a parameter for proxying if not tracking
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23275 (view as bug list)
Depends on:
Blocks: 30261 30803 23836
  Show dependency treegraph
 
Reported: 2019-07-17 07:37 UTC by Steven Hampton
Modified: 2022-05-19 07:05 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00


Attachments
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.59 KB, patch)
2019-07-17 11:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.62 KB, patch)
2019-07-18 11:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Dash symbol at line 82 (312.15 KB, image/png)
2019-07-18 11:16 UTC, Mohd Hafiz Yusoff
Details
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.67 KB, patch)
2019-07-23 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23329: (follow-up) Use any instead of grep (1.17 KB, patch)
2019-07-23 19:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.77 KB, patch)
2019-08-09 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23329: (follow-up) Use any instead of grep (1.26 KB, patch)
2019-08-09 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter (1.59 KB, patch)
2019-08-09 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber (1.25 KB, patch)
2019-08-09 09:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23329: Add regression tests (5.99 KB, patch)
2019-08-14 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Add regression tests (6.00 KB, patch)
2019-08-14 12:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter" (1.31 KB, patch)
2019-08-14 18:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23329: Move error page to its own subroutine (2.44 KB, patch)
2019-08-14 18:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23329: Fix tests (3.14 KB, patch)
2019-08-14 18:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.79 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (follow-up) Use any instead of grep (1.29 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter (1.61 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber (1.28 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Add regression tests (6.00 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter" (1.37 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: Move error page to its own subroutine (2.50 KB, patch)
2019-08-15 07:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: Fix tests (3.20 KB, patch)
2019-08-15 07:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Restore DB after test (1.53 KB, patch)
2019-08-15 07:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.86 KB, patch)
2019-08-15 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: (follow-up) Use any instead of grep (1.36 KB, patch)
2019-08-15 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter (1.68 KB, patch)
2019-08-15 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber (1.35 KB, patch)
2019-08-15 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Add regression tests (6.07 KB, patch)
2019-08-15 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter" (1.44 KB, patch)
2019-08-15 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: Move error page to its own subroutine (2.57 KB, patch)
2019-08-15 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: Fix tests (3.27 KB, patch)
2019-08-15 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Restore DB after test (1.64 KB, patch)
2019-08-15 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23329: Only redirect tracklinks.pl to urls contained in records (4.91 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (follow-up) Use any instead of grep (1.40 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter (1.73 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber (1.39 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Add regression tests (6.20 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: Move error page to its own subroutine (2.62 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: Fix tests (2.76 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23329: (RM follow-up) Restore DB after test (1.88 KB, patch)
2019-08-16 09:24 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Hampton 2019-07-17 07:37:32 UTC
Greeting, Reader.

My name is Steven Hampton, and looks like I have discovered a web vulnerability on your web domain. It looks like common issue known as "open redirection" as it would redirect the application toward any destination by any attacker crafted maliciously. It would often have a functionality that would return the users without any authentication. 

OBB-896593: https://www.openbugbounty.org/reports/896593/

CRAFTED POC: https://koha.base-library.nhs.uk/cgi-bin/koha/tracklinks.pl?uri=//openbugbounty.org

VULN POC: https://koha.base-library.nhs.uk/cgi-bin/koha/tracklinks.pl?uri= 



Hopefully this message would get read, and the issue get resolved

Thanks, 

Steven Hampton {Keritzy} // keritzy@protonmail.com


PS: I have received the email message from Jon Turner :)
Comment 1 Nick Clemens 2019-07-17 11:00:18 UTC
*** Bug 23275 has been marked as a duplicate of this bug. ***
Comment 2 Nick Clemens 2019-07-17 11:08:32 UTC
Created attachment 91566 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add www.google.com to the 856 of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks
Comment 3 Steven Hampton 2019-07-17 11:25:18 UTC
(In reply to Nick Clemens from comment #1)
> *** Bug 23275 has been marked as a duplicate of this bug. ***

I already knew this bug would be a possible duplicate issue. I don't think there is any need for redirect urls ( It would allows possible redirect toward third party urls ) Great for small contribution and support with a common bug issue that has been discovered. 

Thanks for fast reply.

Steven
Comment 4 Mohd Hafiz Yusoff 2019-07-17 14:22:31 UTC
(In reply to Nick Clemens from comment #2)
> Created attachment 91566 [details] [review] [review]
> Bug 23329: Only redirect tracklinks.pl to urls contained in records
> 
> Bug 19487 limited redirection to urls contained in a record/item if we were
> tracking.
> We should probably limit forwarding if not tracking as well.
> Additionally, if we don't have a soucre, let's not forward
> 
> To test:
>  0 - Set TrackClicks syspref to 'Don't track'
>  1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
>  2 - You get forwarded to google
>  3 - Set TrackClicks to 'Track anonymously'
>  4 - You get a 404
>  5 - Apply patch
>  6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
>  7 - You get a 404
>  8 - Set TrackClicks syspref to 'Don't track'
>  9 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
>      Choose a biblionumber that exists
> 10 - You get a 404
> 11 - Add www.google.com to the 856 of the record used above
> 12 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
> 13 - You are redirected
> 14 - Confirm redirection and 404 as expected with other settings of
> TrackClicks

Refer on attachment at line 82 (old), It is - or }
Comment 5 Mohd Hafiz Yusoff 2019-07-18 05:17:11 UTC
(In reply to Nick Clemens from comment #2)
> Created attachment 91566 [details] [review] [review]
> Bug 23329: Only redirect tracklinks.pl to urls contained in records
> 
> Bug 19487 limited redirection to urls contained in a record/item if we were
> tracking.
> We should probably limit forwarding if not tracking as well.
> Additionally, if we don't have a soucre, let's not forward
> 
> To test:
>  0 - Set TrackClicks syspref to 'Don't track'
>  1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
>  2 - You get forwarded to google
>  3 - Set TrackClicks to 'Track anonymously'
>  4 - You get a 404
>  5 - Apply patch
>  6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com
>  7 - You get a 404
>  8 - Set TrackClicks syspref to 'Don't track'
>  9 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
>      Choose a biblionumber that exists
> 10 - You get a 404
> 11 - Add www.google.com to the 856 of the record used above
> 12 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=//www.google.com&biblionumber=1
> 13 - You are redirected
> 14 - Confirm redirection and 404 as expected with other settings of
> TrackClicks

I get error if remove line 82 on old file. If I ignore line 82 on old file, it's work, but test number 13 not work (get a 404)
Comment 6 Nick Clemens 2019-07-18 11:03:47 UTC
Created attachment 91572 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks
Comment 7 Nick Clemens 2019-07-18 11:06:29 UTC
I updated the test plan, the protocol needs to be included in the 856$u since GetMarcUrls adds it and we are checking for an equality, not a contains

Probably we need to perform the same action when building the links on the details page, but maybe that is another report (not security)

I am not sure what the comment below means?
> 
> I get error if remove line 82 on old file. If I ignore line 82 on old file,
> it's work, but test number 13 not work (get a 404)
Comment 8 Mohd Hafiz Yusoff 2019-07-18 11:16:57 UTC
Created attachment 91573 [details]
Dash symbol at line 82
Comment 9 Nick Clemens 2019-07-18 11:23:03 UTC
(In reply to Mohd Hafiz Yusoff from comment #8)
> Created attachment 91573 [details]
> Dash symbol at line 82

That is just part of the diff, it was never actually part of the file I believe - you can ignore that
Comment 10 Mohd Hafiz Yusoff 2019-07-19 09:48:13 UTC
(In reply to Nick Clemens from comment #6)
> Created attachment 91572 [details] [review] [review]
> Bug 23329: Only redirect tracklinks.pl to urls contained in records
> 
> Bug 19487 limited redirection to urls contained in a record/item if we were
> tracking.
> We should probably limit forwarding if not tracking as well.
> Additionally, if we don't have a soucre, let's not forward
> 
> To test:
>  0 - Set TrackClicks syspref to 'Don't track'
>  1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
>  2 - You get forwarded to google
>  3 - Set TrackClicks to 'Track anonymously'
>  4 - You get a 404
>  5 - Apply patch
>  6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
>  7 - You get a 404
>  8 - Set TrackClicks syspref to 'Don't track'
>  9 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.
> com&biblionumber=1
>      Choose a biblionumber that exists
> 10 - You get a 404
> 11 - Add http://www.google.com to the 856$u of the record used above
> 12 - Hit
> localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.
> com&biblionumber=1
> 13 - You are redirected
> 14 - Confirm redirection and 404 as expected with other settings of
> TrackClicks

This patch is OK, how to signoff?
Comment 11 Mohd Hafiz Yusoff 2019-07-19 15:04:57 UTC
Using Sandbox, look not good
http://sandboxes.ptfs-europe.co.uk/provision_log/bug23329fy
Comment 12 Mohd Hafiz Yusoff 2019-07-23 17:19:53 UTC
Cannot create sandbox to signoff because Authentication or permission failure
Comment 13 Nick Clemens 2019-07-23 17:29:52 UTC
(In reply to Mohd Hafiz Yusoff from comment #12)
> Cannot create sandbox to signoff because Authentication or permission failure

The sandboxes won't be able to retrieve security bugs, I am going to see if I can get a community tester
Comment 14 Tomás Cohen Arazi 2019-07-23 19:42:19 UTC
Created attachment 91759 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2019-07-23 19:42:24 UTC
Created attachment 91760 [details] [review]
Bug 23329: (follow-up) Use any instead of grep

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Jonathan Druart 2019-08-05 17:06:31 UTC
Should not we move that code to Koha::LinkTracker and provide tests?
Comment 17 Tomás Cohen Arazi 2019-08-05 17:09:33 UTC
(In reply to Jonathan Druart from comment #16)
> Should not we move that code to Koha::LinkTracker and provide tests?

Yes, but this was targeting the stable branches.
Comment 18 Marcel de Rooy 2019-08-09 08:07:26 UTC
QA: Looking here
Comment 19 Marcel de Rooy 2019-08-09 09:37:36 UTC
Created attachment 92109 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2019-08-09 09:37:41 UTC
Created attachment 92110 [details] [review]
Bug 23329: (follow-up) Use any instead of grep

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2019-08-09 09:37:47 UTC
Created attachment 92111 [details] [review]
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter

If you pass a URI with a biblionumber without specifying the itemnumber,
tracklinks did not redirect an item URI.

Test plan:
[1] Add URI in an item.
[2] Pass this URI with the itemnumber to tracklinks. Should pass.
[3] Pass this URI with the biblionumber to tracklinks. Should pass now too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2019-08-09 09:37:51 UTC
Created attachment 92112 [details] [review]
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber

Passing a wrong biblionumber generates a warning:
    GetMarcUrls called on undefined record at opac/tracklinks.pl line 58.

Test plan:
[1] Try it again with a wrong biblionumber and check the logs.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2019-08-09 09:38:38 UTC
Martin: If you insist ;) I will move the two follow-ups somewhere else
Comment 24 Martin Renvoize 2019-08-13 14:07:31 UTC
As harsh as it sounds.. I'd really like to see some regression tests for this.. I realise it's not a module change but as a security issue I think a regression test is suitable..

I'll take a look at trying to write a quick one before Fridolin looks at pushing it.
Comment 25 Martin Renvoize 2019-08-14 12:15:18 UTC
Created attachment 92187 [details] [review]
Bug 23329: Add regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2019-08-14 12:15:50 UTC
Created attachment 92188 [details] [review]
Bug 23329: (RM follow-up) Add regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2019-08-14 12:23:44 UTC
Right....

I've added the promised regressions tests and I believe they're proving all cases, please read and challenge me if you feel they are incorrect.

A few points here:
1) The tests fail!
- I believe we should always 404 when there is neither a biblionumber nor an itemnumber passed regardless of the TrackClicks setting (otherwise we still have an open redirection vulnerability which this is trying to solve)
- I'm pretty sure the 'Allow item URI with a biblionumber parameter' patch fundamentally re-opens the open redirection vulnerability that the original patches resolved by allowing any URI to be passed into tracklinks so long as biblionumber is passed and contains ANY existing biblionumber (regardless of whether said biblio includes a URI or not)

As mentioned, please check my workings.. but this is certainly a QA fail as it stands.
Comment 28 Marcel de Rooy 2019-08-14 12:36:18 UTC
(In reply to Martin Renvoize from comment #27)
> Right....
> 
> I've added the promised regressions tests and I believe they're proving all
> cases, please read and challenge me if you feel they are incorrect.
> 
> A few points here:
> 1) The tests fail!
> - I believe we should always 404 when there is neither a biblionumber nor an
> itemnumber passed regardless of the TrackClicks setting (otherwise we still
> have an open redirection vulnerability which this is trying to solve)
> - I'm pretty sure the 'Allow item URI with a biblionumber parameter' patch
> fundamentally re-opens the open redirection vulnerability that the original
> patches resolved by allowing any URI to be passed into tracklinks so long as
> biblionumber is passed and contains ANY existing biblionumber (regardless of
> whether said biblio includes a URI or not)
> 
> As mentioned, please check my workings.. but this is certainly a QA fail as
> it stands.

Will have a look later this week!
Comment 29 Jonathan Druart 2019-08-14 18:58:29 UTC
Created attachment 92198 [details] [review]
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter"

This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.
Comment 30 Jonathan Druart 2019-08-14 18:58:33 UTC
Created attachment 92199 [details] [review]
Bug 23329: Move error page to its own subroutine
Comment 31 Jonathan Druart 2019-08-14 18:58:37 UTC
Created attachment 92200 [details] [review]
Bug 23329: Fix tests

- www.google.com vs https://www.google.com
- Remove transaction otherwise data are not available from webserver
- Use new C4::Output::output_error to avoid 302 (redirect)
Comment 32 Jonathan Druart 2019-08-14 18:58:59 UTC
My try
Comment 33 Martin Renvoize 2019-08-15 06:49:34 UTC
Nice work, feels like you've caught all the issues, thankyou.

(In reply to Jonathan Druart from comment #31)
> Created attachment 92200 [details] [review] [review]
> Bug 23329: Fix tests
>
> - Remove transaction otherwise data are not available from webserver

Ack, I should have spotted that one, my bad.  Hmmm.. I don't like they we now leave the DB with test data after the test has completed.. I took a look at implementing your cleanup routines as per the selenium tests but it quickly felt cumbersome as the build_sample_* routines actually result in a fairly large chunk of DB activity with all the linked data and I'm not 100% confident about all our CASCADE yet.

I did wonder whether we should start thinking in terms of SQL binary logs and point in time restores for this.. but that's probably another story entirely.  Nothing ever simple is it ;)
Comment 34 Martin Renvoize 2019-08-15 07:57:36 UTC
Created attachment 92212 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2019-08-15 07:57:40 UTC
Created attachment 92213 [details] [review]
Bug 23329: (follow-up) Use any instead of grep

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2019-08-15 07:57:43 UTC
Created attachment 92214 [details] [review]
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter

If you pass a URI with a biblionumber without specifying the itemnumber,
tracklinks did not redirect an item URI.

Test plan:
[1] Add URI in an item.
[2] Pass this URI with the itemnumber to tracklinks. Should pass.
[3] Pass this URI with the biblionumber to tracklinks. Should pass now too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2019-08-15 07:57:46 UTC
Created attachment 92215 [details] [review]
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber

Passing a wrong biblionumber generates a warning:
    GetMarcUrls called on undefined record at opac/tracklinks.pl line 58.

Test plan:
[1] Try it again with a wrong biblionumber and check the logs.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2019-08-15 07:57:50 UTC
Created attachment 92216 [details] [review]
Bug 23329: (RM follow-up) Add regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2019-08-15 07:57:53 UTC
Created attachment 92217 [details] [review]
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter"

This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2019-08-15 07:57:56 UTC
Created attachment 92218 [details] [review]
Bug 23329: Move error page to its own subroutine

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2019-08-15 07:58:00 UTC
Created attachment 92219 [details] [review]
Bug 23329: Fix tests

- www.google.com vs https://www.google.com
- Remove transaction otherwise data are not available from webserver
- Use new C4::Output::output_error to avoid 302 (redirect)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Martin Renvoize 2019-08-15 07:58:03 UTC
Created attachment 92220 [details] [review]
Bug 23329: (RM follow-up) Restore DB after test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2019-08-15 07:59:56 UTC
Right..

All tests no pass and the test itself is idempotent after my final followup..

I think we're now very close with this one but I'd be interested in any comments on my 'hack' to get the test resetting the database as we can't rely on transactions.. with the DB available to us in koha-testing-docker it's not too much of a performance burden.. but it could prove interesting if run on a larger db.. thoughts?
Comment 44 Marcel de Rooy 2019-08-15 08:23:21 UTC
(In reply to Martin Renvoize from comment #39)
> Created attachment 92217 [details] [review] [review]
> Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter"
> 
> This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Whats the reason behind this revert btw ?
Comment 45 Marcel de Rooy 2019-08-15 08:32:26 UTC
(In reply to Martin Renvoize from comment #42)
> Created attachment 92220 [details] [review] [review]
> Bug 23329: (RM follow-up) Restore DB after test
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I dont think that this is a good idea. It is not an action you would expect to be in a test script. You could restore the database at the point you start this test?
If I understand well, the need arises because of problems with nested transactions or so. Well if that is the case, we should address that code; this is just a workaround.
Also note that there is a report on running tests on a separate database. Might that help?
Comment 46 Kyle M Hall 2019-08-15 13:25:42 UTC
Created attachment 92223 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Kyle M Hall 2019-08-15 13:25:52 UTC
Created attachment 92224 [details] [review]
Bug 23329: (follow-up) Use any instead of grep

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 Kyle M Hall 2019-08-15 13:25:56 UTC
Created attachment 92225 [details] [review]
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter

If you pass a URI with a biblionumber without specifying the itemnumber,
tracklinks did not redirect an item URI.

Test plan:
[1] Add URI in an item.
[2] Pass this URI with the itemnumber to tracklinks. Should pass.
[3] Pass this URI with the biblionumber to tracklinks. Should pass now too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Kyle M Hall 2019-08-15 13:25:59 UTC
Created attachment 92226 [details] [review]
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber

Passing a wrong biblionumber generates a warning:
    GetMarcUrls called on undefined record at opac/tracklinks.pl line 58.

Test plan:
[1] Try it again with a wrong biblionumber and check the logs.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Kyle M Hall 2019-08-15 13:26:02 UTC
Created attachment 92227 [details] [review]
Bug 23329: (RM follow-up) Add regression tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Kyle M Hall 2019-08-15 13:26:05 UTC
Created attachment 92228 [details] [review]
Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter"

This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Kyle M Hall 2019-08-15 13:26:09 UTC
Created attachment 92229 [details] [review]
Bug 23329: Move error page to its own subroutine

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Kyle M Hall 2019-08-15 13:26:12 UTC
Created attachment 92230 [details] [review]
Bug 23329: Fix tests

- www.google.com vs https://www.google.com
- Remove transaction otherwise data are not available from webserver
- Use new C4::Output::output_error to avoid 302 (redirect)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 54 Kyle M Hall 2019-08-15 13:26:15 UTC
Created attachment 92231 [details] [review]
Bug 23329: (RM follow-up) Restore DB after test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 55 Martin Renvoize 2019-08-16 08:57:51 UTC
(In reply to Marcel de Rooy from comment #44)
> (In reply to Martin Renvoize from comment #39)
> > Created attachment 92217 [details] [review] [review] [review]
> > Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter"
> > 
> > This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.
> > 
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> Whats the reason behind this revert btw ?

Please see comment 27
> - I'm pretty sure the 'Allow item URI with a biblionumber parameter' patch fundamentally re-opens the open redirection vulnerability that the original patches resolved by allowing any URI to be passed into tracklinks so long as biblionumber is passed and contains ANY existing biblionumber (regardless of whether said biblio includes a URI or not)

This hypothesis was proved correct by the tests I added and Jonathan varified it for me.
Comment 56 Martin Renvoize 2019-08-16 09:08:16 UTC
(In reply to Marcel de Rooy from comment #45)
> (In reply to Martin Renvoize from comment #42)
> > Created attachment 92220 [details] [review] [review] [review]
> > Bug 23329: (RM follow-up) Restore DB after test
> > 
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> I dont think that this is a good idea. It is not an action you would expect
> to be in a test script. 

I agree it's not especially nice.. and perhaps the 'run on a distinct db' patches are the resolution though we still have the problem of this test not being idempotent.. i.e. it changes the DB so could have knock on effects for other tests run after it.

>You could restore the database at the point you start this test?

Erm, that's what this patch does.. we dump the current DB at the beggining of the test.. run the test.. then restore the dump.. so it's entire point is to leave your DB 'as it was'.

> If I understand well, the need arises because of problems with nested
> transactions or so. Well if that is the case, we should address that code;
> this is just a workaround.

Incorrect... it's nothing to do with nested transactions. The issue is distinct connections.  DB transactions work on the connection level.. so you can run a series of DB actions within a transaction on one connection and everything inside the transaction has access to the DB changes resulting from those actions.  However, until you 'commit' the transaction no other connections have access to the data changes. (and so, our 'app' which is sitting on it's own DB connection can't see the test data we're creating - It's the same in the selenium and other www tests).  We don't want to 'commit' our transactions.. we're using them in tests to allow a final 'rollback' instead to restore the DB to it's original state.. but in this case we simply can't do that.. I'm afraid there is no way to code around it.  The alternative option to a dump and load which I do is the approach Jonathan has taken in some of the Selenium tests. He records the results of the client interactions (and local db actions too) into an array of row ID's and then runs 'Delete' actions using that list.  It's not a bad approach but it doesn't achieve the same level of DB reset as a transaction does (incrementing keys are still incremented etc) and also risks leaving some relational data behind as it relies heavily on our CASCADE constraints all being setup to CASCADE DELETE.

I'm certainly open to suggestions of a better way though.. I'm not sure about this heavy-handed approach either but it's the best I could come up with so far.

> Also note that there is a report on running tests on a separate database.
> Might that help?

As I said above.. it may help.. but it'll still leave the DB in a different state after this particular test has run unless we have a simple 'reset' action available to us.
Comment 57 Marcel de Rooy 2019-08-16 09:09:29 UTC
(In reply to Martin Renvoize from comment #27)
> - I'm pretty sure the 'Allow item URI with a biblionumber parameter' patch
> fundamentally re-opens the open redirection vulnerability that the original
> patches resolved by allowing any URI to be passed into tracklinks so long as
> biblionumber is passed and contains ANY existing biblionumber (regardless of
> whether said biblio includes a URI or not)

We are searching for uri => $uri and a biblionumber. Not any URI.
Comment 58 Martin Renvoize 2019-08-16 09:13:10 UTC
Comment on attachment 92225 [details] [review]
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter

Review of attachment 92225 [details] [review]:
-----------------------------------------------------------------

::: opac/tracklinks.pl
@@ +56,4 @@
>  
>      my $record = C4::Biblio::GetMarcBiblio({ biblionumber => $biblionumber });
>      my $marc_urls = C4::Biblio::GetMarcUrls($record, C4::Context->preference('marcflavour'));
> +    my $search_crit = { uri => $uri };

ok.. I missed the above line :(.. This patch should be OK
Comment 59 Martin Renvoize 2019-08-16 09:24:12 UTC
Created attachment 92262 [details] [review]
Bug 23329: Only redirect tracklinks.pl to urls contained in records

Bug 19487 limited redirection to urls contained in a record/item if we were tracking.
We should probably limit forwarding if not tracking as well.
Additionally, if we don't have a soucre, let's not forward

To test:
 0 - Set TrackClicks syspref to 'Don't track'
 1 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 2 - You get forwarded to google
 3 - Set TrackClicks to 'Track anonymously'
 4 - You get a 404
 5 - Apply patch
 6 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com
 7 - You get a 404
 8 - Set TrackClicks syspref to 'Don't track'
 9 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
     Choose a biblionumber that exists
10 - You get a 404
11 - Add http://www.google.com to the 856$u of the record used above
12 - Hit localhost:8080/cgi-bin/koha/tracklinks.pl?uri=http://www.google.com&biblionumber=1
13 - You are redirected
14 - Confirm redirection and 404 as expected with other settings of TrackClicks

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Martin Renvoize 2019-08-16 09:24:16 UTC
Created attachment 92263 [details] [review]
Bug 23329: (follow-up) Use any instead of grep

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Martin Renvoize 2019-08-16 09:24:20 UTC
Created attachment 92264 [details] [review]
Bug 23329: (follow-up) Allow item URI with a biblionumber parameter

If you pass a URI with a biblionumber without specifying the itemnumber,
tracklinks did not redirect an item URI.

Test plan:
[1] Add URI in an item.
[2] Pass this URI with the itemnumber to tracklinks. Should pass.
[3] Pass this URI with the biblionumber to tracklinks. Should pass now too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2019-08-16 09:24:23 UTC
Created attachment 92265 [details] [review]
Bug 23329: (QA follow-up) Resolve warning on wrong biblionumber

Passing a wrong biblionumber generates a warning:
    GetMarcUrls called on undefined record at opac/tracklinks.pl line 58.

Test plan:
[1] Try it again with a wrong biblionumber and check the logs.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2019-08-16 09:24:27 UTC
Created attachment 92266 [details] [review]
Bug 23329: (RM follow-up) Add regression tests

Test plan:

Run the new tests and they should all pass once we've caught all cases

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2019-08-16 09:24:30 UTC
Created attachment 92267 [details] [review]
Bug 23329: Move error page to its own subroutine

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2019-08-16 09:24:34 UTC
Created attachment 92268 [details] [review]
Bug 23329: Fix tests

- www.google.com vs https://www.google.com
- Remove transaction otherwise data are not available from webserver
- Use new C4::Output::output_error to avoid 302 (redirect)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Martin Renvoize 2019-08-16 09:24:38 UTC
Created attachment 92269 [details] [review]
Bug 23329: (RM follow-up) Restore DB after test

Test plan:
1) Dump your DB before the tests is run
2) Run the test
3) Dumper your DB again and compare to the first dump
Success if there are no differences (other than the timestamp of the
dump)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Martin Renvoize 2019-08-16 09:26:12 UTC
I'm very confident in these as a whole now.. lots of eyes, lots of testing..

Passing to Fridolin for pushing to stables :)

Well done all.
Comment 68 Marcel de Rooy 2019-08-16 09:27:29 UTC
(In reply to Martin Renvoize from comment #56)

> The issue is distinct connections.
Okay, I see.

> I'm certainly open to suggestions of a better way though.. I'm not sure
> about this heavy-handed approach either but it's the best I could come up
> with so far.
I am switching to pragmatic mode for now. Go ahead. The discussion has been recorded..
Comment 69 Jonathan Druart 2019-08-19 19:02:31 UTC
(In reply to Martin Renvoize from comment #66)
> Created attachment 92269 [details] [review] [review]
> Bug 23329: (RM follow-up) Restore DB after test
> 
> Test plan:
> 1) Dump your DB before the tests is run
> 2) Run the test
> 3) Dumper your DB again and compare to the first dump
> Success if there are no differences (other than the timestamp of the
> dump)
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

I strongly disagree with this patch.
We already use Test::Mojo and transactions in other test files, we should understand why it does not work here.
Comment 70 Martin Renvoize 2019-08-20 07:14:21 UTC
(In reply to Jonathan Druart from comment #69)
> (In reply to Martin Renvoize from comment #66)
> > Created attachment 92269 [details] [review] [review] [review]
> > Bug 23329: (RM follow-up) Restore DB after test
> > 
> > Test plan:
> > 1) Dump your DB before the tests is run
> > 2) Run the test
> > 3) Dumper your DB again and compare to the first dump
> > Success if there are no differences (other than the timestamp of the
> > dump)
> > 
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> > Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> I strongly disagree with this patch.
> We already use Test::Mojo and transactions in other test files, we should
> understand why it does not work here.

Hmmm, that's fair.. it did feel somewhat nasty.

The difference here regarding use of Test::Mojo and transactions is that for the API we are running the mojolicious app using the Test::Mojo application server and as such the server and client share the db transaction I believe.. I'm not sure we can easily do the same for the CGI side of the app.. perhaps mounting koha as a plack app within Test::Mojo.. I'll have to ponder that..
Comment 71 Jonathan Druart 2019-09-01 14:42:45 UTC
In that case what not use Selenium and the @cleanup trick? I am suggesting to use Selenium for consistency only.
Comment 72 Martin Renvoize 2019-09-12 11:34:20 UTC
(In reply to Jonathan Druart from comment #71)
> In that case what not use Selenium and the @cleanup trick? I am suggesting
> to use Selenium for consistency only.

I felt the tests were simpler to read using Test::Mojo than using Selenium.

I've not tried the mount PSGI option of Test::Mojo.. I don't think it's a goer due to the mechanics of how mojo mounts the psgi app (and how we in term mount CGI within it).. although the test 'works' in so much as we can hit the right endpoints.. we're still not sharing a DB handle and as such rollbacks will not work :(.

I still think this is the cleanest approach.. but will add a followup to revert to using the 'cleanup' routines much like the selenium tests if you feel strongly that is the way to go.. my issue with the cleanup methods is that it's very easy to miss some area's that require said cleanup (as Test::Builder, for example, can and will create all sorts of related data which is hard to track).. cleanup also doesn't restore auto-increments which means it's not as clean as the rollback method.
Comment 73 Marcel de Rooy 2019-09-12 11:47:42 UTC
Although I do not really like the current 'restore' as commented before, I doubt if the cleanup approach is the way to go. We do not return to the previous point, auto increments etc.
Comment 74 Liz Rea 2019-10-16 22:12:58 UTC
Comment on attachment 92266 [details] [review]
Bug 23329: (RM follow-up) Add regression tests

Review of attachment 92266 [details] [review]:
-----------------------------------------------------------------

All of the patches after this one break the 404 for the OPAC.
Comment 75 Martin Renvoize 2019-10-17 06:48:08 UTC
This one has now been announced to the world via https://www.openbugbounty.org/reports/896547/ and needs a resolution asap please... even if we end up pushing without the tests for the stable branches, that would be better than nothing.
Comment 76 Liz Rea 2019-10-17 12:00:21 UTC
My vote is push without the test.
Comment 77 Liz Rea 2019-10-17 12:02:49 UTC
And without the follow-ups.
Comment 78 Liz Rea 2019-10-17 12:04:16 UTC
*rm follow ups sorry it's early for me and thumb typing.
Comment 79 Liz Rea 2019-10-17 17:31:32 UTC
I've got the patches up to https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=92266 applied to 18.05.x and can do this release at any time. They have not been pushed to git.
Comment 80 Jonathan Druart 2019-10-23 15:05:45 UTC
(In reply to Liz Rea from comment #74)
> Comment on attachment 92266 [details] [review] [review]
> Bug 23329: (RM follow-up) Add regression tests
> 
> Review of attachment 92266 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> All of the patches after this one break the 404 for the OPAC.

Works for me, make sure you restarted plack.
Comment 82 Lucas Gass 2019-11-22 19:01:50 UTC
this is applied to the 18.11.x security branch for 18.11.11
Comment 83 Martin Renvoize 2019-11-27 10:33:37 UTC
Nice work!

Pushed to master for 19.11.00
Comment 84 Jonathan Druart 2019-12-02 15:55:42 UTC
The dump and restore approach is bad and will break the tests if ran in parallel (what we are supposed to support). However it seems that jenkins has nodes returning 1 to nproc as they are not failing (that much).
Comment 85 Jonathan Druart 2020-11-10 10:48:57 UTC
(In reply to Jonathan Druart from comment #84)
> The dump and restore approach is bad and will break the tests if ran in
> parallel (what we are supposed to support). However it seems that jenkins
> has nodes returning 1 to nproc as they are not failing (that much).

Under MySQL 8:
kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/www/regressions.t
t/db_dependent/www/regressions.t .. mysqldump: Error: 'Access denied; you need (at least one of) the PROCESS privilege(s) for this operation' when trying to dump tablespaces


Can we now deal with that please?
Comment 86 Marcel de Rooy 2021-07-30 06:21:46 UTC
From David Cook on koha-devel:

I was looking at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23329#c85 where Jonathan points out under MySQL 8 that we’re now getting this error with dumping the database:

“mysqldump: Error: 'Access denied; you need (at least one of) the PROCESS privilege(s) for this operation' when trying to dump tablespaces”

I’ve seen this pop a few times myself on Koha projects, and it looks like it traces back to the security notes listed here: https://dev.mysql.com/doc/relnotes/mysql/5.7/en/news-5-7-31.html

“Incompatible Change: Access to the INFORMATION_SCHEMA.FILES table now requires the PROCESS privilege.”

“This change affects users of the mysqldump command, which accesses tablespace information in the FILES table, and thus now requires the PROCESS privilege as well. Users who do not need to dump tablespace information can work around this requirement by invoking mysqldump with the --no-tablespaces option. (Bug #30350829)”
 
I haven’t looked into it too deeply yet, but I imagine adding the --no-tablespaces option to mysqldump in koha-dump is the way to go.
Comment 87 David Cook 2022-03-10 03:26:07 UTC
For opac/tracklinks.pl, we changed 'print $cgi->redirect("/cgi-bin/koha/errors/404.pl");' to 'output_error( $cgi, '404' );', but that's wrong as that's an intranet style 404 when we need an opac style 404. It'll never work correctly...
Comment 88 David Cook 2022-03-10 03:40:46 UTC
(In reply to David Cook from comment #87)
> For opac/tracklinks.pl, we changed 'print
> $cgi->redirect("/cgi-bin/koha/errors/404.pl");' to 'output_error( $cgi,
> '404' );', but that's wrong as that's an intranet style 404 when we need an
> opac style 404. It'll never work correctly...

See Bug 30261 for a fix for that.