🐛 [RUM-0000] Fix cookie parsing losing pairs after a valueless segment - #4978
🐛 [RUM-0000] Fix cookie parsing losing pairs after a valueless segment#4978ofri-peretz wants to merge 2 commits into
Conversation
findCommaSeparatedValue and friends shared a regex that resumed matching on
the semicolon after a segment with no "=" in it, so the name came out as
";second" and every pair beyond that point became unreachable:
findCommaSeparatedValue("a=1;;b=2", "b") // undefined
findCommaSeparatedValue("noequals;foo=1", "foo") // undefined
Browsers produce those: document.cookie = "foo" stores a cookie with no
name, and reading document.cookie back gives a bare value with no "=". One
of those anywhere in the jar hid every cookie written after it, including
the SDK own session cookie.
The same pattern also backtracked quadratically, and document.cookie is
attacker-influenced on any site that lets a visitor set one:
8,000 chars 18.5ms
32,000 chars 274.0ms
128,000 chars 4,464.6ms
512,000 chars 69,608.6ms
Scanning by index has neither behaviour and is linear; the last row becomes
0.07ms.
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
…acking Adds eslint-plugin-secure-coding and enables no-redos-vulnerable-regex on packages/browser-core/src/tools/utils, the directory the previous commit fixed. Without a rule the next lazy quantifier written against document.cookie ships the same stall again. Scoped rather than repository-wide: tools/utils is clean under the rule today, while tools/stackTrace/computeStackTrace.ts:156 reports and deserves its own change. Pinned to 4.3.0, published nine days ago, so it clears the 7-day minimumReleaseAge in renovate.json rather than asking for an exception.
|
Added the regression guard, which the first push was missing.
Three things I checked before proposing a dependency:
The lockfile was updated with |
1 participant
Motivation
findCommaSeparatedValue,findAllCommaSeparatedValuesandfindCommaSeparatedValuesshare one regex:It has two problems, and the correctness one is the reason for this PR.
It stops finding cookies after a segment with no
=After such a segment the next match starts on the
;, so the captured name becomes;secondand every pair beyond that point is unreachable:Browsers produce those.
document.cookie = 'foo'stores a cookie with no name, and readingdocument.cookieback returns a bare value with no=. One of those anywhere in the jar hides every cookie written after it — including_dd_s, sincegetCookie()andgetInitCookie()both go through this function, as doesgetDocumentTraceId()fortrace-id/trace-time.It backtracks quadratically
(\S+?)expands one character at a time from every start position looking for an=, and thegflag repeats that from each index:document.cookieis attacker-influenced on any site that lets a visitor set a cookie, and this runs on the main thread on every session read.Changes
One
forEachCommaSeparatedValuehelper that walks the string by index —indexOf(';'),indexOf('='), slice — and the three public functions built on it. Linear, and the 512,000-character row above becomes 0.07 ms.Worth noting what is deliberately unchanged:
\s*=\s*trimmed whitespace around the separator but nothing trimmed the end of a value, sofoo = a ;yields'a '. The new code reproduces that exactly, and the existing white-space test pins it.Test plan
stringUtils.spec.tsverified against the new implementation, including the special-characters case (!#$%&'*+-.^_\|~),foo=a=b`, and the leading/trailing empty-value cases.a=1;;b=2,noequals;foo=1) and forfindAllCommaSeparatedValuesacross a valueless segment. Each fails on the old regex and passes on this one.The commit is SSH-signed per CONTRIBUTING; it may show as unverified until I finish registering the key with GitHub.
🤖 Generated with Claude Code