Repository: amzn/ion-go
Pull request #50 (2020-06-03 00:51:37)

Invalid Ion Timestamps
Issue #44 
Fix malformed Ion timestamps.

Fixes include:
- Year, month, day must be larger than 0 (minimum date is 0001-01-01)
- Months cannot be larger than 12.
- Days, depending on which month, should be either 28,29,30 or 31.
- All different valid formats should be parsed correctly.
- Timestamps with Hour, must have minute too.
- Parse leap years properly.

This fix only aims invalid timestamps. There are still issues with timezone and fractional seconds which are not part of this fix (marked in the code as TODO)

The only outstanding invalid timestamp test in `malformedIonsSkipList` is `timestampLenTooLarge.10n`


codecov-commenter: # [Codecov](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=h1) Report
> Merging [#50](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-go/commit/c6d91a2c33946dec146c4946c614503a9f411482&el=desc) will **increase** coverage by `0.21%`.
> The diff coverage is `87.03%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-go/pull/50/graphs/tree.svg?width=650&height=150&src=pr&token=OBDT1TNoNE)](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   75.83%   76.05%   +0.21%     
==========================================
  Files          22       22              
  Lines        4573     4610      +37     
==========================================
+ Hits         3468     3506      +38     
+ Misses        681      680       -1     
  Partials      424      424              
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ion/textutils.go](https://codecov.io/gh/amzn/ion-go/pull/50/diff?src=pr&el=tree#diff-aW9uL3RleHR1dGlscy5nbw==) | `81.32% <84.44%> (+3.12%)` | :arrow_up: |
| [ion/bitstream.go](https://codecov.io/gh/amzn/ion-go/pull/50/diff?src=pr&el=tree#diff-aW9uL2JpdHN0cmVhbS5nbw==) | `78.35% <100.00%> (+0.31%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=footer). Last update [c6d91a2...70c21c6](https://codecov.io/gh/amzn/ion-go/pull/50?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Armanbqt: Thanks Zack and David for the review.
Updated the pull request.

Patch:
diff --git a/ion/bitstream.go b/ion/bitstream.go
index 276573b..88fa0e9 100644
--- a/ion/bitstream.go
+++ b/ion/bitstream.go
@@ -512,6 +512,13 @@ func (b *bitstream) ReadTimestamp() (time.Time, error) {
 		}
 		len -= vlen
 		ts[i] = int(val)
+
+		// When i is 3, it means we are setting hour component. A timestamp with
+		// hour, must follow by minute. Hence, len cannot be zero at this point.
+		if i == 3 && len == 0 {
+			return time.Time{},
+				&SyntaxError{"Invalid timestamp - Hour cannot be present without minute", b.pos}
+		}
 	}
 
 	nsecs, err := b.readNsecs(len)
@@ -522,8 +529,17 @@ func (b *bitstream) ReadTimestamp() (time.Time, error) {
 	b.state = b.stateAfterValue()
 	b.clear()
 
-	utc := time.Date(ts[0], time.Month(ts[1]), ts[2], ts[3], ts[4], ts[5], int(nsecs), time.UTC)
-	return utc.In(time.FixedZone("fixed", int(offset)*60)), nil
+	return tryCreateTimeWithNSecAndOffset(ts, nsecs, offset)
+}
+
+func tryCreateTimeWithNSecAndOffset(ts []int, nsecs int, offset int64) (time.Time, error) {
+	date := time.Date(ts[0], time.Month(ts[1]), ts[2], ts[3], ts[4], ts[5], nsecs, time.UTC)
+	// time.Date converts 2000-01-32 input to 2000-02-01
+	if ts[0] != date.Year() || time.Month(ts[1]) != date.Month() || ts[2] != date.Day() {
+		return time.Time{}, fmt.Errorf("ion: invalid timestamp")
+	}
+
+	return date.In(time.FixedZone("fixed", int(offset)*60)), nil
 }
 
 // ReadNsecs reads the fraction part of a timestamp and truncates it to nanoseconds.
diff --git a/ion/textutils.go b/ion/textutils.go
index e4a03de..7cfb872 100644
--- a/ion/textutils.go
+++ b/ion/textutils.go
@@ -9,6 +9,24 @@ import (
 	"time"
 )
 
+const (
+	// layoutMinutesAndOffset layout for time.date with yyyy-mm-ddThh:mm±hh:mm format. time.Parse()
+	// uses "2006-01-02T15:04Z07:00" explicitly as a string value to identify this format.
+	layoutMinutesAndOffset = "2006-01-02T15:04Z07:00"
+
+	// layoutMinutesZulu layout for time.date with yyyy-mm-ddThh:mmZ format. time.Parse()
+	// uses "2006-01-02T15:04Z07:00" explicitly as a string value to identify this format.
+	layoutMinutesZulu = "2006-01-02T15:04Z"
+
+	// layoutNanosecondsAndOffset layout for time.date of RFC3339Nano format.
+	// Such as 2006-01-02T15:04:05.999999999Z07:00.
+	layoutNanosecondsAndOffset = time.RFC3339Nano
+
+	// layoutSecondsAndOffset layout for time.date of RFC3339 format.
+	// Such as: 2006-01-02T15:04:05Z07:00.
+	layoutSecondsAndOffset = time.RFC3339
+)
+
 // Does this symbol need to be quoted in text form?
 func symbolNeedsQuoting(sym string) bool {
 	switch sym {
@@ -309,12 +327,12 @@ func parseTimestamp(val string) (time.Time, error) {
 	}
 
 	year, err := strconv.ParseInt(val[:4], 10, 32)
-	if err != nil {
+	if err != nil || year < 1 {
 		return invalidTimestamp(val)
 	}
 	if len(val) == 5 && (val[4] == 't' || val[4] == 'T') {
 		// yyyyT
-		return time.Date(int(year), 1, 1, 0, 0, 0, 0, time.UTC), nil
+		return tryCreateTimeDate(val, year, 1, 1)
 	}
 	if val[4] != '-' {
 		return invalidTimestamp(val)
@@ -331,7 +349,7 @@ func parseTimestamp(val string) (time.Time, error) {
 
 	if len(val) == 8 && (val[7] == 't' || val[7] == 'T') {
 		// yyyy-mmT
-		return time.Date(int(year), time.Month(month), 1, 0, 0, 0, 0, time.UTC), nil
+		return tryCreateTimeDate(val, year, month, 1)
 	}
 	if val[7] != '-' {
 		return invalidTimestamp(val)
@@ -348,33 +366,85 @@ func parseTimestamp(val string) (time.Time, error) {
 
 	if len(val) == 10 || (len(val) == 11 && (val[10] == 't' || val[10] == 'T')) {
 		// yyyy-mm-dd or yyyy-mm-ddT
-		return time.Date(int(year), time.Month(month), int(day), 0, 0, 0, 0, time.UTC), nil
+		return tryCreateTimeDate(val, year, month, day)
 	}
 	if val[10] != 't' && val[10] != 'T' {
 		return invalidTimestamp(val)
 	}
 
+	// At this point timestamp must have hour:minute
 	if len(val) < 17 {
 		return invalidTimestamp(val)
 	}
-	if val[16] != ':' {
-		return time.Parse("2006-01-02T15:04Z07:00", val)
+	if val[16] == 'z' || val[16] == 'Z' {
+		return time.Parse(layoutMinutesZulu, val)
+	}
+	if val[16] == '+' || val[16] == '-' {
+		if isValidOffset(val, 16) {
+			return time.Parse(layoutMinutesAndOffset, val)
+		}
+		return invalidTimestamp(val)
 	}
+	if val[16] == ':' {
+		//yyyy-mm-ddThh:mm:ss
+		if len(val) < 20 {
+			return invalidTimestamp(val)
+		}
+
+		idx := 19
+		if val[idx] == '.' {
+			idx++
+			for idx < len(val) && isDigit(int(val[idx])) {
+				idx++
+			}
+		}
 
-	if len(val) > 19 && val[19] == '.' {
-		i := 20
-		for i < len(val) && isDigit(int(val[i])) {
-			i++
+		if val[idx] == 'z' || val[idx] == 'Z' {
+			if idx >= 29 {
+				// Too much precision for a go Time.
+				// TODO: We should probably round instead of truncating? Ah well.
+				return time.Parse(layoutNanosecondsAndOffset, val[:29]+val[idx:])
+			}
+			return time.Parse(layoutSecondsAndOffset, val)
 		}
 
-		if i >= 29 {
-			// Too much precision for a go Time.
-			// TODO: We should probably round instead of truncating? Ah well.
-			return time.Parse(time.RFC3339Nano, val[:29]+val[i:])
+		if val[idx] == '+' || val[idx] == '-' {
+			if isValidOffset(val, idx) {
+				if idx >= 29 {
+					// Too much precision for a go Time.
+					// TODO: We should probably round instead of truncating? Ah well.
+					return time.Parse(layoutNanosecondsAndOffset, val[:29]+val[idx:])
+				}
+				return time.Parse(layoutSecondsAndOffset, val)
+			}
+			return invalidTimestamp(val)
 		}
 	}
+	return invalidTimestamp(val)
+}
+
+func tryCreateTimeDate(val string, year int64, month int64, day int64) (time.Time, error) {
+	date := time.Date(int(year), time.Month(month), int(day), 0, 0, 0, 0, time.UTC)
+	// time.Date converts 2000-01-32 input to 2000-02-01
+	if int(year) != date.Year() || time.Month(month) != date.Month() || int(day) != date.Day() {
+		return invalidTimestamp(val)
+	}
+	return date, nil
+}
+
+func isValidOffset(val string, idx int) bool {
+	// +hh:mm
+	if idx+5 > len(val) || val[idx+3] != ':' {
+		return false
+	}
+
+	hourOffset, errH := strconv.ParseInt(val[idx+1:idx+3], 10, 32)
+	minuteOffset, errM := strconv.ParseInt(val[idx+4:], 10, 32)
+	if errH != nil || errM != nil {
+		return false
+	}
 
-	return time.Parse(time.RFC3339Nano, val)
+	return hourOffset < 24 && minuteOffset < 60
 }
 
 func invalidTimestamp(val string) (time.Time, error) {


Test patch:
diff --git a/ion/integration_test.go b/ion/integration_test.go
index 607cc91..8a14252 100644
--- a/ion/integration_test.go
+++ b/ion/integration_test.go
@@ -180,13 +180,6 @@ var malformedIonsSkipList = []string{
 	"clob_u0020.ion",
 	"clob_u00FF.ion",
 	"clob_u01FF.ion",
-	"dateDaysInMonth_1.ion",
-	"dateDaysInMonth_2.ion",
-	"dateDaysInMonth_3.ion",
-	"dateDaysInMonth_4.ion",
-	"dateDaysInMonth_5.ion",
-	"day_1.ion",
-	"day_2.ion",
 	"decimalLenTooLarge.10n",
 	"decimalWithMultipleUnderscores.ion",
 	"decimalWithTrailingUnderscore.ion",
@@ -204,9 +197,6 @@ var malformedIonsSkipList = []string{
 	"invalidVersionMarker_ion_1234_0.ion",
 	"invalidVersionMarker_ion_1_1.ion",
 	"invalidVersionMarker_ion_2_0.ion",
-	"leapDayNonLeapYear_1.10n",
-	"leapDayNonLeapYear_1.ion",
-	"leapDayNonLeapYear_2.10n",
 	"localSymbolTableImportNegativeMaxId.ion",
 	"localSymbolTableImportNonIntegerMaxId.ion",
 	"localSymbolTableImportNullMaxId.ion",
@@ -217,18 +207,11 @@ var malformedIonsSkipList = []string{
 	"longStringRawControlCharacter.ion",
 	"minLongWithLenTooLarge.10n",
 	"minLongWithLenTooSmall.10n",
-	"month_1.ion",
-	"month_2.ion",
 	"negativeIntZero.10n",
 	"negativeIntZeroLn.10n",
 	"nopPadTooShort.10n",
 	"nopPadWithAnnotations.10n",
 	"nullDotCommentInt.ion",
-	"offsetHours_1.ion",
-	"offsetHours_2.ion",
-	"offsetMinutes_1.ion",
-	"offsetMinutes_2.ion",
-	"offsetMinutes_3.ion",
 	"sexpOperatorAnnotation.ion",
 	"stringLenTooLarge.10n",
 	"stringRawControlCharacter.ion",
@@ -246,37 +229,8 @@ var malformedIonsSkipList = []string{
 	"symbolIDUnmapped.10n",
 	"symbolIDUnmapped.ion",
 	"symbolLenTooLarge.10n",
-	"timestampHourWithoutMinute.10n",
 	"timestampLenTooLarge.10n",
-	"timestampSept31.10n",
-	"timestamp_0000-00-00.ion",
-	"timestamp_0000-00-00T.ion",
-	"timestamp_0000-00-01.ion",
-	"timestamp_0000-00-01T.ion",
-	"timestamp_0000-00T.ion",
-	"timestamp_0000-01-00.ion",
-	"timestamp_0000-01-00T.ion",
-	"timestamp_0000-01-01.ion",
-	"timestamp_0000-01-01T.ion",
-	"timestamp_0000-01T.ion",
-	"timestamp_0000-12-31.ion",
-	"timestamp_0000T.ion",
-	"timestamp_0001-00-00.ion",
-	"timestamp_0001-00-00T.ion",
-	"timestamp_0001-00-01.ion",
-	"timestamp_0001-00-01T.ion",
-	"timestamp_0001-00T.ion",
-	"timestamp_0001-01-00.ion",
-	"timestamp_0001-01-00T.ion",
-	"timestamp_10.ion",
-	"timestamp_11.ion",
-	"timestamp_5.ion",
-	"timestamp_6.ion",
-	"timestamp_7.ion",
-	"timestamp_8.ion",
-	"timestamp_9.ion",
 	"type_3_length_0.10n",
-	"year_3.ion",
 }
 
 var equivsSkipList = []string{