fix(isoDate): pad a bare-hour timeshift with a colon, not just zeros - #3143
Open
yfwmaniish wants to merge 1 commit into
Open
fix(isoDate): pad a bare-hour timeshift with a colon, not just zeros#3143yfwmaniish wants to merge 1 commit into
yfwmaniish wants to merge 1 commit into
Conversation
internals.isoDate() pads a bare-hour UTC timeshift (e.g. "+04") before handing the string to Date() by appending '00' directly, producing "...+0400" instead of the ISO 8601 extended-format "...+04:00". Node's own Date parser tolerates the malformed form, which is why this went unnoticed, but it breaks stricter parsers (Safari) since ISO 8601 doesn't allow mixing basic-format offsets into an otherwise extended-format string. Flagged by a reviewer on hapijs#2421 (the PR that introduced this line) but never addressed after merge; tracked since as hapijs#2434. Also drops the leading `.*` in the trigger regex, per the same review thread - matching doesn't require anchoring at the string start, so it was redundant. Since Node's Date constructor doesn't distinguish "+0700" from "+07:00", a test asserting on Joi's converted output wouldn't catch a regression here. The new test instead spies on the Date constructor to assert the actual string produced. Fixes hapijs#2434.
There was a problem hiding this comment.
Pull request overview
This pull request fixes string.isoDate() coercion so that ISO 8601 datetimes with a bare-hour UTC offset (e.g. +07) are normalized to the extended offset form (+07:00) before being parsed by Date(), avoiding invalid mixed-format offsets that break stricter parsers (notably Safari).
Changes:
- Update
internals.isoDate()normalization to append:00(extended) instead of00(basic) when padding a bare-hour timeshift. - Simplify the trigger regex by removing the redundant leading
.*. - Add a regression test that spies on the argument passed into
Date()to ensure the colonized offset is used.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/types/string.js | Fixes bare-hour offset padding to use +HH:00 (extended format) before calling new Date(value). |
| test/types/string.js | Adds a regression test that temporarily replaces global.Date to assert the exact string passed to Date(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #2434.
What
`internals.isoDate()` (`lib/types/string.js`) pads a bare-hour UTC timeshift like `+04` before handing the string to `Date()`:
```js
if (/.T.[+-]\d\d$/.test(value)) { // Add missing trailing zeros to timeshift
value += '00';
}
```
That produces `...+0400` — ISO 8601 basic-format offset — glued onto an otherwise extended-format string (`YYYY-MM-DDTHH:mm:ss`). ISO 8601 doesn't allow mixing the two; the correct extended-format offset is `...+04:00`.
This was flagged by @kanongil in review on #2421 (the PR that introduced the line): "You need to add the
:as well to make it a proper extended iso date string. As it is, it breaks in Safari." The PR was merged before the fix landed, and #2434 was opened to track it but never addressed.Also drops the leading
.*in the trigger regex, per the same review thread (.test()doesn't need start-anchoring, so it was redundant).Fix
```js
if (/T.*[+-]\d\d$/.test(value)) { // Add missing separator and trailing zeros to timeshift
value += ':00';
}
```
Testing
Node's own `Date` constructor parses `+0700` and `+07:00` identically, so a test asserting on Joi's converted output (`.toISOString()`) can't distinguish the bug from the fix — I confirmed this empirically before writing the test. Instead, the new test spies on the global `Date` constructor to assert the exact string Joi passes to it.