fix(settings): the timezone field could store a value that breaks every report - #99
Merged
Conversation
…ry report
The America/New_York default is fine as a default. What was not fine is how it
could be changed: the admin Localization form is free text and the API
validated it as "a non-empty string", so "Eastern" or "America/Nowhere" saved
successfully.
Nothing rejected it and nothing fell back. getAppSettings only guarded EMPTY
(`row.timezone?.trim() || DEFAULT`), so a non-empty invalid value passed
straight through to Intl.DateTimeFormat, which throws a RangeError. That is the
same string salesDaily, generateSalesJournal and computeDailyReconciliation all
resolve their day against -- so one typo in an admin form broke all three money
paths at once, and the first symptom would have been tomorrow's empty report.
Three layers, because each catches a case the others cannot:
GUI the field is a picker, not free text. Intl.supportedValuesOf omits
"UTC" (it lists "Etc/UTC"), so UTC is prepended explicitly -- it is
the neutral answer and should not need hunting for. A value already
stored that the list does not contain still renders, marked.
WRITE the API rejects a zone this runtime cannot format with, naming the
value, so an operator learns immediately rather than tomorrow.
READ getAppSettings falls back to the default on an invalid STORED value
and logs it. Rows written before this guard, or by any other path,
cannot keep every report broken. Falling back rather than throwing is
this function's existing contract: reads never fail on bad settings.
isValidTimeZone is a try/catch probe against Intl rather than a membership test
against supportedValuesOf, and the test pins why: that list omits "UTC", so an
allow-list would reject the most obviously valid answer.
Worth recording, because I got it wrong first: Intl is looser than an IANA-only
reading suggests. Zone names match CASE-INSENSITIVELY, so "america/new_york" is
valid and my first draft asserted -- in the test AND in a code comment -- that
it was the classic typo that breaks. It is not. Legacy aliases ("EST", "GMT",
"US/Eastern") resolve too. What genuinely fails is a colloquial name, a
non-existent zone, and surrounding whitespace, which is why callers trim before
validating. The guard now inherits Intl's real boundary instead of being
stricter than the code it protects, and the test pins both directions.
Verified: next build exit 0 (a client component changed), tsc 0, jest unit 0 --
212 suites, 3,299 tests, 41 of them new.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
America/New_Yorkdefault is fine as a default. What wasn't fine is how it could be changed.The admin Localization form is free text, and the API validated it as "a non-empty string" — so
"Eastern"or"America/Nowhere"saved successfully. Nothing rejected it and nothing fell back:getAppSettingsonly guarded empty (row.timezone?.trim() || DEFAULT), so a non-empty invalid value went straight toIntl.DateTimeFormat, which throws aRangeError.That's the same string
salesDaily,generateSalesJournalandcomputeDailyReconciliationall resolve their day against. One typo in an admin form broke all three money paths at once — and the first symptom would have been tomorrow's empty report.Three layers, each catching what the others can't
Intl.supportedValuesOfomits"UTC"(it listsEtc/UTC), so UTC is prepended explicitly — it's the neutral answer and shouldn't need hunting for. An already-stored value the list doesn't contain still renders, marked.getAppSettingsfalls back to the default on an invalid stored value and logs it. Rows written before this guard — or by any other path — can't keep every report broken. Falling back rather than throwing is this function's existing contract: reads never fail on bad settings.Why a probe, not an allow-list
isValidTimeZoneis a try/catch againstIntlrather than a membership test againstsupportedValuesOf. The test pins the reason: that list omits"UTC", so an allow-list would reject the most obviously valid answer.I got the boundary wrong first
Worth recording.
Intlis looser than an IANA-only reading suggests:america/new_yorkis validEST,GMT,US/EasternMy first draft asserted, in the test and in a code comment, that
America/New_york(lowercase y) was "the classic typo" that breaks. It isn't. The test caught it, and both the comment and the assertion are corrected.What genuinely fails: a colloquial name (
Eastern), a non-existent zone (America/Nowhere), and surrounding whitespace — which is why callers trim before validating. The guard now inherits Intl's real boundary instead of being stricter than the code it protects, and the test pins both directions: every accepted value is safe to hand the date helpers, every rejected one really does throw.Still open
timezoneis GUI- and API-configurable but not expressible in a config preset — so "via code" isn't covered for this field yet. That's a separate piece of work (a settings preset kind), not folded in here.Verification
next build— exit 0 (a client component changed; this is the checktsccan't do)tsc --noEmit— 0jest --selectProjects unit— 212 suites, 3,299 tests, 41 of them neweslint0 errors (1 pre-existing warning, untouched),prettierclean🤖 Generated with Claude Code