Any patron can set any checkout's circulation note by just going to the URL localhost:8080/cgi-bin/koha/opac-issue-note.pl?issue_id=XXXX It is quite easy to loop through all the issue ids. Gladly this is not exposing any circulation info but could be used for spamming other patrons (the checkout notes are shown on OPAC), or depending on how the circulation notes are used in the library maybe something more harmful could be done, as the notes are processed by the staff by going to the page http://localhost:8081/cgi-bin/koha/circ/checkout-notes.pl But seems quite minor and whatever harm was caused by this bug could be probably dealt with, so putting this as severity "normal"
Created attachment 127930 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note
This patch is not great and keep the script weak. Anything else to suggest?
Could we compare logged-in borrowernumber with issue's one ? Like in opac/svc/checkout_notes : https://git.koha-community.org/Koha-community/Koha/src/commit/5b9902487b76d0adcd2e40442165171b5942af46/opac/svc/checkout_notes#L73
My patch actually modifies opac/svc/checkout_notes, that was impacted as well, even with the test on borrowernumber. opac/opac-issue-note.pl is used when JS is disabled. Code is duplicated and buggy, we need to provide a simple fix for backporting but a refactoring should be considered.
Created attachment 128148 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts
+if ( $action eq 'issuenote' && C4::Context->preference('AllowCheckoutNotes') && $issue ) { Do we need this ? There is an early exit "if !$issue"
Created attachment 128946 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 128947 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Fridolin Somers from comment #6) > +if ( $action eq 'issuenote' && > C4::Context->preference('AllowCheckoutNotes') && $issue ) { > > Do we need this ? > There is an early exit "if !$issue" I see what you mean, but on the other side it doesn't hurt, right?
(In reply to Katrin Fischer from comment #9) > (In reply to Fridolin Somers from comment #6) > > +if ( $action eq 'issuenote' && > > C4::Context->preference('AllowCheckoutNotes') && $issue ) { > > > > Do we need this ? > > There is an early exit "if !$issue" > > I see what you mean, but on the other side it doesn't hurt, right? It does not ;)
Some notes: - Works as expected - Shouldn't we actually remove the 'hidden' feature completely?
Created attachment 129336 [details] [review] Bug 29544: Ensure logged in user is allowed to modify checkout note Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129337 [details] [review] Bug 29544: Fix opac-issue-note.pl We must check if logged in user is trying to modify one of their checkouts Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129338 [details] [review] [OPTIONAL] Bug 29544: (QA follow-up) Simplify code I think this is a better approach for the same thing. Posting it just in case it helps. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #14) > Created attachment 129338 [details] [review] [review] > [OPTIONAL] Bug 29544: (QA follow-up) Simplify code > > I think this is a better approach for the same thing. Posting it just in > case it helps. > > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Ha nice, i like this new approach.
Backported: Pushed to 20.11.x-security branch for 20.11.14 Even with the optional patch, it's applies perfectly without conflicts on 20.11.x :)
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Pushed to security/21.05.x for 21.05.09