[Bug 37785] New: Remove dead code in tools/letter.tt that looks like a form that would POST without an op
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Bug ID: 37785 Summary: Remove dead code in tools/letter.tt that looks like a form that would POST without an op Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P3 Component: Tools Assignee: phil@chetcolibrary.org Reporter: phil@chetcolibrary.org QA Contact: testopia@bugs.koha-community.org Depends on: 36192 Blocks: 37728 We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. In tools/letter.tt that's two forms, https://git.koha-community.org/Koha-community/Koha/src/commit/4f524635516325... which is shown IF ( add_validate or copy_validate) and one a bit further down at #L594 which is shown IF ( delete_confirmed ). To someone that knows how are templates work, that would sound like either the op which wants to show them would be add_validate or copy_validate or delete_confirmed, or more likely in the current era it would be cud-add_validate etc., which either sets the param add_validate, or was broken by the change to cud-ops. These two are different, though. If you look back through blame history for every change to letter.pl for the ops in question, they have never, ever, let their op be defined for a template to show a confirmation message. Currently they do "$op = q{}; # we return to the default screen for the next operation", if you go back to the dawn of time, prior to rel_3_0 moved to HEAD, they started out doing "print $input->redirect("letter.pl");". I'd like to have a better term than "dead code" for something which has been unreachable since it was copy-pasted from some other template, but I can't come up with an inoffensive one. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 [Bug 36192] [OMNIBUS] CSRF Protection for Koha https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37728 [Bug 37728] More "op" are missing in POSTed forms -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 --- Comment #1 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 170899 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170899&action=edit Bug 37785: Remove dead code in tools/letter.tt that looks like a form that would POST without an op We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. The two in tools/letter.tt are blocks of never-used code which would display a message confirming that you saved a notice, or that a notice was deleted after you confirmed that you wanted to delete it, but neither one has ever been executed. Now, the names of the ops don't match, because they are cud-add_validate etc. and would have to explicitly set a param for add_validate, but even before the CSRF change to cud- ops, they explicitly unset their $op so that as they say "# we return to the default screen for the next operation". Prior to that, they just did "print $input->redirect("letter.pl");" No test plan is possible, since this code has never once done anything. Sponsored-by: Chetco Community Public Library -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- Hum, yes... Better to fix the confirmation messages but I do agree it's out of the scope of this bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170899|0 |1 is obsolete| | --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 170959 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170959&action=edit Bug 37785: Remove dead code in tools/letter.tt that looks like a form that would POST without an op We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. The two in tools/letter.tt are blocks of never-used code which would display a message confirming that you saved a notice, or that a notice was deleted after you confirmed that you wanted to delete it, but neither one has ever been executed. Now, the names of the ops don't match, because they are cud-add_validate etc. and would have to explicitly set a param for add_validate, but even before the CSRF change to cud- ops, they explicitly unset their $op so that as they say "# we return to the default screen for the next operation". Prior to that, they just did "print $input->redirect("letter.pl");" No test plan is possible, since this code has never once done anything. Sponsored-by: Chetco Community Public Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37823 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #170959|0 |1 is obsolete| | --- Comment #4 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 171266 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171266&action=edit Bug 37785: Remove dead code in tools/letter.tt that looks like a form that would POST without an op We intend not to have forms with method="post" without an op variable (so we can check that the op starts with "cud-" as part of the CSRF protection), but because of bug 37728 some were missed. The two in tools/letter.tt are blocks of never-used code which would display a message confirming that you saved a notice, or that a notice was deleted after you confirmed that you wanted to delete it, but neither one has ever been executed. Now, the names of the ops don't match, because they are cud-add_validate etc. and would have to explicitly set a param for add_validate, but even before the CSRF change to cud- ops, they explicitly unset their $op so that as they say "# we return to the default screen for the next operation". Prior to that, they just did "print $input->redirect("letter.pl");" No test plan is possible, since this code has never once done anything. Sponsored-by: Chetco Community Public Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.derscheid@lmscloud.de QA Contact|testopia@bugs.koha-communit |paul.derscheid@lmscloud.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |24.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 --- Comment #5 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |lucas@bywatersolutions.com Status|Pushed to main |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37785 Bug 37785 depends on bug 36192, which changed state. Bug 36192 Summary: [OMNIBUS] CSRF Protection for Koha https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36192 What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org