Fix #788: Fix quadratic ReDoS in cURL/Postman import placeholder regexes - #789
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
keysersoft
left a comment
keysersoft
left a comment
There was a problem hiding this comment.
Thanks for this, the repro in #788 is solid. I confirmed it on main: {{{{… at 32k chars already takes ~370 ms through the old {{([^}]+)}}, and your main pattern brings that down to ~0.1 ms.
One problem before I can merge though. The strip patterns you added, \{\{\s*[^{}]+\s*\}\} (curl.parser.ts line 320 in generateToolName, postman.parser.ts lines 181 and 290), open a worse ReDoS than the one they replace. \s is a subset of [^{}], so {{ followed by a run of whitespace with no closing }} gets split three ways between \s*, [^{}]+ and \s* and backtracks cubically.
Numbers from my machine:
- bare regex,
https://a/{{+ 8,000 spaces: 121 s (the old pattern does it in ~26 ms) - end to end, a Postman collection whose raw url is
https://api.example.com/{{+ 2,000 spaces: 0 ms on main, 1.6 s on this branch, and it keeps growing with the cube of the length
So as it stands it trades one ReDoS for a bigger one on the same import endpoint.
What I'd suggest instead is one shape everywhere: \{\{([^{}]+)\}\}, then .trim() the capture where the name matters. [^{}] is disjoint from both braces, so it's linear on brace runs and on whitespace runs (I measured 0.0 to 0.1 ms on all three hostile inputs), and it keeps the current semantics. That second part matters in unresolved-placeholders.util.ts: with [^{}\s]+ a placeholder like {{ my var }} is no longer recognised, so the guard would let the literal go out to the vendor instead of failing closed.
Three smaller things:
- Please drop the
package-lock.jsonchange. Thoselibcremovals come from your npm version, not from the fix. - Could you add a spec with the hostile inputs (a brace run, and
{{plus a long whitespace run) for both parsers and forenv-interpolation.util.ts, asserting each finishes quickly at ~50k chars? Otherwise the next regex tweak can bring this back without anyone noticing. - The CLA check is red. You sign it by posting the sentence from the bot's comment as a PR comment exactly as written there, since the action matches the text character for character.
Once you push, I'll approve the CI run (fork PRs need that on our side).
|
I have read the CLA Document and I hereby sign the CLA |
… everywhere
- Replace all {{\s*([^{}\s]+)\s*}} with {{([^{}]+)\}} for linear-time matching
- Add .trim() where variable names are captured
- Revert package-lock.json change
- Add ReDoS regression tests for env-interpolation, curl.parser, postman.parser
|
recheck |
…, last loose placeholder regex
- package-lock.json back to main's version (the libc removals came from a
different npm, not from the fix).
- The ReDoS specs allow 250 ms instead of 100: the fixed patterns take ~1 ms,
the old ones take seconds at 50k chars, so the test still catches a
regression without flaking on a slow CI runner.
- adapters.service.ts hasUsableValue() had the same /\{\{[^}]+\}\}/ shape:
3.2 s on 50k '{' against 1 ms now.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (typeof url === 'string') { | ||
| try { | ||
| const parsed = new URL(url.replace(/\{\{[^}]+\}\}/g, 'placeholder')); | ||
| const parsed = new URL(url.replace(/\{\{[^{}]+\}\}/g, 'placeholder')); |
keysersoft
left a comment
keysersoft
left a comment
There was a problem hiding this comment.
Thanks @pamod-madubashana, this is exactly what was needed: one linear shape everywhere, and the hostile-input specs to keep it that way.
I pushed one small follow-up commit on top so it could go in today:
package-lock.jsonback to main's version;- the timing bounds in the new specs raised from 100 to 250 ms, so they don't flake on a slow CI runner (the fixed patterns take ~1 ms, the old ones take seconds at 50k chars, so they still catch a regression);
- the same fix for
hasUsableValue()inadapters.service.ts, which still had/\{\{[^}]+\}\}/(3.2 s on 50k{before, 1 ms now).
Merging.
3 participants
Fixes #788
I fixed the quadratic ReDoS in the cURL/Postman import placeholder regexes (#788).
What changed:
{{([^}]+)}}shape with disjoint classes{{\s*([^{}\s]+)\s*}}so a brace run with no closing}}fails fast in linear time. This matches the pattern I already use incaller-context.util.ts:34.packages/backend/src/common/env-interpolation.util.ts,packages/backend/src/common/unresolved-placeholders.util.ts,packages/backend/src/connectors/connector-secrets.util.ts,packages/backend/src/connectors/catalog-env-rebuild.util.ts,packages/backend/src/connectors/parsers/curl.parser.ts,packages/backend/src/connectors/parsers/postman.parser.ts.curl.parser.tsI also removed the greedy.*{{...}}.*header extraction in favor of a single safe.match()for the variable name, and made thegenerateToolNamestrip patterns ({{...}}/{...}) use[^{}]+with doubles stripped first.postman.parser.tsI applied the same treatment to the path-param, URL-sanitize, body-sentinel, and tool-name patterns.{{ SPACED }}(the surrounding\s*preserves the existing.trim()handling).Why it addresses the issue:
CurlParser.parseUrlover user-supplied URLs with a 10mb body limit, so one 80KB paste of{{{{...stalled the event loop for seconds (4x input ~16x time). With the fixed shape the same 8K/32K/80K inputs scan in well under a millisecond, and the shared runtime interpolation/detection helpers get the same protection.Verification
'https://api.example.com/'+'{{{{'.repeat(k)):chars=8024 0.3ms / chars=32024 0.3ms / chars=80024 0.7ms— all under 5ms, linear.npm run test --workspace=packages/backendon the 8 related suites (env-interpolation, unresolved-placeholders, caller-context, base-url-variable, connector-secrets, catalog-env-rebuild, curl.parser, postman.parser) — 8 suites passed, 257 tests passed, 0 failed.