Bug 37056 - CSRF error on login when user js contains a fetch of svc/report
Summary: CSRF error on login when user js contains a fetch of svc/report
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 37283
  Show dependency treegraph
 
Reported: 2024-06-07 16:59 UTC by Nick Clemens (kidclamp)
Modified: 2024-07-25 11:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00


Attachments
Bug 37056: Don't generate new CSRF token when fetching a report using the svc API (1.90 KB, patch)
2024-06-07 17:16 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37056: (Non-working POC) Attempt to preserve 403 response (2.56 KB, patch)
2024-06-07 17:16 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37056: Mount a new intranet_svc api to avoid redirects (4.97 KB, patch)
2024-06-10 17:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37056: Add app endpoint for OPAC SVC (2.58 KB, patch)
2024-06-10 17:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37056: Mount a new intranet_svc api to avoid redirects (5.02 KB, patch)
2024-06-10 19:29 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37056: Mount a new intranet_svc api to avoid redirects (4.55 KB, patch)
2024-06-13 17:17 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37056: Mount a new intranet_svc api to avoid redirects (4.61 KB, patch)
2024-07-02 11:01 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-06-07 16:59:39 UTC
It is quite common for libraries to add JS to fetch report results using svc/report on the intranet

Currently, this will break logins, because svc/report uses "get_template_and_user" to verify against "intranet-main.tt"

This generates a new csrf token, and fails the login.

To recreate:
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
Comment 1 Nick Clemens (kidclamp) 2024-06-07 17:16:42 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!
Comment 2 Nick Clemens (kidclamp) 2024-06-07 17:16:44 UTC
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
Comment 3 Nick Clemens (kidclamp) 2024-06-10 17:54:22 UTC
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!
Comment 4 Nick Clemens (kidclamp) 2024-06-10 17:54:24 UTC
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
Comment 5 Brendan Lawlor 2024-06-10 19:29:57 UTC
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 6 David Cook 2024-06-11 01:03:22 UTC
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.
Comment 7 Nick Clemens (kidclamp) 2024-06-13 17:17:27 UTC
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>
Comment 8 Nick Clemens (kidclamp) 2024-07-01 18:11:37 UTC
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');
          });
      }
    });
Comment 9 Martin Renvoize 2024-07-02 11:01:47 UTC
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>
Comment 10 Martin Renvoize 2024-07-02 11:03:02 UTC
Working well for me, code reads OK and there's not QA failures or regressions.

Passing QA
Comment 11 Katrin Fischer 2024-07-02 15:22:22 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 12 Jonathan Druart 2024-07-04 14:02:27 UTC
        #   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.
Comment 13 Jonathan Druart 2024-07-09 11:30:22 UTC
(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.