Bug 36700 - CSRF: Cannot save a systempreference (when nginx drops the CSRF header with an underscore)
Summary: CSRF: Cannot save a systempreference (when nginx drops the CSRF header with a...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
: 36681 (view as bug list)
Depends on:
Blocks: 36598
  Show dependency treegraph
 
Reported: 2024-04-25 12:43 UTC by Marcel de Rooy
Modified: 2024-05-03 07:18 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:
24.05.00


Attachments
Bug 36700: Add CSRF token to the systemprefences POST (1.75 KB, patch)
2024-04-25 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36700: [ALTERNATIVE] Fix underscore in CSRF header (3.69 KB, patch)
2024-04-26 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36700: Update svc to use CSRF-TOKEN (1.38 KB, patch)
2024-04-29 01:23 UTC, David Cook
Details | Diff | Splinter Review
Bug 36700: Fix CSRF-TOKEN header in image upload (1.08 KB, patch)
2024-04-29 01:36 UTC, David Cook
Details | Diff | Splinter Review
Bug 36700: Update svc to use CSRF-TOKEN (1.48 KB, patch)
2024-04-29 13:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36700: Fix CSRF-TOKEN header in image upload (1.17 KB, patch)
2024-04-29 13:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36700: Fix underscore in CSRF header (3.71 KB, patch)
2024-05-01 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36700: Update svc to use CSRF-TOKEN (1.50 KB, patch)
2024-05-01 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36700: Fix CSRF-TOKEN header in image upload (1.20 KB, patch)
2024-05-01 12:34 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 Marcel de Rooy 2024-04-25 12:43:30 UTC
[2024/04/25 11:50:14] [WARN] Programming error - No CSRF token passed for POST http://172.20.21.1:18081/intranet/svc/config/systempreferences (referer: https://master.rijkskoha.nl:28443/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=CancelOrdersInClosedBaskets&ok=Search) at /usr/share/koha/Koha/Middleware/CSRF.pm line 82.

Fix underway. Diving into js, HTTPClient etc.
Comment 1 Marcel de Rooy 2024-04-25 12:45:22 UTC
Created attachment 165537 [details] [review]
Bug 36700: Add CSRF token to the systemprefences POST

Test plan:
Go to Admin/Preferences.
Change value and Save.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Jonathan Druart 2024-04-25 19:31:37 UTC
Cannot test right now but this was working after the main csrf fix. The token is supposed to be injected in http-client for POST requests.
Comment 3 Marcel de Rooy 2024-04-26 07:42:45 UTC
(In reply to Jonathan Druart from comment #2)
> Cannot test right now but this was working after the main csrf fix. The
> token is supposed to be injected in http-client for POST requests.

Still need to apply this one each time when I want to change a pref in testing :)
Comment 4 Marcel de Rooy 2024-04-26 07:50:21 UTC
Please note also koha-tmpl/intranet-tmpl/prog/js/vue/fetch/system-preferences-api-client.js:
            update: (variable, value) =>
                this.post({
                    endpoint: "",
                    body: "pref_%s=%s&csrf_token=%s".format(
                        variable,
                        value,
                        csrf_token
                    ),
                    headers: {
                        "Content-Type":
                            "application/x-www-form-urlencoded;charset=utf-8",
                    },
                }),
Comment 5 Marcel de Rooy 2024-04-26 08:26:51 UTC
(In reply to Jonathan Druart from comment #2)
> Cannot test right now but this was working after the main csrf fix. The
> token is supposed to be injected in http-client for POST requests.

http-client does insert a token in the header but it does not include a token in the body.
        let csrf_token = { csrf_token: this.csrf_token };
        let headers = { ...csrf_token, ...params.headers };

Middleware looks at:
my $csrf_token = $req->param('csrf_token') || $req->header('CSRF_TOKEN');

HTTP::Headers says this: Get or set the value of one or more header fields. The header field name ($field) is not case sensitive. To make the life easier for perl users who wants to avoid quoting before the => operator, you can use '_' as a replacement for '-' in header names.

So the lowercase csrf_token should not be an issue.

Are we bitten here by the underscore versus hyphen stuff? Is HTTP::Headers only looking for the hypenated one?
Note that is recommended to not use underscores in HTTP headers. There are several issues in that area.
Comment 6 Marcel de Rooy 2024-04-26 08:29:27 UTC
(In reply to Marcel de Rooy from comment #5)
> Note that is recommended to not use underscores in HTTP headers. There are
> several issues in that area.

Nginx widely used does not like them by default. My requests go via nginx ;)
Comment 7 Marcel de Rooy 2024-04-26 08:43:56 UTC
Created attachment 165618 [details] [review]
Bug 36700: [ALTERNATIVE] Fix underscore in CSRF header

Also change the cache key in template plugin. And the X- header
that had an underscore in svc script.
Comment 8 Marcel de Rooy 2024-04-26 08:56:50 UTC
Note that we also have the meta tag csrf-token in lowercase.
And several lookups with $('meta[name="csrf-token"]').attr('content').

These should still work. But note that they are case sensitive (in contrast with http headers).
Comment 9 Fridolin Somers 2024-04-26 09:50:09 UTC
Arf we use 2 levels of nginx reverse proxies.
Comment 10 David Cook 2024-04-29 01:03:24 UTC
Good call on replacing the underscore with a hyphen. 

I think we can just go with the ALTERNATIVE patch and rename this bug to reflect changing the underscore to a hyphen.

--

On a side note: worst case, Nginx can support underscores in headers with the "underscores_in_headers" option.

Technically speaking, according to RFC2822 and RFC3864 it's fine to use hyphens in  HTTP headers, but conventionally all standard headers use hyphens rather than underscores, so I think it makes sense to match convention.

Plus Nginx is a very popular reverse proxy, and it seems a shame to require a configuration change in it just to support this header.

(I wonder if there are other Koha headers that Nginx is silently ignoring...)
Comment 11 David Cook 2024-04-29 01:12:50 UTC
(In reply to Marcel de Rooy from comment #5)
> HTTP::Headers says this: Get or set the value of one or more header fields.
> The header field name ($field) is not case sensitive. To make the life
> easier for perl users who wants to avoid quoting before the => operator, you
> can use '_' as a replacement for '-' in header names.
 
I just noticed this on my own, as I notice in git commit 0fe82b601b4b06ce0e9fcc5ca677810886f26717 that I mixed and matched...

I outputted a X-CSRF_TOKEN header from svc/authentication, and in misc/migration_tools/koha-svc.pl I look for "X-CSRF-TOKEN". 

In hindsight, this should be converted just to "CSRF-TOKEN".
Comment 12 David Cook 2024-04-29 01:19:52 UTC
So looking at the patches... strictly speaking we don't need to change Koha::Middleware::CSRF, but we may as well.

We don't need to change Koha::Template::Plugin::Koha either as that's just a hash key and not a HTTP header name. But... not a blocker if it's changed anyway.

I'll send a follow-up to change svc/authentication to use CSRF-TOKEN in svc/authentication and to update misc/migration_tools/koha-svc.pl
Comment 13 David Cook 2024-04-29 01:23:40 UTC
Created attachment 165715 [details] [review]
Bug 36700: Update svc to use CSRF-TOKEN
Comment 14 David Cook 2024-04-29 01:35:25 UTC
We're missing places that don't use http-client.js

For example:
koha-tmpl/intranet-tmpl/prog/js/upload-images.js

 xhr.setRequestHeader("csrf_token", $('meta[name="csrf-token"]').attr("content"));

--

It looks like a bunch of API calls actually add the csrf_token to the body instead of using a header...

e.g.
koha-tmpl/intranet-tmpl/prog/js/file-upload.js
koha-tmpl/intranet-tmpl/prog/js/mana.js
koha-tmpl/intranet-tmpl/prog/js/subscription-add.js

--

Actually I think maybe we're just missing upload-images.js so I'll send a patch for that too..
Comment 15 David Cook 2024-04-29 01:36:31 UTC
Created attachment 165716 [details] [review]
Bug 36700: Fix CSRF-TOKEN header in image upload
Comment 16 Marcel de Rooy 2024-04-29 11:42:20 UTC
(In reply to David Cook from comment #12)
> So looking at the patches... strictly speaking we don't need to change
> Koha::Middleware::CSRF, but we may as well.

To make it easier, I did.

> We don't need to change Koha::Template::Plugin::Koha either as that's just a
> hash key and not a HTTP header name. But... not a blocker if it's changed
> anyway.

I did so to reduce the hits for the 'wrong spelling'.

> I'll send a follow-up to change svc/authentication to use CSRF-TOKEN in
> svc/authentication and to update misc/migration_tools/koha-svc.pl

Great
Comment 17 Marcel de Rooy 2024-04-29 13:16:21 UTC
Created attachment 165744 [details] [review]
Bug 36700: Update svc to use CSRF-TOKEN

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2024-04-29 13:16:23 UTC
Created attachment 165745 [details] [review]
Bug 36700: Fix CSRF-TOKEN header in image upload

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2024-04-29 13:18:28 UTC
(In reply to David Cook from comment #10)
> Plus Nginx is a very popular reverse proxy, and it seems a shame to require
> a configuration change in it just to support this header.

Yep. We can simply switch to a hyphen. Without requiring people to change nginx default settings.
Comment 20 Martin Renvoize 2024-05-01 12:34:11 UTC
Created attachment 165962 [details] [review]
Bug 36700: Fix underscore in CSRF header

Also change the cache key in template plugin. And the X- header
that had an underscore in svc script.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2024-05-01 12:34:14 UTC
Created attachment 165963 [details] [review]
Bug 36700: Update svc to use CSRF-TOKEN

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2024-05-01 12:34:17 UTC
Created attachment 165964 [details] [review]
Bug 36700: Fix CSRF-TOKEN header in image upload

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2024-05-01 12:36:23 UTC
With my testing and the work from both Marcel and David I'm going to count them as cross signing each other and upgrade my signoff as a QA
Comment 24 Martin Renvoize 2024-05-01 12:41:23 UTC
Thanks all, Pushed to main as RM Assistant.
Comment 25 Julian Maurice 2024-05-03 06:10:11 UTC
*** Bug 36681 has been marked as a duplicate of this bug. ***