Repository navigation
internal/escape: make HexEscape injective across escaped-rune boundaries - #3802
Conversation
HexEscape escapes a rune to "__0x<hex>__". Since google#3751 it also escapes any substring of the input that HexUnescape would parse as an escape sequence, but it never looked at sequences created in the output: a literal "__0x<hex>" (or "__0x<hex>_") right before an escaped rune is completed into a sequence by the "__" that the escaped rune emits. "__0x2f/" and "/0x2f__" therefore both escaped to "__0x2f__0x2f__", and HexUnescape(HexEscape("__0x2f/")) returned "/0x2f__". Two caller-chosen blob keys could still address the same object. Escape the leading "_" of any such dangling prefix as well, and re-check the newly escaped position. Add the collision pairs to TestHexEscapeInjective, an exhaustive round-trip test over short strings built from the pieces of an escape sequence, and a fuzz target.
Under -race the test took about 28s, and about 95s with -cover. Strings
up to length 7 still include both forms of dangling prefix ("__0x2/" and
"__0x2_/"), and over this alphabet the "non-alnum-not-_" rule escapes
exactly what "slash" does, so drop it. The test still fails without the
escape.go change, and now takes about 3s under -race.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the HexEscape function in internal/escape/escape.go to correctly handle dangling escape prefixes, preventing potential string collisions during unescaping. It also adds comprehensive tests, including exhaustive round-trip testing and fuzzing, to ensure correctness. The review feedback suggests optimizing the counting of escaped runes by maintaining a counter during the marking process instead of performing a separate O(N) loop over the escapeAt slice.
No behavior change; this drops the second pass over escapeAt.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3802 +/- ##
==========================================
+ Coverage 80.02% 80.05% +0.03%
==========================================
Files 104 104
Lines 12318 12337 +19
==========================================
+ Hits 9857 9877 +20
+ Misses 2460 2459 -1
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
#3751 made HexEscape escape any part of the input that HexUnescape would parse as an escape sequence. That does not cover sequences that only appear in the output: a literal
__0x<hex>(or__0x<hex>_) right before a rune that gets escaped is completed into an escape sequence by the__that the escaped rune emits. With a shouldEscape that escapes/:so two different inputs still escape to the same string.
HexEscape now also escapes the leading
_of such a dangling prefix (new helperdanglingEscapePrefix). That emits another__, so the newly escaped position is checked the same way. Inputs that do not contain this pattern are escaped exactly as before.Tests:
TestHexEscapeInjectivefor both forms; they fail without the change to escape.go.TestHexEscapeRoundTripExhaustivechecksHexUnescape(HexEscape(s)) == sfor every string of length <= 7 over{_, 0, x, 2, f, /}under three shouldEscape rules, which also shows HexEscape is injective on that set (about 0.4s, 3s with -race).FuzzHexEscapeRoundTripfor inputs of arbitrary shape.go test -race ./internal/escape/ ./blob/ ./blob/fileblob/ ./blob/gcsblob/ ./blob/s3blob/ ./blob/azureblob/ ./pubsub/awssnssqs/Fixes https://github.com/google/go-cloud/security/advisories/GHSA-m939-mf28-4wc3.