Skip to content

reject non-RFC3339 timestamp strings in timestamp() conversion #1338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alhudz wants to merge 4 commits into
base: master
Choose a base branch
from
+175 −1
Open

reject non-RFC3339 timestamp strings in timestamp() conversion #1338

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98e5600
reject non-rfc3339 timestamp strings in timestamp() conversion
alhudz Jun 10, 2026
a6f83c3
return early with a specific error when the timestamp format is invalid
alhudz Jun 12, 2026
ea57bf4
add benchmark for timestamp() string conversion
alhudz Jun 18, 2026
2e16e64
replace RFC 3339 regex gate with a hand-rolled scan
alhudz Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions common/types/string.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,14 @@ func (s String) ConvertToType(typeVal ref.Type) ref.Val {
return durationOf(d)
}
case TimestampType:
if t, err := time.Parse(time.RFC3339, s.Value().(string)); err == nil {
if t.Unix() < minUnixTime || t.Unix() > maxUnixTime {
return celErrTimestampOverflow
str := s.Value().(string)
if strictRFC3339Pattern.MatchString(str) {

@TristonianJones TristonianJones Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Prefer early return on an error for an invalid format as this will provide a more helpful message to the user (or agent).

@alhudz alhudz Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The pattern check now returns early with invalid RFC 3339 timestamp "<value>" rather than falling through to the generic conversion error, and the regression test asserts the exact message for each rejected form. go test ./... still passes.

@TristonianJones TristonianJones Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding a little benchmark to see the perf impact of the regex?

@alhudz alhudz Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added BenchmarkStringConvertToTimestamp over the conversion path. On an M4, converting a valid 2025-01-01T12:34:56.123456789Z runs around 600 ns/op at 1 alloc/op. The regex MatchString is the bulk of that (~500 ns/op, 0 allocs) against ~90 ns/op for the bare time.Parse, so the gate adds no allocations but does roughly 6x the parse cost. Happy to swap the regex for a small hand-rolled scan if that overhead matters on the hot path.

@TristonianJones TristonianJones Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you wouldn't mind a small hand-rolled scan. I looked into it a bit and it's doable, but roughly 100-ish lines ... seems easy to conformance test against the regex though, so maybe keep the regex for testing purposes.

@alhudz alhudz Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The conversion path now goes through a hand-rolled isStrictRFC3339 scan instead of the regex. Kept strictRFC3339Pattern as the reference and added TestIsStrictRFC3339MatchesPattern which asserts the scan and the regex agree across the field boundaries (hour/minute/second/offset edges, leap second, comma separator, single-digit fields, missing/empty zone).

The scan is allocation-free and roughly an order of magnitude cheaper than the regex MatchString on the same machine, so the gate no longer dominates the convert. go test ./... passes.

if t, err := time.Parse(time.RFC3339, str); err == nil {
if t.Unix() < minUnixTime || t.Unix() > maxUnixTime {
return celErrTimestampOverflow
}
return timestampOf(t)
}
return timestampOf(t)
}
case StringType:
return s
Expand Down
29 changes: 29 additions & 0 deletions common/types/string_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,35 @@ func TestStringConvertToType(t *testing.T) {
}
}

func TestStringConvertToTimestampStrict(t *testing.T) {
valid := []string{
"2025-01-17T01:00:00.001Z",
"2025-01-01T12:34:56Z",
"2025-01-01T12:34:56.123456789Z",
"2025-01-01T12:34:56+05:30",
"2025-01-01T12:34:56-08:00",
"2025-01-01T12:34:56+14:00",
}
for _, s := range valid {
if IsError(String(s).ConvertToType(TimestampType)) {
t.Errorf("String(%q).ConvertToType(TimestampType) errored, wanted a timestamp", s)
}
}
// RFC 3339 violations that time.Parse accepts loosely.
invalid := []string{
"2025-01-17T01:00:00,001Z", // ',' fractional separator
"2025-01-17T1:00:00Z", // single-digit hour
"2025-01-17T01:5:00Z", // single-digit minute
"2025-01-18T01:01:01.001+24:01", // offset hour out of range
"2025-01-17T01:01:01.001+00:60", // offset minute out of range
}
for _, s := range invalid {
if !IsError(String(s).ConvertToType(TimestampType)) {
t.Errorf("String(%q).ConvertToType(TimestampType) succeeded, wanted an error", s)
}
}
}

func TestStringEqual(t *testing.T) {
if !String("hello").Equal(String("hello")).(Bool) {
t.Error("Two equivalent strings were not equal")
Expand Down
10 changes: 10 additions & 0 deletions common/types/timestamp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package types
import (
"fmt"
"reflect"
"regexp"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -52,6 +53,15 @@ const (
maxUnixTime int64 = 253402300799
)

// strictRFC3339Pattern gates the strings accepted by the `timestamp()` overload.
// time.Parse accepts inputs that RFC 3339 forbids: a ',' fractional-second
// separator, single-digit time fields, and numeric offsets whose hours exceed
// 23 or minutes exceed 59. Those slip past unnoticed and shift the parsed
// instant, so they are rejected before time.Parse runs. The remaining calendar
// validation (month, day, leap year) is left to time.Parse.
var strictRFC3339Pattern = regexp.MustCompile(
`^\d{4}-\d{2}-\d{2}[Tt]([01]\d|2[0-3]):[0-5]\d:([0-5]\d|60)(\.\d+)?([Zz]|[+-]([01]\d|2[0-3]):[0-5]\d)$`)

// Add implements traits.Adder.Add.
func (t Timestamp) Add(other ref.Val) ref.Val {
switch other.Type() {
Expand Down