On subscriptions, operations close and reopen can be launched without authentication : For example in 22.11 : Find an existing open subscription, ie subscriptionid=3. Call : https://<staff interface>/cgi-bin/koha/serials/subscription-detail.pl?subscriptionid=3&op=close See : https://git.koha-community.org/Koha-community/Koha/src/branch/22.11.x/serials/subscription-detail.pl#L42 The operations close and reopen are before get_template_and_user(). In >= 22.11 there is CSRF but maybe still an issue.
We must also move : my $subscription = Koha::Subscriptions->find( $subscriptionid ); I'll create a patch soon
Woof. Good catch.
Created attachment 168504 [details] [review] Bug 37247: Fix subscriptions operation allowed without authentication Move close and reopen after get_template_and_user(). Also move Koha::Subscriptions->find(), not a good idea to run DB queries before authentication. Test plan : 1) Apply patch 2) Authenticate to staff interface 3) Go to an existing open subscription 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed 9) Check you can close and reopen subscription
Created attachment 168513 [details] [review] Bug 37247: Fix subscriptions operation allowed without authentication Move close and reopen after get_template_and_user(). Also move Koha::Subscriptions->find(), not a good idea to run DB queries before authentication. Test plan : 1) Apply patch 2) Authenticate to staff interface 3) Go to an existing open subscription 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed 9) Check you can close and reopen subscription Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 168535 [details] [review] Bug 37247: Fix display of "closed" The subscription was not shown as closed after we closed it. This is because "closed" is not passed to the template. It seems more reliable to rely on the subscription object (that is passed to both serials/serials-collection.tt and serials/subscription-detail.tt, the others are not showing the Reopen/Close buttons) Also fetch the subscription object after and reopen/close it to display accurate values.
Upping severity to get attention.
Created attachment 168769 [details] [review] Bug 37247: Fix display of "closed" The subscription was not shown as closed after we closed it. This is because "closed" is not passed to the template. It seems more reliable to rely on the subscription object (that is passed to both serials/serials-collection.tt and serials/subscription-detail.tt, the others are not showing the Reopen/Close buttons) Also fetch the subscription object after and reopen/close it to display accurate values. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 168782 [details] [review] Bug 37247: Fix subscriptions operation allowed without authentication Move close and reopen after get_template_and_user(). Also move Koha::Subscriptions->find(), not a good idea to run DB queries before authentication. Test plan : 1) Apply patch 2) Authenticate to staff interface 3) Go to an existing open subscription 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed 9) Check you can close and reopen subscription Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168783 [details] [review] Bug 37247: Fix display of "closed" The subscription was not shown as closed after we closed it. This is because "closed" is not passed to the template. It seems more reliable to rely on the subscription object (that is passed to both serials/serials-collection.tt and serials/subscription-detail.tt, the others are not showing the Reopen/Close buttons) Also fetch the subscription object after and reopen/close it to display accurate values. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Code change makes sense, no regressions were found in manual testing, QA scripts happy. Passing QA
Patch doesn't apply cleanly for 23.05.x, could this please be rebased?
Surely this should be pushed to 24.05 first and you cherry pick from there for 23.11 and then 23.05 as and when required. I really don't understand how people are doing security bugs now, there seems to be no process being followed.
> I really don't understand how people are doing security bugs now, there > seems to be no process being followed. Me neither
Created attachment 169282 [details] [review] Bug 37247: [23.11.x] Fix subscriptions operation allowed without authentication Move close and reopen after get_template_and_user(). Also move Koha::Subscriptions->find(), not a good idea to run DB queries before authentication. Test plan : 1) Apply patch 2) Authenticate to staff interface 3) Go to an existing open subscription 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed 9) Check you can close and reopen subscription Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169283 [details] [review] Bug 37247: [23.11.x] Fix display of "closed" The subscription was not shown as closed after we closed it. This is because "closed" is not passed to the template. It seems more reliable to rely on the subscription object (that is passed to both serials/serials-collection.tt and serials/subscription-detail.tt, the others are not showing the Reopen/Close buttons) Also fetch the subscription object after and reopen/close it to display accurate values. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
23.11.x patches a bit different because no 'cud-'.
I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly but I was able to rebase it - however, I still cannot close and reopen the subscription reliably
(In reply to wainuiwitikapark from comment #17) > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly > but I was able to rebase it - however, I still cannot close and reopen the > subscription reliably The first patch works, except I have to choose "Close" and "Reopen" twice for them to work
(In reply to wainuiwitikapark from comment #18) > (In reply to wainuiwitikapark from comment #17) > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly > > but I was able to rebase it - however, I still cannot close and reopen the > > subscription reliably > > The first patch works, except I have to choose "Close" and "Reopen" twice > for them to work The second patch doesn't seem to work, I have tried rebasing it a few different ways
(In reply to wainuiwitikapark from comment #19) > (In reply to wainuiwitikapark from comment #18) > > (In reply to wainuiwitikapark from comment #17) > > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly > > > but I was able to rebase it - however, I still cannot close and reopen the > > > subscription reliably > > > > The first patch works, except I have to choose "Close" and "Reopen" twice > > for them to work > > The second patch doesn't seem to work, I have tried rebasing it a few > different ways I've rebased on 23.05.x and compared with 23.11.x I think I found the cause In 23.05.x the Koha::Subscription object in not sent to template => I create a follo-wup for this branch.
Created attachment 169452 [details] [review] Bug 37247: [23.05.x] Fix subscriptions operation allowed without authentication Move close and reopen after get_template_and_user(). Also move Koha::Subscriptions->find(), not a good idea to run DB queries before authentication. Test plan : 1) Apply patch 2) Authenticate to staff interface 3) Go to an existing open subscription 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed 9) Check you can close and reopen subscription Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169453 [details] [review] Bug 37247: [23.05.x] Fix display of "closed" The subscription was not shown as closed after we closed it. This is because "closed" is not passed to the template. It seems more reliable to rely on the subscription object (that is passed to both serials/serials-collection.tt and serials/subscription-detail.tt, the others are not showing the Reopen/Close buttons) Also fetch the subscription object after and reopen/close it to display accurate values. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 169454 [details] [review] Bug 37247: [23.05.x] Send Koha::Subscription to template
(In reply to Fridolin Somers from comment #20) > (In reply to wainuiwitikapark from comment #19) > > (In reply to wainuiwitikapark from comment #18) > > > (In reply to wainuiwitikapark from comment #17) > > > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly > > > > but I was able to rebase it - however, I still cannot close and reopen the > > > > subscription reliably > > > > > > The first patch works, except I have to choose "Close" and "Reopen" twice > > > for them to work > > > > The second patch doesn't seem to work, I have tried rebasing it a few > > different ways > > I've rebased on 23.05.x and compared with 23.11.x > > I think I found the cause > > In 23.05.x the Koha::Subscription object in not sent to template => I create > a follo-wup for this branch. Thanks so much Fridolin! Unfortunately with these patches, when do the following testing: 4) Open a new browser tab and use it to log-out 5) Go to first tab and click on 'Close' 6) You get login page 7) Authenticate 8) Check subscription is not closed The subscription is closed when I log back in
(In reply to wainuiwitikapark from comment #24) > (In reply to Fridolin Somers from comment #20) > > (In reply to wainuiwitikapark from comment #19) > > > (In reply to wainuiwitikapark from comment #18) > > > > (In reply to wainuiwitikapark from comment #17) > > > > > I tried backporting the 23.11 patches to 23.05 - they don't apply cleanly > > > > > but I was able to rebase it - however, I still cannot close and reopen the > > > > > subscription reliably > > > > > > > > The first patch works, except I have to choose "Close" and "Reopen" twice > > > > for them to work > > > > > > The second patch doesn't seem to work, I have tried rebasing it a few > > > different ways > > > > I've rebased on 23.05.x and compared with 23.11.x > > > > I think I found the cause > > > > In 23.05.x the Koha::Subscription object in not sent to template => I create > > a follow-up for this branch. > > Thanks so much Fridolin! > > Unfortunately with these patches, when do the following testing: > > 4) Open a new browser tab and use it to log-out > 5) Go to first tab and click on 'Close' > 6) You get login page > 7) Authenticate > 8) Check subscription is not closed > > The subscription is closed when I log back in Apologies, Chris helped me understand how to test better. I had to login in the new tab not the original one. Can confirm the 23.05 patches work, thanks Fridolin!
This has been backported to 24.05.x-security branch for 24.05.02
Pushed for 24.11! Well done everyone, thank you!