Bound compare_body input to prevent quadratic DeepDiff scan stall - #3384
Bound compare_body input to prevent quadratic DeepDiff scan stall#3384singlerider wants to merge 7 commits into
Conversation
HttpCompare.compare_body ran DeepDiff with ignore_order=True on line-lists. When two bodies are highly similar but differ on many lines (the exact shape wildcard detection produces on large catch-all hosts), DeepDiff's pairing is quadratic in the differing-line count and holds the GIL, stalling entire scans for tens of minutes. Short-circuit to 'different' when the multiset difference of two line-lists exceeds web.http_compare_max_differing_lines (default 500) before invoking DeepDiff. Caps worst-case comparison at a couple seconds. Dict inputs (XML/JSON) keep the full diff path unchanged. Fixes blacklanternsecurity#3339
defaults.yml added the key; the strict pydantic WebConfig model must accept it or test_defaults_yml_validates_against_schema fails.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3384 +/- ##
======================================
+ Coverage 90% 90% +1%
======================================
Files 454 454
Lines 46858 47225 +367
======================================
+ Hits 42119 42466 +347
- Misses 4739 4759 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- dict inputs still take the DeepDiff path (guard must not short-circuit them) - reordered identical line-lists stay equal (multiset is order-insensitive) - at-threshold vs over-threshold boundary around max_differing_lines - http_compare_max_differing_lines config value reaches the helper
en0f
left a comment
There was a problem hiding this comment.
- Broken — the pre-check bypasses ddiff_filters, producing false "body changed" verdicts
diff.py:244-250 runs before the DeepDiff(..., exclude_paths=self.ddiff_filters) call. ddiff_filters is exactly the set of lines the baseline established as dynamic and expected to differ, so those lines are counted toward the threshold even though the comparison is supposed to ignore them.
Reproduced with real DeepDiff: baseline pair with 600 auto-filtered dynamic lines out of 1000, subject differing only in those same filtered lines →
current dev: compare_body → True (match, as intended)
this PR: symmetric-difference count = 1200 > 500 → False
Any target with more than ~250 dynamic lines (per-request nonces, timestamps, rotating ad/session markup) now reports "body" as a diff reason on every probe. That breaks canary_check and wildcard detection in the direction that generates findings. The count should be taken after excluding ddiff_filters paths, or the threshold should only short-circuit when the differing lines aren't filtered.
- Broken — the stall this PR targets still reproduces on the XML/dict path
_baseline and _compare_body_sync both try xmltodict.parse() first and only fall back to .split("\n") on ExpatError. The guard is isinstance(content_1, list) and isinstance(content_2, list), so any response xmltodict parses successfully goes into DeepDiff(ignore_order=True) completely unbounded — same quadratic scan, same hang. Also note the mixed case (baseline parses as dict, subject falls back to list) silently skips the guard.
- Broken test — the boundary assertions are vacuous
test_web.py:434-443: with max_differing_lines = 10, at_threshold has 10 differing lines (not > 10), so it falls through to DeepDiff, which returns False because the lists genuinely differ. Both at_threshold and over_threshold assert is False. The test passes identically with the entire feature deleted — it can't detect a regression in either direction. To test the boundary you need the at-threshold case to be one DeepDiff would call a match (e.g. differences confined to ddiff_filters, or same multiset).
- Crash on explicit null config
http_compare_max_differing_lines: Optional[int] = None permits null. web_config.get(key, 500) returns None when the key is present-but-null, and sum(differing.values()) > None raises TypeError. Either coerce (or 500) or drop Optional.
Four focused tests, all failing against the current guard: filtered_lines_not_counted asserts a subject differing only in lines the baseline already marked dynamic still compares equal. The guard counts those lines before DeepDiff excludes them, so it returns a false "changed" on any page with many nonces. bounds_dict_bodies asserts an XML/JSON body settles in bounded time. xmltodict.parse succeeds on ordinary HTML, so those bodies are dicts and skip the list-only guard into the unbounded quadratic path. threshold_boundary makes the at-threshold case one DeepDiff calls a match, so the assertion can distinguish the guard firing from the lists simply differing. null_threshold_config covers an explicit null, which resolves to None and raises TypeError on the comparison. Replaces the previous boundary assertions, which expected False on both sides and passed with the feature deleted.
The guard counted raw list items, which broke two ways. It ignored ddiff_filters, the paths the baseline established as dynamic. Those lines are excluded from the DeepDiff that follows, but they were still counted toward the threshold, so a page with enough per-request nonces reported its body as changed on every probe. That generates findings rather than suppressing them. It also required both sides to be lists, but xmltodict.parse succeeds on ordinary HTML, so those bodies arrive as dicts and dropped into the unbounded quadratic DeepDiff the guard exists to prevent. _leaf_counts walks either shape, skips any subtree whose path is already filtered, and counts leaves, so the threshold sees the same content the diff will.
http_compare_max_differing_lines is Optional[int], so an explicit null is valid config. dict.get returns None for present-but-null, not the default, and the threshold comparison then raised TypeError on every body compare.
|
All four addressed in b50f65f. Tests first, each one red against the old guard.
I also wrote a fifth test for the mixed dict/list case you mentioned, then deleted it. It passed before the fix, so it was vacuous for the same reason you flagged in 3. |
en0f
left a comment
There was a problem hiding this comment.
FYI -
Broken — the guard never fires in the scenario the PR exists to fix
compare_body (diff.py:257-266) only short-circuits on return False. On a catch-all host the probe matches the baseline: the differing lines are exactly the ones _baseline already put in ddiff_filters, so _leaf_counts excludes them, sum(differing.values()) is 0, and control falls straight through to the unbounded DeepDiff(ignore_order=True) at line 268.
exclude_paths does not save you — DeepDiff does the ignore_order pairing first and filters the results afterward. Measured identically with the filter list and with a set:
lines=8000, 2000 dynamic (all in ddiff_filters):
_leaf_counts (both bodies) 0.19s
DeepDiff exclude_paths=list 16.89s -> keys=0
DeepDiff exclude_paths=set 16.61s -> keys=0
PR compare_body total 16.62s -> True
lines=16000, 4000 dynamic: 65.79s -> True
_probe_wildcard_host (web.py:704-735) builds its baseline from two random-path URLs, then calls compare(root_url) and returns the HttpCompare when root_match is true. The wildcard-detected path is the slow path. So issue #3339 still reproduces on a real wildcard responder — the guard bounds the cheap-to-decide case and leaves the expensive one alone.
Broken — _baseline's own DeepDiff is unguarded and runs on the event loop
diff.py:176 runs DeepDiff(baseline_1_json, baseline_2_json, ignore_order=True, threshold_to_diff_deeper=0) inside async def _baseline, with no guard and no run_in_executor_cpu (line 320 is the only executor call in the file). On a catch-all host these two pages differ on every line echoing the random path — the worst-case shape, at the first call, before ddiff_filters exists. Reproducing the issue's curve:
2000 lines / 400 echoed: 8.45s
4000 lines / 800 echoed: 33.32s
8000 lines / 1600 echoed: did not finish (>78s)
This blocks the loop directly rather than starving it through the CPU executor, so it's a harder stall than the one the PR fixes.
There was a problem hiding this comment.
Diagnosis in the Problem section is right. My concern is that the guard landed one call site upstream of where that analysis points.
The guard never fires in the wildcard path. _baseline() builds ddiff_filters from the two baseline samples, so it holds precisely the lines that differ. _leaf_counts excludes them, differing comes out 0, and DeepDiff runs anyway. Measured with filters populated the way _probe_wildcard_host does:
| shape | _baseline() |
compare_body() |
guard fires? |
|---|---|---|---|
| 2,000 lines / 400 echoed | 4.16 s | 0.53 s | no (differing=0) |
| 4,000 lines / 1,000 echoed | 25.53 s | 2.41 s | no (differing=0) |
The dominant cost is _baseline (diff.py:176), untouched here, running the same DeepDiff directly on the event loop instead of via run_in_executor_cpu, so it blocks every queue rather than one worker thread. As written this doesn't close #3339.
Suggest splitting:
- Fix
_baseline. A diff-count guard can't work there, since there are no filters yet and the samples differ maximally by design, so it has to bound the input. Possibly skip DeepDiff for line-lists entirely:ddiff_filtersis just "which lines vary between two samples", which yourCounterapproach computes directly. - Land the
compare_bodyguard separately. Real value for paramminer/bypass403/lightfuzz. I ran it against the unguarded path over 900 randomized shapes plus adversarial index-misalignment cases and got zero verdict disagreements, so it is behavior-preserving.
Smaller things for (2):
path in self.ddiff_filtersis a list scan per node, so the guard is O(nodes x filters): 221 ms at 2,000 filters vs ~8 ms with aset.- Body says dict/XML is untouched, but the guard does fire on dicts, and
test_web_http_compare_bounds_dict_bodiesasserts it. - Threshold counts leaf diffs, not lines: N changed lines counts as 2N, so the default 500 admits about 250.
or 500swallows a deliberate0, and with noge=0a-1makes every non-identical body returnFalse.- New key has no comment in
defaults.yml, which is embedded verbatim into the config docs. - Tests and benchmark all run with empty or trivial filters, which is why they pass while the stall remains.
Unrelated pre-existing: __init__ accepts timeout=10 but line 102 hard-codes self.timeout = 10, so the argument is ignored for every caller. Separate issue.
_baseline ran DeepDiff(ignore_order=True) directly on the event loop over two full page samples that differ maximally by design, so the quadratic pairing ran unguarded and blocked every queue rather than one worker thread. For line-list bodies the filter set is just "which lines are not shared between the two samples", so compute it with set membership instead. Dict bodies keep the DeepDiff path, now dispatched through run_in_executor_cpu. Verified identical to the DeepDiff filter output over 300 randomized realistic shapes and at 2000/400, 4000/1000, and 8000/2000, where the old path took 14s, 90s, and 302s against 1.2ms, 2.5ms, and 5.4ms. Also from review: - hoist ddiff_filters into a frozenset so the guard is O(nodes), not O(nodes x filters) - compare each direction of the multiset difference against the threshold instead of their sum, so N changed lines counts as N - honor a deliberate 0 instead of swallowing it with `or 500`, and reject negatives with ge=0 in WebConfig - document the new key in defaults.yml, which is embedded into the config docs
|
All addressed, pushed as 7fd5ffe. Guard never fires in the wildcard path: correct, and Filters are now hoisted into a frozenset per call, so the guard is O(nodes). Threshold compares each direction of the multiset difference separately, so N changed lines counts as N.
New key has a comment in PR body rewritten: it no longer claims the dict path is untouched, and it documents both bounds. Kept the
21 in test_web.py, 28 across paramminer and test_scan. ruff clean. |
|
Three problems: 1. DeepDiff runs with Repeated lines are normal in HTML. Script tags with a CSP nonce, a timestamp in every row, a CSRF token in every row. I generated 200 pages like that and all 200 came out different. On a 20 row page DeepDiff gives 3 filters and yours gives 43. Yours is always a superset, so it filters too much and never too little. That means it hides real differences. I changed one of those repeated rows to real content: dev returns False, this returns True. That's a missed finding in paramminer, lightfuzz and bypass403. Maybe flagging every index is the better answer. I'm not sure it isn't. But the body says the output is identical over 300 shapes and it isn't, and the test only covers distinct tokens at matching indices, which can't catch this. So either match DeepDiff, or keep your version, say that in the body, and add a test with duplicate lines. 2. I deleted the whole guard and ran the file. That test still passes. Only 2 of them fail. It did work at b50f65f. The switch to 3. The defaults.yml comment is wrong. At 0 the guard is at its most aggressive, not disabled, and no value disables it. That line ends up in the config docs as written. On compare_body in the wildcard path: the guard still can't fire there, 0.52/2.41/10.05/40.3s at 2k/4k/8k/16k. I don't think that blocks this. |
Fixes #3339.
Problem
HttpComparerunsDeepDiff(..., ignore_order=True, threshold_to_diff_deeper=0)on two HTTP bodies in two places, and itsignore_orderpairing is quadratic in the number of differing lines._baseline()is the one that stalls scans. It diffs two full page samples that differ maximally by design, runs directly on the event loop rather than throughrun_in_executor_cpu, and so blocks every queue instead of one worker thread: queues drain, no events emit, one module sits inprocessing, and the scan never finishes.compare_body()hits the same pairing on subject comparisons, but its inputs are already filtered byddiff_filters.Wildcard detection (
_probe_wildcard_host) manufactures the worst-case shape on purpose: on a catch-all host all responses are the same large page differing only where the path/token/timestamp is echoed in. It is reached from the web helper, not a module, so it can fire with no fuzzing modules enabled.Fix
Two independent bounds.
_baseline: for line-list bodies,ddiff_filtersis just "which lines are not shared between the two samples", so compute it with set membership instead of DeepDiff. Dict bodies (parsed XML/JSON) keep the DeepDiff path, now dispatched throughrun_in_executor_cpuso it no longer blocks the loop.compare_body: before the DeepDiff call, take the multiset difference of leaves outsideddiff_filters(a cheap O(n)Counterop). If either direction exceedsweb.http_compare_max_differing_lines(default 500), the bodies are clearly different, so returnFalsewithout running the pairing. This fires on dict bodies too, since the leaf walk is shape-agnostic.Benchmark
_baselinefilter constructionTwo page samples of
body linestotal, differing ondiffering lineswhere a token or timestamp is echoed back. Filter output is identical to the DeepDiff path in every row.Also verified identical over 300 randomized realistic shapes (
test_web_http_compare_line_filters_match_deepdiffpins one).compare_bodyguardEach shape is two bodies of
body linestotal, identical except fordiffering lines. Every row is in the quadratic zone, so the guard fires in all of them. Each run is an isolated subprocess; wall is monotonic, CPU isprocess_time, peak RSS isru_maxrss.Below the threshold the guard does not fire and behavior is unchanged.
Tests
test_web_http_compare_line_filters_match_deepdiff: set-based filters match DeepDiff exactly on the realistic shapetest_web_http_compare_baseline_bounded_on_dynamic_pages:_baselineagainst a 4,000-line page with 1,000 dynamic lines completes in bounded time with 1,000 filterstest_web_http_compare_filtered_lines_not_counted: filtered lines do not count toward the thresholdtest_web_http_compare_bounds_dict_bodies: the guard bounds dict bodies tootest_web_http_compare_threshold_boundary: at-threshold passes, over-threshold short-circuitstest_web_http_compare_null_threshold_config: a null config value falls back to the defaulttest_web.py(21) plus paramminer headers/getparams andtest_scan.py(28) pass. ruff clean.