Code Review Findings - #5
Open
JuliDi wants to merge 15 commits into
Open
Conversation
There is no way to point the client at a proxy and no way to change the destination because https://challenges.cloudflare.com/turnstile/v0/siteverify is hardcoded.
It was named/renamed to remote_ip which is not what cloudflare expects. Also add skip_serializing_if for Option::is_none, so None/nil fields are skipped in the json payload. Also drop the `secret` field, it is now populated from the turnstile client. See https://developers.cloudflare.com/turnstile/get-started/server-side-validation/
Before it was necessary to check the success field, which can easily be missed by the caller. It is better to return an error, because one usually expects the verification to pass and everything else can be considered an error
Right now, a malicious response might cause us to buffer a huge response message (depending on any internal limitations of reqwest or so). Since we expect only a couple of hundred bytes, we limit it.
Right now, if cloudflare adds a new error code, this prematurely errors in the parser, losing some more info from the response. This introduces a catchall and removes some old error codes that are no longer used by cloudflare. Also update broken link
test_fail only asserts is_err(). A DNS failure, a TLS handshake failure, a timeout, or a SerdeError from an unparseable body all satisfy it equally; so if valid rejections regressed into parse errors, the test would still pass
Cargo features are additive, so we must allow multiple tls backends so that when cf-turnstile is pulled in to the dependency graph twice with different TLS backends, it must still compile.
Run taplo fmt on the toml files
And update the docs accordingly, mentioning the idempotency feature
joelsa
approved these changes
Aug 6, 2026
| return Err(TurnstileError::UnexpectedStatus(status)); | ||
| } | ||
|
|
||
| let body_bytes = match Limited::new(response.into_body(), MAX_RESPONSE_BYTES) |
Member
There was a problem hiding this comment.
I think limiting this is smart, but maybe should it also be wrapped in a tokio timeout? Otherwise a peer could send headers but never finish.
Comment on lines
+169
to
+171
| // The serialized body contains the secret key. Hand `Bytes` a zeroizing owner | ||
| // so the buffer is wiped when the request is done rather than merely freed, | ||
| // which would leave the key readable in a core dump or swapped-out page. |
Member
There was a problem hiding this comment.
I think this over-promises, a core dump at the right time would still contain the key, because the Zeroize Wrapper is only created once the serialization succeeds. I would just state in the comment, that this is best-practive yet best-effort.
| let http = | ||
| hyper_util::client::legacy::Client::builder(TokioExecutor::new()).build(connector); | ||
|
|
||
| Self { http, secret } |
Member
There was a problem hiding this comment.
Suggested change
| Self { secret, http } |
| missing_docs = "warn" | ||
|
|
||
| [lints.clippy] | ||
| all = "deny" |
Member
There was a problem hiding this comment.
What do you think of this?
Suggested change
| all = "deny" | |
| all = "deny" | |
| pedantic = { level = "deny", priority = -1 } | |
| nursery = { level = "deny", priority = -1 } |
Currently this does not pass with 10 findings, but they are easily fixable and I like the "umm, actually" behavior of pedantic clippy.
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
2 participants
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.
I performed a code review with Claude and it found a few things.
This PR implements fixes (approx. 1 commit per fix) and improves both safety and ergonomics of this crate.