fix(icaptcha): return unsolved instead of blocking or overflowing - #444
beardthelion wants to merge 1 commit into
Conversation
interactive_prompt waited on stdin().read_line even when stdin was an open, silent non-TTY (an orchestrator holding the pipe open), which blocked obtain_proof forever. Check IsTerminal first and return None so the caller surfaces the "couldn't auto-solve" error. The deterministic solvers evaluated prompts with unchecked i64 arithmetic; service-controlled literals could push add/sub/mul/div/abs past the range, panicking in debug builds or wrapping to a wrong answer in release. Use checked arithmetic throughout and return None on overflow. Fixes #345
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe iCaptcha client now skips prompting on non-terminal stdin and uses checked arithmetic throughout its deterministic solvers. Regression tests cover open pipes, overflow boundaries, unsolvable prompts, clean process termination, and zero answer submissions. ChangesiCaptcha hardening
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The non-TTY prompt path and checked solver arithmetic are covered without a retained merge-blocking issue. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR makes iCaptcha failure handling terminate cleanly rather than blocking, panicking, or submitting a wrapped answer.
Confidence Score: 5/5The PR appears safe to merge, with the non-terminal and arithmetic-overflow failure paths handled consistently and covered by focused regressions. No actionable failure remains: unsupported or overflowing challenges now return the existing unsolved error without reading non-terminal stdin or submitting overflow-derived answers.
|
| Filename | Overview |
|---|---|
| crates/icaptcha-client/src/lib.rs | Guards the interactive fallback with IsTerminal and adds a held-open-pipe regression test. |
| crates/icaptcha-client/src/solvers.rs | Replaces overflow-prone solver arithmetic with checked operations and adds boundary coverage. |
| crates/icaptcha-client/tests/icaptcha_stdin_e2e.rs | Exercises real challenge retrieval and unsolved handling in a child with silent non-terminal stdin. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Fetch iCaptcha challenge] --> B[Try deterministic solver]
B -->|Solved safely| C[Submit answer]
B -->|Unsupported or overflow| D{stdin is a terminal?}
D -->|Yes| E[Prompt interactively]
D -->|No| F[Return cannot-solve error]
Reviews (1): Last reviewed commit: "fix(icaptcha): return unsolved instead o..." | Re-trigger Greptile
2 participants
Summary
obtain_proofcould block forever onstdin().read_linewhen stdin was an open, silent non-TTY (a pipe an orchestrator or agent harness holds open), and the deterministic iCaptcha solvers evaluated service-controlled prompts with uncheckedi64arithmetic, which panics in debug builds and wraps to a wrong answer in release.Motivation & context
Closes #345. The interactive prompt is the documented last resort (returns
Nonewhen stdin isn't a usable interactive source), but it never checked whether stdin was a terminal, so the fallback itself was the hang. Separately, every literal in a challenge prompt can individually parse asi64while the evaluation still overflows.Kind of change
What changed
icaptcha-client:interactive_promptchecksIsTerminalbefore prompting and returnsNoneon non-TTY stdin, so the caller surfaces the existing cannot-solve error.icaptcha-client:solvers.rsuseschecked_*arithmetic throughout (add/sub chains, algebra coefficient/constant/denominator/numerator includingi64::MIN / -1, sequence diffs, geometric multiply,abs()ati64::MIN, and theisqrt_exactcandidate square). Overflow returnsNone(unsolvable) rather than panicking or submitting a wrapped answer.tests/icaptcha_stdin_e2e.rs) runs the realobtain_proofin a child process over real HTTP against a mocked iCaptcha service with stdin held open as a silent pipe, for both an unsolvable type and an overflowing arithmetic prompt;/v1/answeris pinned at zero calls so a wrapped answer would fail the test.How a reviewer can verify
cargo test -p icaptcha-clientReverting the
IsTerminalcheck makes the e2e block for the full 15s deadline; revertingchecked_addto+=panics the overflow test.Before you request review
cargo test -p icaptcha-clientpasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNone: client-side challenge solving only; no wire format, signature, or API change.
Notes for reviewers
The
gl-binary path throughsend_signedis not exercised end-to-end locally becauseresolve_solver_urldeliberately refuses non-https advertised solver URLs (the SSRF guard); the child-process e2e drives the sameobtain_proofglcalls viaspawn_blocking.Summary by CodeRabbit