fix(python): eliminate ReDoS in chat parser role-boundary regex

[Sadok Barbouche (cr-sbarbouche)]

Summary

• rewrite the role-boundary attribute regex in PromptyChatParser to remove catastrophic backtracking on unquoted, unterminated attributes
• replace the ambiguous \"?[^\"]*\"? value class with a quoted/unquoted alternation that shares no characters with the separator or closing bracket, plus possessive quantifiers (Python 3.11+ floor)
• add a regression test asserting bounded runtime on a ~24-repetition adversarial input

Matches the fix already applied to the Java parser in #444; the TypeScript half of the report is left for a separate PR to keep this one scoped.

Fixes #446

Test plan

• pytest tests/test_parsers.py -v — 24 passed, including the new ReDoS regression test
• verified functional parity between old and new regex against the issue's own positive/negative cases (system:, # assistant:, user[name="Alice"]:, user[name=Bob, id=7]:, notarole:)
• confirmed the reported adversarial payload ("user[" + "a=b," * n + "!") resolves in <1ms at n=24 instead of exploding exponentially
• ruff check / ruff format --check clean on both changed files

[Sadok Barbouche (cr-sbarbouche)]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Eliminates a potential ReDoS vector in the Python chat role-boundary parser regex and adds a regression test to ensure runtime stays bounded on adversarial malformed inputs.

Changes:

• Rewrites the role-boundary attribute-matching regex to prevent catastrophic backtracking.
• Adds a ReDoS regression test that asserts bounded runtime on an adversarial payload.
• Documents the rationale for the regex rewrite and links behavior to issue #446.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
runtime/python/prompty/prompty/parsers/prompty.py	Updates _BOUNDARY_RE to a safer, linear-time regex construction.
runtime/python/prompty/tests/test_parsers.py	Adds a timing-based regression test for the role-boundary regex ReDoS case.

[Sadok Barbouche (cr-sbarbouche)]

Already covered — pyproject.toml has requires-python = ">=3.11", so the possessive-quantifier syntax can't hit an older interpreter. No change needed here.

[Sadok Barbouche (cr-sbarbouche)]

Switched to a doubling-input comparison instead of an absolute wall-clock budget (small=20 reps vs large=40 reps, asserting the runtime doesn't blow up past a small constant factor) — catches the exponential-backtracking regression without being sensitive to a slow/contended runner.

[Sadok Barbouche (cr-sbarbouche)]

Fixed — the test now goes through the public PromptyChatParser.parse() API instead of importing _BOUNDARY_RE directly.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

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

Suppressed comments (2)

runtime/python/prompty/prompty/parsers/prompty.py:37

• The quoted-value branch \"[^\"]*\" allows commas inside quoted attribute values, but _parse_attrs() (per current implementation) tokenizes on commas and therefore won’t correctly parse values like name=\"A,B\" even though the boundary regex accepts them. Consider either (a) tightening the quoted branch to disallow commas to match what _parse_attrs() can parse, or (b) updating _parse_attrs() to properly handle quoted values containing commas so boundary detection and attribute parsing stay consistent.

    r"(\[(?:\w++\s*+=\s*+(?:\"[^\"]*\"|[^\",\]]*+)\s*+,?+\s*+)+\])?\s*:\s*$"

runtime/python/prompty/tests/test_parsers.py:294

• This is a wall-clock timing test, which can be flaky under noisy/contended CI (scheduler pauses, CPU throttling, first-run effects). To reduce variance while keeping the same intent, consider adding a short warm-up parse before timing and/or timing multiple iterations and comparing medians/minima instead of a single sample, so transient spikes don’t cause unrelated failures.

    def _time_adversarial_parse(self, reps: int) -> float:
        payload = "user[" + "a=b," * reps + "!"
        start = time.perf_counter()
        self.parser.parse(self.agent, payload)
        return time.perf_counter() - start

    def test_doubling_input_does_not_blow_up_runtime(self):
        small = self._time_adversarial_parse(20)
        large = self._time_adversarial_parse(40)
        # Exponential (catastrophic-backtracking) behavior would roughly
        # square the runtime when the repeat count doubles; linear-time
        # matching keeps it within a small constant factor.
        assert large < max(small * 20, 0.1)

[Sadok Barbouche (cr-sbarbouche)]

This is pre-existing, not new to this PR:

prompty/runtime/python/prompty/pyproject.toml

Line 7 in c4e7bd4

requires-python = ">=3.11"

already pins requires-python = ">=3.11" on main, unchanged by this diff. Possessive quantifiers are safe on the package's actual floor.

[Copilot]

Pull request overview

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

Suppressed comments (3)

runtime/python/prompty/prompty/parsers/prompty.py:38

• The new possessive pattern makes a trailing comma inside the attribute list unmatchable (e.g. user[a=b,]:) because ,?+ can’t be backtracked away to allow ]. The previous regex accepted a trailing comma, so this is a behavioral change that may break existing prompts. If trailing commas are intended to remain valid, restructure the separator so commas are only consumed when followed by another key/value (e.g., pair (comma pair)* with an optional final comma) while keeping the pattern linear-time.

