From 4bd25c093ef5d8fe294ea2b56136549abd6394f5 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Wed, 23 Sep 2026 05:46:04 +0000 Subject: [PATCH] =?UTF-8?q?review:=20rfc1928=20post-fork=20full=20review?= =?UTF-8?q?=20=E2=80=94=20001?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security + code-smell + coverage review of the targeted fork as owned code (the extraction review was narrow; this is the full pass). Findings: 1 major (F-1: reply_success/reply_error write replies with .write() — a partial-write hazard invisible to clippy's unused_io_amount under the err_when-chaining idiom, executable-probe verified; rfc1928.md's write_all rationale is false for these sites), 7 minors (2 security-posture notes, 2 extraction leftovers, 3 coverage gaps), 4 observations. No critical findings. Verification: 38+32 tests, clippy (all-targets + wasm32) -D warnings, fmt, doc, wasm check, publish dry-run, MSRV 1.88 native+wasm — all green. Coverage 92.63% regions / 95.40% lines (cargo-llvm-cov); gaps attributed per finding. Differential suite's 15 upstream citations spot-checked against the v1.0.0 reference checkout — all accurate. --- docs/reviews/001-rfc1928-full-review.md | 653 ++++++++++++++++++++++++ 1 file changed, 653 insertions(+) create mode 100644 docs/reviews/001-rfc1928-full-review.md diff --git a/docs/reviews/001-rfc1928-full-review.md b/docs/reviews/001-rfc1928-full-review.md new file mode 100644 index 0000000..9fa99c9 --- /dev/null +++ b/docs/reviews/001-rfc1928-full-review.md @@ -0,0 +1,653 @@ +--- +status: open +last_updated: 2026-09-23 +reviewed_artifacts: + - src/rfc1928/mod.rs + - src/rfc1928/server.rs + - src/rfc1928/client.rs + - src/rfc1928/util/mod.rs + - src/rfc1928/util/stream.rs + - src/rfc1928/util/target_addr.rs + - src/rfc1928/LICENSE + - src/lib.rs + - tests/rfc1928-differential.rs + - Cargo.toml +tool: manual source read + cargo test/clippy/fmt/doc/wasm + cargo-llvm-cov + reference-checkout probes +reviewer: post-fork full review (security + code smells + coverage) +--- + +# Review 001 — `src/rfc1928/` Post-Fork Full Review + +## Purpose + +First full review of the targeted fork (`src/rfc1928/`) as owned code. +The extraction review (`tasks/implementation/ +review-vendor-extraction.md`) was deliberately narrow — extraction +fidelity vs the reference checkout — and its six criteria are now +superseded in part by ADR-013 (the fork posture dissolves the +verbatim-preservation frame). This review asks the three questions +the extraction review did not: + +1. **Security**: where RFC 1928 wire data meets allocation, panic, + error-mapping, or trust boundaries — reviewed independently of + upstream (upstream's choices are not this crate's security + posture anymore). +2. **Code smells**: convention-compliance (AGENTS.md) plus + dead/duplicated/misleading surface left over from the extraction + and the fork pass. +3. **Coverage** (`cargo-llvm-cov`): important gaps, not raw numbers — + attributed to specific behaviors and their risk. + +The scope is `src/rfc1928/` only (the forked core). The +producer/consumer/adapter layers do not exist yet (the tree is the +core plus the differential suite); the doors and sessions named in +the architecture docs are future work. That boundary is what makes +several findings below "dead code today, load-bearing when the +wrapper lands" — flagged as such rather than as current bugs. + +## Methodology + +- Full read of all five forked source files + the differential suite + + the module docs, cross-referenced against ADR-011/013 and + `docs/architecture/rfc1928.md`. +- Line-by-line behavior comparison against the reference checkout + (`/workspace/fast-socks5` v1.0.0) for every security-relevant path: + length-driven allocation, reply-code mapping, error→reply-code + mapping, auth state machine, the `read_exact!` macro's + allocation expression, `copy_bidirectional` semantics. +- Independent verification of RFC 1928 edge behaviors (unassigned + reply codes, domain caps, RSV/FRAG handling, method-preference + order) against RFC text — not against upstream. +- `cargo test` (38 lib + 32 differential + 2 doc), `cargo clippy + --all-targets -- -D warnings` (clean), `cargo fmt --check` (clean), + `cargo doc --no-deps` (0 warnings), `cargo check/clippy --target + wasm32-unknown-unknown -- -D warnings` (clean), `cargo publish + --dry-run --allow-dirty` (packages cleanly, 35 files 279.5 KiB). +- MSRV: `cargo +1.88 check` native + wasm32 — passes (the manifest's + `rust-version = "1.88"` is real; 1.85 was never claimed). +- `cargo llvm-cov` (summary + `--show-missing-lines`) — per-file + attribution below. +- **Executable probes for the clippy-suppression finding** (F-1): + a scratch crate (`/tmp/opencode/io-probe`) reproducing the exact + generic/`err_when` shape; five variants driven to isolate the + mechanism. All probe files deleted after the run. + +## Verification Baseline + +All commands on the reviewed tree (commit `f8c3444`, clean): + +- `cargo test`: **38 lib + 32 integration + 2 doc (ignored) — all + pass**. +- `cargo clippy --all-targets -- -D warnings`: **clean**. +- `cargo clippy --target wasm32-unknown-unknown -- -D warnings`: + **clean**. +- `cargo fmt --check`: **clean**. +- `cargo doc --no-deps`: **0 warnings**. +- `cargo check --target wasm32-unknown-unknown`: **clean** (the + wasm-clean invariant holds — no `net`-shaped feature, tokio on the + wasm-clean subset). +- `cargo publish --dry-run --allow-dirty`: **completes** (35 files, + 279.5 KiB). +- `cargo +1.88 check` (native + wasm32): **clean**. +- `cargo llvm-cov`: **92.63% regions, 95.08% functions, 95.40% + lines** (1499 covered / 69 missed). + +### Coverage breakdown + +| Module | Lines | Functions | Regions | +|---|---:|---:|---:| +| client.rs | 95.44% | 98.31% | 93.81% | +| mod.rs | 97.30% | 95.65% | 93.36% | +| server.rs | **92.75%** | **89.86%** | 91.09% | +| util/stream.rs | 100.00% | 100.00% | 78.57% | +| util/target_addr.rs | 98.47% | 100.00% | 93.37% | +| **TOTAL** | **95.40%** | **95.08%** | **92.63%** | + +Overall healthy for a protocol codec. The uncovered lines +(`--show-missing-lines`) decompose into: the client's v6 +request-encoding arm (testable, untested), the `AsyncWrite` +impl's `poll_flush`/`poll_shutdown` passthroughs, the +`StandardAuthentication` macro arms (dead public surface today), +`SocksServerError::to_reply_error`'s non-`Bug` fall-through arms, +`accept_password_auth`'s `CheckResult` impls for `Option`/`bool`, +and assorted error arms. Attribution lives with the findings below. + +## Summary Statistics + +| Severity | Count | IDs | +|----------|------:|-----| +| Critical | 0 | — | +| Major | 1 | F-1 | +| Minor | 7 | S-1, S-2, C-1, C-2, L-1, L-2, L-3 | +| Observation | 4 | N-1, N-2, N-3, N-4 | + +No critical findings. The one major is a latent correctness hazard +masked by a clippy-lint gap (F-1 — the same class as the vendor +review's F-1: a hazard invisible to the automated gates). The minors +split: two security-posture notes that are dead-code today (S-1, S-2), +four extraction leftovers that become real decisions when the wrapper +lands (C-1, C-2, L-1, L-2), two small coverage gaps (L-2, L-3), and +the smell findings (F-1's doc face, N-1..N-4). + +Findings use the alkty/alktunnels prefixes adapted to a single-file +scope: `F` (found-correctness/security), `S` (security posture), +`C` (code smells/convention), `L` (coverage gaps), `N` +(observations/notes). + +--- + +# Part A — Findings + +## F-1 [major] — `reply_success`/`reply_error` write replies with `.write()`, not `.write_all()`: a real partial-write hazard, invisible to clippy because the `err_when` chain suppresses `unused_io_amount` — and rfc1928.md's rationale claim is false for these sites + +**Files**: `src/rfc1928/server.rs:516` (`reply_success`), +`src/rfc1928/server.rs:529` (`reply_error`); +`docs/architecture/rfc1928.md:155-158` (the rationale claim) + +**Problem**: both reply writers send the 10/22-byte reply with +`tokio::io::AsyncWriteExt::write`, which may write fewer bytes than +offered (a partial write). Upstream has the same calls verbatim +(`src/server.rs:932`, `src/server.rs:948`) — this site rode the +extraction unchanged. The client-side sibling (`request_header`, +upstream `src/client.rs:331`) was correctly converted to `write_all` +at extraction, and rfc1928.md records exactly that: *"`request_header` +writes with `write_all` where upstream wrote with `write` (a partial +write of a request header would corrupt the conversation)"*. But the +record frames it as a `clippy::unused_io_amount` consequence +("byte-identical on a healthy stream — `clippy::unused_io_amount` is +deny-by-default"), and **the server-side `reply_*` sites survived +despite the same lint applying** — so the claim as written is false +for them, and the hazard they carry is live: + +1. **The hazard is real on the target substrates.** A reply is + written to a `BiStream`/duplex half in the channels world, where + `poll_write` is a bounded-channel send and *can* accept a prefix + under memory pressure (the channel's bounded per-channel buffer is + exactly the partial-write producer on this substrate — the same + class of behavior tokio documents for `AsyncWrite` generally: + "a call to write represents an attempt to write some bytes"). A + short write of a 10-byte reply desynchronizes the conversation: + the client's `read_request_reply` parses the tail of the reply as + the start of proxied data (or vice versa on a second reply — + reply#2 of BIND), and the two sides' protocol streams separate. + The failure is silent misalignment, not a typed error. +2. **Why the gate misses it (executable probe, scratch crate, five + variants)**: `clippy::unused_io_amount` is deny-by-default and + fires on `.write()` in three control shapes (bare generic + receiver, struct-field-of-generic receiver, concrete receiver — + all confirmed erroring in the probe). It is suppressed by the + `err_when(...)?` chain: `ErrorContext`'s blanket impl is generic + over `T` for `Result`, so `.write(..).await + .err_when(...)?` type-infers `T = usize` *without the lint's + type-walk ever binding a concrete `usize`* — the lint's + expression-shape check misses it. The probe's minimal + disambiguation: the same `.write().await.err_when(...)?` under a + concrete receiver type fires when the chain resolves through + `io::Result`, and stays silent through the generic + blanket-impl path (exactly the typestate's shape); `map(|_| ())` + chains show the same suppression; `write_all` never trips the + lint regardless. So the codebase's standard error-chaining + idiom is a lint-suppressor for exactly this class of bug — worth + knowing beyond these two sites. +3. **The docs currently mis-teach.** rfc1928.md's adjustment list + presents the `write_all` conversion as a lint-driven, client-side + fix and leaves the reader to conclude the `reply_*` sites were + somehow out of the lint's reach. The truth is worse than the doc + says and better than "unknown": the sites are identical hazards + that the lint *would* flag in isolation but doesn't flag here. + A review that leaves the doc claim standing certifies a false + invariant. + +**Fix (small)**: `.write(&reply).await` → `.write_all(&reply).await` +at both sites (identical semantics on a healthy stream, the RFC +reply is a single bounded frame, and `write_all` is the project's +established posture — the client site and every other write in the +fork already use it). Update the rfc1928.md bullet to record the +server-side sites as part of the same adjustment (one clause: "and +`reply_success`/`reply_error` likewise — the `unused_io_amount` +linter's blind spot under `err_when`-chaining is not a substitute +for the conversion"). No wire change, no API change. + +**Lift**: closes the one live correctness hazard in the forked code +and makes the doc invariant true. Trivial effort (two call sites + +one doc bullet); the differential suite already covers both reply +forms byte-for-byte, so the fix is regression-guarded. + +--- + +## S-1 [minor] — `read_address`'s domain arm allocates from an attacker-controlled byte with no cap before allocation — `vec![0u8; len as usize]` trusts a 255-max that only the *encoder* enforces + +**Files**: `src/rfc1928/util/target_addr.rs:222-229` (`read_address`) + +**Problem**: the domain arm reads a 1-byte length, then immediately +allocates `vec![0u8; len as usize]`. The cap is real — the wire +length field is one byte, so `len ≤ 255` by construction, and 255 +bytes is bounded — **but the code's safety rests on that implicit +fact with no assertion, doc note, or test pinning it**. The parallel +sites in the same file and the client bound *explicitly*: the +client's `request_header` checks `domain.len() > u8::MAX` before +encoding; `to_be_bytes` checks `DomainLenTooLong` before encoding. +The decode side has no corresponding visible bound — a reader (or a +future refactor that widens the length field, e.g. a hypothetical +FRAG-style extension or a misreading of the "1-byte length" rule) +would turn this into an unbounded allocation from wire input. + +Today the 1-byte field makes it safe: this is a posture finding, not +a bug. But the crate's own security posture elsewhere treats +wire-driven allocation as a named, guarded boundary (`TargetAddr` +doc: "The domain name will be passed along to the proxy server"; the +differential suite pins the 255 cap on the encode side). The decode +side — the side where the attacker controls the bytes — deserves the +same visible guard. + +**Recommendation**: one doc line on `read_address` ("the 1-byte +length field bounds the allocation at 255 — widening the field +requires a cap here") plus a pinning test asserting the 255-byte +domain decodes and a 256-byte claim is impossible by construction +(the suite's `command_domain_255_bytes_is_the_wire_maximum` already +pins 255 on the *server* path — the gap is only that nothing states +the invariant). Alternatively add an explicit +`DomainLenTooLong`-shaped check; zero behavior change. Low priority +— filed so the invariant is written down before the producer half +routes attacker traffic through this codec in production. + +## S-2 [minor] — The server's auth-reject path sends `SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE` (0xff) as the *password sub-protocol's* status byte — faithful to upstream, but it is not the sub-protocol's defined rejection byte, and the ambiguity is undocumented + +**Files**: `src/rfc1928/server.rs:352-359` (`reject`), +`src/rfc1928/server.rs:344` (`accept`'s success byte) + +**Problem**: RFC 1929 (the username/password sub-protocol) defines +the status byte as: `0x00` = success, *any other value* = failure. +The fork (following upstream) writes `[1, 0xff]` on reject — 0xff +being `SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE`, the *method-negotiation* +phase's constant, repurposed here. This is RFC-1929-compatible (any +non-zero works) but conflates two vocabularies, and the module docs +present the auth flow as RFC-facing. The observable consequence is +benign today (interoperable servers/clients accept any non-zero), +but: (a) a strict peer that treats the status byte as a boolean +"0x00 vs anything" is fine, while one that echoes the value back for +logging may mislabel it; (b) the in-tree `client.rs` check +(`is_success != SOCKS5_REPLY_SUCCEEDED`) is written against 0x00 — +the pairing works but only because both sides happen to use the +same arbitrary constant. + +**Recommendation**: a doc comment on `reject` naming the choice +("0xff is the convention upstream picked; RFC 1929 requires only +non-0x00") — or switch the constant to a literal `0xff` with a +comment detaching it from the method-negotiation vocabulary. No +wire change (the bytes are upstream-faithful and interoperate); +the finding is the undocumented vocabulary conflation. Trivial. + +--- + +## C-1 [minor] — Dead error vocabulary: `SocksServerError::Bug` and `ConnectError` are constructed nowhere — extraction leftovers with no consumer in the forked region + +**Files**: `src/rfc1928/server.rs:72-73` (`Bug`), +`src/rfc1928/server.rs:66-67` (`ConnectError(#[from] ...)`), +`src/rfc1928/util/stream.rs:46-60` (`ConnectError` enum), +`src/rfc1928/util/stream.rs:62-74` (`to_reply_error`) + +**Problem**: upstream constructs `SocksServerError::Bug(...)` three +times — all in the cut kernel-net region (`src/server.rs:1066`, +`1174`, `1250`: `DnsResolveHelper`/`run_udp_proxy_custom`'s addr +post-processing). Upstream constructs `ConnectError` exactly once — +in the cut `tcp_connect` dialers (`src/util/stream.rs:89-104`). In +the fork, both are **unconstructable dead code**: grep across `src/` +and `tests/` finds zero construction sites. `SocksError`'s +`ServerError(#[from] SocksServerError)` transport keeps them +reachable in the public API (they compile, they document, they +map — but nothing can ever produce one). The +`to_reply_error` mappings for `ConnectError` are tested +(`util/stream.rs` tests) — testing a mapping for an error no code +can raise. + +The extraction review's F-4 accepted a *string* naming a cut +function as a fidelity keep; the fork dissolved that rationale, and +these two are the remaining dead vocabulary from the same category. +`rfc1928.md`'s not-forked inventory says the `tcp_connect*` dialers +stayed in the reference checkout — but `ConnectError` rode along +because the cut table's `util/stream.rs` row ("the `read_exact!`/ +`ready!` macros + `ConnectError` stay") predates the fork's +dead-code sweep and was never revisited. + +**Fix (small, behavioral-no-op)**: either (a) delete +`SocksServerError::Bug` and the `ConnectError` enum + +`SocksServerError::ConnectError` variant + its `to_reply_error` + +its tests, or (b) keep them with a one-line module-doc note that +they are the hand-port map's companions (the reply-code mapping is +what a future kernel-net port would reuse). Option (a) is cleaner — +`SocksServerError::Bug`'s `"BUG: ..."` display is misleading in a +crate that pins no-panics as a convention — but (b) is defensible if +the kernel-net hand-port is still anticipated. Decide when the +producer layer lands (it will either consume `ConnectError`'s +mappings — the dial-callback shape maps dial errors to reply codes — +or not; that is the deciding consumer). Until then this is dead +weight on the public error surface. + +## C-2 [minor] — `StandardAuthentication` (the macro-generated enum + `allow_no_auth`) has zero in-tree callers — the only forked surface nothing consumes + +**Files**: `src/rfc1928/server.rs:396-459` (`auth_method_enums!` +expansion + `StandardAuthentication` + `allow_no_auth`), coverage: +`server.rs:417-427` (the macro's match arms) and `445-458` +(`allow_no_auth`) are uncovered + +**Problem**: upstream's `StandardAuthentication` is consumed by the +cut `Socks5Socket`/`Config` legacy layer (`src/server.rs:134-144`, +`762-768`). In the fork, `grep StandardAuthentication` finds it only +in its own definition and one differential-suite *comment*. Nothing +constructs `StandardAuthentication::*` — the typestate's +`negotiate_auth` is driven with `&[NoAuthentication]` / +`&[PasswordAuthentication]` everywhere. Its uncovered lines are the +largest contiguous uncovered block in `server.rs` (the macro match +arms + `allow_no_auth`'s two branches — 0% covered). + +This is the ADR-013 posture's stated shape ("the `#[allow(dead_code)]` +on `AuthenticationMethod` swept with actual usage") applied +selectively: `AuthenticationMethod` got its sweep, the +macro-generated enum did not — but the macro itself +(`auth_method_enums!`) is documented as a user-facing extension +point (its doc: "If you want to add your own custom methods, you can +generate a similar enum"), and the producer layer's per-door auth +selection (ADR-005's auth-split) plausibly wants it. So this is a +judgment call, not a violation: keep the machinery (it is small, +generic, and the producer's auth seams may drive it), or cut it +until a consumer exists (the crate's "no speculative surface" lean, +matching the client `Config`'s minimal keep). + +**Recommendation**: keep, but make the posture explicit — one module +doc line on `StandardAuthentication` ("kept as the negotiation +machinery's combinator — the producer layer's auth-split (ADR-005) +is its anticipated consumer; currently uncalled in-tree") and note +the coverage block as accepted-uncovered. If the producer layer +lands without consuming it, cut it then. Zero code change now. + +## L-1 [minor] — The client's v6 request-encoding arm is untested (0% of `request_header`'s V6 branch) + +**Files**: `src/rfc1928/client.rs:293-300` (uncovered), +`tests/rfc1928-differential.rs` (no v6 request test) + +**Problem**: coverage's single most important gap on the client +side: `request_header`'s `TargetAddr::Ip(SocketAddr::V6)` arm — the +ATYP=4, 22-byte request form — is never exercised. The v6 *reply* +form is tested twice (`new_reply_v6_form`, +`reply_v6_form_is_22_bytes`, `udp_codec_round_trip_ipv6`), and +`read_address`'s v6 decode is tested (`read_address_ipv6_round_trip`) +— but the *request* encoding of a v6 target has no test. A regression +in the v6 arm (e.g. the `packet[4..20]`/`packet[20..22]` slice +bounds, the ATYP byte, the port offset) would pass the entire suite. +The v6 slice arithmetic is exactly the kind of boundary code that +drifts silently. + +**Fix (small)**: one differential-suite test: mock server reads a +22-byte request for a v6 target and pins +`[05, 01, 00, 04, <16 octets>, ]`. Small effort, closes the +highest-value gap. + +## L-2 [minor] — `Socks5Stream`'s `AsyncRead`/`AsyncWrite` passthrough impls are barely covered: only the happy `poll_read`/`poll_write` ride the tests + +**Files**: `src/rfc1928/client.rs:357-396` (the impls; `390-395` +— `poll_flush`/`poll_shutdown` — uncovered; `383-388` covered only +via one `flush()` call in `io_impls_pipe_data_through_the_session`) + +**Problem**: the impls are 5-line passthroughs, so the risk is +near-zero — but they are the consumer half's data plane (every +`pump_bidi`-shaped consumer rides them), and `poll_shutdown` in +particular is load-bearing for the EOF-sentinel convention (the +channels data plane's half-close rides `poll_shutdown` reaching the +underlying `BiStream`). A test that shuts down a `Socks5Stream` and +asserts the underlying stream sees the shutdown would pin the +half-close propagation the two-pump contract relies on. The +`DuplexStream`-based harness can observe it (shutdown of one half → +EOF for the peer). + +**Recommendation**: one test: complete the handshake, call +`session.shutdown().await`, assert the mock reads EOF. Small +effort, pins the consumer half-close shape before the consumer +layer's pumps depend on it. + +## L-3 [minor] — `server.rs`'s uncovered block is the macro + auth-selection machinery (C-2's face) plus two error arms worth one test each + +**Files**: `src/rfc1928/server.rs:96-97` +(`to_reply_error`'s `AddrError` arm), `155-163` +(`CheckResult` for `Option`/`bool`), `177-179` +(`skip_auth_this_is_not_rfc_compliant`), `568` +(`read_command`'s version-mismatch return — covered via the +differential suite's variant only at a different call shape), `632` +(`wait_on_tcp`'s io-error arm) + +**Problem**: mostly dead-surface coverage debt (see C-2). Two arms +deserve a note: `wait_on_tcp`'s `Err(err)` arm (the io-error path of +the UDP-associate control stream — reachable by dropping the duplex +mid-read; one line of test) and `to_reply_error`'s fall-through +(`GeneralFailure` for the `Io`/`FromUtf8`/`EOF` arms — the mapping +the producer's error surface will rely on when mapping session +errors to reply codes; one test asserting the mapping table). +Both are one-test-cheap; the rest is dead weight until C-2's +decision. + +--- + +# Part B — Non-findings (verified correct, recorded to bound the re-review) + +- **The F-1-class panic surface is clean.** Post-extraction-review + sweep re-verified: no `unwrap`/`expect`/`panic!`/`unreachable!`/ + `todo!` outside `#[cfg(test)]` modules (grep-verified; every hit + is inside a `mod tests` region). The three translated panics + (`from_u8`'s unassigned-code arm, `use_password_auth`'s + `unreachable!`, `reply_error`'s runtime parse) are all present in + their translated forms with pinning tests + (`reply_error_unassigned_codes_map_to_general_failure`, the + typed-error translation note, the compile-time + `V4_UNSPECIFIED_ZERO`). +- **Length-driven allocation is bounded everywhere else.** The + greeting/methods read (`vec![0u8; methods_len as usize]`, 1-byte + cap = 255), the password sub-protocol's username/password reads + (1-byte caps), the domain length byte (S-1) — all bounded by a + 1-byte wire field ≤ 255. No u16/u32-driven allocation exists in + the forked region (the client's `request_header` uses a fixed + `MAX_ADDR_LEN + 3` stack buffer — 263 bytes, bounds-checked + slices). The differential suite's `command_domain_255_bytes_is + _the_wire_maximum` pins the boundary. +- **The method-negotiation preference order is genuinely + server-authoritative.** The outer-loop-in-`server_methods`-order + shape matches upstream (`src/server.rs:901-902`) and RFC 1928 §3's + "the server selects one" reading; the differential suite pins both + directions (client-offers-password-first gets no-auth when the + server offers only none; `accept_password_auth` prefers password + over none when both are offered). The 0xff refusal path is pinned. +- **`reply_success` carries the caller's `sock_addr` verbatim** — + the typestate does not substitute its own address (upstream's + `127.0.0.1` substitution lives in the cut `run_tcp_proxy` caller); + the differential suite pins this distinction with the correct + upstream citation. This matters for BIND's reply#1 (the bound + address must be the *listener's*, supplied by the caller). +- **`copy_bidirectional`'s result is discarded intentionally** — + `transfer` maps neither direction's error to a reply because at + the transfer interception point the reply has already been sent; + the errors are observable only as stream termination. Upstream + does the same (`let _ =`). The two-pump contract (AGENTS.md + convention 8) is the *caller's* shape (`pump_bidi`), not + `transfer`'s; `transfer` remains the upstream-faithful + interception-point helper. The differential suite pins the + both-directions-to-completion behavior. +- **The differential suite's 15 upstream file+line citations were + spot-checked against the reference checkout** — all accurate + (`server.rs:903-918` negotiation loop, `server.rs:944-956` error + reply, `server.rs:1380` `new_reply` privacy, `server.rs:1000-1003` + request header read, `server.rs:1087` the 127.0.0.1 substitution, + `server.rs:572-582`/`587-596` password reads, `server.rs:620-627` + reject write, `client.rs:277-285` UDP-associate fallback + + missing-target, `client.rs:310-317`/`331` domain encode + write, + `client.rs:86-93` skip_auth branch, `lib.rs:313-327`/`332-336`/ + `342` UDP codec, `util/target_addr.rs:40-48` the reply-code + mapping). The suite pins bytes, not upstream text — it survives + upstream churn, and the citations are provenance, not assertions. +- **The MSRV (`rust-version = "1.88"`) is real**: `cargo +1.88 + check` passes native and wasm32. (alktty's N6-class finding — + declared-but-unverified MSRV — does not reproduce here.) +- **Dependency hygiene**: the dependency set is exactly + tokio (wasm-clean subset, `default-features = false`) / + tracing / thiserror — `tracing` is currently referenced by no + source file (see N-2) but is a declared, load-bearing posture for + the fork ("Upstream `log` calls ... are dropped or re-pointed at + the crate's `tracing`", rfc1928.md). `default-features = false` + on tokio keeps the wasm graph minimal; verified by the wasm + checks. +- **The LICENSE notice is the extended form ADR-013 specifies**: + upstream MIT text + copyright preserved, fork-point statement + (v1.0.0, extraction range `57b107b..5b18d4d`, fork at `5b18d4d`), + and the modification note (post-fork changes are the project's + under MIT OR Apache-2.0). The module docs carry the same + provenance. Convention 17's "extend, never replace" holds. +- **Wasm-clean invariant re-verified**: no `tokio::net` anywhere in + `src/`; the only `tokio::io` uses are the portable `AsyncRead`/ + `AsyncWrite`/`copy_bidirectional`; `cargo check` + + `clippy -D warnings` both clean on wasm32. +- **`cargo publish --dry-run` completes** — the manifest is + publishable (no alkty-N9-class missing-README issue; there is no + `readme` field and none needed). + +--- + +# Part C — Notes and observations + +## N-1 [observation] — rfc1928.md's "write_all adjustment" bullet is half-true (F-1's doc face) + +The bullet (`docs/architecture/rfc1928.md:153-158`) records the +client-side `write_all` conversion and attributes it to +`clippy::unused_io_amount` being deny-by-default — which would make +the server-side `.write()` sites impossible. The probe (F-1) +resolves the contradiction: the lint fires on bare `.write()?` but +is suppressed by the `err_when`/`map(|_| ())` chaining idiom these +sites use. The bullet should be updated to record *both* conversions +(client `request_header` — landed; server `reply_success`/ +`reply_error` — F-1's fix) and drop the linter attribution in favor +of the real rule (replies are small, single-shot, and must be +all-or-nothing). Recorded as the doc face of F-1; fixing F-1 without +fixing this bullet leaves the invariant false in docs. + +## N-2 [observation] — `tracing` is a declared dependency referenced by no source file + +`grep -rn tracing src/` is empty; the dependency exists because the +extraction adjustment list says log calls were "dropped or re-pointed +at the crate's `tracing`" — in practice all were *dropped* (the +typestate has no logging surface). Two postures are defensible: +(a) keep the dep as the fork's declared logging seam for the +producer layer's diagnostics (the `try_notify!` secondary-error +site — upstream logged "extra error while reporting an error to the +client" — is the natural first consumer); or (b) drop it from +`[dependencies]` until the first real use. (a) is the likely +outcome and costs nothing; filed so the dependency's purpose is +stated rather than assumed. `Cargo.toml`'s comment policy already +carries the feature-note precedent. + +## N-3 [observation] — `CheckResult`'s `bool`/`Option` impls and `ErrorContext`'s `FromUtf8Error` impl are unused in-tree (the accept_password_auth machinery's loose ends) + +`accept_password_auth`'s closure contract (`R: CheckResult`) is +designed for `Result`/`Option`/`bool` returns; in-tree every caller +(tests only) returns `bool`. The `Option`/`bool` impls are uncovered +(`server.rs:155-163`). `ErrorContext`'s `FromUtf8Error` impl *is* +used (`read_username_password`'s two `err_when("converting ...")` +sites) — covered. No action now; the same "keep or cut when the +producer lands" decision as C-2 applies (the producer's +username/password mapping may want `Result`-returning checkers with +typed rejections). + +## N-4 [observation] — The `Addr` enum's `Domain(String)` variant comment (`// Vec<[u8]> or Box<[u8]> or String ?`) rode the extraction + +`src/rfc1928/util/target_addr.rs:207` carries upstream's +deliberating comment. It is exactly the "non-obvious +design-alternative" class AGENTS.md convention 1 neither requires +nor forbids — but it reads as an unresolved question rather than a +pinned decision (the `String` choice is upstream's and is fine: +UTF-8 validation happens at parse). One-line resolution: either pin +it ("`String` — validated UTF-8 at the codec boundary") or drop the +comment. Trivial. + +--- + +# What's Good + +- **The wire vocabulary is sound and pinned.** The reply form + (10/22-byte), the zero-address error form, the method-negotiation + preference order, the 255-byte domain cap, the UDP RSV/FRAG + discipline, and the client's request encoding are all + byte-pinned in a suite that names its RFC basis per test. The + 92.63% region coverage with no uncovered *wire-format* path + (the gaps are machinery, not codecs) is the right shape for a + protocol core. +- **The panic-surface discipline survived the fork.** All three + extraction-time panic translations are in place with pinning + tests, and the whole-region sweep finds no new panic path — + wire-reachable values map to typed errors everywhere (the F-1 of + the extraction review stays fixed). +- **The fork's two carried changes are real and guarded.** + `new_reply` is published and is the single reply-form encoder + (no re-encoding anywhere); the BIND surface (`TCPBind` flowing + through `read_command`, `reply_success`'s addr parameter) is + present exactly as ADR-007 describes, with the driving correctly + deferred to producer-side code. +- **The typestate genericity is intact** — `T: + AsyncRead + AsyncWrite + Unpin` throughout, no substrate type + anywhere in the core (the wasm checks verify the structural + half; the grep sweep verifies the naming half). +- **The differential suite is the right artifact** — bytes over + text, upstream citations as provenance, six groups that map to + the fork's actual risk surface, duplex-only (wasm-clean crate's + native build). Its citations verified accurate against the v1.0.0 + checkout. + +--- + +# Remediation + +Volume is small — one trivial code fix plus doc/test riders — so +recommended remediations are included per finding: + +| ID | Finding | Recommended fix | Effort | Risk | +|----|---------|----------------|--------|------| +| F-1 | `.write()` reply sites + false doc rationale | `.write_all` at 2 sites + rfc1928.md bullet update | trivial | none | +| S-1 | domain-length allocation invariant undocumented | doc line + pinning test | small | none | +| S-2 | 0xff vocabulary conflation undocumented | doc line on `reject` | trivial | none | +| C-1 | `Bug`/`ConnectError` dead vocabulary | decide keep-with-note vs cut (decide at producer layer) | trivial | none | +| C-2 | `StandardAuthentication` uncalled | note posture now; cut if still uncalled when producer lands | trivial | none | +| L-1 | client v6 request form untested | one differential test | small | none | +| L-2 | `poll_shutdown` passthrough unpinned | one EOF-propagation test | small | none | +| L-3 | `wait_on_tcp` io-error arm uncovered | one test | trivial | none | +| N-1 | rfc1928.md bullet (F-1's face) | rides F-1 | trivial | none | +| N-2 | `tracing` dependency purpose | note or first consumer | trivial | none | +| N-3 | `CheckResult` impls unused | decide at producer layer | none | none | +| N-4 | `Addr::Domain` comment | pin or drop | trivial | none | + +Suggested decomposition: (1) the F-1 batch (F-1 + N-1 + S-2 + N-4 — +one commit, code + doc + no behavior change beyond the +partial-write hardening); (2) the coverage batch (L-1 + L-2 + L-3 + +S-1's pinning test — one commit, tests only); (3) the +decide-at-producer-layer items (C-1, C-2, N-2, N-3) — record the +posture, no code now. + +Per the established workflow, every finding should be re-verified +in the follow-up session before task decomposition (the F-1 +mechanism claim in particular rests on an executable probe that +should be reproduced fresh). + +--- + +# References + +- ADR-011 (the extraction mechanics, the cut table), ADR-013 (the + targeted fork — the ownership posture this review assumes), + ADR-007 (BIND), ADR-005 (auth-split), ADR-004 (the dial-callback + shape — C-1's deciding consumer), ADR-009 (feature gates) +- `docs/architecture/rfc1928.md` (the fork spec — N-1's bullet) +- `tasks/implementation/review-vendor-extraction.md` (the prior, + narrower review; its F-1 is re-verified clean here) +- `/workspace/fast-socks5` (v1.0.0 — the behavioral reference; all + citations in this review refer to it) +- The house pattern: alkty `docs/reviews/001-code-review.md` (the + coverage/correctness/smell triad), alktunnels + `docs/reviews/001-implementation-review.md` (the severity legend + and re-validation discipline) +- RFC 1928 (§6 the reply form, §3 method negotiation, §7 the UDP + associate frame), RFC 1929 (the password sub-protocol's status + byte — S-2) \ No newline at end of file