Skip to content

Report parser failures and blocks honestly, cap paid solves per page, write output atomically - #7

Merged
jehrr merged 2 commits into
mainfrom
fix/audit-2026-10-14
Oct 15, 2026
Merged

jehrr merged 2 commits into
mainfrom
fix/audit-2026-10-14

Conversation

@jehrr

@jehrr jehrr commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

An audit of main at b21c30a (2026-10-14) found these contract gaps, each reproduced offline:

  • Parser failure was reported as an empty search. If a page rendered its listing grid (more than 5 listing links seen) and the parser returned 0 rows, the run said "zero products", exit 4. Also, a run where page 1 was served empty and page 2 failed exited 4. The Scraper API path exited 5 both for a served empty page and for a Cloudflare challenge.
  • No per-page solve budget. One page that answered 503 with a Turnstile widget bought retries + 1 solves (Playwright --retries 5 → 6). A page with both a Turnstile and a reCAPTCHA marker bought 2.
  • Non-atomic writes. A failed json.dump truncated a 497-byte run.json (and the .meta.json sidecar) to 13 bytes of invalid JSON.
  • No CSV formula escaping. Text cells such as =HYPERLINK(...) were written to the CSV unchanged.
  • Bad flag values were accepted. --retries -1 ran zero navigations and reported exit 5. --pages 0 and --concurrency 0 were accepted. --proxy with --cdp-endpoint silently dropped the proxy, and --captcha-api was parsed but never read.
  • The credential scan missed some files. It never opened .json fixtures, and it could report a tree clean after reading no files.
  • The canary never tested the site. The ZIMMO_PROXY secret exists but reaches the workflow empty, so the live job was skipped every day while the workflow showed green.
  • Public files cited private maintainer notes about 80 times. The README stated a "credentials last about a day" figure that has since been retracted. The Dockerfile described a CI check that does not exist.

Resulting behaviour

  • Pages are classified by the shared classify_page(), which now has a parser_failed outcome. With no rows recovered the run exits 5; with rows from other pages it exits 6 (partial), and the sidecar gets parser_failed_pages and a stop_reason that names the parser. If no rows were recovered and any page failed, the exit is 5. The Scraper API path uses the same page classification (http_code, challenge detection, rendered grid) and records selection.url.

  • captcha_solver.SolveBudget: each engine creates one budget per page, before its attempt loop. solve_turnstile() and solve_recaptcha_v3() require the budget and spend it immediately before submitting a solve. A missing key or a Managed Challenge without a sitekey does not spend it.

  • All three output files are written through a temp file in the same directory, then flush, fsync and os.replace; the temp file is removed on error. A new file gets mode 0o666 & ~umask, and an existing file keeps its mode.

  • In the CSV, text cells starting with = + - @ \t \r \n get a leading '. Numbers and the JSON output are unchanged. The sidecar gets csv_cells_escaped.

  • Argparse now rejects bad values with exit 2 in all three engines:

    • --retries, --delay and --retry-delay must be ≥ 0;
    • --pages, --concurrency and --proxy-block-retries must be ≥ 1;
    • --proxy or --proxy-file together with --cdp-endpoint is refused, whether set by flags or by .env.

    --captcha-api is removed.

  • Product.currency defaults to null. The JSON-LD path no longer assumes EUR.

  • The credential scan:

    • also reads .json, .csv, .html and .cfg files;
    • skips virtualenvs, detected by pyvenv.cfg;
    • matches the sample_output allowlist on the file name rather than the full path;
    • prints the number of files scanned, and fails if it scanned 0.
  • The canary now fails with ::error:: naming ZIMMO_PROXY when the secret is missing or empty. It no longer has a gate or skip, and any nonzero exit is red. The row floor and the non-null field checks are unchanged.

  • Citations of private maintainer notes are removed from every tracked file, including two lines in released CHANGELOG sections (citation only). A suite check (check_no_private_citations) fails if one comes back. Release notes for v0.4.0 through v1.2.1 contained none, so none were edited.

  • README: the credential-lifetime wording is corrected, the flag table and exit codes are updated, and the Docker image is documented as needing --cdp-endpoint or a display. The Dockerfile comment is now true: smoke_test.py checks the COPY list against Playwright's import closure.

