Description
Nick Clemens (kidclamp)
2024-06-07 16:59:39 UTC
Created attachment 167580 [details] [review] Bug 37056: Don't generate new CSRF token when fetching a report using the svc API To test: 1 - Create a report 2 - Add to IntranetUserJs: $(document).ready(function() { // Your report ID var reportId = '492'; // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); }); 3 - Log out 4 - Attempt to login 5 - KO 6 - Apply patch 7 - Restart all 8 - Repeat login 9 - Success! Created attachment 167581 [details] [review] Bug 37056: (Non-working POC) Attempt to preserve 403 response The former patch will prevent the issue, however, it makes svc/report return 200 when not authorized. I thought maybe the issue was that we were generating a new csrf token on the 403 page/template and tried to prevent that on this patch, however, it doesn't work. Looking for some help on this one Created attachment 167610 [details] [review] Bug 37056: Mount a new intranet_svc api to avoid redirects When an unauthroized call to svc is made, we use the ErrorDocument middleware to respond with an HTML page. The API doens't do this, it simply returns its status. We should mount the svc as it own app to avoid the redirect to HTML for unauthroized responses To test: 1 - Create a report 2 - Add to IntranetUserJs: $(document).ready(function() { // Your report ID var reportId = '492'; // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); }); 3 - Log out 4 - Attempt to login 5 - KO 6 - Apply patch 7 - Reset all (or copy the necessary changes to your plack/apache files) 8 - Generate report and update user js again 8 - Logout, login 9 - Success! Created attachment 167611 [details] [review] Bug 37056: Add app endpoint for OPAC SVC I think this needs to happen on another bug, there are several scripts here that use templates for auth as well as shelfbrowser.pl which does return rendered html - so maybe shouldn't be treated this way Created attachment 167617 [details] [review] Bug 37056: Mount a new intranet_svc api to avoid redirects When an unauthroized call to svc is made, we use the ErrorDocument middleware to respond with an HTML page. The API doens't do this, it simply returns its status. We should mount the svc as it own app to avoid the redirect to HTML for unauthroized responses To test: 1 - Create a report 2 - Add to IntranetUserJs: $(document).ready(function() { // Your report ID var reportId = '492'; // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); }); 3 - Log out 4 - Attempt to login 5 - KO 6 - Apply patch 7 - Reset all (or copy the necessary changes to your plack/apache files) 8 - Generate report and update user js again 8 - Logout, login 9 - Success! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Comment on attachment 167617 [details] [review] Bug 37056: Mount a new intranet_svc api to avoid redirects Review of attachment 167617 [details] [review]: ----------------------------------------------------------------- I think overall this is probably the way to go in order to get proper API responses. Going to Fail QA for now because of an unnecessary change and an incorrect path for the prod app root. ::: debian/templates/plack.psgi @@ +56,5 @@ > root => $ENV{DEV_INSTALL}? $home: "$home/intranet/cgi-bin" > )->to_app; > > +my $intranet_svc = Plack::App::CGIBin->new( > + root => $ENV{DEV_INSTALL}? "$home/svc": "$home/svc/cgi-bin" That prod path looks wrong. I think it would need to be $home/intranet/cgi-bin/svc. @@ +121,4 @@ > enable "+Koha::Middleware::CSRF"; > $intranet; > }; > + mount '/intranet_svc' => builder { I'm a bit concerned about the impact of mounting another PSGI application, although I don't have any numbers to support that concern at the moment. That said, I've proposed the same thing in bug 31380 and friends. There could be merit to this as well, since it is an API so we wouldn't want the normal HTML exception handling... ::: errors/403.pl @@ +33,4 @@ > authnotrequired => 1, > } > ); > +$template->param( This change looks unnecessary. Created attachment 167692 [details] [review] Bug 37056: Mount a new intranet_svc api to avoid redirects When an unauthroized call to svc is made, we use the ErrorDocument middleware to respond with an HTML page. The API doens't do this, it simply returns its status. We should mount the svc as it own app to avoid the redirect to HTML for unauthroized responses To test: 1 - Create a report 2 - Add to IntranetUserJs: $(document).ready(function() { // Your report ID var reportId = '492'; // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); }); 3 - Log out 4 - Attempt to login 5 - KO 6 - Apply patch 7 - Reset all (or copy the necessary changes to your plack/apache files) 8 - Generate report and update user js again 8 - Logout, login 9 - Success! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> It seems you can workaround by preventing the JS from firing at login: $(document).ready(function() { // Your report ID var reportId = '1'; if( $('body #main_auth').length > 0 ){ // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); } }); Created attachment 168342 [details] [review] Bug 37056: Mount a new intranet_svc api to avoid redirects When an unauthroized call to svc is made, we use the ErrorDocument middleware to respond with an HTML page. The API doens't do this, it simply returns its status. We should mount the svc as it own app to avoid the redirect to HTML for unauthroized responses To test: 1 - Create a report 2 - Add to IntranetUserJs: $(document).ready(function() { // Your report ID var reportId = '492'; // Fetch the report $.get('/cgi-bin/koha/svc/report?id=' + reportId, function(data) { console.log('Kaboom'); }); }); 3 - Log out 4 - Attempt to login 5 - KO 6 - Apply patch 7 - Reset all (or copy the necessary changes to your plack/apache files) 8 - Generate report and update user js again 8 - Logout, login 9 - Success! Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Working well for me, code reads OK and there's not QA failures or regressions. Passing QA Pushed for 24.11! Well done everyone, thank you! # Failed test 'Patron cannot access svc script where circulate permissions are required' # at t/db_dependent/selenium/authentication.t line 135. # 'An Error Occurred' # doesn't match '(?^u:Error 500)' # Looks like you failed 1 test of 17. (In reply to Jonathan Druart from comment #12) > # Failed test 'Patron cannot access svc script where circulate > permissions are required' > # at t/db_dependent/selenium/authentication.t line 135. > # 'An Error Occurred' > # doesn't match '(?^u:Error 500)' > # Looks like you failed 1 test of 17. See bug 37283. |