Bug 29544 - A patron can set everybody's checkout notes
Summary: A patron can set everybody's checkout notes
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-20 21:19 UTC by Joonas Kylmälä
Modified: 2022-02-08 00:37 UTC (History)
13 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:
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 (894 bytes, patch)
2021-11-22 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29544: Fix opac-issue-note.pl (1.54 KB, patch)
2021-12-02 08:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29544: Ensure logged in user is allowed to modify checkout note (949 bytes, patch)
2022-01-02 15:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29544: Fix opac-issue-note.pl (1.60 KB, patch)
2022-01-02 15:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29544: Ensure logged in user is allowed to modify checkout note (1004 bytes, patch)
2022-01-12 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29544: Fix opac-issue-note.pl (1.65 KB, patch)
2022-01-12 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[OPTIONAL] Bug 29544: (QA follow-up) Simplify code (989 bytes, patch)
2022-01-12 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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