[slice-P3C] fix(fetch): a length heuristic was declaring short legitimate pages to be bot walls - #272
Merged
KnockOutEZ merged 12 commits intoAug 10, 2026
Conversation
…o be bot walls `classifyChallenge`'s final arm used `isChallengeSkeleton` unpaired. That predicate's last arm is a bare `visibleText < 600`, and its own sibling docstring says it is "deliberately NOT sufficient on its own — callers pair it with an anti-bot STATUS". Used unpaired, every page below the content floor carrying no vendor marker at all classified `behavioral`. Measured: example.com returns 559 bytes at HTTP 200 and classified `behavioral`. `cdpDirectFetch` polls `classifyChallenge(html) === 'none'` as its clear-check and has no HTTP status to pair with by construction, so it burned the full clear-poll budget and declined legitimate short pages. Second instance of one pattern: P0 fixed `isAntiBotSignal`, which matched markers ALONE at 2xx so an article about bot protection read as a wall. Same shape, opposite half. The 600 floor is unchanged. What changed is its logical role — from a sufficient condition to the corroborating half of a pair. A challenge verdict now requires a positive interstitial artifact (interstitial title, vendor template signature, or a shared challenge marker), which is the same rule the shipped HTTP layer already applies to a 2xx body. Ladder behaviour is unchanged: `runSolveLadder` runs no rung for either `behavioral` or `none`, so bodies moving between those two classes engage exactly the same rungs as before. The studio-fetch gate test asserted the old false positive as its justification. Restated on the disagreement that survives: `classifyChallenge` is status-free, so it cannot reach the status-gated general density rule and UNDER-fires on a markerless wall — the worse direction for a gate, and a firmer justification than the one it replaces.
The fix has two arms: a high-precedence interstitial-signal arm (this module's own template signatures and titles) and a marker-pair arm (tls-tier's shared CHALLENGE_MARKERS paired with the skeleton reading). Neither is redundant, and a reviewer could plausibly delete either as duplication. `px-captcha-error` and the "access to this page has been denied" / "verifying you are human" titles exist only in this module's vocabulary and are absent from the shared marker list, so the marker-pair arm alone would miss a PerimeterX denial. `dd-loader` is the mirror case. One test each, verified to red when its arm is removed.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… arm The previous commit claimed "one test each, verified to red when its arm is removed". That was true for the interstitial-signal arm and FALSE for the marker-pair arm: the probe was never run for arm 2, and running it now shows the fixture stayed green with the arm deleted (46 passed, 0 failed). Cause: the mirror case used `dd-loader`, which `hasBehavioralPositiveMarker` already catches at step 1. Classification returned before reaching step 4 at all, so the test exercised neither arm. Replaced with the genuine arm-2-only shape — `Just a moment` in the BODY. The title regex requires the phrase inside <title>, no template signature is present, and no behavioral-positive marker matches, so the shared marker paired with the skeleton reading is the only thing that can classify it. Probe re-run against the real arm: 1 failed / 45 passed with arm 2 deleted. A commit asserting a probe that was not run is worse than one asserting nothing, because the next reader trusts it.
…xtures Sweeping the rest of the must-still-fire fixtures after the vacuous arm-2 case, by deleting each return-branch in turn and recording which tests red: step1 behavioralPositive -> 2 red step4 arm1 -> 1 red step2 image -> 3 red step4 arm2 -> 1 red step3 interactive -> 3 red step4 BOTH -> 9 red No further interception found. Six must-still-fire fixtures red under no SINGLE deletion, but that is multiple coverage, not interception: a realistic interstitial carries both an interstitial title and a shared marker, so two arms independently reach the right verdict. They red when both arms go, so they do guard step 4 collectively. Two failure modes worth keeping apart, since only one is a defect: interception means an earlier branch returns and the test cannot fail for the reason it claims; multiple coverage means several branches agree. Rewriting the realistic fixtures to isolate a branch would trade real interstitial shapes for contrived ones. Also recorded: the DataDome fixture returns at step1 and never reds even with both step4 arms deleted, so it does not exercise this slice's change at all. And all of these fixtures passed BEFORE the fix — they are regression guards, not validators. The validators are the six negatives and the two arm tests.
Review found a real regression via a base-vs-tip differential — running both revisions of the classifier side by side against real vendor shapes. This suite was green throughout; a self-check could not find it. The arm replaced earlier was: isChallengeSkeleton(slice) || isCloudflareShell(lower) Only ONE thing in it was defective: the `visibleText < 600` arm inside isChallengeSkeleton. But that predicate also short-circuits on two positive MARKERS, and isCloudflareShell is a pure marker check. Removing the whole expression to kill the heuristic removed the markers with it, and four wall shapes flipped behavioral -> none: a modern-CF skeleton with no interstitial title, an Imperva/Incapsula stub, an Akamai denial, and a LOWERCASE Cloudflare body phrase (the shared catalogue compares case-sensitively where isCloudflareShell compared on the lowercased slice). Not cosmetic. cdpDirectFetch breaks its clear-poll on `=== 'none'` and returns the body as content at a synthesized HTTP 200, which the terminal guardChallengeShell then sees as clean — so these walls would have reached the agent as real pages. WIGOLO_HARDCORE=on flips cdpDirect to auto. Two layers, because one is not enough: 1. Classifier: vendor template markers absent from the shared catalogue, PAIRED with the skeleton reading, never used alone. Matched on the lowercased slice. The Akamai rule requires BOTH "access denied" and "reference #" — either alone is ordinary error copy. 2. cdp-direct: the clear-poll now also requires !isLowContentDensity. The proposed guard alone closed only 1 of 5 regressions — the other four sit under the density rule's 1KB floor — so it is defence in depth for uncatalogued vendors, not the primary fix. Measured after: example.com and a thin genuine page still classify none; Imperva, Akamai, modern-CF and lowercase-CF are behavioral again; the markerless scaffold wall no longer breaks the poll. Over-fire probe for every new marker: articles quoting _Incapsula_Resource, "Access Denied"+"Reference #", and the challenge-platform path all classify none, as does a genuine short 403 saying "Access Denied" with no reference id.
…t all
The falsifiability probe caught this, not review: deleting
`!isLowContentDensity(html)` from cdpDirectFetch's clear-poll reddened
NOTHING — 966 tests passed with the guard gone. Half of the blocker-1 fix was
shipped unverified, which is the same vacuous-verification failure this slice
has now hit three times, this time inside the fix for it.
Two tests at the rung boundary, pinning both directions of the break
condition, because that condition returns the body to the agent as content at
a synthesized HTTP 200:
- a MARKERLESS wall (no catalogued vendor string, so the classifier honestly
says 'none' and only the density rule recognises it) must return null
- a legitimately SHORT page (example.com's shape, 142 visible chars) must be
returned as content
Probes, each with the mutation's diffstat confirmed non-zero:
remove !isLowContentDensity -> 1 failed / 17 passed (the wall test)
revert the classifier arm -> 1 failed / 17 passed (the short-page test)
Neither a length heuristic nor a blanket refusal can satisfy both, which is
the property worth pinning.
… CF sensors
Review found three more instances of this slice's own defect class, inside the
fix for it.
N1 — the pairing was vacuous. `hasVendorTemplateMarker` matched
`/cdn-cgi/challenge-platform/`, and `isChallengeSkeleton` short-circuits TRUE
on that same string (tls-tier.ts:614), so `marker && isChallengeSkeleton`
reduced to `marker && marker`: the marker alone. Structurally identical to the
`'slider'.includes('slide')` vacuity fixed elsewhere in this same file.
tls-tier documents the trap twice, for that exact marker (:414-418, :648-653,
the latter using a text-length gate "deliberately NOT isChallengeSkeleton,
which short-circuits true on the marker itself").
It bit because Cloudflare JS Detections injects
`/cdn-cgi/challenge-platform/scripts/jsd/main.js` into pages served
SUCCESSFULLY. Measured behavioral: a 193-byte thin SPA shell, a 157-byte
landing page, an 80-byte CSP prose citation. Through cdpDirectFetch that is
this slice's own defect narrowed to Cloudflare-protected thin pages.
Two changes: narrow the marker to `orchestrate/chl_`, which appears in the
interstitial's script path and in neither the sensor path nor a prose
citation; and corroborate vendor markers with `isNearEmptyBody`, a pure
text-length gate that is genuinely independent of the marker that triggered
the check. The shipped `hasChallengeBody && isChallengeSkeleton` rule is left
untouched.
N3 — the Akamai phrase pair was too weak. "Access Denied" is ordinary 403 copy
and a bare "Reference #" ordinary support copy, so a 99-byte help snippet and
a genuine app 403 carrying `Reference #4821` both classified behavioral. Now
requires the reference id's STRUCTURE. Also requires the bang in "Attention
Required!", the CF WAF page's actual title.
N2 — three of the four new MUST-NOT-FIRE tests could not fail. They were long,
so the content guard released them before the vendor arm ran: they asserted
the content guard and proved nothing about the markers. Replaced with SHORT
fixtures that reach the arm and test marker PRECISION. Interception probe
(hasVendorTemplateMarker forced true): 13 red, every negative reaches the arm.
The long articles are kept, relabelled for what they exercise.
N5 — the "never disagrees with the shipped detector" docstring was false, and
is now false in the inverted direction. States both: stricter on four vendor
shapes, looser wherever a status is required.
N4 — the cdp-direct comment stated only the under-fire half of its residual.
Now states both, since naming one direction reads as if the only cost were
incomplete coverage.
M1. `isChallengeSkeleton` returns false on a server-rendered interactive form —
a carve-out written in so many words for "a text-light login screen".
`isNearEmptyBody` has none. Swapping corroborators to break a
self-corroborating pairing inherited what the new predicate LACKS along with
what it does better, and a 299-byte Imperva-protected login page went
none -> behavioral. A new regression on a legitimate short page: the exact
class this slice exists to eliminate, carried inside its own fix. Same shape
as N1 one round earlier.
Worse, the source asserted the gap was "not reachable in practice" and nothing
in the suite could contradict it — the only login-form test pads its body so
the CONTENT GUARD releases it, so the exemption was never exercised. A ceiling
claimed in a comment with no test able to falsify it, which is the same defect
this slice has been fixing all along. Claim removed; a SHORT text-light form
test now covers it.
Two fixes, both measured before adopting:
- the real-form exemption is applied explicitly in the pairing, via a new
exported `hasRealForm` in tls-tier so the carve-out is reusable rather than
re-derivable. This is the structural half: it also inoculates against the
NEXT rider marker, since any vendor string that turns out to ride on served
pages is exempted wherever a real form is present.
- the Imperva marker narrows from the bare `_incapsula_resource` path to the
interstitial's own `CWUDNSAI`/`SWUDNSAI` parameters. Imperva injects that
path into pages it serves successfully, so the bare match was a rider — the
identical sensor-vs-template error fixed for Cloudflare's JSD path a few
lines above, left behind on this entry.
M2. The reference-id regex matched a dotted SECTION number, so
`reference #1.2.3 of the policy` on a 90-byte page classified behavioral. Now
requires at least one hex group of 6+ characters, tested against the MATCHED id
rather than the whole body so an unrelated asset hash cannot license it.
Measured after: Imperva login page and section-number page both none; the
Imperva interstitial stub and a real Akamai denial both still behavioral.
The clause probe caught it: deleting the real-form exemption left the suite GREEN (62 passed), so the test written specifically to cover M1 proved nothing about the carve-out it existed for. Cause: the fixture used Imperva's rider path, and the marker narrowing in the same commit already releases that path. The vendor arm was never reached, so the exemption could not be the thing saving the page. A fixture chosen against one fix was silently rescued by the other. Replaced the marker with "Just a moment..." as a submit-status label — ordinary UI copy on a thin login screen, and one that still matches after narrowing, so the form exemption is the only thing between this page and 'behavioral'. Clause probes now, each mutation diffstat-confirmed: remove real-form exemption -> 1 failed / 61 passed widen Imperva marker to path -> 1 failed / 61 passed drop reference-id long-group -> 1 failed / 61 passed Seventh instance in this slice of one shape: a check that cannot fail for the reason it claims. It was found the same way as the other six — mutate the thing the test names, and confirm the mutation landed.
P1, and the first finding in this slice that fails OPEN. The real-form exemption ANDed onto the vendor arm inverted the precedence. In isChallengeSkeleton the marker short-circuits FIRST and the exemption guards only the LENGTH arm; ANDing it here let a form outrank a positive vendor marker — a carve-out scoped to "is this thin body a skeleton?" promoted to "is this a challenge at all?". Measured: six real walls classified none and were returned to the agent as page content at a synthesized 200 — an Imperva wall behind a cookie banner, an Akamai denial with a site-search box, a Cloudflare wall with a "Try again" button, and three whose "form" was an input outside the element, a commented-out block, or a JS string literal. All are under 1KB, so the density guard in cdpDirectFetch does not catch them either. Every earlier finding cost a rung, a budget or a label; this one produced the outcome the slice exists to prevent. Exemption removed from the vendor arm rather than rescoped. This IS a trade, not a free deletion: the login page it was added for is released by the marker narrowing, but the fixture chosen after the substitutability repair survives that narrowing, so removal costs it. Taken deliberately — six ordinary wall shapes beat one login page nobody has observed live, and the direction differs: the six fail OPEN, the one fails CLOSED and merely costs a rung. Recorded as an accepted limitation at the clause, with the rescope named as the right repair if that shape is ever seen live, plus a test asserting it so the cost is visible and can fail rather than living only in a comment. hasRealForm un-exported — with the vendor-arm consumer gone it had none, and an exported loose predicate is reusable looseness waiting for a second caller. tls-tier.ts is now a byte-for-byte revert to base. REAL_FORM_PATTERN deliberately NOT tightened here. With the vendor arm no longer consulting it, tightening only moves isChallengeSkeleton toward more behavioral — the over-fire direction, in a shared classifier, needing its own negatives and fixture sweep. It is pre-existing and unchanged from base, so it is not this slice's regression. Measured: all six shapes close without it. Transferable, and the inverse of the mistake that introduced it: that change inherited what the new predicate LACKED; this one inherited what it PERMITS. A loose predicate is harmless while it merely suppresses a heuristic and becomes dangerous the moment it can veto a positive match.
Asking "what does this fix open?" — the reviewer's closing instruction — immediately turned up three more shapes in the same class as the one documented: an "Attention Required!" validation banner on a checkout form, an application 403 whose correlation id happens to be dot-separated hex, and a short page naming the Imperva parameters in prose. All classify behavioral. A ceiling described by a single example reads as narrower than it is, which is the same under-statement this slice has been correcting throughout — and it was in the note written to be honest about a trade. Restated generally: ANY of these markers appearing as ordinary content, on a body under the visible-text floor, now classifies behavioral. The measured instances are listed rather than one being offered as representative. Encoded as a table of assertions so the breadth is visible and can fail, rather than living only in a comment.
Q1. The table listed a short page naming the Imperva parameters in prose but not the identical Cloudflare shape. Both measured behavioral: a 102-byte page citing the FULL challenge-platform path in CSP prose, and a 75-byte changelog line naming orchestrate/chl_page. Attribution verified rather than inferred. On a LONG page both classify none via the vendor arm, which places them in this table's class, while cf-browser-verification and _cfChlOpt stay behavioral on a long page through arm 1 and are correctly excluded. This is the enumeration being one short again, in the parallel position — the same shape as the previous round, which is itself the argument for keeping the "general in form, empirical in origin" caveat attached permanently. Also corrected the scope of the prefix negative. It pins the NARROWING and reds when the marker is widened back to the prefix, but it cannot fail for the marker actually in use, so reading it as "prose citations are safe" is what left the Cloudflare entries missing. That limit is now stated at the test with a pointer to where the full-path case lives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
classifyChallenge(src/fetch/challenge-classify.ts) usedisChallengeSkeletonunpaired. That predicate's last arm is a barevisibleText < 600, and its own sibling docstring says it is "deliberately NOT sufficient on its own — callers pair it with an anti-bot STATUS." Used unpaired, every page below the content floor carrying no vendor marker at all classifiedbehavioral— a bot wall.Measured live, same machine, same minutes,
https://example.com/(544-byte DOM read, 142 visible chars) through the realcdp-directrung:DECLINED—challenge-did-not-clearbehavioralCONTENTnoneIdentical byte count in both arms, so this is the classifier flipping, not a content difference. The 11.3 s delta is the clear-poll being burned before the decline.
cdpDirectFetchis the call site that matters because it structurally has no HTTP status to pair with — it navigates and reads the DOM, then pollsclassifyChallenge(html) === 'none'as its clear-check. Since PR #269 madechallenge-did-not-cleara transient decline that warns every time, this false positive was default-visible on any short page.Second instance of one pattern
P0 fixed
isAntiBotSignal, which matched challenge markers alone at 2xx, so an article about bot protection read as a wall. Same shape, opposite half:Both halves are load-bearing, in opposite directions.
The decision procedure — not a tuned threshold
The 600 floor is unchanged. What changed is its logical role: from a sufficient condition to the corroborating half of a pair. A challenge verdict now requires a positive interstitial artifact:
The pairing input a status-free caller can supply is the marker scan — which is exactly what the shipped HTTP-layer rule already uses for a 2xx body (
isChallengeShell: markers AND skeleton, neither alone).cdp-directsynthesizesstatusCode: 200, so the 2xx rule is the status-free rule. Reusing it keeps this classifier in agreement with the shipped detectors instead of inventing a second, weaker procedure.Independent corroboration that this is the right seam:
src/studio/studio-fetch.ts:150-152already reached the same conclusion for the same structural reason — "2xx is assumed because a DOM read carries no HTTP status. That is the CONSERVATIVE assumption: the 2xx branch is the strict one (marker AND skeleton)."Why a threshold tweak could not work: a page can legitimately be 80 bytes, and a real interstitial can be verbose. Both directions are asserted as tests.
Solve-ladder impact: none
runSolveLadder(solve-ladder.ts:95-97) returns UNSOLVED for bothbehavioralandnonebefore any rung. Every body whose class moves under this change moves between exactly those two, so no rung engages differently.browser-pool.ts:1079(the only site feeding the ladder) is unaffected in behaviour.router.ts:934andstudio-fetch.ts:161useclassifyChallengeonly to label an already-confirmed challenge.challenge_class: 'none'on a block was already reachable there pre-change (a header-only challenge with a substantial body), so this introduces no new response shape.Both arms proven non-redundant
px-captcha-errorand the "access to this page has been denied" / "verifying you are human" titles exist only in this module's vocabulary and are absent from the sharedCHALLENGE_MARKERS, so the marker-pair arm alone would miss a PerimeterX denial.dd-loaderis the mirror case. One test each, each verified to red when its arm is deleted.Fixture sweep
One existing fixture intercepted:
tests/unit/studio/studio-fetch.test.tsassertedclassifyChallenge(THIN_BUT_REAL)).not.toBe('none')— a test that encoded this bug as a fact, using it to justify why the studio gate isisChallengeShellrather thanclassifyChallenge.That gate choice is still correct, so the test is restated on the disagreement that survives the fix:
classifyChallengeis status-free, so it cannot reach the status-gated general density rule and UNDER-fires on a markerless novel-vendor wall (verified:isChallengeShell(403, MARKERLESS_WALL) === truewhileclassifyChallenge(MARKERLESS_WALL) === 'none'). Under-firing is the worse direction for a gate, so this is a firmer justification than the one it replaces.No other fixture was intercepted or released.
Also fixed
isNearEmptyBodywas a dead import inchallenge-classify.ts.cdp-directchallenge-did-not-clearremedy text existed purely to hedge against this false positive ("a legitimately short page can classify this way — checkbytes"). It can now assert a challenge honestly.bytesis retained for triage.Verification
npm test: see PR comment (baseline 9539/0/20 skip/7 todo, 826 files)tsc --noEmit: 0challenge-classify.test.tsFalsifiability probe, both directions:
No vacuous tests.
Does this explain D14's
indeed.comdiscrepancy?No — and that is a measurement, not a guess. The classifier cannot decline a body of the reported size. The content guard returns
'none'for anything at or above 600 visible chars measured over the whole document, before the skeleton arm is consulted. Reconstructing D14's shape (600,194 bytes / 600,047 visible chars, anti-bot sensor riding along) classifies'none'— and does so under the old arm too, since that guard is untouched by this change.The better candidate is the rung's budget:
CHALLENGE_CLEAR_TIMEOUT_MS = 12_000withNAV_SETTLE_MS = 1_200. D14's zillow pass took 164 s. If indeed's body needs longer than 12 s to cross the content floor, the rung declines at the clear deadline regardless of any classifier defect — a budget discrepancy, not a classifier one. Confirming that needs a paired same-minute A/B against a walled target with the clear-poll instrumented per tick; deliberately not built here.Follow-up owed by another owner
src/studio/studio-fetch.ts:144-145(FORBIDDEN for this slice) still claimsclassifyChallenge"still under-reports a thin-but-genuine page asbehavioral(measured on example.com in the d14 spike)". That claim is now false. Comment-only; the code is correct and unchanged.