Skip to content

fix(a2a-core): eliminate ReDoS in Message code-block parsing#9374

Open
rllyy97 wants to merge 2 commits into
mainfrom
rllyy97-fix-code-scanning-alerts-86-87
Open

fix(a2a-core): eliminate ReDoS in Message code-block parsing#9374
rllyy97 wants to merge 2 commits into
mainfrom
rllyy97-fix-code-scanning-alerts-86-87

Conversation

@rllyy97

@rllyy97 rllyy97 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Fixes two js/polynomial-redos code scanning alerts (#86 and #87) in libs/a2a-core/src/react/components/Message/Message.tsx.

Both alerts flagged lazy fenced-code-block regexes applied to untrusted message.content:

  • handleDownload: /```[\w]*\n([\s\S]*?)\n```/
  • processedContent: /```(\w*)\n([\s\S]*?)\n```/g

The lazy ([\s\S]*?) body scans to the end of the string from every ``` start position when the closing fence is missing. For input that starts with ```\n and repeats ```\na many times, this is O(n²) — a denial-of-service risk on agent/library-supplied content.

The fix introduces a single module-level extractFencedCodeBlocks(content) helper that finds fenced code blocks in one linear pass using String.indexOf (no backtracking regex), faithfully reproducing the original matching semantics (opening fence + word-character info string + \n, code up to the first closing \n + ```, and identical handling of unterminated / non-word-language fences). Both call sites now use the helper; behavior is unchanged for all legitimate inputs and the quadratic worst case is gone.

Impact of Change

  • Users: No behavior change for valid content; the A2A chat message renderer is no longer vulnerable to a ReDoS hang on malicious/malformed code-fence input.
  • Developers: New exported extractFencedCodeBlocks() helper in the Message module (exported for unit testing). No public API change to the Message component.
  • System: Fenced-code-block extraction is now O(n) instead of O(n²) worst case, removing a denial-of-service vector. No new dependencies.

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: pnpm --filter @microsoft/logic-apps-chat exec vitest run src/react/components/Message141 passed (3 skipped); pnpm --filter @microsoft/logic-apps-chat run typecheck → clean; no new Biome/ESLint diagnostics vs. main

Added direct unit tests for extractFencedCodeBlocks covering single/multiple blocks, no-language blocks, unterminated fences, non-word info strings, lazy-first-close parity, and a ReDoS guard that runs the exact CodeQL-flagged pathological input (200k fence repetitions) and asserts it completes in linear time.

Contributors

Co-authored-by: Copilot

Screenshots/Videos

N/A — no visual changes.

Replace the two lazy fenced-code-block regexes in Message.tsx with a linear
String.indexOf scanner. The regexes ran in polynomial time
(js/polynomial-redos, code scanning alerts 86 and 87) on untrusted message
content containing many unterminated code fences.

Add extractFencedCodeBlocks() plus unit tests covering extraction parity and a
ReDoS guard for the pathological input CodeQL flagged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 7, 2026 17:24
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(a2a-core): eliminate ReDoS in Message code-block parsing
  • Issue: None — the title is specific, concise, and clearly describes the scope and intent.
  • Recommendation: No change needed.

Commit Type

  • Properly selected (fix).
  • Only one commit type is selected, which matches the template requirement.

Risk Level

  • Properly selected (Low) and it matches the risk:low label on the PR.

What & Why

  • Current: Detailed and clear explanation of the ReDoS issue, affected code, and rationale for the fix.
  • Issue: None.
  • Recommendation: No change needed.

Impact of Change

  • The impact section is well filled out and aligned with the diff.
  • Recommendation:
    • Users: Correctly states no behavior change for valid content and improved safety.
    • Developers: Correctly mentions the exported helper and no public API change to the component.
    • System: Correctly notes the O(n) improvement and removal of the DoS vector.

Test Plan

  • Unit tests are added/updated in the diff, which satisfies the test-plan requirement.
  • The added tests cover the new helper and the rendering path, including a ReDoS guard.
  • No E2E tests are required because unit tests are present.

Contributors

  • Co-authored-by: Copilot is present.
  • This section is optional, so there is no issue.

Screenshots/Videos

  • Marked as N/A and the change is non-visual.
  • No screenshots are required.

Summary Table

Section Status Recommendation
Title No changes needed
Commit Type No changes needed
Risk Level No changes needed
What & Why No changes needed
Impact of Change No changes needed
Test Plan No changes needed
Contributors Optional; already present
Screenshots/Videos Optional; N/A is appropriate

This PR passes review for title and body compliance.


Last updated: Tue, 07 Jul 2026 20:31:46 GMT

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

📊 Coverage Check

The following changed files need attention:

⚠️ libs/a2a-core/src/react/components/Message/Message.tsx - 79% covered (needs improvement)

Please add tests for the uncovered files before merging.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses CodeQL js/polynomial-redos findings in the chat Message renderer by replacing lazy fenced-code-block regex parsing on untrusted message.content with a linear-time scanner.

Changes:

  • Added extractFencedCodeBlocks(content) to parse fenced code blocks via String.indexOf in a single pass.
  • Updated artifact download and markdown rendering paths to use the new extractor instead of regexes.
  • Added unit tests for the extractor, including a pathological-input guard to prevent regression to polynomial-time behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
libs/a2a-core/src/react/components/Message/Message.tsx Replaces regex-based fenced code block extraction with a linear scan helper and updates call sites.
libs/a2a-core/src/react/components/Message/tests/Message.codeblock.test.tsx Adds extractor-focused unit tests and a ReDoS regression guard test.

const elapsed = performance.now() - start;

expect(blocks).toEqual([]);
expect(elapsed).toBeLessThan(1000);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in f5bb10e. I removed the absolute wall-clock assertion (elapsed < 1000ms) and replaced it with a machine-speed-independent scaling-ratio check.

The guard now times a 10x-larger pathological input and asserts largeTime / smallTime stays below a generous 30x ceiling. A linear scan grows ~10x with 10x input; the old O(n²) regex would grow ~100x. Asserting the ratio rather than absolute time keeps the test stable on slow/contended CI runners while still catching polynomial blow-up. It also warms up the JIT and averages over 20 iterations to reduce noise.

Ran it 3x locally — all green (14/14 each run).

@rllyy97 rllyy97 added the risk:low Low risk change with minimal impact label Jul 7, 2026
…akiness

Replace the absolute wall-clock assertion (elapsed < 1000ms) in the ReDoS
regression guard with a machine-speed-independent scaling-ratio check. The
test now compares per-call time for a 10x larger pathological input and
asserts the ratio stays well below quadratic growth, so it stays green on
slow or contended CI runners while still catching polynomial blow-up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:low Low risk change with minimal impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants