Bug 33702 - Patrons should only see their own ILLs in the OPAC
Summary: Patrons should only see their own ILLs in the OPAC
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Magnus Enger
QA Contact: Marcel de Rooy
URL:
Keywords: rel_21_05_candidate
Depends on:
Blocks:
 
Reported: 2023-05-09 13:09 UTC by Magnus Enger
Modified: 2023-12-28 20:47 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06,22.05.13,21.11.21


Attachments
Bug 33702; Patrons should only see their own ILLs in the OPAC (2.17 KB, patch)
2023-05-09 16:32 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 33702: Patrons should only see their own ILLs in the OPAC (2.25 KB, patch)
2023-05-10 05:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33702: Prevent ILL requests to be modified by somebody else (3.17 KB, patch)
2023-05-10 05:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33702: Prevent ILL requests to be modified by somebody else (3.35 KB, patch)
2023-05-10 08:29 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 33702: Patrons should only see their own ILLs in the OPAC (2.34 KB, patch)
2023-05-12 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33702: Prevent ILL requests to be modified by somebody else (3.45 KB, patch)
2023-05-12 07:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33702: (QA follow-up) Do not crash on borrowernumber (1.21 KB, patch)
2023-05-12 07:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2023-05-09 13:09:53 UTC
In the OPAC, a user can see their own ILL requests in the OPAC at this URL: 

https://<opac>/cgi-bin/koha/opac-illrequests.pl

Users can then click on "View" to see more details for one request: 

https://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=4714

But by changing the illrequest_id at the end of that URL, the patron can see details of any and all ILL requests, not just their own. There is no standardized information about what user a request is connected to (since the display is designed for users looking at their own requests), but notes are also displayed, which might reveal the identity of the patron, or other sensitive information. 

There should be a check that the currently active patron is connected to the ILL request before details are displayed.
Comment 1 Magnus Enger 2023-05-09 16:32:51 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2023-05-09 16:36:23 UTC
The patch above redirects patrons to the 404 page if they try to view an ILL request that does not belong to them. We could perhaps be nicer and display a more friendly "You are not allowed to see this" message, but I don't think there are any good reasons why anyone would end up at the URL of a request they are not supposed to see, so I think a plain old 404 should do the job.
Comment 3 Jonathan Druart 2023-05-10 05:35:48 UTC
Other actions are affected as well.
Comment 4 Jonathan Druart 2023-05-10 05:40:55 UTC
Created attachment 150923 [details] [review]
Bug 33702: Patrons should only see their own ILLs in the OPAC

To reproduce:
- Enable the ILL module
- Install the FreeForm backend as described here:
  https://wiki.koha-community.org/wiki/ILL_backends
- Go to the ILL module and add two different ILL requests by
  clicking on "New ILL request" and entering the necessary details.
- Make sure you connect the two requests to two *different* patrons
  in the field marked "Card number, username or surname"
- Make the two titles different, and make a not of which title is
  connected to which patron
- Log in as one of the two patrons who now have an ILL request each,
  in the OPAC
- Go to the "Interlibrary loan requests" tab
- Click on "View" for the request connected to this patron. The URL
  will look like something like this:
  http://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=2
- Now change the number at the end to correspond to the the ILL request
  connected to the *other* patron
- Verify you can see the details of an ILL request conncted to another
  patron than the patron you are logged in as

To test:
- Apply the patch
- Restart all the things if you are testing with ktd
- Reload the detail view of the ILL request that belongs to the patron
  you are not logged in as
- Verify you are redirect to the 404 page and can not see the details
  of the request that belongs to the patron you are not logged in as

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2023-05-10 05:40:58 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2023-05-10 05:41:39 UTC
This looks very bad.
Comment 7 Jonathan Druart 2023-05-10 06:03:43 UTC
It affects all stable branches. We should release the last 21.05.x with this fix.
Comment 8 Magnus Enger 2023-05-10 08:29:09 UTC
Created attachment 150939 [details] [review]
Bug 33702: Prevent ILL requests to be modified by somebody else

Same as previous patch, but for 'update' and 'cancreq'.
We remove the redirect, but here we only want to focus on the security
fix.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Confirmed. Without this patch a patron can modify and cancel any ILL
request in the OPAC. With this patch the patron is redirected to the
404 page if modification or cancellation is attempted.
Comment 9 Magnus Enger 2023-05-10 08:34:32 UTC
The code currently in master uses code like this to find and display details of an ILL request: 

    my $request = Koha::Illrequests->find({
        borrowernumber => $loggedinuser,
        illrequest_id  => $params->{illrequest_id}
    });

I wonder if this was intended to only find requests connected to the currently logged in patron, because of the "borrowernumber => $loggedinuser,"? Is "find" supposed to work that way? I guess the real question is if a false assumption was made about how find works, or if find is not working the way it should?
Comment 10 Jonathan Druart 2023-05-10 08:58:31 UTC
It should either be

    my $request = Koha::Illrequests->search({
        borrowernumber => $loggedinuser,
        illrequest_id  => $params->{illrequest_id}
    })->next;

Or what I suggested in the previous patch.

Find is searching on a unique index or PK, here we pass the PK (illrequest_id) and so the other parameters are ignored.
Comment 11 Marcel de Rooy 2023-05-12 06:24:22 UTC
Looking here
Comment 12 Marcel de Rooy 2023-05-12 07:03:59 UTC
Created attachment 151111 [details] [review]
Bug 33702: Patrons should only see their own ILLs in the OPAC

To reproduce:
- Enable the ILL module
- Install the FreeForm backend as described here:
  https://wiki.koha-community.org/wiki/ILL_backends
- Go to the ILL module and add two different ILL requests by
  clicking on "New ILL request" and entering the necessary details.
- Make sure you connect the two requests to two *different* patrons
  in the field marked "Card number, username or surname"
- Make the two titles different, and make a not of which title is
  connected to which patron
- Log in as one of the two patrons who now have an ILL request each,
  in the OPAC
- Go to the "Interlibrary loan requests" tab
- Click on "View" for the request connected to this patron. The URL
  will look like something like this:
  http://<opac>/cgi-bin/koha/opac-illrequests.pl?method=view&illrequest_id=2
- Now change the number at the end to correspond to the the ILL request
  connected to the *other* patron
- Verify you can see the details of an ILL request conncted to another
  patron than the patron you are logged in as

To test:
- Apply the patch
- Restart all the things if you are testing with ktd
- Reload the detail view of the ILL request that belongs to the patron
  you are not logged in as
- Verify you are redirect to the 404 page and can not see the details
  of the request that belongs to the patron you are not logged in as

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-05-12 07:04:02 UTC
Created attachment 151112 [details] [review]
Bug 33702: Prevent ILL requests to be modified by somebody else

Same as previous patch, but for 'update' and 'cancreq'.
We remove the redirect, but here we only want to focus on the security
fix.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Confirmed. Without this patch a patron can modify and cancel any ILL
request in the OPAC. With this patch the patron is redirected to the
404 page if modification or cancellation is attempted.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-05-12 07:04:05 UTC
Created attachment 151113 [details] [review]
Bug 33702: (QA follow-up) Do not crash on borrowernumber

Resolve:
Can't call method "borrowernumber" on an undefined value at /usr/share/koha/opac/opac-illrequests.pl line 66

Test plan:
Put an unexisting illrequest_id in the URL parameter.
You should see a 404, not a crash.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Jonathan Druart 2023-05-30 12:54:05 UTC
What happened here? It's part of 22.11.x only now? RMaints?
Comment 16 Lucas Gass 2023-06-01 16:30:31 UTC
Backported to 22.05.x for 22.05.13
Comment 17 Mason James 2023-06-02 01:21:10 UTC
(In reply to Lucas Gass from comment #16)
> Backported to 22.05.x for 22.05.13


hi team, all 3 following packages include the 3x 33702 patches

 22.11.06
 22.05.13
 21.11.21
Comment 18 Jonathan Druart 2023-06-02 05:32:27 UTC
Still missing in 21.05 then?