Implement encoding.TextUnmarshaler in Digest#101
Implement encoding.TextUnmarshaler in Digest#101mtrmac wants to merge 1 commit intoopencontainers:masterfrom
encoding.TextUnmarshaler in Digest#101Conversation
... to ensure invalid values are rejected. Otherwise Go would allow setting a Digest value to arbitrary strings, causing a later panic or other misuse if users forget to call Validate(). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
stevvooe
left a comment
There was a problem hiding this comment.
LGTM
I'd like to get another opinion on this, as it can create bugs in other systems that delay validation after serialization.
|
This is definitely a breaking change, right? It's going to be hard to quantify exactly how widespread the breakage is, but it doesn't seem terribly unusual IMO for a project to have used this |
|
Yes, this is a tricky one; I wonder if this code could be used in situations where the consumer doesn't actually need it to be valid (I guess that's roughly what @tianon describes above, but in better words). Also not gonna lie that this module has quite some potential footguns, and I cursed a few times when we had code that didn't validate, and panicked after it turned out that code depended on I wonder if we can make overall use less error-prone, but this very likely would require a "v2". |
|
An alternative implementation could use just a As for users who store completely invalid strings while marshaling/unmarshaling |
| } | ||
|
|
||
| value, err := Parse(string(text)) | ||
| if err != nil { |
There was a problem hiding this comment.
We could ignore ErrDigestUnsupported here
|
An example invalid string I've personally stored and seen in these many times is the empty string, and I think that one is probably pretty common (although I don't know if it's also common to marshal that to/from JSON), but that case is already covered here. 😅 |
... to ensure invalid values are rejected.
Otherwise Go would allow setting a
Digestvalue to arbitrary strings, causing a later panic or other misuse if users forget to callValidate(). (e.g. containers/image#2403 , CVE-2024-3727 .)I don’t think adding this validation should break correct programs, although it can’t quite be ruled out. I did observe this breaking unit tests which were using unrealistic invalid
Digestvalues.