fix: friends whole-tool review — 15 fabrication/silent-drop/coverage fixes#14
Merged
Conversation
…h-bypass fabrications
Friends full-tool review (Group A). Three scanner markers landed in mfa/ and
saml/ findings.txt, which map to the (critical/9.8) auth_bypass template, and no
prefix suppressed them — so each shipped as a fabricated CRITICAL Broken-Auth
finding in client reports:
- [MFA-RESPONSE-MANIP] fires when an OTP endpoint returns {"success":false}
for a WRONG code — i.e. SECURE behaviour. scanner.sh's own comment called it
'indicator only'. Pure false positive.
- [MFA-NO-RATE-LIMIT] a missing-429 rate-limit gap (at most MEDIUM), not an
auth bypass.
- [SAML-METADATA-EXPOSED] a public SP/IdP metadata document (public by design);
a LEAD for XSW, not a bypass. Its extracted saml/certs.txt cert blobs were
ALSO each ingested as their own CRITICAL.
Fixes:
- reporter.py: suppress the three prefixes + add certs.txt to NON_FINDING_FILES.
- reporter.py: remove the dead shadowed high/8.1 'Authentication Bypass' template
literal (silently overwritten by the critical/9.8 'Broken Authentication'
reassignment) — the footgun that made these resolve to 9.8.
- scanner.sh: route all three to manual_review/ as leads (root cause), matching
the existing -CANDIDATE siblings. The CONFIRMED [MFA-WORKFLOW-SKIP] and
[SAML-SIG-STRIP] markers stay in findings.txt.
TDD: 3 new/extended tests (fabrication-leaks SUPPRESS+KEEP, template-not-shadowed,
scanner marker routing). Reporter/mfa/saml/scanner regression set: 366 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ient reports Friends full-tool review (Group B) — three engines wrote CONFIRMED CRITICALs into finding dirs the reporter never ingested (only a WARNING), so real vulnerabilities the tool had already proven were silently dropped from the client report: F5 nextjs_bypass/ — whitebox/nextjs_bypass.py writes a CONFIRMED CVE-2025-29927 middleware auth bypass as '[CRITICAL] ... url'. Dir was unmapped -> dropped. Fix: map SUBDIR_VTYPE['nextjs_bypass'] = 'auth_bypass'. F6 sqlmap_reqfile/ + sqlmap_post/ — run_sqlmap_request_file (--request-file) and run_sqlmap_targeted's POST pass write confirmed sqlmap results CSVs into these sibling dirs; reporter Method 1f read ONLY sqlmap/. Fix: Method 1f now reads all three (and treats results.txt as a primary CSV name); both added to meta_dirs. F13 HAR report — main() passed reporter.py the result JSON *file*; reporter __main__ requires a DIRECTORY (Method 1c reads har_vapt_*.json inside it) and exits 1 'Not a directory', so EVERY HAR report produced nothing while 'Done' printed. Fix: extract _dispatch_har_report(output_dir) — passes the dir, surfaces a non-zero reporter exit instead of swallowing it. Verified end-to-end: a real reporter.py run over a dir with nextjs_bypass/ + sqlmap_reqfile/ now emits VN-001 (Broken Auth 9.8) and VN-002 (sqlmap-confirmed SQLi 9.8). TDD: 6 new tests. Reporter/sqlmap/har/vikramaditya set: 377 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… C part 1)
Friends full-tool review — autopilot + reporter fabrications:
F12 autopilot_api_hunt.py — the AWS Access Key ID in a SigV4 presigned URL
(X-Amz-Credential=AKIA...) is PUBLIC/non-secret by design (the secret signs
the URL but never appears in it). Reporting it as HIGH aws_key_exposed was a
false positive on any working S3 presigned-upload API. Removed the emission.
F11 autopilot_api_hunt.py IDORScanner — flagged HIGH idor whenever a 200 body
held any PII-named field, with NO owner baseline: an endpoint that ignores
the id and returns the CALLER's own record was flagged. Now collects the PII
value-set per (request shape, id) and only confirms when >=2 ids return >=2
DISTINCT records; emits once per endpoint. (A hardened cross-user tester
already existed in api_idor_scanner.py but was orphaned.)
F10 reporter.py — the IDOR / chained-attack / timing PoCs hard-coded fake victim
PII ('Alice' / victim@example.com / 9000000000) and fake latencies
(6.6s,6.1s,6.4s) under 'WHAT THE SERVER RETURNS (actual response)' /
'ACTUAL DATA LEAKED' / 'ACTUAL TIMING DATA' headers — invented data shown as
real evidence in a client report. All four blocks now render the finding's
REAL captured evidence (or neutral language when absent).
TDD: 5 new tests (SigV4 FP, IDOR owner-baseline benign+positive, IDOR PoC uses
real evidence, no fabricated victim constants in source). RED confirmed for F10 via
stash. reporter/autopilot/idor set: 238 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(F4)
Friends full-tool review F4 — har_vapt_engine declared CRITICAL SSTI whenever the
response contained the substring '49' after injecting {{7*7}}, with no baseline,
no distinctive canary and no reflection guard. Any page containing 49 (a price,
an id, '49 results') became a fabricated CRITICAL.
New _probe_ssti: injects a DISTINCTIVE arithmetic canary (91193*90007), confirms
the product is ABSENT from a baseline (un-injected) response, and requires the raw
expression NOT to be reflected verbatim (reflection != evaluation). Fires once per
param for both {{...}} and ${...} wrappers.
TDD: 2 tests (reflection+incidental-49 is benign; real evaluation fires). HAR set:
124 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Friends full-tool review — coverage gaps:
F14 auth_utils.AuthSession.request — collapsed EVERY exception (TLS-verify
failure, timeout, conn reset) to {'status': 0}. Downstream 'status in
(200,201)' checks treat that like a benign non-hit, so a systematic
transport failure (bad/expired bearer, proxy outage) completed the scan
with 0 findings and NO coverage-loss signal. Now returns transport_error=
True + error, counts self.transport_errors/total_requests, and logs a
stderr WARNING at the 1st/5th/25th/every-100th failure.
F8 ldap_injection_tester.looks_like_ldap_backed_auth — intersected
cve.detect_technologies names (php/iis/tomcat) with a marker set
(active-directory/adfs/spring-security/...) those names never contain, so
the LDAP phase ALWAYS skipped — an AD/ADFS login on IIS read clean. The
gate now also fires on enterprise SSO/ADFS/CAS login URL paths and on
NTLM/Negotiate/Kerberos WWW-Authenticate challenges; hunt.py feeds it the
crawled recon URLs.
TDD: 6 tests (transport-error marked+counted vs real 404; gate fires on tech tag
/ ADFS-SSO URL / NTLM, still skips a plain PHP app). ldap+auth_utils sets green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on (F9, F7)
Friends full-tool review — last two Group C findings:
F9 brain_scanner._verdict_findings — a verdict was tagged [VERIFIED — grounded
run] whenever ANY script produced output (grounded = successful_runs > 0),
so a script printing only 'Server: nginx/1.24' grounded an unrelated
'SQL injection CONFIRMED' verdict, which the reporter KEPT as a verified
critical. Now accumulates the grounded stdout and DOWNGRADES to [MODEL CLAIM]
when that output is purely passive fingerprint/banner data (no exploitation
evidence). Conservative: substantive-but-unclassified output is never
downgraded (no regression), and omitting grounded_output preserves legacy
behaviour for existing callers.
F7 jwt_kid_injection.confirm_replay — implemented + tested but NEVER called, so
a genuinely accepted RS256->HS256 / kid forgery produced no finding. New
confirm_replay_any bounds the replay over candidate endpoints and fails
closed; run_jwt_audit now replays the forged token against recon API
endpoints and writes [JWT-KEY-CONFUSION-CONFIRMED] (a real critical) ONLY on
confirmed acceptance — otherwise it stays a manual-review lead.
TDD: 8 tests (grounding passive-banner downgrade / real-evidence keep / legacy /
ungrounded; confirm_replay_any first-confirmed / none / skips-error / cap-bounded).
brain_scanner + jwt sets green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Whole-codebase review by the friends (codex
gpt-5.6-sol+ grokgrok-4.5, independent, in parallel). Every finding was primary-source-verified against live code (several reproduced viareporter.load_findings) before fixing. 15 findings fixed across 6 TDD commits; full suite 2373 passed / 5 skipped / 30 subtests.The dominant theme: the tool both invented client-facing CRITICALs and hid real ones it had already proven. Both classes are now closed.
Fabrication — invented client CRITICALs
[MFA-RESPONSE-MANIP](fired on secure behaviour),[MFA-NO-RATE-LIMIT],[SAML-METADATA-EXPOSED]+saml/certs.txtall shipped as CRITICAL 9.8 auth-bypass. Suppressed + routed tomanual_review/; removed the dead shadowedhigh/8.1auth_bypass template literal that made them resolve to 9.8.Alice/victim@example.com/9000000000/6.6s) under "actual response" headers → now render the finding's real evidence.aws_key_exposed→ removed.'49' in body→ distinctive canary + baseline + not-reflected.[VERIFIED]) → downgrades to[MODEL CLAIM]when grounded output is a passive banner only.Silent drops — hid real CRITICALs
nextjs_bypass/(CVE-2025-29927) unmapped, F6sqlmap_reqfile/+sqlmap_post/confirmed SQLi never read by Method 1f, F13 HAR report handed reporter a file where it needs a dir (exit 1, "Done" still printed). All fixed; verified end-to-end with a realreporter.pyrun.Coverage gaps
confirm_replayorphaned → wired viaconfirm_replay_any(bounded, fail-closed).detect_technologiesnever emits → now fires on ADFS/SSO URLs + NTLM/Negotiate.auth_utilstransport errors collapsed tostatus:0→ markedtransport_error+ counted.Testing
27 new TDD tests (RED→GREEN for each). Full suite green. No client identifiers in the diff.
🤖 Generated with Claude Code