_BOUNDARY_RE = re.compile(
    r"(?im)^\s*#?\s*(" + _ROLE_NAMES + r")"
    r"(\[(?:\w++\s*+=\s*+(?:\"[^\"]*\"|[^\",\]]*+)\s*+,?+\s*+)+\])?\s*:\s*$"
)

runtime/python/prompty/tests/test_parsers.py:280

• The docstring says the test avoids an absolute wall-clock budget, but the assertion below uses an absolute floor (0.1). Either update the docstring to reflect the absolute threshold, or adjust the assertion to be purely relative so the documentation matches the implemented behavior.

class TestRoleBoundaryReDoS:
    """Parsing must stay roughly linear in input size on adversarial input.

    Unquoted, unterminated attributes used to let the value class overlap
    with the `,` separator and closing `]`, giving the backtracking engine
    an exponential number of equivalent splits to try before failing.
    Comparing runtime at two input sizes — rather than an absolute
    wall-clock budget — keeps this robust on slow/contended CI runners
    while still catching a regression back to exponential behavior.
    """

    def setup_method(self):
        self.parser = PromptyChatParser()
        self.agent = _make_agent()

runtime/python/prompty/tests/test_parsers.py:294

• This is a timing-based test and is likely to be flaky across noisy/contended CI environments (single measurements + wall-clock comparisons). To make it more robust, consider taking multiple samples and asserting on median/min, adding a short warm-up run, and/or using a higher-level timing helper (if the repo has one) so transient scheduler noise doesn’t cause false failures.

    def test_doubling_input_does_not_blow_up_runtime(self):
        small = self._time_adversarial_parse(20)
        large = self._time_adversarial_parse(40)
        # Exponential (catastrophic-backtracking) behavior would roughly
        # square the runtime when the repeat count doubles; linear-time
        # matching keeps it within a small constant factor.
        assert large < max(small * 20, 0.1)

[Sadok Barbouche (cr-sbarbouche)]

