On top of bug 35560 we need to pass the CSRF token to the ajax/svc scripts.
Created attachment 162108 [details] [review] Bug 36084: C4::Auth+plack.psgi for svc? Suggestion to move the CSRF check to CGI->new so that we will check it for every request, and it will cover svc scripts as well (they are not using get_template_and_user). The token will be retrieve from the param list *or the csrf_token header* (do we want to name it x-koha-csrf-token instead?). This will be done for *every* request that are not GET: CSRF token is now required everywhere CGI is used (side-effects possible?).
Created attachment 162109 [details] [review] Bug 36084: svc - article_request - POC This is a proof of concept
Created attachment 162110 [details] [review] Bug 36084: Bring fetch for everywhere We are retrieving the awesome fetch modules from Vue, so that it can be used in other areas. Here we will use it to inject the CSRF token to the header of every POST request.
Created attachment 162111 [details] [review] Bug 36084: Add a Dialog class To display potential errors.
Created attachment 162112 [details] [review] Bug 36084: Add a global #messages div
We can do it this way, with a strong base. Or you are doing it the naive way and add a new csrf_token param to the body and check it in every svc scripts (without me). In discussion, waiting for feedback.
+ } elsif ( $request_method ne 'GET' ) { + # Get the CSRF token from the param list or the header + my $csrf_token = $q->param('csrf_token') || $q->http('HTTP_CSRF_TOKEN'); + Doesnt this include REST API already via plack.psgi ?
(In reply to Jonathan Druart from comment #6) > We can do it this way, with a strong base. > > Or you are doing it the naive way and add a new csrf_token param to the body > and check it in every svc scripts (without me). > > In discussion, waiting for feedback. All for a smarter central approach.
(In reply to Marcel de Rooy from comment #7) > + } elsif ( $request_method ne 'GET' ) { > + # Get the CSRF token from the param list or the header > + my $csrf_token = $q->param('csrf_token') || > $q->http('HTTP_CSRF_TOKEN'); > + > > Doesnt this include REST API already via plack.psgi ? No, the REST API does not use CGI.
For discussion: note that we have a breach in plack.psgi. We do not die if the the token is not valid, we only undef $op. I think it is enough (at least for now), but it does not cover us from future bad usage (from devs). We don't really want to explode as we want to propagate the error to the end-user.
Created attachment 162133 [details] [review] Bug 36084: Do not allow absence of token Well, this test was silly, I was focussed on propagating an error to the UI, but we really need to explode in this case. Note that this requires more work as login is now broken.
Created attachment 162147 [details] [review] Bug 36084: Add cud-login to the login form TODO This needs to be covered by tests.
If this does not apply, please use the branch bug_34478_svc from the security repo.
(In reply to Jonathan Druart from comment #12) > Created attachment 162147 [details] [review] [review] > Bug 36084: Add cud-login to the login form > > TODO This needs to be covered by tests. This feels hacky, but I didn't find something better. Also not sure about the side-effects. Please test and read carefully! I've added this patch here as it fixes a regression from the changes in plack.psgi.
Created attachment 162150 [details] [review] Bug 36084: svc - authorised_values - APIClient now global APIClient is not a global variable, which will make the next changes much easier!
(In reply to Jonathan Druart from comment #15) > Created attachment 162150 [details] [review] [review] > Bug 36084: svc - authorised_values - APIClient now global > > APIClient is not a global variable, which will make the next changes > much easier! FIXME here. We need to lazy import (in api-client.js) and use Asset for the import in doc-head-close (but keep the type="module"!)
(In reply to Jonathan Druart from comment #12) > Created attachment 162147 [details] [review] [review] > Bug 36084: Add cud-login to the login form > > TODO This needs to be covered by tests. Got your message on IRC. Shouldn't we be doing this separately to the svc/ work? I'd have to re-test, but last time I tried a patch like this the CSRF test was using the logged in user session against the CSRF token generated for the anonymous session. If things haven't changed since then, I don't know that this patch will work. But maybe bug 36092 fixes that...
If we're going to make changes at the plack.psgi level, why not just do a Koha::Middleware? Perhaps we should think about making it work similar to C4::Context->set_remote_address and Koha::Middleware::RealIP. For Plack scripts, the middleware comes into effect. If it's non-Plack (like the non-proxied scripts like export.pl), then it uses C4::Context->set_remote_address, which is invoked by C4::Auth at compile time (ie in the BEGIN block).
Comment on attachment 162108 [details] [review] Bug 36084: C4::Auth+plack.psgi for svc? Review of attachment 162108 [details] [review]: ----------------------------------------------------------------- ::: debian/templates/plack.psgi @@ +55,3 @@ > $q->param( 'op', '' ); > $q->param( 'debug_programming_error', "'$original_op' must not start with 'cud-' for GET" ); > + } elsif ( $request_method ne 'GET' ) { I don't think this condition makes sense, because it means we require CSRF tokens for HEAD requests. (Also for OPTIONS and TRACE although those two are less common request methods.) I know it's annoying, but I think we'd have to test specifically for POST, PUT, DELETE, PATCH. That said, we can just do $stateful_methods{ $request_method } to still make the elsif() pretty clean.
Bug 36094 is an interesting one. I don't think that we can get rid of "svc/authentication" yet, but it does expose a challenging problem. How do we handle CSRF and an API authentication endpoint? DSpace appears to expose "/api/authn/status" and "/api/authn/login" endpoints. So you'd first GET "/api/authn/status" (which includes a CSRF token in the response headers), and then you post to "/api/authn/login" using the CSRF token. If we want to continue supporting external use of "svc/authentication" then we probably want to add "svc/auth/status" or "svc/authn/status" and use a similar model. -- I suppose we could just say that we don't support external usage of the SVC API anymore...
misc/bin/connexion_import_daemon.pl will be broken if we require CSRF for SVC API. It should probably be use "svc/authentication"
(In reply to David Cook from comment #17) > (In reply to Jonathan Druart from comment #12) > > Created attachment 162147 [details] [review] [review] [review] > > Bug 36084: Add cud-login to the login form > > > > TODO This needs to be covered by tests. > > Got your message on IRC. Shouldn't we be doing this separately to the svc/ > work? comment 14: "I've added this patch here as it fixes a regression from the changes in plack.psgi." It could be moved somewhere else, but to be honest it's already a lot of bug to deal with and keep up-to-date... > I'd have to re-test, but last time I tried a patch like this the CSRF test > was using the logged in user session against the CSRF token generated for > the anonymous session. If things haven't changed since then, I don't know > that this patch will work. Please define "does not work". (In reply to David Cook from comment #18) > If we're going to make changes at the plack.psgi level, why not just do a > Koha::Middleware? Changes to plack.psgi are already made from bug 34478. Yes, why not? Why not refactor C4::Auth at the same time? :) > Perhaps we should think about making it work similar to > C4::Context->set_remote_address and Koha::Middleware::RealIP. > > For Plack scripts, the middleware comes into effect. If it's non-Plack (like > the non-proxied scripts like export.pl), then it uses > C4::Context->set_remote_address, which is invoked by C4::Auth at compile > time (ie in the BEGIN block). More changes, more questions. KISS for now please (it's already complex enough). (In reply to David Cook from comment #19) > Comment on attachment 162108 [details] [review] [review] > Bug 36084: C4::Auth+plack.psgi for svc? > > Review of attachment 162108 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: debian/templates/plack.psgi > @@ +55,3 @@ > > $q->param( 'op', '' ); > > $q->param( 'debug_programming_error', "'$original_op' must not start with 'cud-' for GET" ); > > + } elsif ( $request_method ne 'GET' ) { > > I don't think this condition makes sense, because it means we require CSRF > tokens for HEAD requests. (Also for OPTIONS and TRACE although those two are > less common request methods.) > > I know it's annoying, but I think we'd have to test specifically for POST, > PUT, DELETE, PATCH. Are we using HEAD, OPTIONS or TRACE already? > That said, we can just do $stateful_methods{ $request_method } to still make > the elsif() pretty clean. Please provide a patch. (In reply to David Cook from comment #20) > Bug 36094 is an interesting one. I don't think that we can get rid of > "svc/authentication" yet, but it does expose a challenging problem. Answering there.
TODO koha-tmpl/intranet-tmpl/prog/en/modules/admin/didyoumean.tt SKIPPED (in remaining pad) This is part of the svc operation. The form does not submit, there is still an ajax post with token.
See the remote branch for the update - security/bug_34478_svc
(In reply to Jonathan Druart from comment #22) > > I'd have to re-test, but last time I tried a patch like this the CSRF test > > was using the logged in user session against the CSRF token generated for > > the anonymous session. If things haven't changed since then, I don't know > > that this patch will work. > > Please define "does not work". Don't worry about it. I'll look at it today with bug 36102. > (In reply to David Cook from comment #18) > > If we're going to make changes at the plack.psgi level, why not just do a > > Koha::Middleware? > > Changes to plack.psgi are already made from bug 34478. > > Yes, why not? Why not refactor C4::Auth at the same time? :) Writing a new middleware and refactoring C4::Auth are completely different things. Writing a new middleware is quick and easy. Also easier to test. It's also been done before. > More changes, more questions. KISS for now please (it's already complex > enough). What I'm talking about would be less complex. But it's fine. Teamwork is hard. Happy to follow your lead. > Are we using HEAD, OPTIONS or TRACE already? HEAD is very common and useful. I use it in plugins and monitoring. I imagine a lot of people use it in their monitoring. OPTIONS and TRACE are not so common. > > That said, we can just do $stateful_methods{ $request_method } to still make > > the elsif() pretty clean. > > Please provide a patch. Will do.
(In reply to Jonathan Druart from comment #24) > See the remote branch for the update - security/bug_34478_svc I just noticed you've removed svc/authentication in that branch. I really don't think we should do that.
I just realized that the changes to debian/templates/plack.psgi aren't automatically reflected in /etc/koha/sites/kohadev/plack.psgi Has anyone been testing those changes?
(In reply to David Cook from comment #27) > I just realized that the changes to debian/templates/plack.psgi aren't > automatically reflected in /etc/koha/sites/kohadev/plack.psgi > > Has anyone been testing those changes? Yes, I think people working on this are aware of that. The file must be copied.
From remaining pad: koha-tmpl/intranet-tmpl/prog/js/pages/preferences.js MOVED: This is part of the svc stuff too. Comment on 36084 Please fix here
I've added support for CSRF tokens to svc/authentication, but there's more work to do on the svc/ API as a whole I'm happy to look at this
(In reply to David Cook from comment #30) > I've added support for CSRF tokens to svc/authentication, but there's more > work to do on the svc/ API as a whole > > I'm happy to look at this I reckon that all svc/* endpoints should return a X-CSRF_TOKEN in their response headers, so that API consumers outside of the Koha web app can interact with the SVC API. If they encounter errors, they can do a GET svc/authentication to get a new CSRF token in the X-CSRF_TOKEN response header, but that won't be as smooth as getting it in any SVC API response. -- Happy to discuss this more, especially if I've made too many assumptions in my communications. It's been a long day.
(In reply to David Cook from comment #30) > I've added support for CSRF tokens to svc/authentication, but there's more > work to do on the svc/ API as a whole > > I'm happy to look at this Not before we discussed it. This bug is about the other svc scripts. authentication and CSRF token for external consumer needs to be discussed somewhere else I think.
Just a dumb question: Was thinking if we could use ajaxSend event to include the token (from the meta tag) in a general header file for catching all cases where we do these ajax requests in Koha itself based on cookie auth? And check the token in check_cookie_auth? Other REST API use should probably use API tokens where there seems to be no need for an extra csrf token? Not talking about Basic etc. ;)
(In reply to Marcel de Rooy from comment #33) > Just a dumb question: > > Was thinking if we could use ajaxSend event to include the token (from the > meta tag) in a general header file for catching all cases where we do these > ajax requests in Koha itself based on cookie auth? > And check the token in check_cookie_auth? > > Other REST API use should probably use API tokens where there seems to be no > need for an extra csrf token? Not talking about Basic etc. ;) Please see what is done on the branch :)
koha-tmpl/intranet-tmpl/prog/js/fetch/http-client.js 24 this.csrf_token = $('meta[name="csrf-token"]').attr("content");
So basically the idea here (for svc scripts) is to have: * An api-client.js file to have the same interface everywhere (that was not necessary but I liked the idea of forcing the use here..) * OR do not have an api-client.js file if there is much interest in the changes (I did that for creator_batches). It is much easier, but less fun. * Have 'op' or not! We could early exit in the script of request_method ne 'POST' (or DELETE, PUT, etc.). This is not done everywhere and not consistent (neither after my changes). If the svc script rejects other methods we don't need an 'op' parameter (eg. the ones using C4::Service or testing $query->request_method eq "POST").
Pushed to master for 24.05.00.
Created attachment 162712 [details] [review] Bug 36084: Fix cookie domain for www/ tests This patch changes the hardcoded `koha.local` value on t::lib::Mocks::Zebra so tests don't fail when the domain is not `koha`. To test: 1. Run: $ ktd --shell k$ export KOHA_INTRANET_URL=http://kohadev-intra.myDNSname.org:8081 k$ prove t/db_dependent/www/batch.t => FAIL: Tests fail! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Last patch pushed to master.
Depends on Bug 36102 not in 23.11.x