Bug 36700

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>

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.

(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 :)

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",
                    },
                }),

(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.

(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 ;)

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.

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).

Arf we use 2 levels of nginx reverse proxies.

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...)

(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".

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

Created attachment 165715 [details] [review]
Bug 36700: Update svc to use CSRF-TOKEN

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..

Created attachment 165716 [details] [review]
Bug 36700: Fix CSRF-TOKEN header in image upload

(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

Created attachment 165744 [details] [review]
Bug 36700: Update svc to use CSRF-TOKEN

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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>

(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.

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>

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>

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>

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

Thanks all, Pushed to main as RM Assistant.

*** Bug 36681 has been marked as a duplicate of this bug. ***