Bug 409912 - Quietly "saves" invalid event (incidence) start or end times as midnight (particularly affects fr_CA sub-locale)
Summary: Quietly "saves" invalid event (incidence) start or end times as midnight (par...
Status: RESOLVED FIXED
Alias: None
Product: korganizer
Classification: Applications
Component: incidence editors (show other bugs)
Version: 5.10.3
Platform: Other Linux
: NOR major
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-17 19:15 UTC by Philippe Cloutier
Modified: 2020-12-22 23:23 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 5.16.0
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Cloutier 2019-07-17 19:15:58 UTC
incidenceeditor does not properly validate incidence start and end times. This occurs when either loading or saving:
1. IncidenceDateTime::setDateTimes(), which is called by load(), starts with a validation of time. But when the time is invalid, the time is simply quietly set to the current time: https://sources.debian.org/src/libkf5incidenceeditor/18.08.3-3/src/incidencedatetime.cpp/#L797
2. IncidenceDateTime::save() does not validate: https://cgit.kde.org/incidenceeditor.git/tree/src/incidencedatetime.cpp#n724
  No matter whether the times are valid, setDtStart() and setDtEnd() proceed to update():
https://api.kde.org/kdepim/kcalcore/html/incidence_8cpp_source.html#l00411
https://api.kde.org/kdepim/kcalcore/html/classKCalCore_1_1Event.html#a0d6840be8adf1a8fd37617ecf3da535b
  IncidenceBase::setDtStart() comes close, but just logs a warning: https://api.kde.org/kdepim/kcalcore/html/incidencebase_8cpp_source.html#l00314
  In the end, both call update(), which proceeds to incidenceUpdate() without ever throwing an exception: https://api.kde.org/kdepim/kcalcore/html/incidencebase_8cpp_source.html#l00600


The saving defect is particularly problematic, as:
1. KTimeComboBox is used without WarnOnInvalid.
2. The KTimeComboBox input mask does not necessarily guarantee that the input string is valid, even if the mask is respected. Here are 2 examples:
   1. With the en_US locale, with KDE Frameworks 5.60.0, the mask makes the AM/PM specifier entirely optional. If the user enters "23:45", for example, the entry is considered invalid and "saved" as midnight. If the user enters "10:00 P", the entry is invalid again and "saved" as midnight.
   2. Some locales have simply broken masks. For example, with the fr_CA sub-locale, due to issue #409867, with Qt Core 5.13, when the users wants to enter, say, 23:45, incidenceeditor will get back an invalid time string which - if I understand correctly - would be in this example "2345". It is obviously impossible for incidenceeditor to do the right thing in this case, but instead of reporting an issue, incidenceeditor quietly "saves" a midnight value.


Given that the saving defect can result in quiet data corruption/loss, which - when combined with issues like KDE #409867 and Qt Core's usage of "23 h 59" as format for fr_CA - can be unavoidable, this issue can make KOrganizer largely unusable.
Comment 1 Philippe Cloutier 2019-07-21 21:02:08 UTC
It seems this has also multiplied the impact of another issue, reported in issue report #361764. However, that mentions the impact could be either a quiet data loss, *or an inability to save events*, depending on the calendar time. Perhaps incidenceeditor has changed since then.
Comment 2 gjditchfield 2020-11-01 15:17:14 UTC
Git commit 701db76dc643d04fff14f9ee3875cd6654efdd34 by Glen Ditchfield.
Committed on 01/11/2020 at 02:01.
Pushed by gditchfield into branch 'master'.

Detect invalid start and end times

If the user enters an invalid string such as "25:00" into the start time field
or the end/due time field, IncidenceDateTime does not detect it, because the
QDateTime constructor used by currentStartDateTime() and currentEndDateTime()
treats it as "00:00".

This patch
 * detects and reports the error,
 * uses more specific error messages,
 * focuses on the erroneous field, and
 * logs at "debug" level, because the problem isn't very big.

M  +14   -0    autotests/CMakeLists.txt
A  +90   -0    autotests/incidencedatetimetest.cpp     [License: LGPL(v2.0+)]
M  +55   -10   src/incidencedatetime.cpp
M  +2    -0    src/incidencedatetime.h

https://invent.kde.org/pim/incidenceeditor/commit/701db76dc643d04fff14f9ee3875cd6654efdd34