[Backend][Bug] UTC timestamps parsed without RoundtripKind — TZ-dependent shift breaks phone-code expiry + ship/character DeletedAt (SonarQube S6580) #657

New Issue

2026-07-17T14:58:11Z

spikerj commented

2026-07-17 14:58:11 +00:00

Found via SonarQube S6580 triage (2026-07-17), verified against the write side.

Several timestamps are WRITTEN as DateTime.UtcNow.ToString("O") (ISO-8601 with 'Z', UTC) but READ with DateTime.Parse/TryParse WITHOUT DateTimeStyles.RoundtripKind. Default parsing converts a 'Z' string to LOCAL time, so on any server whose timezone is not UTC the value shifts by the UTC offset:

ProcessTwilioWebhookCommandHandler:371 (worst): phoneVerificationCodeGeneratedAt is parsed then generatedAt.AddMinutes(CodeExpirationMinutes) is compared to DateTime.UtcNow. On a non-UTC server the code-expiration window is wrong — codes rejected far too early (west of UTC) or valid far too long (east of UTC), a real security concern for phone verification.
RedisShipCache:326 + GetDateTime helper :349, RedisCharacterCache:408 + helper :431: DeletedAt and other cached timestamps shift, corrupting deletion times / game-state timestamps on non-UTC hosts.
ConfirmEmailCommandHandler:75: already uses RoundtripKind (correct) but passed null culture; tidied to InvariantCulture.

Currently latent because containers usually run UTC, but fragile — a dev machine or any non-UTC deployment breaks it. The ConfirmEmail handler already does it right (RoundtripKind), which is the pattern the fix applies everywhere.

Fix (PR incoming): parse with CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind at all sites, preserving UTC Kind. Zero behavior change on UTC servers (identical ticks); correct on non-UTC. No unit test added: a UTC CI runner cannot isolate the shift (the 'Z' string yields identical ticks when local==UTC), so a test would give false confidence — correctness follows the already-tested ConfirmEmail precedent. GameServer 915 + ConfirmEmail 7 + slnf green.

**Found via SonarQube S6580 triage (2026-07-17), verified against the write side.** Several timestamps are WRITTEN as `DateTime.UtcNow.ToString("O")` (ISO-8601 with 'Z', UTC) but READ with `DateTime.Parse/TryParse` WITHOUT `DateTimeStyles.RoundtripKind`. Default parsing converts a 'Z' string to LOCAL time, so on any server whose timezone is not UTC the value shifts by the UTC offset: - **ProcessTwilioWebhookCommandHandler:371** (worst): `phoneVerificationCodeGeneratedAt` is parsed then `generatedAt.AddMinutes(CodeExpirationMinutes)` is compared to `DateTime.UtcNow`. On a non-UTC server the code-expiration window is wrong — codes rejected far too early (west of UTC) or valid far too long (east of UTC), a real security concern for phone verification. - **RedisShipCache:326 + GetDateTime helper :349**, **RedisCharacterCache:408 + helper :431**: `DeletedAt` and other cached timestamps shift, corrupting deletion times / game-state timestamps on non-UTC hosts. - **ConfirmEmailCommandHandler:75**: already uses RoundtripKind (correct) but passed `null` culture; tidied to InvariantCulture. Currently latent because containers usually run UTC, but fragile — a dev machine or any non-UTC deployment breaks it. The ConfirmEmail handler already does it right (RoundtripKind), which is the pattern the fix applies everywhere. **Fix (PR incoming):** parse with `CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind` at all sites, preserving UTC Kind. Zero behavior change on UTC servers (identical ticks); correct on non-UTC. No unit test added: a UTC CI runner cannot isolate the shift (the 'Z' string yields identical ticks when local==UTC), so a test would give false confidence — correctness follows the already-tested ConfirmEmail precedent. GameServer 915 + ConfirmEmail 7 + slnf green.

spikerj commented

2026-07-18 17:48:06 +00:00

Verified fixed on master — closing. (Skeptical re-check 2026-07-18, ticket-triage loop.)

The fix shipped in commit 80b8cd00 fix(time): preserve UTC when parsing O-format timestamps (S6580) — but its message cited #651 (the IndexedDB offline-book ticket, unrelated and already closed) instead of this ticket, so #657 stayed open. Re-verified every named site on current master:

ProcessTwilioWebhookCommandHandler L377 — TryParse(..., CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, ...) ✔
RedisShipCache L327 (DeletedAt) + GetDateTime helper L349-350 ✔
RedisCharacterCache L409 + helper L431-432 ✔
ConfirmEmailCommandHandler L75 — now InvariantCulture + RoundtripKind ✔
Sweep of both cache files: no remaining DateTime.Parse/TryParse without RoundtripKind.

No code change required; closing as completed (fix merged, wrong ticket ref in the commit message).

**Verified fixed on master — closing.** (Skeptical re-check 2026-07-18, ticket-triage loop.) The fix shipped in commit `80b8cd00` *fix(time): preserve UTC when parsing O-format timestamps (S6580)* — but its message cited **#651** (the IndexedDB offline-book ticket, unrelated and already closed) instead of this ticket, so #657 stayed open. Re-verified every named site on current master: - `ProcessTwilioWebhookCommandHandler` L377 — `TryParse(..., CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, ...)` ✔ - `RedisShipCache` L327 (`DeletedAt`) + `GetDateTime` helper L349-350 ✔ - `RedisCharacterCache` L409 + helper L431-432 ✔ - `ConfirmEmailCommandHandler` L75 — now InvariantCulture + RoundtripKind ✔ - Sweep of both cache files: no remaining `DateTime.Parse/TryParse` without `RoundtripKind`. No code change required; closing as completed (fix merged, wrong ticket ref in the commit message).

spikerj closed this issue

2026-07-18 17:48:06 +00:00

spikerj referenced this issue

2026-07-18 17:57:52 +00:00

[Backend][Bug] UpdateUserCommandHandler silently swallows all Keycloak failures — failed user updates report success #656

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#657