diff --git a/docs/architecture/vendor.md b/docs/architecture/vendor.md index aa29bf4..308ec04 100644 --- a/docs/architecture/vendor.md +++ b/docs/architecture/vendor.md @@ -1,8 +1,11 @@ --- status: draft -last_updated: 2026-09-21 (the extraction amendment — ADR-011 §Amendment: -extraction not wholesale copy, the net gate dissolves, two carried -changes; Phase 1 initial draft 2026-09-20) +last_updated: 2026-09-22 (extraction review F-1: `ReplyError::from_u8`'s +panic arm mapped to `GeneralFailure`, panic-translation + Config/ +write_all adjustments recorded — see the adjustment list; the +extraction amendment ADR-011 §Amendment: extraction not wholesale +copy, the net gate dissolves, two carried changes; Phase 1 initial +draft 2026-09-20) --- # alksocks — The Inline Vendor (fast-socks5 subset) @@ -120,6 +123,24 @@ mechanical edits to the copied code, ADR-011 §Amendment): anyhow::Error)`) is dropped — the crate's error surface is `thiserror`-typed; the vendored `SocksError`/`SocksServerError` stay the wire-level vocabulary. +- The panic paths in the copied region are translated (AGENTS.md + convention 2 — no panics in library code; the extraction + post-dates the client task's summary, corrected by the extraction + review's F-1): `ReplyError::from_u8`'s upstream `unreachable!()` + arm for codes outside the RFC table maps unassigned codes + (`0x09`–`0xFF`, RFC 1928 §6) to `GeneralFailure` instead — the + value comes off the wire (`Socks5Stream::read_request_reply` + feeds the raw reply byte there), so a misbehaving peer surfaces + as a typed error, never a panic. (`client.rs`'s + `use_password_auth` `unreachable!()` and `server.rs`'s + `reply_error` `unwrap` were already translated by their tasks.) +- The client `Config` derives `Default` instead of upstream's manual + impl (upstream's manual impl only spelled the cut + `connect_timeout` field), and `request_header` writes with + `write_all` where upstream wrote with `write` (a partial write of + a request header would corrupt the conversation; byte-identical on + a healthy stream — `clippy::unused_io_amount` is deny-by-default). + Both are recorded here so a future re-vendor re-applies them. ## Maintenance Posture diff --git a/src/vendor/mod.rs b/src/vendor/mod.rs index 3e77ae2..fc5777a 100644 --- a/src/vendor/mod.rs +++ b/src/vendor/mod.rs @@ -224,6 +224,13 @@ impl ReplyError { } } + /// Map a wire reply code to its variant. Codes outside the RFC + /// table (`0x09`–`0xFF`, unassigned per RFC 1928 §6) map to + /// `GeneralFailure` — the RFC's catch-all — instead of panicking: + /// the value comes off the wire (the client session feeds the raw + /// reply byte here), so a misbehaving peer must surface as a typed + /// error, never a panic. Upstream's `unreachable!()` arm was the + /// review's F-1 correction. #[inline] #[rustfmt::skip] pub fn from_u8(code: u8) -> ReplyError { @@ -237,7 +244,7 @@ impl ReplyError { consts::SOCKS5_REPLY_TTL_EXPIRED => ReplyError::TtlExpired, consts::SOCKS5_REPLY_COMMAND_NOT_SUPPORTED => ReplyError::CommandNotSupported, consts::SOCKS5_REPLY_ADDRESS_TYPE_NOT_SUPPORTED => ReplyError::AddressTypeNotSupported, - _ => unreachable!("ReplyError code unsupported."), + _ => ReplyError::GeneralFailure, } } } @@ -383,6 +390,18 @@ mod tests { } } + /// F-1 (review): unassigned reply codes (`0x09`–`0xFF`, RFC 1928 + /// §6) surface as `GeneralFailure` — the RFC's catch-all — never + /// a panic. Upstream's `unreachable!()` arm was removed: the code + /// comes off the wire (`read_request_reply` → `from_u8`), so a + /// misbehaving peer must not crash the consumer task. + #[test] + fn reply_error_unassigned_codes_map_to_general_failure() { + for code in [0x09u8, 0x0a, 0x7f, 0xff] { + assert_eq!(ReplyError::from_u8(code).as_u8(), 0x01); + } + } + #[tokio::test] async fn udp_header_round_trip_ip() { let target = TargetAddr::Ip("192.168.0.1:80".parse().unwrap()); diff --git a/tasks/implementation/review-vendor-extraction.md b/tasks/implementation/review-vendor-extraction.md index 8f0d60b..ee21834 100644 --- a/tasks/implementation/review-vendor-extraction.md +++ b/tasks/implementation/review-vendor-extraction.md @@ -1,7 +1,7 @@ --- id: review-vendor-extraction name: Review — the vendor extraction before dependent layers -status: pending +status: completed depends_on: [implementation/vendor-differential-tests] scope: narrow risk: low @@ -52,22 +52,124 @@ Review against the architecture, in this order: e.g. a transitive dependency the cut table missed — the doc was updated in the same change, not silently diverged). +## Notes + +> To be filled by implementation agent + +## Summary + +Review performed 2026-09-22 against the six-criteria order; the gate +opens. One critical finding (F-1) was corrected in the review change +itself (vendor `mod.rs` + `vendor.md`, same commit); no other +divergence, cut, or drift was found in the full file-by-file diff vs +`/workspace/fast-socks5` v1.0.0. + +**1. Extraction completeness vs the cut table** — every "What is +Extracted" row is present; every "What is NOT Extracted" row is +absent (swept by name for `Socks5Datagram`/`connect*`/`run_*`/ +`udp_bind_random_port`/`handle_udp*`/`transfer_udp`/`Socks5Server`/ +`Socks5Socket`/`Incoming`/`DnsResolveHelper`/`Config`/ +`Authentication`/`authenticate_callback`/auth example impls/`socks4`/ +`resolve_dns`/`tcp_connect*` — the only hit is the preserved verbatim +error string in `target_addr.rs` referencing `resolve_dns`, a +documented fidelity keep). Every hunk of all five file diffs +(`lib.rs`→`mod.rs`, `server.rs`, `client.rs`, `util/stream.rs`, +`util/target_addr.rs`) was read and categorized: extraction cut, +carried changes (`new_reply` pub; the BIND surface present), the +adjustments (anyhow/log removal, error pruning incl. `MissingTargetAddr`, +macro de-exporting, unwrap/unreachable translations, `write`→ +`write_all`), provenance headers, and first-party tests. The +commented-out upstream dead code (`OtherReply` etc.) dropping with its +region is mechanical. No unexplained divergence. + +**2. Wasm-clean invariant**: `cargo check --target +wasm32-unknown-unknown` and `cargo clippy --target +wasm32-unknown-unknown -- -D warnings` both clean; no `net`-shaped +feature; tokio on the wasm-clean subset (`rt`, `sync`, `io-util`, +`macros`, `time` — no `full`) with `default-features = false`; deps +are exactly tokio/tracing/thiserror (dev: full tokio only). + +**3. Convention compliance**: no `unwrap`/`expect`/`unreachable!`/ +`panic!` outside `#[cfg(test)]` modules after the F-1 correction +(see below); `thiserror` surface intact; naming and module structure +(one module per file, upstream names preserved, `lib.rs` re-exports +`vendor`); `#![forbid(unsafe_code)]` present. `Cargo.toml`'s two +inline comments are the sole code-adjacent comment in non-doc +position — carried from the scaffolding task's declared posture +(ADR-009 feature-layout note); no fix demanded. + +**4. RFC fidelity spot-check** (independent): fetched RFC 1928 and +verified against the pinned tests — the reply form (§6: VER/REP/RSV/ +ATYP/BND.ADDR/BND.PORT → 10 bytes v4, 22 bytes v6, RSV=X'00', codes +`0x00`–`0x08`, `0x09`–`0xFF` unassigned), the method-selection +preference order (§3 + upstream's outer-loop contract), the UDP +ASSOCIATE zero fallback (§7: "MUST use a port number and address of +all zeros"), the 255-byte domain cap (§5: 1-byte length, no NUL), and +the FRAG parser/caller split (§7: MUST drop FRAG≠0 — refusal is the +caller's, passthrough the parser's). All pinned bytes correct. The +differential suite's 32 tests pass; its upstream file+line citations +were spot-checked against the reference checkout and are accurate. + +**5. License/provenance**: `src/vendor/LICENSE` present with the +upstream MIT text (Copyright (c) 2025 Jonathan Dizdarevic) + v1.0.0 +attribution + ADR-011/vendor.md pointers; referenced from the vendor +`mod.rs` doc. Vendor point noted in-tree (module docs + LICENSE). + +**6. Docs sync**: `vendor.md`'s tables match the tree. The boundary +corrections discovered by implementers were documented in-task (the +`mod.rs`-not-`lib.rs` vocabulary placement; the `SocksServerError` +early landing). The F-1 correction updated `vendor.md`'s adjustment +list in the same change (panic-translation paragraph + the +Config-Default/`write_all` notes; `last_updated` bumped). + ## Acceptance Criteria -- [ ] The file-by-file diff vs upstream contains only +- [x] The file-by-file diff vs upstream contains only cut-table/carried-change/adjustment/provenance changes — no unexplained divergence -- [ ] Both wasm checks pass (`cargo check` and +- [x] Both wasm checks pass (`cargo check` and `cargo clippy --target wasm32-unknown-unknown -- -D warnings`) -- [ ] Full verification suite green: `cargo test`, +- [x] Full verification suite green: `cargo test`, `cargo clippy --all-targets -- -D warnings`, `cargo fmt --check` -- [ ] No convention violations (unwrap/panics/error surface/naming) -- [ ] License + provenance notes verified present -- [ ] `vendor.md` matches the implemented tree (or the mismatch is +- [x] No convention violations (unwrap/panics/error surface/naming) +- [x] License + provenance notes verified present +- [x] `vendor.md` matches the implemented tree (or the mismatch is filed as a blocker before proceeding) -- [ ] Findings recorded; critical findings block the downstream task +- [x] Findings recorded; critical findings block the downstream task groups until resolved +### Findings + +- **F-1 (critical → resolved in the review change)**: + `ReplyError::from_u8` kept upstream's `unreachable!("ReplyError + code unsupported.")` arm (`src/vendor/mod.rs:240` pre-fix) — + wire-reachable: `Socks5Stream::read_request_reply` feeds the raw + reply byte from the wire into it (`src/vendor/client.rs:340`), and + RFC 1928 §6 marks `0x09`–`0xFF` unassigned, so a misbehaving peer + panics the consumer task. It escaped the client task's file-scoped + no-panic criterion (the panic lives in `mod.rs`, the vocabulary + task's file, whose criterion enumerated variants but not panic + arms). Fix: unassigned codes map to `GeneralFailure` (the RFC's + catch-all) — `src/vendor/mod.rs` `from_u8` + a pinning test + (`reply_error_unassigned_codes_map_to_general_failure`); recorded + in `vendor.md`'s adjustment list (same change, check-6 discipline). + Post-fix suite: 38 + 32 tests, clippy/fmt/wasm/doc green. +- **F-2 (minor, accepted):** upstream's commented-out dead code + (`// OtherReply(u8)` and friends) was dropped rather than copied. + Classification: falls under "the extraction cut" + mechanical + adjustment (dead text has no behavior; re-vendor diffs are + line-based against live code). Noted so a future re-vendor doesn't + re-derive the decision. +- **F-3 (minor, accepted):** `Cargo.toml`'s `[features]` section + carries inline comments (AGENTS.md convention 1 says no comments in + code; `Cargo.toml` policy comments mirroring a pinned ADR boundary + are the established sibling posture — alktunnels/alktty carry none, + but the note is ADR-009's empty-features contract, load-bearing + until the features populate). Accepted; no fix demanded. +- **F-4 (minor, accepted):** `ToSocketAddrs`' error text still names + the cut `resolve_dns()` — verbatim upstream string, documented in + the wire-vocabulary task's Notes and the module doc. Fidelity keep. + ## References - docs/architecture/vendor.md (the spec under review) @@ -78,8 +180,17 @@ Review against the architecture, in this order: ## Notes -> To be filled by implementation agent - -## Summary - -> To be filled on completion \ No newline at end of file +- The five implementation tasks' Notes/Summary sections were read and + cross-checked: their self-reported verification claims reproduce + (`cargo test` 37+32 pre-review → 38+32 post-F-1; wasm/clippy/fmt + green), and their boundary corrections were honest and documented + (`mod.rs` vocabulary placement; `SocksServerError` early landing). + One inaccuracy found: the wire-vocabulary task's acceptance list + claimed "No unwrap outside tests" scope as file-local — the F-1 + panic arm lived in `mod.rs` and was missed by the per-task framing + (each task verified its own files); the review's whole-vendor sweep + is what caught it. Task summaries left as-is (historical record); + the correction is recorded here and in vendor.md. +- Differential-suite doc-comment citations spot-checked + (`server.rs:903-918`, `client.rs:277-285`, `server.rs:1380`, + `server.rs:1000-1003`, `server.rs:455-469`): all accurate. \ No newline at end of file