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 :)
*** Bug 23275 has been marked as a duplicate of this bug. ***
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
(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
(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 }
(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)
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
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)
Created attachment 91573 [details] Dash symbol at line 82
(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
(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?
Using Sandbox, look not good http://sandboxes.ptfs-europe.co.uk/provision_log/bug23329fy
Cannot create sandbox to signoff because Authentication or permission failure
(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
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>
Created attachment 91760 [details] [review] Bug 23329: (follow-up) Use any instead of grep Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Should not we move that code to Koha::LinkTracker and provide tests?
(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.
QA: Looking here
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>
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>
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>
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>
Martin: If you insist ;) I will move the two follow-ups somewhere else
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.
Created attachment 92187 [details] [review] Bug 23329: Add regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 92188 [details] [review] Bug 23329: (RM follow-up) Add regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.
(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!
Created attachment 92198 [details] [review] Revert "Bug 23329: (follow-up) Allow item URI with a biblionumber parameter" This reverts commit abdd0f7639d9e9ef81d48835724db9a8e5eb3e6d.
Created attachment 92199 [details] [review] Bug 23329: Move error page to its own subroutine
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)
My try
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 ;)
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>
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>
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>
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>
Created attachment 92216 [details] [review] Bug 23329: (RM follow-up) Add regression tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 92218 [details] [review] Bug 23329: Move error page to its own subroutine Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 92220 [details] [review] Bug 23329: (RM follow-up) Restore DB after test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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?
(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 ?
(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?
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>
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>
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>
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>
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>
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>
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>
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>
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>
(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.
(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.
(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 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
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>
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>
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>
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>
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>
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>
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>
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>
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.
(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..
(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.
(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..
In that case what not use Selenium and the @cleanup trick? I am suggesting to use Selenium for consistency only.
(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.
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 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.
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.
My vote is push without the test.
And without the follow-ups.
*rm follow ups sorry it's early for me and thumb typing.
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.
(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.
this is applied to the 18.11.x security branch for 18.11.11
Nice work! Pushed to master for 19.11.00
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).
(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?
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.
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...
(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.