Bug 29544

Summary: A patron can set everybody's checkout notes
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: OPACAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to oldoldoldstable --- QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: major    
Priority: P5 - low CC: 1joynelson, andrewfh, fridolin.somers, jonathan.druart, katrin.fischer, lucas, m.de.rooy, martin.renvoize, nick, nugged, tomascohen, victor, wainuiwitikapark
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25
Attachments: Bug 29544: Ensure logged in user is allowed to modify checkout note
Bug 29544: Fix opac-issue-note.pl
Bug 29544: Ensure logged in user is allowed to modify checkout note
Bug 29544: Fix opac-issue-note.pl
Bug 29544: Ensure logged in user is allowed to modify checkout note
Bug 29544: Fix opac-issue-note.pl
[OPTIONAL] Bug 29544: (QA follow-up) Simplify code

Description Joonas Kylmälä 2021-11-20 21:19:35 UTC
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"
Comment 1 Jonathan Druart 2021-11-22 13:57:51 UTC
Created attachment 127930 [details] [review]
Bug 29544: Ensure logged in user is allowed to modify checkout note
Comment 2 Jonathan Druart 2021-11-22 13:58:27 UTC
This patch is not great and keep the script weak. Anything else to suggest?
Comment 3 Fridolin Somers 2021-12-02 00:48:39 UTC
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
Comment 4 Jonathan Druart 2021-12-02 07:59:27 UTC
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.
Comment 5 Jonathan Druart 2021-12-02 08:09:10 UTC
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
Comment 6 Fridolin Somers 2021-12-22 06:28:00 UTC
+if ( $action eq 'issuenote' && C4::Context->preference('AllowCheckoutNotes') && $issue ) {

Do we need this ?
There is an early exit "if !$issue"
Comment 7 Katrin Fischer 2022-01-02 15:48:39 UTC
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>
Comment 8 Katrin Fischer 2022-01-02 15:48:43 UTC
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>
Comment 9 Katrin Fischer 2022-01-02 15:49:07 UTC
(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?
Comment 10 Fridolin Somers 2022-01-03 08:35:53 UTC
(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 ;)
Comment 11 Tomás Cohen Arazi 2022-01-12 12:45:49 UTC
Some notes:
- Works as expected
- Shouldn't we actually remove the 'hidden' feature completely?
Comment 12 Tomás Cohen Arazi 2022-01-12 12:47:09 UTC
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>
Comment 13 Tomás Cohen Arazi 2022-01-12 12:47:13 UTC
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>
Comment 14 Tomás Cohen Arazi 2022-01-12 12:47:18 UTC
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>
Comment 15 Fridolin Somers 2022-01-15 04:10:52 UTC
(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.
Comment 18 Victor Grousset/tuxayo 2022-01-24 20:18:06 UTC
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 :)
Comment 19 wainuiwitikapark 2022-01-26 04:51:20 UTC
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 20 Andrew Fuerste-Henry 2022-01-26 20:28:44 UTC
Pushed to security/21.05.x for 21.05.09