Skip to content

fix(isoDate): pad a bare-hour timeshift with a colon, not just zeros - #3143

Open
yfwmaniish wants to merge 1 commit into
hapijs:masterfrom
yfwmaniish:fix/isodate-bare-hour-timeshift
Open

fix(isoDate): pad a bare-hour timeshift with a colon, not just zeros#3143
yfwmaniish wants to merge 1 commit into
hapijs:masterfrom
yfwmaniish:fix/isodate-bare-hour-timeshift

Conversation

@yfwmaniish

Copy link
Copy Markdown

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.

  • Negative control: reverted just the source fix, kept the test — it fails, showing the un-colonized string (`+0700`) reaching `Date()`. Restored.
  • Full suite: `lab` — 1824/1824 passing.
  • `eslint` clean on both changed files.

Copilot AI lite review requested due to automatic review settings September 1, 2026 08:08
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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of 00 (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix time regex

2 participants