Skip to content
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.

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

feature: Allow decoding a byte string into time.Time value #497

Closed
ssuriyan7 opened this issue Feb 24, 2024 · 8 comments
Closed

feature: Allow decoding a byte string into time.Time value #497

ssuriyan7 opened this issue Feb 24, 2024 · 8 comments

Comments

@ssuriyan7
Copy link
Contributor

ssuriyan7 commented Feb 24, 2024

Is your feature request related to a problem? Please describe.
Currently, when cbor.Unmarshal is called with a byte string data item and time.Time as destination type, it causes UnmarshalTypeError with message "cbor: cannot unmarshal byte string into Go value of type time.Time". I would like this operation to be allowed.

Describe the solution you'd like
In parseToTime() method of decoder, if the decoded content is of type []byte, convert the content to string and parse it with time.Parse(). One thing to note: parseToTime() is used for decoding tag 0 & 1 data items. As per https://www.rfc-editor.org/rfc/rfc8949.html#tags, allowed types for a tag 0 data item is text string, and int or float for tag 1. So, parseToTime() should raise UnmarshalTypeError for a tagged item with a content of type []byte.

Additional context
I'd be happy to contribute the changes needed for this feature.

@fxamacker
Copy link
Owner

Currently, when cbor.Unmarshal is called with a byte string data item and time.Time as destination type, it causes UnmarshalTypeError with message "cbor: cannot unmarshal byte string into Go value of type time.Time". I would like this operation to be allowed.

In parseToTime() method of decoder, if the decoded content is of type []byte, convert the content to string and parse it with time.Parse(). One thing to note: parseToTime() is used for decoding tag 0 & 1 data items. As per https://www.rfc-editor.org/rfc/rfc8949.html#tags, allowed types for a tag 0 data item is text string, and int or float for tag 1. So, parseToTime() should raise UnmarshalTypeError for a tagged item with a content of type []byte.

@ssuriyan7 Thanks for opening this feature request. I'd like to confirm my understanding and get more info.

First, to confirm my understanding:

RFC8949 allows date/time to encode to CBOR data items of type:

  • CBOR integers (major types 0 and 1)
  • CBOR float (major type 7)
  • CBOR text string (major type 3)

Given this, cbor.Unmarshal() forbids decoding CBOR byte string (major type 2) directly totime.Time.

You'd like cbor.Unmarshal() to allow decoding untagged CBOR byte string (major type 2) to time.Time.

Can you share more context?

  • What are the use cases that require allowing this forbidden operation?
  • Does your proposed solution change the default behavior of cbor.Unmarshal() to allow this operation?
  • Are there any security considerations or potential vulnerabilities related to allowing this operation?

Thanks!

@benluddy
Copy link
Contributor

benluddy commented Feb 27, 2024

I share the use case for this or a similar proposal.

Clients can be compiled without knowledge of the application-specific struct types used by a server. The same client and server also support a text representation (i.e. JSON). Since time.Time implements json.Marshaler, if the server marshals a time.Time field to JSON, that field is represented as a JSON string containing an RFC 3339 timestamp. When the client unmarshals from JSON, it doesn't know that this particular JSON string originated from a time.Time.

If the client then takes the object it received via JSON and tries to marshal to CBOR, it can't marshal the timestamp to a tag-0-enclosed text string. It can only marshal to an untagged string. In my case, we're using the StringToByteString encode option so the CBOR type is byte string. Other client implementations could encode to text string, and we would still want the server to able to decode that into time.Time. So we would like to be able to decode either byte strings or text strings to time.Time.

I recognize that this use case isn't universal, and that typically both the encode and decode sides will have been compiled with knowledge of some version of the same Go types, so it makes sense to me that any approach that would allow this behavior would require opt-in via a decode option. I wonder if, since time.Time implements TextUnmarshaler and tag 0 support is built-in, you'd support an option that decodes untagged CBOR text string to time.Time using its UnmarshalText implementation? Doing the same for either byte or text string could be a second option value, or possibly byte string support could be dependent on the setting of ByteStringToStringMode (I'm not very enthusiastic about intersecting options in that way, but I think it's worth mentioning)? WDYT?

@benluddy
Copy link
Contributor

@ssuriyan7 just let me know that text string to time.Time is already allowed, d'oh! On second look, I see that the implementation of parseToTime is essentially "decode into interface{}, then type assert to map back to Time". This means that any option affecting decode to interface{} can have side effects on decodes into time.Time. Is that desired? My use case is satisfied when the decode option DefaultByteStringType is string, which it always is.

@fxamacker
Copy link
Owner

@benluddy Thanks for providing context and use case.

On second look, I see that the implementation of parseToTime is essentially "decode into interface{}, then type assert to map back to Time". This means that any option affecting decode to interface{} can have side effects on decodes into time.Time. Is that desired?

Good catch! This is an unintended side affect from implementation details. I'll open an issue to decouple parseToTime from other decoding options.

I recognize that this use case isn't universal, and that typically both the encode and decode sides will have been compiled with knowledge of some version of the same Go types, so it makes sense to me that any approach that would allow this behavior would require opt-in via a decode option

I agree! 👍 Maybe we can add a decoding option for decoding CBOR byte string into time.Time.

@ssuriyan7 Thoughts? Also, would you still be interested in opening a PR for this? 🙏

@ssuriyan7
Copy link
Contributor Author

Yes, I will open a PR for the new decoding option.

@ssuriyan7
Copy link
Contributor Author

Waiting for #503 to merge as this option should be implemented on top of that change.

@fxamacker
Copy link
Owner

Waiting for #503 to merge as this option should be implemented on top of that change.

Thanks for letting me know! I reviewed the PR tonight:

@fxamacker
Copy link
Owner

Thanks Ben & Suriyan! 🎉 Closed by #524.

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

No branches or pull requests

3 participants