This patch adds a new xt test (xt/use-test-no-warnings.t) to make sure
all our Perl test files will use Test::NoWarnings.
It also adds Test::NoWarnings to some files that were missing it and
that didn't need any other changes.
However we still have 7 non-trivial tests that we will consider
exception for now, until they are fixed.
Test plan:
prove xt/use-test-no-warnings.t
should return green.
And all other tests modified by this patch (the qa script will do that
for you)
You can also test the 2 selenium tests:
1. DROP and CREATE the DB
DROP DATABASE koha_kohadev;
CREATE DATABASE koha_kohadev;
restart_all
2. KOHA_TESTING=1 prove t/db_dependent/selenium/00-onboarding.t
3. Repeat 1
2. KOHA_TESTING=1 prove t/db_dependent/selenium/01-installation.t
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 63eb2e1f4a)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This database update fixes existing RECEIPT and ACCOUNT_CREDIT notice
templates that may still use the old 'collected' variable instead of
'tendered'. It handles various formatting patterns and provides
verification that the conversion was successful.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 4b300b1c1d)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
"One Ring to rule them all, one Ring to find them, One Ring to bring
them all and in the darkness bind them"
We've been battling against this for years, with a mixed use of
terminology and thus confusion in templates vs controllers and notice
templates. I've decided to just try and make sure all the terminology
matches throughout, and use the more popular 'tendered' term over the
'collected' term.
This patch standardizes the terminology used throughout the payment
system to consistently use 'tendered' instead of the mixed usage of
'collected' and 'tendered' that has caused confusion over the years.
Changes include:
- Update all payment form templates to use 'tendered' input names
- Update JavaScript validation and change calculation logic
- Update controller parameter handling in pos/pay.pl, pos/printreceipt.pl,
and members/paycollect.pl
- Update sample notice templates (RECEIPT) to use 'tendered' variable
- Add null check for $total_tendered to prevent perl warnings
Test plan:
1. Test Point of Sale payments with tendered amounts and change calculation
2. Test member account payments with tendered amounts
3. Test receipt generation (both print and email)
4. Verify JavaScript validation works correctly
5. Confirm no perl warnings when tendered amount is undefined
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit f91310306c)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit d801d4b16a)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This patch updates the /auth/password/validation endpoint to use the new
api_validate_password permission instead of the generic borrowers
permission.
This provides more granular control over who can use the password
validation API endpoint.
To test:
1. Apply this patches
2. Rebuild the API spec
$ ktd --shell
k$ yarn api:bundle
3. Run:
$ ktd --shell
k$ prove t/db_dependent/api/v1/password_validation.t
=> SUCCESS: Tests pass!
4. Sign off :-D
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 812b38d02c)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This change adds a `api_validate_password` permission which allows a user to only
validate borrowers by using the /api/v1/auth/password/validation endpoint.
This avoids scenarios where you want third-parties to authenticate a user without
giving them full permissions to perform CRUD operations on user data.
To test:
1. Apply patch
2. Run "koha-upgrade-schema kohadev"
3. koha-plack --reload kohadev
4. prove -v t/db_dependent/api/v1/password_validation.t
5. Visit http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=51
6. Note that a new subpermission `api_validate_password` appears under the "borrowers" permission
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 79c1853b2b)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 02aa7d08f2)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit d6883d9e3c)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
To test:
1. Apply patch, restart_all, and regenerate CSS ( https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_interface ) I also recommend clearing your browser cache.
2. Do a catalog search and notice the 'Show local items only' button above the "Location" column.
3. Click it to show information ( in that column ) that is only related to the branch you are currently logged in at.
4. In the same result set have some items that are checked out, withdrawn, lost, damaged, and notforlaon.
5. Play more with the search results and ensure everything is accurate.
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 580484e2da)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
No idea why it was not NULL in DB when I did my first SELECT
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit fe14f8b7c6)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
When reviewing the different sysprefs that had "NULL" as default, I
only found those 2 occurrences that could be problematic. I think we
should default to 0 here.
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit ca6cbc3cf9)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Some sysprefs have NULL as their default value, and systempreferences.value allows NULL values.
However C4::Context->set_preference does not unset a syspref if "undef" is passed.
Additionally some of the sysprefs that have NULL as default will generate warnings in the logs (and test suite output).
opac/opac-suggestions.pl: for my $f ( split( /\s*\,\s*/, C4::Context->preference("OPACSuggestionUnwantedFields") ) ) {
Here is the current list:
+-------------------------------+-------+
| variable | value |
+-------------------------------+-------+
| decreaseLoanHighHoldsDuration | NULL |
| decreaseLoanHighHoldsValue | NULL |
| ILLHiddenRequestStatuses | NULL |
| ILLOpacbackends | NULL |
| ILLSendStaffNotices | NULL |
| ListOwnerDesignated | NULL |
| MarcFieldDocURL | NULL |
| MaxFine | NULL |
| OPACSuggestionUnwantedFields | NULL |
| ReportsExportLimit | NULL |
| SerialsSearchResultsLimit | NULL |
+-------------------------------+-------+
Test plan:
Code review required here, please confirm the above and that the
different occurrences do not use "undef" as a value.
Note that some tests now do not longer generate warnings
(eg. t/db_dependent/selenium/opac_ill_requests.t)
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit a3b4b5cdd0)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 9fefdd5674)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
- Update the dbrev to use say_success, and make dbrev and installer
consistent
- Change error div in moredetail.tt from info to warning
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 3096833130)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
To test:
1. APPLY PATCH, updatedatabase, and restart_all
2. Search for the system preference, PreventWithDrawingItemsStatus
3. Select both in transit and checked out, then save
4. Find an item and put it in transit.
5. Now from the item detail page, go to manage items > edit item
6. Using the item that was put in transit, attempt to change the status to withdrawn.
7. You should see an error at the top of the page 'In transit item cannot be withdrawn.' Make sure the item did not get marked as withdrawn.
8. Using a different item, do a checkout
9. Now from the item detail page, go to manage items > edit item
10. Using the checked out item, attempt to change the status to withdrawn.
11. vYou should see an error at the top of the page 'Onloan item cannot be withdrawn.' Make sure the item did not get marked as withdrawn.
12. Now, using the in transit item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=x )
13. Under Statuses, attempt to withdraw the item.
14. You should see an error near the top of the page "Cannot withdraw item in transit".
15. Now, using the checked out item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=144 )
16. Under Statuses, attempt to withdraw the item.
17. You should see an error near the top of the page "Cannot withdraw checked out item.".
18. Using both the in-transit and checked out item, attempt to modify the status through batchMod.
19. Ensure that the items are not being updated to withdrawn.
20. Turn off PreventWithDrawingItemsStatus system preferece.
21. Going through each of the steps again, except this time your items should successfully withdraw.
prove t/db_dependent/Koha/Item.t
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 4e920be91f)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This patch addresses CSV formula injection vulnerabilities by ensuring
all Text::CSV instances use formula protection.
Changes made:
* Add formula => 'empty' to all Text::CSV constructors in:
- Koha::Patrons::Import
- Koha::BackgroundJob::ImportKBARTFile
- Koha::ERM::EUsage::CounterFile
The 'empty' formula mode causes potentially dangerous formulas (starting
with =, +, -, @) to be treated as empty strings rather than executed,
preventing CSV injection attacks while maintaining data integrity.
Security impact:
* Prevents execution of malicious formulas in CSV imports
* Protects against data exfiltration via CSV formula injection
* Maintains backward compatibility for legitimate CSV data
To test:
1. Apply the previous patch (author test)
2. Run:
$ ktd --shell
k$ prove xt/author/Text_CSV_Various.t
=> FAIL: Should show 3 files without formula protection
3. Apply this patch
4. Repeat 2
=> SUCCESS: All tests should now pass
5. Try importing CSV with formula content (=cmd|"/c calc",test,data)
=> SUCCESS: Formula content is rejected/emptied, not executed
6. Import normal CSV data
=> SUCCESS: Normal data imports correctly
7. Sign off :-D
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This test ensures all Text::CSV usage in Koha includes proper formula
injection protection to prevent CSV formula injection attacks.
The test scans all Perl modules and verifies that Text::CSV constructors
include the 'formula' parameter set to a safe mode ('empty', 'die',
'croak', 'diag', or numeric 1-5).
To test:
1. Apply this patch
2. Run:
$ ktd --shell
k$ prove xt/author/Text_CSV_Various.t
=> FAIL: Test should fail showing files without formula protection:
- Koha/Patrons/Import.pm
- Koha/BackgroundJob/ImportKBARTFile.pm
- Koha/ERM/EUsage/CounterFile.pm
3. Apply the follow-up patch to fix the violations
4. Repeat 2
=> SUCCESS: All tests should pass
5. Sign off :-D
Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This patch fixes a XSS vulnerability in the
suspend hold modal in the staff interface.
By building our Javascript using Web APIs rather
than with strings and jQuery's html() method,
we're able to securely handle malicious payloads.
Test plan:
0. Apply the patch
1. Go to http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=29
2. Add a hold for "koha" patron
3. Go to http://localhost:8081/cgi-bin/koha/circ/circulation.pl?borrowernumber=51
4. Click "Holds" tab and click "Suspend"
5. Note the modal loads properly
6. Go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51
7. Click "Holds" tab and click "Suspend"
8. Note the modal loads properly
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
See also comment33, point 4.
Test plan:
cp debian/templates/apache-shared-opac-antibot.conf /etc/koha/
Test enabling/disabling by toggling the comment of the SetEnvIf line,
restart apache and remove the KOHA_INIT and CGISESSID cookie.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
This patch adds a fast simple bot challenge via Apache.
To test:
0. Apply the patch
1. sudo cp debian/templates/apache-shared-opac-antibot.conf /etc/koha/.
2. sudo cp debian/templates/apache-shared-opac-plack.conf /etc/koha/.
3. curl http://localhost:8080/cgi-bin/koha/opac-search.pl
4. Note an instant response with a very simple HTML response
5. Open an incognito browser and go to http://localhost:8080/cgi-bin/koha/opac-search.pl
6. Note a very brief loading animation before the OPAC search page appearing
7. Do a search
8. Note no more loading animation appears
9. Close the incognito browser
10. Open an incognito browser and go to http://localhost:8080/cgi-bin/koha/opac-main.pl
11. Do a search via the "Library catalog" search bar
12. Note no loading animation appears
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Cornelius Hertfelder <cornelius.hertfelder@bsz-bw.de>
Signed-off-by: Magnus Enger <magnus@libriotech.no>
We have tested this in production on at least three live sites for
several weeks, and it has worked as expected. We have not noticed any
negative side effects.
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
To Test:
Before applying:
1. Turn on the UseTransportCostMatrix system preference
2. Create holds on a record with multiple items. (I used Lanark in KTD)
3. Set the item location to all be the same (I used MID)
4. Set the holds to pickup at Centerville, Fairfield, and Pleasant Valley
5. Set the transport cost matrix to
Centerville\fairfield 1
Centerville\Midway 1
Fairfield\Centerville 1
Fairfield\Midway 1
Midway\Centerville 1
Midway\Fairfield 1
Midway\Pleasant Valley 4
6. Build the holds queue (either turn on realtime hold queue and then edit the bib to update or run the cron on the command line)
7. Run a report: select * from hold_fill_targets where biblionumber=9 order by borrowernumber;
8. Rebuild the hold queue
9. Run the report again, the results should be the same.
10. Apply the patch
11. Run the hold queue again
12. Observe the results should now be different.
13. Try a few more times and ensure the numbers are changing, create some more items at midway and try again to see more variety in numbers.
Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 5832521086)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
To test:
0 - APPLY PATCH
1 - Place 50 holds on a bib record
https://github.com/kidclamp/handy-koha-script/blob/main/randhold.pl
2 - View the holds page for the record
3 - Sort the holds alphabetically
4 - Change the dropdown priortiy for the alphabetically first hold to 1
5 - All the priorities should now ajust accordingly but they will NOT be resorted on the fly
6 - Play with adjusting some priorities, making sure everything correctly adjusts.
7 - Click 'Update holds' to make sure the priorities adjust right
8 - Make sure you test with the 'del' option and the adjustments still work.
Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
(cherry picked from commit 4b38f5acd5)
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Jonathan Druart
Paul Derscheid
Martin Renvoize
Lucas Gass
David Cook
Emily Lamancusa
Tomas Cohen Arazi
Marcel de Rooy
Lisette Scheer