Same point as above (already answered on the earlier duplicate at #discussion_r3732083116 and r3732328648) — pyproject.toml has pinned requires-python = ">=3.11" on main since before this PR; unaffected by this diff.

[Sadok Barbouche (cr-sbarbouche)]

Two more items from the latest automated review that came through as "suppressed" (no direct comment thread to reply on):

Trailing-comma claim — checked empirically, it doesn't hold: old and new regex agree on every case, including user[a=b,]: and user[a=b,c=d,]:.

>>> [bool(NEW_RE.match(c)) for c in ('user[a=b,]:', 'user[a=b,c=d,]:', 'user[a="b",]:')]
[True, True, True]  # identical to OLD_RE

The outer (?:...)+ still stops cleanly after a completed iteration that ends in a trailing comma — there's nothing to backtrack away, since the possessive quantifiers only apply inside a single completed iteration, not to whether the group attempts another one.

Docstring/threshold wording — legitimate, fixed in e7d62f1: reworded to say the 0.1 is a noise floor for near-zero measurements, not the enforced budget (the enforced check is the relative 20x growth bound).

[Copilot]

Pull request overview

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

Suppressed comments (2)

runtime/python/prompty/prompty/parsers/prompty.py:37

• The updated pattern is security-sensitive but quite dense, which makes it harder to audit and safely modify later. Consider compiling it with re.VERBOSE and splitting it across logical, commented sub-patterns (and/or naming key groups) to preserve the ReDoS fix while improving readability for future maintenance.

    r"(\[(?:\w++\s*+=\s*+(?:\"[^\"]*\"|[^\",\]]*+)\s*+,?+\s*+)+\])?\s*:\s*$"

runtime/python/prompty/tests/test_parsers.py:296

• This test is timing-based and may be flaky on contended/slow CI runners (or under heavy system load), since it enforces an absolute 0.1s cap when small is near-zero. To reduce flakiness while keeping the regression signal, consider warming up once (to avoid first-run overhead), running multiple trials and asserting on a robust statistic (e.g., min/median), and/or using a looser absolute floor (or purely relative scaling with a larger sample) so slow environments don’t fail despite linear behavior.

    def test_doubling_input_does_not_blow_up_runtime(self):
        small = self._time_adversarial_parse(20)
        large = self._time_adversarial_parse(40)
        # Exponential (catastrophic-backtracking) behavior would roughly
        # square the runtime when the repeat count doubles; linear-time
        # matching keeps it within a small constant factor.
        assert large < max(small * 20, 0.1)

[Sadok Barbouche (cr-sbarbouche)]

Same already-answered point (see r3732083116, r3732328648, r3732454506) — pinned by pyproject.toml's requires-python = ">=3.11" on main, unaffected by this diff. Pushed e967c0a to state the floor directly in the code comment next to the regex so it's visible without cross-referencing pyproject.toml.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

runtime/python/prompty/tests/test_parsers.py:296

• This test asserts on wall-clock timing from a single sample per size, which is prone to CI flakiness (scheduler pauses, noisy neighbors, CPU scaling), and the 0.1 absolute floor can fail spuriously on slow/contended runners even if the regex is linear-time. Consider stabilizing the measurement by (a) running multiple iterations for each size and comparing totals/minimums, and/or (b) increasing the work per measurement (loop parse() N times) so the ratio is less sensitive to jitter. That keeps the regression signal while reducing false failures.

    def _time_adversarial_parse(self, reps: int) -> float:
        payload = "user[" + "a=b," * reps + "!"
        start = time.perf_counter()
        self.parser.parse(self.agent, payload)
        return time.perf_counter() - start

    def test_doubling_input_does_not_blow_up_runtime(self):
        small = self._time_adversarial_parse(20)
        large = self._time_adversarial_parse(40)
        # Exponential (catastrophic-backtracking) behavior would roughly
        # square the runtime when the repeat count doubles; linear-time
        # matching keeps it within a small constant factor.
        assert large < max(small * 20, 0.1)

runtime/python/prompty/prompty/parsers/prompty.py:40

• The new pattern is (understandably) dense, but it will be hard to safely modify later. Consider building it from named sub-pattern constants (e.g., KEY, WS, VALUE_QUOTED, VALUE_UNQUOTED, PAIR, ATTRS) or compiling with re.VERBOSE and inline comments. That makes the ReDoS hardening properties (possessive quantifiers + disjoint alternation) easier to audit and reduces the risk of reintroducing ambiguity.

# Possessive quantifiers + a quoted/unquoted alternation (no shared chars between
# branches) keep this linear-time — a plain `\w+\s*=\s*"?[^"]*"?` value class lets
# unquoted attributes overlap with the separator/`]`, causing catastrophic
# backtracking on malformed input. See issue #446.
# Possessive quantifiers (`++`, `*+`, `?+`) require Python 3.11+, which pyproject.toml
# already sets as this package's floor (`requires-python = ">=3.11"`).
_BOUNDARY_RE = re.compile(
    r"(?im)^\s*#?\s*(" + _ROLE_NAMES + r")"
    r"(\[(?:\w++\s*+=\s*+(?:\"[^\"]*\"|[^\",\]]*+)\s*+,?+\s*+)+\])?\s*:\s*$"
)

runtime/python/prompty/prompty/parsers/prompty.py:39

• The boundary regex accepts quoted values containing commas (e.g. name=\"A,B\"), but _parse_attrs() later uses ([^\",]*) which will truncate at the comma and parse the attribute incorrectly. To keep behavior consistent, either (mandatory) tighten the quoted-value branch to disallow commas (and/or ]) if those aren’t supported, or (alternative) update _parse_attrs() to correctly parse quoted strings that may include commas (so the regex and attribute parser agree on the grammar).

    r"(\[(?:\w++\s*+=\s*+(?:\"[^\"]*\"|[^\",\]]*+)\s*+,?+\s*+)+\])?\s*:\s*$"

[Sadok Barbouche (cr-sbarbouche)]

Latest review pass (00:17:28) — good news, no new possessive-quantifier comment this time, so the explicit code-comment call-out in e967c0a seems to have landed. Three more items came through as "suppressed" (no reply-able thread), addressing here:

Timing flakiness (single sample) — fair, and cheap to actually fix: ac431eac switches _time_adversarial_parse to best-of-20 samples instead of one. Doesn't weaken the check — an exponential-backtracking regression would still blow past this by orders of magnitude, it just stops a single scheduler blip from failing the build.

Extract the regex into named sub-patterns / re.VERBOSE — declining. This is a single-use pattern with a comment immediately above it explaining exactly what the possessive quantifiers and alternation buy (linear-time, no shared chars between branches). Splitting it into KEY/WS/VALUE_QUOTED/etc. constants adds a layer of indirection for something used in exactly one place — more surface to keep in sync, not less. Happy to reconsider if a maintainer wants it, but not adding it speculatively.

Quoted commas vs. _parse_attrs() — checked, this predates the PR: user[name="A,B"]: mis-tokenizes to {'name': 'A'} on both the old and new boundary regex, since _parse_attrs()'s own comma-splitting is unchanged by this diff. Not something this ReDoS fix introduces or should expand scope to fix — worth its own issue if it matters in practice.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.