Skip to content

Only allow "T" or a space as the RFC 3339 separator - #85

Open
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:fix-rfc3339-separator
Open

Only allow "T" or a space as the RFC 3339 separator#85
youdie006 wants to merge 1 commit into
ruby:masterfrom
youdie006:fix-rfc3339-separator

Conversation

@youdie006

Copy link
Copy Markdown

The inconsistency

Time.xmlschema's pattern has a bare T (lib/time.rb:632). Time.rfc3339's
has [T\s] (lib/time.rb:661), so it also accepts a tab, newline, vertical
tab, form feed or carriage return between the date and the time.

RFC 3339 section 5.6 defines the separator as "T" and only notes that an
application may, for readability, use a space. The other \s characters are not
permitted.

What a caller sees on Ruby 3.2+

Since 95ab5fa, _xmlschema matches the pattern and then hands the string to
Time.new (lib/time.rb:673-678). Time.new rejects those five characters, so
it raises before the documented error can be reached — and because _xmlschema
raises rather than returning nil, the or raise at lib/time.rb:666-667
never runs and Time.new's internal message escapes:

Time.rfc3339("2011-10-05\t22:26:12Z")
# ArgumentError: "+HH:MM", "-HH:MM", "UTC" or "A".."I","K".."Z" expected for utc_offset: 22:26:12Z

