[net] internal/httpsfv: add support for consuming Display String and Date type
Nicholas Husin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
return s[:i+1], s[i+1:], trueI think this accepts a partial rune at the end of the string. If we're validating UTF-8 in this function, then we need to fail here if `runeLen > 0`.
I'm undecided on whether validating UTF-8 here is necessary. Perhaps this function should just look for the ending ", and we can use utf8.ValidString if/when the string is decoded?
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 1 comment![]( "Open in Gerrit")
I think this accepts a partial rune at the end of the string. If we're validating UTF-8 in this function, then we need to fail here if `runeLen > 0`.
I'm undecided on whether validating UTF-8 here is necessary. Perhaps this function should just look for the ending ", and we can use utf8.ValidString if/when the string is decoded?
Oops right, thanks! Fixed that and added a test case for it.
I'm undecided on whether validating UTF-8 here is necessary. Perhaps this function should just look for the ending ", and we can use utf8.ValidString if/when the string is decoded?
I'm not wedded to the idea, but I think validating here is probably still preferable. If we do not validate UTF-8 here, our behavior will diverge from the spec when someone uses `ParseDictionary()` on a dictionary that contains an invalid display string that they are not actively using, for example. In this case, I believe an RFC 9651-compliant parser would fail due to the invalid display string. On the other hand, our parser will happily ignore the display string if the user does not explicitly use `ParseDictionary()`.
Having this compliance check be done in the `consumeDisplayString()` does introduce a minor bit of duplicated work, where the `ParseDisplayString()` will have to re-decode the non-ASCII characters using `decOctetHex()` when constructing the output string (we won't have to use `utf8.ValidString` though since the `consumeDisplayString()` will have done the validation). But, that seems minor enough, and probably worth it to stay compliant with the spec and be able to fail early on bad input?
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil voted Code-Review+2![]( "Open in Gerrit")
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 1 comment![]( "Open in Gerrit")
return s[:i+1], s[i+1:], trueNicholas HusinI think this accepts a partial rune at the end of the string. If we're validating UTF-8 in this function, then we need to fail here if `runeLen > 0`.
I'm undecided on whether validating UTF-8 here is necessary. Perhaps this function should just look for the ending ", and we can use utf8.ValidString if/when the string is decoded?
Oops right, thanks! Fixed that and added a test case for it.
I'm undecided on whether validating UTF-8 here is necessary. Perhaps this function should just look for the ending ", and we can use utf8.ValidString if/when the string is decoded?
I'm not wedded to the idea, but I think validating here is probably still preferable. If we do not validate UTF-8 here, our behavior will diverge from the spec when someone uses `ParseDictionary()` on a dictionary that contains an invalid display string that they are not actively using, for example. In this case, I believe an RFC 9651-compliant parser would fail due to the invalid display string. On the other hand, our parser will happily ignore the display string if the user does not explicitly use `ParseDictionary()`.
Having this compliance check be done in the `consumeDisplayString()` does introduce a minor bit of duplicated work, where the `ParseDisplayString()` will have to re-decode the non-ASCII characters using `decOctetHex()` when constructing the output string (we won't have to use `utf8.ValidString` though since the `consumeDisplayString()` will have done the validation). But, that seems minor enough, and probably worth it to stay compliant with the spec and be able to fail early on bad input?
I'm convinced on validating here being useful.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin submitted the change![]( "Open in Gerrit")
Change information
internal/httpsfv: add support for consuming Display String and Date type
This CL adds consumeDisplayString() and consumeDate() function, meaning
that we can now consume all types that are defined within RFC 9651. In
future CL, we will add the corresponding parsing function for all the
types, so callers of this package will not have to implement their own
parsing / formatting.
For golang/go#75500
- M internal/httpsfv/httpsfv.go
- M internal/httpsfv/httpsfv_test.go
Code-Review: +2 by Damien Neil, +1 by Nicholas Husin
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |