Bug 30540 - Double processing invalid dates can lead to ISE
Summary: Double processing invalid dates can lead to ISE
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-14 07:40 UTC by Martin Renvoize
Modified: 2022-05-16 04:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.06,21.05.15,20.11.19


Attachments
Bug 30540: eval on set_time_zone (923 bytes, patch)
2022-04-14 08:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30540: Unit test (1.61 KB, patch)
2022-04-14 09:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30540: eval on set_time_zone (923 bytes, patch)
2022-04-14 09:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30540: Unit test (1.67 KB, patch)
2022-04-14 10:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30540: eval on set_time_zone (980 bytes, patch)
2022-04-14 10:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30540: Fix perl critic complaint of leading zeros (856 bytes, patch)
2022-04-14 10:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30540: Unit test (1.76 KB, patch)
2022-04-15 06:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30540: eval on set_time_zone (1.05 KB, patch)
2022-04-15 06:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30540: Fix perl critic complaint of leading zeros (954 bytes, patch)
2022-04-15 06:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-04-14 07:40:04 UTC
In the KohaDates filter we process our passed date using dt_from_string before passing it to output_pref.

dt_from_string handles bad dates in an eval block, falling back to assigning a floating timezone. However, in the subsequent call to output_pref we try to set the timezone a second time and this time it is not in an eval and so will throw an internal server error for such dates.
Comment 1 Martin Renvoize 2022-04-14 08:15:56 UTC
Created attachment 133290 [details] [review]
Bug 30540: eval on set_time_zone

This patch adds an eval around the call to set_time_zone so that we
don't crash on invalid local dates and instead just keep the passed
floating timezone.
Comment 2 Martin Renvoize 2022-04-14 08:17:51 UTC
2017-03-26 01:35:00 is such a date in Europe/London time.. it's a DST boundary day so anything between 12.59.59 and 02.00.00 doesn't exist ;)

Set any checkout date in the database to this and set your tz config to Europe/London and you'll find the issuehistory page will crash without the patch here.

Unit test to follow.. once my brain has worked out how to write one.
Comment 3 Martin Renvoize 2022-04-14 09:22:09 UTC
Created attachment 133292 [details] [review]
Bug 30540: Unit test

This patch adds tests for checking the output_pref method against
a timezone and datetime that would cause an Invalid local time failure

To test:
1. Apply this patch
2. Run:
   $ kshell
   k$ prove t/DateUtils.t
=> FAIL: The feature is not implemented, tests fail to complete
Comment 4 Martin Renvoize 2022-04-14 09:22:13 UTC
Created attachment 133293 [details] [review]
Bug 30540: eval on set_time_zone

This patch adds an eval around the call to set_time_zone so that we
don't crash on invalid local dates and instead just keep the passed
floating timezone.
Comment 5 Nick Clemens 2022-04-14 10:57:08 UTC
Created attachment 133297 [details] [review]
Bug 30540: Unit test

This patch adds tests for checking the output_pref method against
a timezone and datetime that would cause an Invalid local time failure

To test:
1. Apply this patch
2. Run:
   $ kshell
   k$ prove t/DateUtils.t
=> FAIL: The feature is not implemented, tests fail to complete

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2022-04-14 10:57:11 UTC
Created attachment 133298 [details] [review]
Bug 30540: eval on set_time_zone

This patch adds an eval around the call to set_time_zone so that we
don't crash on invalid local dates and instead just keep the passed
floating timezone.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2022-04-14 10:57:16 UTC
Created attachment 133299 [details] [review]
Bug 30540: Fix perl critic complaint of leading zeros

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Marcel de Rooy 2022-04-15 06:36:50 UTC
Created attachment 133336 [details] [review]
Bug 30540: Unit test

This patch adds tests for checking the output_pref method against
a timezone and datetime that would cause an Invalid local time failure

To test:
1. Apply this patch
2. Run:
   $ kshell
   k$ prove t/DateUtils.t
=> FAIL: The feature is not implemented, tests fail to complete

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2022-04-15 06:36:55 UTC
Created attachment 133337 [details] [review]
Bug 30540: eval on set_time_zone

This patch adds an eval around the call to set_time_zone so that we
don't crash on invalid local dates and instead just keep the passed
floating timezone.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2022-04-15 06:36:59 UTC
Created attachment 133338 [details] [review]
Bug 30540: Fix perl critic complaint of leading zeros

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Fridolin Somers 2022-04-20 07:24:30 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 12 Kyle M Hall 2022-04-29 16:53:43 UTC
Please rebase and ping me if needed in 21.11, I tried but get an error on the unit test.
Comment 13 Kyle M Hall 2022-04-29 16:56:09 UTC
Pushed to 21.11.x for 21.11.06
Comment 14 Andrew Fuerste-Henry 2022-05-03 19:58:36 UTC
Pushed to 21.05.x for 21.05.15
Comment 15 Victor Grousset/tuxayo 2022-05-09 21:52:48 UTC
Backported: Pushed to 20.11.x branch for 20.11.19
Comment 16 wainuiwitikapark 2022-05-16 04:23:02 UTC
Does this need to be backported to 19.11.x?