That is the same class of problem as 69dffef ("Fix exception message for
Time.rfc3339 failures"), for a different input.

95ab5fa added time.upcase! at lib/time.rb:676 precisely because the /i
flag let the pattern accept lowercase t/z that Time.new rejects. [T\s]
is the same mismatch, one line up, for the separator.

The change

Narrow the class to [T ]. The pattern then agrees with the RFC, with
Time.xmlschema, and with what _xmlschema can actually parse, so these inputs
raise invalid rfc3339 format: as documented.

Time.rfc3339("2011-10-05 22:26:12Z") with a space keeps working.

The alternative, and why I did not pick it

The other way to close the gap is to keep [T\s] and normalize, adding
time.tr!("\t\n\v\f\r", " ") next to the existing upcase!. That preserves
what Ruby <= 3.1 accepted. I measured it, because b8c50d2 optimized this path
by ~4x and I did not want to hand back part of that silently.

Prebuilt trees, order alternated each rep, 30 reps of 600k Time.rfc3339 calls,
reporting min / p10 / median in ms:

min p10 median
master 402.23 415.97 433.20
this PR ([T ]) 398.37 409.36 433.70
the tr! alternative 526.27 532.33 562.25

This PR is noise-level (min -0.96%, median +0.12%). The tr! version costs
+28.9% at the min, about +197ns per parse, on every call including the
common T case. That did not seem like a good trade for accepting
tab-separated timestamps, so I went the other way. If you would rather keep the
lenient behaviour, the tr! one-liner is the whole change and I am happy to
swap.

Behaviour change, stated plainly

  • Ruby 3.2+: nothing that parsed before stops parsing. Those five inputs
    already raised ArgumentError; they now raise it with the documented message
    instead of a leaked internal one.
  • Ruby <= 3.1: the legacy branch (lib/time.rb:680) builds from the match
    captures and never calls Time.new, so it does currently parse those five.
    After this change it raises there too. That is the point — one pattern, so all
    supported Rubies agree.

Time.xmlschema is untouched and keeps rejecting every separator but T.

Tests

test_rfc3339_separator pins the class from both sides: T and space parse,
the five \s characters raise with /invalid rfc3339 format/, and
Time.xmlschema is asserted separately so a future change there cannot
silently drift.

Reverting to [T\s] fails on "\t". Over-tightening to a bare T fails the
space row. Widening by one to [T \t] fails on "\t".

Verification

TZ=UTC ruby -Ilib -Itest -Itest/lib -rhelper test/test_time.rb → 53 tests, 915
assertions, 0 failures, 0 errors. Identical without TZ set (this box is KST),
since the expectations are built with Time.utc.

The test uses only assert_equal, assert_raise, assert_match and string
interpolation, so it is valid back to the 2.6 floor in
ruby/actions/ruby_versions.yml, and it exercises the shared pattern rather
than either _xmlschema branch, so it is meaningful on every CI row.

Disclosure

I used an AI assistant to help find and prepare this change. I reviewed and
tested it myself, and the benchmark numbers above are measurements I ran.

Time.rfc3339's pattern used [T\s], so it accepted a tab, newline,
vertical tab, form feed or carriage return between the date and the
time. Time.xmlschema's pattern has a bare T.

RFC 3339 section 5.6 defines the separator as "T" and only notes that an
application may use a space for readability. The other \s characters are
not permitted by either.

Since Ruby 3.2 the pattern also disagrees with what the code does with
the string it matched. _xmlschema hands it to Time.new, which rejects
those five characters, so the documented ArgumentError never gets raised
and Time.new's internal message escapes instead:

  Time.rfc3339("2011-10-05\t22:26:12Z")
  #=> ArgumentError: "+HH:MM", "-HH:MM", "UTC" or "A".."I","K".."Z"
  #   expected for utc_offset: 22:26:12Z

Narrowing the class to [T ] makes the pattern agree with the RFC, with
Time.xmlschema, and with what _xmlschema can actually parse, so these
inputs now raise "invalid rfc3339 format:" as documented.
@jeremyevans

Copy link
Copy Markdown
Contributor

Thank you for the report.

Note that this would make Time.rfc3339 inconsistent with Date.rfc3339/DateTime.rfc3339 in that regard, as Date/DateTime allows the other whitespace characters. I think we have the following options:

  1. Only fix Time.rfc3339, as there are no backwards compatibility issues as it hasn't been released yet. Leave Date.rfc3339/DateTime.rfc3339 as they are for backwards compatibility.
  2. Fix Time.rfc3339 , Date.rfc3339, and DateTime.rfc3339, breaking backwards compatibility for Date.rfc3339/DateTime.rfc3339.
  3. Have Time.rfc3339 be consistent with Date.rfc3339/DateTime.rfc3339 and allow the other whitespace characters (no change).

@byroot What do you think we should do here? I lean toward option 2, with Date/DateTime issuing a warning for one Ruby version before changing it to Date::Error.

@youdie006

Copy link
Copy Markdown
Author

Thanks for looking at it so quickly, and that is a better framing than mine — I only compared against Time.xmlschema and did not weigh Date/DateTime.

Two facts that might help you and @byroot decide, both measured just now on ruby 3.2.3 with date 3.3.3:

Date/DateTime.rfc3339 accept exactly the same seven separators and nothing else. T, space, \t, \n, \v, \f, \r all parse; x, _, an empty separator, two spaces and two tabs all raise Date::Error. So it really is one \s character, i.e. the same rule [T\s] implements — the inconsistency you are pointing at is exact, not approximate.

Time.rfc3339 has never been released. It was added in 9bca339 (2026-04-30) and the last release is v0.4.2 (2025-12-17), which has no rfc3339 at all. So on the Time side options 1 and 2 both look free, and the compatibility question really is only about Date/DateTime.

One thing that is orthogonal to the choice: under option 3 there is still a bug to fix. Since 95ab5fa the 3.2+ _xmlschema hands the matched string to Time.new, which rejects those five characters, so the raise happens inside Time.new and the documented invalid rfc3339 format: from the or raise is never reached:

Time.rfc3339("2011-10-05\t22:26:12Z")
# ArgumentError: "+HH:MM", "-HH:MM", "UTC" or "A".."I","K".."Z" expected for utc_offset: 22:26:12Z

Keeping the lenient set means normalizing before Time.newtime.tr!("\t\n\v\f\r", " ") next to the existing upcase! is the whole change. I measured that too, since b8c50d2 optimized this path: prebuilt trees, order alternated per rep, 30 reps of 600k parses, it costs +28.9% at the min (~+197ns on every parse, including the common T case). That is the only reason I proposed tightening rather than normalizing, but it is a performance argument, not a correctness one, and it does not apply to options 1 or 2.

Happy to do whichever you land on. If it is option 2 I can prepare the Date/DateTime side with the one-version warning as well, and re-target this PR to match.

@byroot

byroot commented Sep 10, 2026

Copy link
Copy Markdown
Member

What do you think we should do here? I lean toward option 2, with Date/DateTime issuing a warning for one Ruby version before changing it to Date::Error.

I entirely agree with that.

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.

3 participants