rework sambrs as an opinionated Windows SMB client#3
Merged
Conversation
- Switch from ANSI (A) to wide (W) WNet APIs: correct Unicode handling for share names, user names, and passwords - Optional credentials (None = logged-on user / SSO), SmbShare builder, type-safe DriveLetter, printer shares, provider override - Full CONNECT_* flag surface via ConnectOptions, incl. SMB signing and encryption enforcement, plus raw-flags escape hatches - connect_auto (WNetUseConnectionW picks the drive letter), RAII Connection guard, cancel_connection by device or remote name - query module: get_connection, get_user, get_universal_name - enumerate module: safe Iterator over WNetOpenEnumW (active/remembered connections, server share listings) - server module (netapi32): share/session/open-file/connection administration with automatic info-level fallback - Error: non_exhaustive, WNetGetLastErrorW capture behind ExtendedError, raw_os_error + io::Error interop - tracing is now an opt-in feature; new zeroize feature wipes password buffers; Debug redacts passwords - windows-sys 0.60, thiserror 2, Windows-gated deps, docs.rs targets, MSRV 1.85 - Env-driven integration suite; CI provisions a real \\localhost share on windows-latest and runs it, plus clippy/fmt/docs/no-op gates Adversarially reviewed by a 2-reviewer + independent-fixer agent workflow; all 6 findings (2 major) verified and fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
to_wide encoded the input into a Vec<u16> and only then scanned for an interior NUL; on the error path that vec (possibly holding a password) was dropped un-wiped, defeating the zeroize guarantee of to_wide_secret. Checking the &str up front is exactly equivalent — a non-NUL char never encodes to a 0 UTF-16 unit — and leaves the error path allocation-free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The #[error] strings were pasted verbatim from the Microsoft docs and referenced FFI internals (lpLocalName, lpNetResource, NETRESOURCE members) that users of a safe wrapper never see. Every variant now displays a concise, user-level message following the Rust convention (lowercase start, no trailing period); the "this error is returned if..." explanations move into /// doc comments, phrased in the crate's own vocabulary. Variant names, from_status, and raw_os_error are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both functions duplicated the same FFI setup verbatim: five wide-string conversions plus the NETRESOURCEW construction. ConnectArgs owns the converted buffers and produces the NETRESOURCEW and credential pointers borrowing from itself, making the lifetime discipline explicit: the struct must outlive the FFI call. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two unbounded-loop hazards against a misbehaving provider: a NO_ERROR result with zero entries made next() call fill() forever, and the ERROR_MORE_DATA resize path could grow-and-retry without limit. A zero-entry success now ends the enumeration, and the resize path is capped at 4 attempts (mirroring query::wide_out), returning Error::Other(ERROR_MORE_DATA) when exhausted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ions delete_session with both client and username as None silently ended every session on the server — a destructive outcome reachable by threading two Nones through. It now returns Error::InvalidParameter in that case; the end-everything operation is the separate, grep-able delete_all_sessions(server). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ShareInfo.remark already mapped empty to None but ShareInfo.path did
not (IPC$ yielded Some("")), and the SessionInfo / OpenFile /
ShareConnection optional strings were inconsistent too. netapi32 uses
null and empty interchangeably for "no value", so a from_pwstr_nonempty
helper now normalizes both to None across every optional string field
of the server info structs. enumerate::NetResource stays untouched: it
is an explicit raw snapshot of NETRESOURCEW.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
server_open_files_are_listable_and_closable silently skipped both the assertion and the close_file exercise when NetFileEnum did not list the file — a green run that tested nothing. The find is now unwrapped with a panic message carrying the full listed vec, and close_file always runs. Loopback listing is reliable under SAMBRS_TEST_LOCAL=1, which gates this test to exactly that setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cargo package shipped the agent tooling (.agents/, .claude/, skills-lock.json, .github/) in the tarball. An explicit include allowlist now ships only /src, /tests, /Cargo.toml, /README.md, /CHANGELOG.md, and /LICENSE (plus cargo's generated Cargo.toml.orig, .cargo_vcs_info.json, and Cargo.lock). Because .cargo/config.toml does not ship, the test docs now tell packaged-copy users to set RUST_TEST_THREADS=1 themselves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The all-sessions guard only checked for None, so an empty client or user filter (e.g. an unset config value) reached NetSessionDel as a non-null pointer to an empty string — which netapi32 treats like a null, i.e. no filter, exactly the unfiltered delete-all the guard exists to prevent. Normalize empty filters to None before the guard so they are rejected with InvalidParameter, and document that empty strings do not count as a filter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Since delete_session started rejecting a filterless call locally, Error::InvalidParameter can carry ERROR_INVALID_PARAMETER without any Windows call having failed. raw_os_error's rustdoc claimed the code only exists for errors originating from the OS and the variant doc listed only WNet causes; state the synthesized case in both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
A guard that cancels by remote name can tear down deviceless connections it never made: WNetCancelConnection2W by remote name cancels every deviceless connection to the resource in the logon session, and Windows does not reference-count them. Documenting that footgun was not enough — an RAII drop handler must not be able to destroy state it does not own. The guard now always owns a redirected local device and cancels exactly that device on drop: connect_guarded requires a local device (rejecting deviceless shares with a synthesized InvalidParameter before any Windows call), and the new connect_auto_guarded lets Windows pick a free device and carries the assigned name (Connection::device). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…::Error Routing ExtendedError through from_raw_os_error reduced it to the generic ERROR_EXTENDED_ERROR (1208) message, discarding the provider code, description, and name that WNetGetLastErrorW was called to capture. Keep the Error itself as the io::Error payload instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows does not document whether a WNetUseConnectionW call that failed with ERROR_MORE_DATA left the connection established. Retrying with a bigger buffer could therefore connect twice — under CONNECT_REDIRECT that is a second drive letter, and the first one leaks because its name is never returned to the caller. Size the access-name buffer to fit any real access name up front (a redirected device name is tiny; a deviceless access name is a provider-adjusted remote name, covered by explicit headroom) and surface ERROR_MORE_DATA as an error instead of retrying. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The system message table behind io::Error::from_raw_os_error does not contain the netapi32 NERR_* range (2100-2999) — those messages live in netmsg.dll — so Error::Other displayed an unknown-error placeholder for any undocumented NERR code. Fall back to FormatMessageW with FORMAT_MESSAGE_FROM_HMODULE on netmsg.dll for that range. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Its soundness rests on a caller-upheld contract (the string pointers in the NETRESOURCEW must be null or valid), so express that in the signature with a # Safety section, matching the raw constructors in server.rs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A fresh zeroed 16 KiB allocation per batch (re-grown every batch when entries are large) is wasted work on long enumerations; keep the buffer in the Resources struct so its capacity carries over. Entries are copied out of the buffer before the next fill, so reuse is sound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mutable tags and branches (@v4, @stable, @master) are a supply-chain risk for a workflow that runs on every fork PR; pin both actions to commit SHAs. dtolnay/rust-toolchain selected the toolchain via the ref, so the pinned form passes it through the toolchain input instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
samvdst
changed the title
samvdst
changed the title
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
1 participant
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.
Summary
Reworks
sambrsfrom a small ANSI connect/disconnect wrapper into a safe, opinionated Windows-only client for connecting to and inspecting existing SMB disk shares.This is a breaking 0.2 release. It adds Unicode connections, safer connection lifetimes and options, focused query and enumeration APIs, and integration tests against a real Windows SMB share. The README includes a migration table for common 0.1 calls.
Changes
SmbSharewith the fluentSmbTargetAPI.WNet*WAPIs throughout, fixing non-ASCII targets and credentials.Connectionguards for temporary drive mappings, including automatically assigned drives.ERROR_EXTENDED_ERRORand other Windows statuses asError::Windows(code).Debugand tracing, and zeroize stored and temporary password buffers.The crate deliberately does not administer shares, handle printers, expose raw
WNetcontrols, or transfer files.Breaking changes
SmbShareis replaced bySmbTarget.connect(persist, interactive)becomesconnect()orconnect_with(ConnectOptions); interactive authentication is removed.disconnect(persist, force)becomesdisconnect()ordisconnect_with(DisconnectOptions); the old disconnect-sidepersistbehavior is now namedforget.Error::Windows(code);Error::CStringConversionbecomesError::InteriorNul.Test plan
cargo fmt --checkcargo clippy --target x86_64-pc-windows-msvc --all-targets -- -D warningsRUSTDOCFLAGS="-D warnings" cargo doc --target x86_64-pc-windows-msvc --no-depscargo check --target x86_64-pc-windows-msvc --all-targetson Rust 1.85cargo test -- --include-ignored --test-threads=1against a provisioned\\localhost\sambrs-testshare