STEPS TO REPRODUCE: 1 - go to /cgi-bin/koha/tools/letter.pl 2 - click on New notice -> claim serial issue (or any other one) 3 - Fill required fields with random string 4 - Save -> KABOOM
Created attachment 179206 [details] [review] Bug 39317: Fix CSRF protection for letters TEST PLAN: 1 - go to /cgi-bin/koha/tools/letter.pl 2 - click on New notice -> claim serial issue (or any other one) 3 - Fill required fields with random string 4 - Save -> KABOOM 5 - Apply patch 6 - Repeat 1-4 and see it works now
I'm a bit disappointed because the promised Kaboom doesn't appear on every install :'( However there is an issue with the action submitted in post, which is by default the one which lead to the form, and which is incorrect. I'm updating the test plan
Created attachment 179209 [details] [review] Bug 39317: Fix CSRF protection for letters TEST PLAN: 1 - go to /cgi-bin/koha/tools/letter.pl 2 - click on New notice -> claim serial issue (or any other one) 3 - Fill required fields with random string 4 - Save -> if it breaks, go to 5 - Open a network panel (F12 -> network) 6 - Filter on letter.pl 7 - Fill another new notice -> the arguments of the form page have been passed in the address of the POST request. 8 - Apply patch 9 - Repeat the whole process and see it works now
I tried several times, with different browsers, and couldn't get a KABOOM. Hopefully someone else is more lucky!
(In reply to David Nind from comment #4) > I tried several times, with different browsers, and couldn't get a KABOOM. > > Hopefully someone else is more lucky! I submitted a plan to test it without crash, see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39317#c3
(In reply to Baptiste Wojtkowski (bwoj) from comment #5) > (In reply to David Nind from comment #4) > > I tried several times, with different browsers, and couldn't get a KABOOM. > > > > Hopefully someone else is more lucky! > > I submitted a plan to test it without crash, see > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39317#c3 Sorry, I still can't produce an error.
No KABOOM for me either. What is the reasoning behind this patch? Is the action attribute required for the CSRF protection to work?
Sounds like the missing Step 0 would be to install a reverse proxy in front of Koha which does something dubious when it sees a POST to a URL with a query string. Is it converting the POST to a GET, or stuffing the params in the query string into the POST body?
Does the patch solve your problem? I don't love form elements without action attributes, so it seems like a reasonable change in general to me.
In my case ? Yup
Created attachment 180818 [details] [review] Bug 39317: Fix CSRF protection for letters TEST PLAN: 1 - go to /cgi-bin/koha/tools/letter.pl 2 - click on New notice -> claim serial issue (or any other one) 3 - Fill required fields with random string 4 - Save -> if it breaks, go to 5 - Open a network panel (F12 -> network) 6 - Filter on letter.pl 7 - Fill another new notice -> the arguments of the form page have been passed in the address of the POST request. 8 - Apply patch 9 - Repeat the whole process and see it works now Signed-off-by: Magnus Enger <magnus@libriotech.no> I can't recreate the problem, but the change seems sensible enough and does not seem to introduce a regression, I can still save a new letter/notice.
I'll look at QAing this soon. As Phil said, I wonder if this is something that Baptiste has noticed on BibLibre installs which run behind Nginx I believe?
Created attachment 180893 [details] [review] Bug 39317: Fix CSRF protection for letters TEST PLAN: 1 - go to /cgi-bin/koha/tools/letter.pl 2 - click on New notice -> claim serial issue (or any other one) 3 - Fill required fields with random string 4 - Save -> if it breaks, go to 5 - Open a network panel (F12 -> network) 6 - Filter on letter.pl 7 - Fill another new notice -> the arguments of the form page have been passed in the address of the POST request. 8 - Apply patch 9 - Repeat the whole process and see it works now Signed-off-by: Magnus Enger <magnus@libriotech.no> I can't recreate the problem, but the change seems sensible enough and does not seem to introduce a regression, I can still save a new letter/notice. Signed-off-by: David Cook <dcook@prosentient.com.au>
I couldn't reproduce the problem either, but it's a sensible change (ie improvement) to make and it works.
Pushed for 25.05! Well done everyone, thank you!