Repository navigation
Conversation
Cookie::parse stores the value text of the cookie string, and Cookie::appendTo percent-encoded every value. So toString() of a parsed cookie encoded the value a second time: "a=b%20c" became "a=b%2520c". Cookie::parse now marks the value as wire text. appendTo writes a marked value back as it is when it is an RFC 6265 cookie-value in which every "%" starts an escape, and percent-encodes every other value as before. The value setter clears the mark. cookie.value does not change.
|
Updated 1:37 PM PT - Nov 9th, 2026
✅ @robobun, your commit e80650d6d1420d638a216eb906c3227b80a4ab2c passed in 🧪 To try this PR locally: bunx bun-pr 48384That installs a local version of the PR into your bun-48384 --bun |
|
Reproduced on Bun 1.4.3 ( Bun.Cookie.parse("a=b%20c").toString();
// "a=b%2520c; Path=/; SameSite=Lax" (expected "a=b%20c; Path=/; SameSite=Lax")
const map = new Bun.CookieMap();
map.set(Bun.Cookie.parse("sid=s%3Aabc; Path=/; HttpOnly"));
map.toSetCookieHeaders();
// ["sid=s%253Aabc; Path=/; HttpOnly; SameSite=Lax"]On a build of this branch the two calls give PR: #48384 |
|
No actionable comments were generated in the recent review. WalkthroughParsed cookies now retain qualifying wire-format values when serialized. Assigned text and parsed values that do not meet the cookie-value conditions are percent-encoded. Documentation and tests cover serialization, ChangesCookie Serialization
Suggested reviewers: Merge Risk:
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked the mechanics of the new path and ruled them out: the % lookahead in isEncodedCookieValue is in bounds (i < size() holds before the subtraction), isCookieOctet rejects every code unit >= 0x7F so the Latin-1 and UTF-16 branches cannot pass non-ASCII through verbatim, and setValue is the only writer of m_value outside the constructor, so no copy or clone path carries a stale m_valueIsWireText. The test files were read, not run.
Extended reasoning...
The change adds a wire-text flag to Cookie set only by Cookie::parse and cleared by setValue, and makes appendTo emit a flagged value verbatim when it is an RFC 6265 cookie-value with well-formed %XX escapes; it touches the Set-Cookie output layer, so the guard is the security-relevant surface. The guard's bounds, encoding branches and flag lifecycle check out on reading; the posted findings concern the behavioral break for apps that round-trip parsed values, which is a design call for a maintainer.
The docs now say that the decoded text can differ from the value of the Cookie that was set, and that an escape that is not valid UTF-8 is read as U+FFFD. The table of values that toString() writes back as parsed gets a row with such an escape.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- 🔴
src/jsc/bindings/Cookie.cpp:253—Apps that set a parsed cookie with a %XX escape that is not UTF-8 (caf%E9, %fF) now read U+FFFD back from req.cookies.g…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The result was stored and tested by an empty branch that held a two-line comment. The call now stands alone with one line that says why it cannot fail.
Problem
toString()percent-encodes the value of a parsed cookie a second time.Bun.Cookie.parse("a=b%20c").toString()givesa=b%2520c; Path=/; SameSite=Lax.Cookie::parse(src/jsc/bindings/Cookie.cpp:76) keeps the value text.Cookie::appendTo(Cookie.cpp:242) runsencodeURIComponentover every value.Fix
Cookie::parsemarks the value as wire text, and thevaluesetter clears the mark.appendTowrites a marked value as it is when it is an RFC 6265 cookie-value with well-formed%XXescapes. It encodes every other value as before.cookie.valuedoes not change. Callers read it as wire text: the IntroduceCookieMap.toCookieRequestHeader()to simplify handling of cookies across multiplefetch()requests #18806 helper sends${name}=${value}in aCookieheader.test/js/bun/cookie/cookie.test.ts,cookie-map.test.tsandtest/js/bun/http/bun-serve-cookies.test.ts. 11 fail on main.Background
",,,;and\. The encoder is the only check on the value, so other values still use it.Cookie.parse(Percent-decode values in Bun.Cookie.parse #32960). It changescookie.value, and undici removed that decode as CVE-2026-9679.c.value = c.valuekeeps the 1.4.x bytes.Downsides
s:abc, CR LF for%0d%0a, U+FFFD forcaf%E9). 1.4.x returned the.valueit set.Cookieobject. A copy by value is still encoded twice:new Bun.Cookie(c.name, c.value),new Bun.Cookie(c).Cookie::parse+16 instructions per call (500 to 516 fora=b),toString()of a cookie that was not parsed +2..textgrows 768 bytes,sizeof(Cookie)stays 72.Notes
Reproduction (Bun 1.4.3, same code on main
2596ac5d39).In
Bun.serve,req.cookies.set(Bun.Cookie.parse(upstream))sendsconnect.sid=s%253Aabc.def%252Fghifor an upstreamconnect.sid=s%3Aabc.def%2Fghi.The rule.
toString(),serialize(),CookieMap#toSetCookieHeaders()and theBun.serveSet-Cookie writer all end inCookie::appendTo. It now has two cases.Bun.Cookie.parse(string)ornew Bun.Cookie(string), its value was not assigned since, and the value matchescookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )(RFC 6265 section 4.1.1) with two hex digits after every%: the value is appended as it is.encodeURIComponent, as on main.a=b%20ca=b%2520ca=b%20cconnect.sid=s%3Aabc.def%2Fghiconnect.sid=s%253Aabc.def%252Fghiconnect.sid=s%3Aabc.def%2Fghiu=%e8%af%bbu=%25e8%25af%25bbu=%e8%af%bbtok=YWJj+/==tok=YWJj%2B%2F%3D%3Dtok=YWJj+/==q="abc"q=%22abc%22q="abc"a=b ca=b%20ca=b%20ca=x,ya=x%2Cya=x%2Cya=caf\xE9a=caf%C3%A9a=caf%C3%A9a=100%a=100%25a=100%25a=b%20c da=b%2520c%20da=b%2520c%20dThe last row still gets one more layer. That happens for a value that has an escape and also a character that is not a cookie-octet, or a
%that does not start an escape (a=b%20c%,a=x%%20y,a=a%20b%zz). The output is a cookie-value with well-formed escapes, so the next hop keeps it. For a parsed value, 18 characters are no longer escaped:# $ & + / : < = > ? @ [ ] ^ { | }and the backtick.Why the guard. The name, the path and the domain are checked where a cookie is made. The value is not, because it was always escaped. Without the guard a parsed value with a control character, a comma, a quote, a backslash or a non-ASCII character would reach the header as it is.
The
%clause goes away with #44664 or #46528.Bun.CookieMapreads a%that does not start an escape as U+FFFD. So a parsed value with such a%stays on the encode path, andBun.CookieMapreads the text back. #44664 makes the reader keep that%, and #46528 carries the same change. This PR does not depend on either. When one of them lands, thetest.failingincookie.test.tsstarts to pass and turns red. The change to make then:%check inisEncodedCookieValue(Cookie.cpp:252-253),strayPercentPairsintokeptPairsincookie.test.tsand delete the two tests below the copies test,%condition fromcookies.mdxand from theserialize()JSDoc.Why
cookie.valuestays. An earlier draft of this change decoded the value inCookie::parse, as #32960 did. A review of that draft found that callers readcookie.valueas wire text. I ran the helper from #18806 unchanged over HTTP on that draft:pref=x%3B%20admin%3Dtruearrived aspref=x; admin=true,sid=s%3Aabc.def%2Fghiarrived assid=s:abc.def/ghi, and a%0D%0Avalue madefetchthrow. On 1.4.3 and on this PR all four pairs arrive unchanged. undici removed the same decode from its Set-Cookie parser in 6.27.0, 7.28.0 and 8.5.0 (GHSA-p88m-4jfj-68fv). RFC 6265 section 5.2 has no decoding step, and tough-cookie does not decode either.Scope: the mark follows the
Cookieobject. Of 19 ways to hand a parsed value back to the writer, 6 now writea=b%20c:toString(),serialize()and a template literal on a cookie fromBun.Cookie.parse(string)ornew Bun.Cookie(string), andmap.set(cookie)for both. A cookie string also gets the mark back:Bun.Cookie.parse(cookie.toString())and a cookie made fromname=valuetext write like their source. The other 13 still writea=b%2520c, as 1.4.3 does, because they pass the value as text:new Bun.Cookie(c.name, c.value),Bun.Cookie.from(c.name, c.value),map.set(c.name, c.value),c.value = c.valueand a value read from aCookieMap.new Bun.Cookie(c.toJSON())andmap.set(c.toJSON()).new Bun.Cookie(c). The constructor reads aCookiethroughCookieInit::fromJS, the way it reads any object.Cookie::parsestays the only producer of the mark. I did not add a copy rule to the constructor.Tests pin both groups. The docs and the JSDoc state the rule per entry point.
What else does not change, on purpose.
Bun.Cookie.parse(x).valuestays different fromnew Bun.CookieMap(x).get(name)for a value with an escape (b%20candb c). Which of the two a parsed cookie should hold is a decision for a maintainer. This PR does not make it, and the docs do not promise either.toString()are not a round trip for the attributes, on main and here.Cookie::parsefillsPath=/andSameSite=Laxand drops the attributes that it does not know. The docs now say so.CookieMapkeeps one cookie for each name. To send several parsed cookies with one name, useheaders.append("Set-Cookie", cookie.toString()). The docs name both ways.Cookie::fieldsEqual. The mark is not a field that a user can read, so it stays out of that comparison.The breaking part. A server that sets a parsed cookie and reads it on the next request now gets the decoded text from
req.cookies.get():s:abcfor a cookie that was parsed froms%3Aabc. On 1.4.x it getss%3Aabc, because the cookie went out ass%253Aabc. An escape that is not UTF-8 (caf%E9) reads back as U+FFFD, as it does for any cookie that a client sends in that form. Escapes for control characters and separators read back decoded too:%0d%0agives CR LF, and%3B%20Securegives; Secure. The header holds only the escapes, and a cookie that is made from the text that was read is encoded again (a=%0D%0A). Of ten probe rows, five read back different from the.valuethat was set (one decoded, four with U+FFFD). On 1.4.3 none does.c.value = c.valueornew Bun.Cookie(c.name, c.value)gives the 1.4.x bytes and read-back. I found no public code that doescookies.set(Bun.Cookie.parse(...))on a server. #32926 changed theExpiresbytes ofBun.Cookie: it merged as a plain fix, shipped in 1.4.0 and was added to the list in #28792 afterwards.Escapes that are not UTF-8. A parsed
caf%E9is a cookie-value with well-formed escapes, so it is written back as parsed. On the next requestBun.CookieMapreads it ascafplus U+FFFD, where 1.4.x readcaf%E9back. The other choice is to keep such a value on the encode path. Then a server readscaf%E9back, and the cookie leaves ascaf%25E9. A client jar that savestoSetCookieHeaders()and parses the saved lines again (BingyanStudio/hust-sdk does this) then sendscaf%25E9to the issuer. With this PR it sendscaf%E9after any number of save and load cycles. On 1.4.3 it sendscaf%2525E9after three. I kept the forward, because these values come from other issuers. A reader that keeps a value raw when its escapes are not UTF-8 would give both. That is a decision aboutBun.CookieMap, and the notes of #44664 name it.Measurements. Release builds of main
2596ac5d39and of this PR from one build directory. Instructions aregdbstepicounts of the 50th call.valgrind,perfandbloatyare not installed here.sizeof(WebCore::Cookie): 72 to 72 bytes release, 80 to 80 bytes debug (static_assert). Theboolsits in padding afterm_partitioned.Cookie::toString, cookie fromnew Bun.Cookie(name, value)(a=b/ 19 characters / value with escapes): 1057 / 1984 / 1716 to 1059 / 1986 / 1718 instructions.Cookie::toString, parsed cookie: 19 characters 1984 to 1668, express-session value 1995 to 1732, 4 KB valid value 109,093 to 76,550, value with one character outside the set 1610 to 1747, 4 KB value that fails at the last character 109,107 to 182,820. The guard is one scan before one append. A value that fails the guard is read a second time by the encoder: 14 of 75 characters on ten probe values.Cookie::toString(constructeda=b/ 19 characters / with escapes, parsed 19 characters / with escapes): 4 / 7 / 6 / 7 / 6 to 4 / 7 / 6 / 6 / 6.Cookie::parse(a=b/ 19 characters /a=b%20c/ with Path and HttpOnly): 500 / 560 / 533 / 2054 to 516 / 576 / 549 / 2071 instructions, 4 allocations in both.Cookie::create: 137 to 138. Thevaluesetter: 128 to 128.llvm-size -A,llvm-nm -S):.text+768.Cookie::appendTo1444 to 2165,Cookie::parse7228 to 7386,Cookie::create890 to 894,Cookie::toString285 to 285,JSC::encodeURIComponent1468 to 1468,CookieMap::create5096 to 5096.Cookie.handCookie.cpp. No host function, no table row and no new API.parse(pair).toString()gives back the pair for 7 of 10 probe pairs (1.4.3: 2).${name}=${value}gives back the pair for 10 of 10 (1.4.3: 10). A client jar that parses each Set-Cookie and callscookieMap.set(cookie)(BingyanStudio/hust-sdk,auth/cookie-manager.ts) writes back 6 of 8 upstream cookie shapes unchanged (1.4.3: 2). The IntroduceCookieMap.toCookieRequestHeader()to simplify handling of cookies across multiplefetch()requests #18806 helper is byte-exact for 4 of 4 (1.4.3: 4).node:httpserver that writestoSetCookieHeaders()for parsed values with 0x01 and 0x7F answers 200 on both builds. A parsedcaf\xE9leavesBun.serveascaf%C3%A9on both. The Set-Cookie bytes for a parsed value with 0x01, a tab, 0x7F, U+00E9, a quote, a space, a comma and a backslash are identical, read from a raw socket.Tests.
test/js/bun/cookie/cookie.test.ts: 19 new. A table of 14 pairs that are written back as parsed and a table of 24 pairs that keep the bytes of 1.4.3, each throughBun.Cookie.parse,new Bun.Cookie(string)and a subclass, from an 8-bit and from a 16-bit string. Then the fixed point after a second and a third parse, attribute setters, thevaluesetter, the copies from a cookie string, the copies by value, the read-back of a stray%throughBun.CookieMap, and thetest.failingthat watches theBun.CookieMapreader.test/js/bun/cookie/cookie-map.test.ts: 2 new.map.set(cookie)withtoSetCookieHeaders(), andmap.set(name, value).test/js/bun/http/bun-serve-cookies.test.ts: 1 new.req.cookies.set(Bun.Cookie.parse(upstream))over HTTP, the unusual value,req.clone().cookies, and the read on the next request. TCP, TLS, HTTP/2 and HTTP/3 share one writer template (CookieMap.cpp:17-53), and the test uses TCP.appendTo, the clear in the setter, the%look-ahead, the quote pair, a wider character set, the mark inparse.test/js/bun/cookie/,test/js/bun/util/cookie.test.js,test/js/bun/http/bun-serve-routes.test.ts,test/js/bun/http/decodeURIComponentSIMD.test.ts,test/js/web/fetch/cookies.test.ts,test/regression/issue/22475.test.tsandtest/bake/dev/request-cookies.test.ts. With the three files above that is 620 pass and 0 fail in 12 files.test/integration/bun-types/bun-types.test.tspasses 30 of 30. Linux x64 only.Self-review. 11 concerns, all taken.
.valueholds are gone. "The attributes are written as they are" was false and is now "does not percent-encode the name,DomainorPath". The rule is stated once, with the character set and the RFC section.CookieMap, and what parse does to the attributes.new Bun.Cookie(cookie)is pinned by a test and listed above. I chose to pin it and not to carry the mark through the constructor: a copy rule there would depend on the content of the argument.req.clone().cookieshas a test row.%clause is tied to Bun.serve: keep a%that is not an escape in route params and cookies #44664 and http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528 by a test and by the list above.[human-review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file