Evidence

  • python smoke_test.py, Python 3.13, venv with only requirements.txt: exit 0, 85 PASS, 3 SKIP (the engines' drivers are not installed). With each engine's venv (requirements.txt + requirements-<engine>.txt, no browser download): exit 0, 86 PASS.

  • pytest tests/: core 9 passed; playwright 37 passed; puppeteer 33 passed; selenium 26 passed. The new tests/test_paid_solves_and_verdicts.py runs each engine's real fetch loop against a fake browser, with both 2captcha submit functions stubbed. Results:

    • --retries 5 on a 503 page with a Turnstile widget makes 6 navigations and buys 1 solve.
    • A page with two challenge markers buys 1 solve.
    • Two pages buy 1 solve each.
    • A rendered page that parses to nothing exits 5 and is not retried.
    • A good page 1 followed by a parser-failed page 2 exits 6 with parser_failed_pages == [2].
    • Bad flag values exit 2.
  • --help works for all three engines and scraper_api_client.py. compileall is clean. check_no_credentials.py reports "No committed credentials found (36 files scanned)".

  • Planted-fault controls, each run with PYTHONDONTWRITEBYTECODE=1 and __pycache__ cleared: each fault turned the named check red, and the suite was green again after restoring.

    Fault planted Check that went red
    open(path, "w") instead of the atomic writer atomic-write check
    chmod of the temp file removed file-mode check
    CSV escape disabled CSV formula check
    budget.spend removed solve-budget check
    SolveBudget() moved inside the attempt loop (Playwright) AST check
    SolveBudget() moved inside the attempt loop (Selenium) pytest test_one_paid_solve_per_page_across_retries
    classify_page parser branch disabled 4 classify cases, Scraper API check, pytest parser test
    --retries type=int (Selenium) flag check
    --retries type=int (Playwright) pytest test_bad_flag_values_are_usage_errors
    Scraper API blocked= dropped Scraper API verdict check
    A private citation added to the README citation guard
    A module dropped from the Dockerfile COPY list COPY-list check
    .json dropped from the credential scan scan check
    Canary secret check neutralised canary check
    The old pages_completed == 0 condition restored mixed empty/failed check
  • No live request, paid call, proxy or credential was used. The 2captcha submit functions and requests.post were stubbed in every probe.

Canary

  • Before: in the last 10 runs, the 4 on b21c30a show gate success and canary skipped (HAS_ZIMMO_PROXY: false). Earlier runs on fa41651 ran without a proxy, got HTTP 403 and exit 3, and passed with a notice.
  • After: the workflow fails red until ZIMMO_PROXY holds a residential proxy URL. Belgian and German exits are known to be served; a US exit was refused on 2026-10-13. The secret currently exists but is empty.

Not changed

  • The challenge markers that a Scraping Browser session may inject (bare turnstile / cf-turnstile-response), the sitekey fallback, and the render=explicit mapping. Changing them needs real served and refused captures over --cdp-endpoint.
  • The family flags --fingerprint, --fp-*, --locale and --mode, fingerprint_client.py, and the fingerprints topic. These are owner decisions.
  • Tagging or releasing 1.3.0, and any version bump. New entries are under [Unreleased].
  • No shared fetch-loop refactor, no new output sinks, no warning for unknown .env keys, no CSP check on the string-based readiness wait.
  • Repo metadata. The org audit notes only the description's length, and that is informational.

🤖 Generated with Claude Code

jehrr and others added 2 commits October 15, 2026 17:48
… and write output atomically

- A rendered listing grid that parses to nothing is a parser failure
  (exit 5, or 6 with other rows), not an empty search (exit 4); the
  Scraper API path uses the same page verdicts.
- One paid captcha solve per page across attempts, rotations and
  markers, checked before purchase in all three engines.
- JSON/CSV/sidecar written via temp file + os.replace, modes preserved;
  formula-shaped CSV text cells escaped and counted.
- Negative --retries/--delay and zero --pages/--concurrency rejected;
  --proxy with --cdp-endpoint refused; inert --captcha-api removed.
- Credential scan reads .json/.csv/.html/.cfg fixtures and detects
  venvs by pyvenv.cfg; refuses to report an unscanned tree clean.
- Canary fails, naming ZIMMO_PROXY, instead of skipping green.
- Public files no longer cite private maintainer notes; a suite check
  keeps it that way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e log masks

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jehrr
jehrr marked this pull request as ready for review October 15, 2026 22:19
@jehrr
jehrr merged commit 2cffce5 into main Oct 15, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant