Repository navigation
enh: validate datetime fields - #8388
luka-nextcloud wants to merge 5 commits into
Conversation
|
🐢 Performance warning. |
blizzz
left a comment
There was a problem hiding this comment.
Already stored values are not repaired, right? One-time repair job could be considered, or command. But I guess it is an edge case, though would be nice for affected users/admins to get out of it?
11bcfe1 to
6544aad
Compare
6544aad to
6b2db55
Compare
|
🐢 Performance warning. |
6b2db55 to
d733901
Compare
|
🐢 Performance warning. |
blizzz
left a comment
There was a problem hiding this comment.
sqlite is not affected, right?
Json import and Trello imports, are they out of scope, or is it worth checking the year there as well?
| parent::__construct(); | ||
| } | ||
|
|
||
| protected function configure() { |
There was a problem hiding this comment.
I understand this is a one time thing to run. What is the motivation to go with a command, not a one-time repair job?
There was a problem hiding this comment.
It is because admin should input the default year to fix those fields.
There was a problem hiding this comment.
Hm, if you were an admin, what would you choose? Like, is there a reason to pick any other number than 9999? What would you pinpoint it at?
| $allowedFormats = [ | ||
| 'Y-m-d', | ||
| 'Y-m-d H:i:s', | ||
| 'Y-m-d\TH:i:sP', | ||
| 'Y-m-d\TH:i:s.v\Z', | ||
| ]; |
There was a problem hiding this comment.
to we need to be restrictive and selective on formats? Would not passing them to \DateTime(Immutable) and evaluating the year be sufficient?
For, other clients (thinking about the Deck mobile app) might send different formats that have been accepted so far.
There was a problem hiding this comment.
I think it is worth to restrict the formats by allowing common formats. I will check the Deck android app to ensure the format sent from mobile is supported.
There was a problem hiding this comment.
@blizzz Could you please provide the datetime formats and some sample values sent from the mobile app so I can ensure the validation logic correctly cover those formats? I am not able to run the app because I have no way to get the apk of https://github.com/stefan-niedermann/nextcloud-deck
There was a problem hiding this comment.
With external clients it can also be someone's curl script that then breaks.
According to Clause, Android Deck does this:
[Certain] Android app source (stefan-niedermann/nextcloud-deck, main branch):
- deprecated/app/.../GsonUTCInstantAdapter.java serialises with DateTimeFormatter.ISO_INSTANT, giving …T10:15:30Z. That is rejected.
- data/remote/.../OffsetDateTimeAdapter.java uses ISO_OFFSET_DATE_TIME. That writes Z for UTC and drops :ss when seconds are zero. Both are rejected.
and further
[Likely] So after this PR the app gets a 400 on every card update that has a due date. Third-party clients (n8n, scripts) will hit the same thing. docs/API.md only says "ISO-8601", and it never listed these four formats.
Suggestion:
try {
$year = (int)(new \DateTimeImmutable($value))->format('Y');
} catch (\Exception) {
return false;
}
return $year >= 1 && $year <= 9999;which still validates and is format agnostic.
| $datetime = \DateTime::createFromFormat($format, $value); | ||
| if ($datetime && $datetime->format($format) === $value) { | ||
| // Check if the year is within the valid range | ||
| if ((int)$datetime->format('Y') > 9999) { |
There was a problem hiding this comment.
at the moment, createFromFormat would not accept years larger than 9999:
php > $format = 'Y-m-d';
php > $datetime = \DateTime::createFromFormat($format, '13731-10-01');
php > var_dump($datetime);
bool(false)
So we have dead code here. Passing this directly to DateTime or DateTimeImmutable would turn this active though:
php > $datetime2 = new \DateTime('13731-10-01');
php > var_dump($datetime2);
object(DateTime)#1 (3) {
["date"]=>
string(26) "2001-10-01 00:00:00.000000"
["timezone_type"]=>
int(3)
["timezone"]=>
string(3) "UTC"
}
php > $datetime3 = new \DateTimeImmutable('13731-10-01');
php > var_dump($datetime3);
object(DateTimeImmutable)#2 (3) {
["date"]=>
string(26) "2001-10-01 00:00:00.000000"
["timezone_type"]=>
int(3)
["timezone"]=>
string(3) "UTC"
}
Personally I prefer DateTimeImmutable when it is not supposed to change, but it aint a stopper.
There was a problem hiding this comment.
You are right about the dead code. I have removed it.
BTW, I still prefer using \DateTime::createFromFormat(), as the fact that it does not accept years greater than 9999 makes more sense from perspective of datetime validation.
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
Signed-off-by: Luka Trovic <luka@nextcloud.com>
d733901 to
6055ec4
Compare
I have just checked. The sqlite was affected. I have updated the command |
|
🐢 Performance warning. |
| $qb->expr()->isNotNull('startdate'), | ||
| $qb->expr()->isNotNull('done'), | ||
| )); | ||
| $cards = $qb->executeQuery()->fetchAllAssociative(); |
There was a problem hiding this comment.
one more suggestion: do not fetch all cards, but cycle through them one by one. We may risk OOM issues on big instances and maybe limit what we fetch – just the necessary fields. Description could be large, but is not needed.
Summary
Checklist