paulk-asert commented on PR #2711:
URL: https://github.com/apache/groovy/pull/2711#issuecomment-4989523393
AI read on your PR below:
> Thanks for this — the underlying concern is real: a small but
deeply-nested document can drive `MarkdownSlurper` into a `StackOverflowError`,
and an untrusted-input caller expecting `MarkdownRuntimeException` gets a raw
`Error` instead. Before we settle on the cap approach, though, I dug into
*where* the overflow actually happens, and it turns out there are **two
distinct vectors** that land on opposite sides of the Groovy/CommonMark
boundary — and the current fix only covers one of them.
>
> ### Two vectors (verified against commonmark 0.29.0)
>
> | Vector | Trigger | Where
it overflows
| Whose code |
> | ------------------------------- | ------------------------------ |
---------------------------------------------------------------------------------------------------
| ------------------------- |
> | **1 — block/container nesting** | `'>' * N`, deeply nested lists |
Groovy's `blocksToList → nodeToMap → blocksToList` and `appendText` recursion
| **MarkdownSlurper** |
> | **2 — inline emphasis nesting** | `('*' * N) + 'a' + ('*' * N)` |
CommonMark `InlineParserImpl.mergeChildTextNodes` / `mergeTextNodesInclusive`,
**inside `parse()`** | **CommonMark (upstream)** |
>
> Probing the two directly:
>
> - `'>' * 50000 + ' x'` → **parses fine**, then Groovy's own recursive walk
overflows. CommonMark did nothing wrong here; it faithfully returns a
50000-deep tree and our recursive conversion blows the stack. This is the case
this PR addresses. 👍
> - `('*' * 50000) + 'a' + ('*' * 50000)` → **overflows inside `parseReader`
itself**, before control ever returns to `MarkdownSlurper`:
>
> ```
> java.lang.StackOverflowError
> at
org.commonmark.internal.InlineParserImpl.mergeIfNeeded(InlineParserImpl.java:859)
> at
org.commonmark.internal.InlineParserImpl.mergeTextNodesInclusive(InlineParserImpl.java:842)
> at
org.commonmark.internal.InlineParserImpl.mergeChildTextNodes(InlineParserImpl.java:824)
> ... (repeats)
> ```
>
> ### The gap
>
> `checkNestingDepth(doc)` runs *after* `buildParser().parseReader(reader)`
returns. For **Vector 2 the overflow happens during that parse call**, so the
new check never executes and the raw `StackOverflowError` escapes exactly as
before — the very thing the PR sets out to prevent. In other words the cap
makes block-nesting safe but leaves an equivalent inline-nesting
`StackOverflowError` DoS wide open, which is worse than the status quo because
callers now believe they're covered.
>
> A test that demonstrates it (fails on this branch — throws raw
`StackOverflowError`, not `MarkdownRuntimeException`):
>
> ```groovy
> @Test
> void testDeeplyNestedInlineEmphasisRejected() {
> def md = ('*' * 50000) + 'a' + ('*' * 50000)
> assertThrows(MarkdownRuntimeException) { new
MarkdownSlurper().parseText(md) }
> }
> ```
>
> ### Suggested direction
>
> We generally prefer to push size/robustness limits onto the dependency and
only harden locally what's genuinely ours and can't be deferred. That split
maps neatly here:
>
> 1. **Vector 2 is upstream and undeferrable.** commonmark-java has no
CVE/advisory for this; it's an acknowledged limitation of the recursive
inline/tree processing (see commonmark-spec#187), there's **no inline nesting
cap**, and it's **not fixed in 0.29.0** (the current latest). Since we can't
defer our untrusted-input obligation to a fix that doesn't exist, the honest
fix is to **catch `StackOverflowError` around the parse+walk in `parse()` and
rethrow `MarkdownRuntimeException`**. It's ~3 lines, covers *both* vectors'
contract, and is safe here — the SO is cleanly catchable and the slurper builds
a fresh parser/tree per call, so there's no retained state to corrupt. I'll
also file an upstream issue for the `mergeChildTextNodes` recursion.
>
> 2. **Vector 1 can be deferred to the dependency.** commonmark exposes
`Parser.builder().maxOpenBlockParsers(int)` (since 0.28.0, default unlimited).
Setting it in `buildParser()` caps block nesting *at parse time* — I confirmed
`maxOpenBlockParsers(100)` yields a tree only ~103 deep for a 50000-deep
block-quote input, so our recursive walk is inherently safe and beyond-limit
nesting degrades to text rather than throwing. That's deterministic (unlike
catching an Error at a stack-size-dependent depth) and uses the library's
supported knob. (Note it does **not** help Vector 2 — inline emphasis isn't a
block.)
>
> So the combination I'd suggest is: `maxOpenBlockParsers(n)` for
deterministic block handling + a `StackOverflowError` catch for the inline
vector we can't defer, keeping your configurable knob as optional DoS
defense-in-depth. Your test scaffolding is a great start — please keep it and
add the inline-emphasis case above.
>
> Happy to pair on the revised version if useful, or push a follow-up commit
onto the branch. And thanks again for chasing these down — this is exactly the
kind of untrusted-input hardening worth getting right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
