fix(security): polynomial ReDoS in comboAgentMiddleware regex → main (CodeQL #612/#613) - #3983
Conversation
…ex on main (CodeQL js/polynomial-redos #612/#613) Brings the release/v3.8.27 fix (#3982) to main so CodeQL alerts #612/#613 close on the next scan. Code + regression test only; the [3.8.27] CHANGELOG bullet lives on release/v3.8.27 and reaches main when v3.8.27 ships (identical file → no merge conflict). Detection pattern drops the unbounded surrounding newline run; global strip pattern bounds it ({0,16}). Behavior unchanged (107 related tests green).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 addresses a polynomial ReDoS vulnerability by removing unbounded newline runs from the CACHE_TAG_PATTERN regex and bounding them to {0,16} in CACHE_TAG_PATTERN_GLOBAL. It also adds regression tests to verify linear performance and correct tag stripping. Feedback on the changes highlights a potential bug in the global stripping pattern: consuming up to 16 newlines on both sides of the tag can cause adjacent text (such as chat history lines) to merge, corrupting the structure. A suggestion is provided to only consume newlines on the trailing side of the tag and to update the regression tests to assert that paragraph breaks are preserved.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const CACHE_TAG_PATTERN_GLOBAL = | ||
| /(?:\\n|\n|\r){0,16}<omniModel>([^<]+)<\/omniModel>(?:\\n|\n|\r){0,16}/g; |
There was a problem hiding this comment.
⚠️ Potential Chat History Corruption / Word Merging Bug
By matching and stripping up to 16 newlines both before and after the <omniModel> tag, any text preceding and succeeding the tag will be merged together.
For example, in an inlined chat history (like the Open WebUI follow-up requests mentioned in the comments), you might have:
Assistant: Hello!
<omniModel>gpt-4</omniModel>
User: How are you?With the current regex, the \n before and \n\n after the tag are both consumed and replaced with "", resulting in:
Assistant: Hello!User: How are you?This merges the assistant's message with the user's message, corrupting the chat history structure sent to the provider.
Suggested Fix
To avoid merging words/lines while still cleaning up the tag and its surrounding blank lines, we should only consume the newline run on one side (e.g., the trailing side). Any leading newlines before the tag will either act as the single natural separator between the preceding and succeeding text, or (if the tag was at the very end of the message) will be cleanly stripped by the subsequent .trimEnd() call.
| const CACHE_TAG_PATTERN_GLOBAL = | |
| /(?:\\n|\n|\r){0,16}<omniModel>([^<]+)<\/omniModel>(?:\\n|\n|\r){0,16}/g; | |
| const CACHE_TAG_PATTERN_GLOBAL = | |
| /<omniModel>([^<]+)<\/omniModel>(?:\\n|\n|\r){0,16}/g; |
| test("stripModelTags removes the newline run wrapping a tag (no blank line left)", () => { | ||
| const out = String( | ||
| stripModelTags([{ role: "user", content: "before\n\n<omniModel>a/b</omniModel>\n\nafter" }])[0] | ||
| .content | ||
| ); | ||
| assert.ok(!out.includes("<omniModel>"), "tag removed"); | ||
| assert.ok(!out.includes("\n\n\n"), "no triple newline left from stripping"); | ||
| }); |
There was a problem hiding this comment.
🧪 Update Regression Test to Prevent Word Merging
Let's update the regression test to assert that the paragraph break is preserved and words are not merged when stripping the tag.
test("stripModelTags removes the newline run wrapping a tag (no blank line left)", () => {
const out = String(
stripModelTags([{ role: "user", content: "before\n\n<omniModel>a/b</omniModel>\n\nafter" }])[0]
.content
);
assert.ok(!out.includes("<omniModel>"), "tag removed");
assert.ok(!out.includes("\n\n\n"), "no triple newline left from stripping");
assert.equal(out, "before\n\nafter", "should preserve the paragraph break and not merge words");
});
CI Coverage Report
Coverage artifact was not available for this run. PR Test PolicyThis PR changes production code in |
|
…iegosouzapw#3983) Brings the release/v3.8.27 fix (diegosouzapw#3982) to main so CodeQL alerts diegosouzapw#612/diegosouzapw#613 close on the next scan. Code + regression test only; the [3.8.27] CHANGELOG bullet lives on release/v3.8.27 and reaches main when v3.8.27 ships (identical file → no merge conflict). Detection pattern drops the unbounded surrounding newline run; global strip pattern bounds it ({0,16}). Behavior unchanged (107 related tests green).
…iegosouzapw#3983) Brings the release/v3.8.27 fix (diegosouzapw#3982) to main so CodeQL alerts diegosouzapw#612/diegosouzapw#613 close on the next scan. Code + regression test only; the [3.8.27] CHANGELOG bullet lives on release/v3.8.27 and reaches main when v3.8.27 ships (identical file → no merge conflict). Detection pattern drops the unbounded surrounding newline run; global strip pattern bounds it ({0,16}). Behavior unchanged (107 related tests green).
…iegosouzapw#3983) Brings the release/v3.8.27 fix (diegosouzapw#3982) to main so CodeQL alerts diegosouzapw#612/diegosouzapw#613 close on the next scan. Code + regression test only; the [3.8.27] CHANGELOG bullet lives on release/v3.8.27 and reaches main when v3.8.27 ships (identical file → no merge conflict). Detection pattern drops the unbounded surrounding newline run; global strip pattern bounds it ({0,16}). Behavior unchanged (107 related tests green).
1 participant
Hotfix to
mainof the ReDoS fix already merged to release/v3.8.27 (#3982), so CodeQL js/polynomial-redos alerts #612/#613 close on the next main scan instead of waiting for the v3.8.27 ship. Code + regression test only (the [3.8.27] CHANGELOG bullet stays on release/v3.8.27; the file is identical on both branches so the v3.8.27 release merge is conflict-free). 107 related comboAgent tests green.