Fix ReDoS vulnerability in FirebaseError template replacement#10146
Fix ReDoS vulnerability in FirebaseError template replacement#10146macastelaz wants to merge 7 commits into
Conversation
🦋 Changeset detectedLatest commit: a592930 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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 replaces the regex-based template replacement in replaceTemplate with a manual string-parsing loop inside a try-catch block. Feedback was provided regarding a bug in the error handling: if an exception occurs during parsing, the function would return an empty or partially constructed string. A code suggestion was provided to ensure the original template is returned as a fallback in the catch block.
| let result = template; | ||
| try { | ||
| let ptr = 0; | ||
| result = ''; | ||
| while (ptr < template.length) { | ||
| const start = template.indexOf('{$', ptr); | ||
| if (start === -1) { | ||
| result += template.substring(ptr); | ||
| break; | ||
| } | ||
| const end = template.indexOf('}', start + 2); | ||
| if (end === -1) { | ||
| result += template.substring(ptr); | ||
| break; | ||
| } | ||
| const key = template.substring(start + 2, end); | ||
| const value = data[key]; | ||
| result += template.substring(ptr, start) + (value != null ? String(value) : `<${key}?>`); | ||
| ptr = end + 1; | ||
| } | ||
| } catch (e) { | ||
| // Should never happen, but fallback just in case | ||
| } | ||
| return result; |
There was a problem hiding this comment.
If an exception is thrown during the template parsing (for example, if data is null/undefined, or if String(value) throws), the catch block does nothing, and the function returns result.
Since result is initialized to '' at the start of the try block, any error will cause the function to return an empty string or a partially constructed string, completely losing the error message template and making debugging extremely difficult.
We should use a local variable for the building process and return the original template in the catch block as a safe fallback.
try {
let ptr = 0;
let result = '';
while (ptr < template.length) {
const start = template.indexOf('{$', ptr);
if (start === -1) {
result += template.substring(ptr);
break;
}
const end = template.indexOf('}', start + 2);
if (end === -1) {
result += template.substring(ptr);
break;
}
const key = template.substring(start + 2, end);
const value = data[key];
result += template.substring(ptr, start) + (value != null ? String(value) : `<${key}?>`);
ptr = end + 1;
}
return result;
} catch (e) {
// Should never happen, but fallback just in case
return template;
}
Changeset File Check
|
1 participant
The Issue:
CodeQL correctly detected a Regular Expression Denial of Service (ReDoS) vulnerability.
In replaceTemplate, the template replacement logic used this regular expression to parse custom data variables:
const PATTERN = /{$([^}]+)}/g;
The + there is what we call "greedy." If the SDK received a malformed string lacking a closing } (e.g.,
{$aaaaaaaaaaaaaaaaaaaaaaaaaaaaa ), the regex engine would consume the whole string in [^}]+ , fail to match the
} , then repetitively backtrack one character at a time, resulting in an exponentially slow O(N²) freeze that
could block the main UI thread.
The Fix:
Bypass the regex engine completely. I implemented the exact same parsing logic in packages/util/src/errors.ts
using native, deterministic indexOf and substring pointer operations. This strictly bounds execution time to
O(N) linear time—making a ReDoS attack mathematically impossible.