Replace SMB file copy with a transfer through the WinRM command shell (zero dependencies) - #120
Conversation
Files in localFileToCopyList are now transferred over the already- authenticated WinRM channel: chunked base64 echo legs (below the cmd.exe 8191-character limit), certutil -f -decode on the remote host, and a certutil -hashfile SHA-256 digest (SHA-1 fallback for old hosts) compared against the locally computed one. A file already present with an identical digest is not transferred again. The commands are sent bare: the WinRM shell already runs each command line through cmd.exe, and a nested CMD.EXE /C (...) wrapper mangles quoted redirection chains (live-verified). This removes the smbj dependency and, with it, BouncyCastle (8.2 MB and recurring CVE churn), slf4j-api, mbassador, and asn-one: winrm-java now has zero runtime dependencies, and the copy no longer needs TCP port 445, a temporary share (net share), or a Windows client (the previous implementation wrote through a UNC path with the client OS ambient credentials). Also fixes WsmanClient.doneExitCode narrowing: Windows reports HRESULT exit codes (e.g. certutil 0x80070002) as unsigned 32-bit values that overflowed Integer.parseInt and failed the whole command. Live-verified against anaxagore (NTLM/HTTP encrypted, Windows 2008 R2, legacy spaced certutil output) and tc-win2016 (NTLM/HTTPS, Windows 2016): 13.7 kB script uploaded in 3 legs, executed, digest-skip on re-run. Closes #117 Closes #116 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43087e89fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P1: on non-Windows clients Utils.getComputerName() fell back to localhost, so every such client shared the same remote transfer directory, and same-named files with different content could overwrite each other between the digest verification and the command execution. Two fixes: - The remote file name now embeds a fragment of the content SHA-256 before the extension (script.1a2b3c4d5e6f.vbs): same name + different bytes = different remote path, so the overwrite race is impossible by construction, for any pair of clients. The digest-match upload skip is preserved (content-addressed names make it exact). - Utils.getComputerName() falls back to HOSTNAME and then the resolver before localhost, so non-Windows clients get distinct directories. Live-verified against anaxagore (upload + execute + digest-skip re-run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ed73b341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 2: ScriptedWindowsRemoteExecutor and ShellFileCopyTest now carry the project license header, and the public isClosed() accessor is documented, per AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c98e81654
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 3 (two P2s): - Concurrent identical uploads could both certutil -f -decode into the same content-addressed destination, letting one operation rewrite/lock the file after another had verified it. The transfer now decodes into an operation-unique ".<unique>.part" staging file, verifies the digest THERE, and publishes it only when the destination does not exist yet (discarding the staging copy otherwise), followed by an existence confirmation: the destination is never rewritten once present, and the loser of a publish race succeeds as long as the destination exists. The base64 sidecar now derives from the staging name (unique by construction). - Content-addressed names are bounded (180 chars, extension capped at 30) so that even with the staging suffixes the NTFS 255-character path-component limit holds for any input basename; the digest fragment keeps truncated names unique. Live-verified against anaxagore (upload + publish + digest-skip re-run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9abd970963
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 4 (P1): when the destination pre-existed with a mismatched digest (e.g. a cached script corrupted in place), the publish step discarded the freshly verified staging file because "the destination exists", and the mere existence confirmation then let the caller execute the known-bad file. Now: - The preflight mismatch is remembered: publish force-replaces a mismatched destination (repair), while a matching destination is still never rewritten. - The final confirmation verifies the DESTINATION's digest, not its existence, on every path (including empty files): the operation fails rather than let the caller execute unproven bytes. On failure the bad destination is left in place so the next transfer repairs it. Transfer steps are also batched to minimize WinRM operations: the digest probe (SHA256 and SHA1 in one command) rides the same leg as the decode and publish steps, cutting a fresh upload from N+7 to N+3 operations. This matters on old hosts: Windows 2008 R2 caps concurrent operations at 15 per user (measured live) and reaps completed ones lazily. A command rejected by that quota is retried with escalating delays - only when the rejection happened at operation creation, before the command could run, so a retry can never duplicate a side effect. Live-verified against anaxagore (2008 R2): corrupted cached copy detected and repaired end-to-end (RepairProbe), quota-drained host recovers (DrainProbe), plus the standard upload/skip probes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 747029be58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 5 (P2): a non-Windows client can legally produce local file names containing Windows-forbidden characters (angle brackets, colon, double quote, slashes, pipe, question mark, asterisk), trailing dots/spaces, or reserved device names (CON, PRN, AUX, NUL, COM1..9, LPT1..9, with or without extension, case-insensitive). These now fail fast with IllegalArgumentException instead of producing an invalid, subpath-interpreted, or device-resolving destination on the remote host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07ff483619
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 6: - P1: Utils.getComputerName() can now source its value from the HOSTNAME environment variable, which (unlike a real host name) is unconstrained; the value is embedded in remote shell commands via the transfer directory name. getComputerName() now sanitizes every source to [A-Za-z0-9._-] (64 chars max, "localhost" when nothing safe remains), and buildCreateRemoteDirectoryCommand quotes the MKDIR argument too (defense at the source AND at the sink). - P2: content-addressing gives every revision of a changing file a new remote name, so the transfer directory grew without bound. A forfiles age purge (30 days, best-effort, stderr suppressed) now rides the directory-creation leg - no extra WinRM operation - and also reclaims staging/base64 files orphaned by interrupted transfers. Trade-off documented: a cached script unmodified for 30 days is re-uploaded once. New UtilsTest covers the sanitization; live-verified against anaxagore (2008 R2 forfiles included). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c82c96fbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 7 (P2): the name bound only covered the path component; a 64-character client name plus a 180-character remote name plus the staging suffixes could exceed the traditional 260-character MAX_PATH that old hosts still enforce. The content-addressed name budget is now derived from the actual remote directory length (component bound further reduced so destination + "." + unique + ".part" + ".b64" fits in 259 characters). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fdc6ba84e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 8 (P2): truncating a name (long base or extension) could split a UTF-16 surrogate pair - e.g. in the middle of an emoji - and the malformed half turns into "?" (illegal, and a wildcard) in Windows paths once the command is UTF-8 encoded. Both truncations now back off one char when they would land on a high surrogate; pinned by round-trip tests with emoji names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87a6379a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 9 (P2): on hosts where cmd.exe delayed expansion is enabled, "!" expands like a variable reference even inside quoted arguments, corrupting every transfer command that references the name. checkTransferableFileName now rejects it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fe30e5c6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review round 10 (P2): millis + 16 random bits left a realistic collision chance for concurrent transfers of the same content-addressed file in the same millisecond, which would make two operations share the same .part/.b64 staging files. The suffix is now a process-wide atomic counter (same-JVM collisions impossible) plus 64 SecureRandom bits (cross-process collisions negligible), at most 20 characters - still within the staging suffix budget used for the MAX_PATH bound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex please review again |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #117
Closes #116
What
Replaces the SMB-based file copy (
SmbTempShare+ smbj) with a transfer through the WinRM command shell itself, pywinrm-style:.b64temp file with chunkedechocommand legs (76-char PEM-style lines, each leg kept under the cmd.exe 8191-character limit).certutil -f -decodereconstructs the file and removes the intermediate.b64in the same leg.certutil -hashfileSHA-256 output (SHA-1 fallback for old certutil versions; both the modern and the legacy space-separated hex outputs are parsed) against the locally computed digest. Mismatch → remote cleanup +WindowsRemoteException.Why
bcprov-jdk18on— 8.2 MB, ~90% of the standalone CLI JAR, and the source of the recent Dependabot churn in Force bcprov-jdk18on 1.85 to fix Dependabot alerts #115),slf4j-api(Remove SLF4J: the library must not depend on any logging framework #116: a library must not depend on a logging framework),mbassador, andasn-one.StandaloneJarITnow asserts no third-party class is present in the standalone JAR.net sharecreation on the remote host, and the copy now works from any client OS — the previous implementation authenticated with smbj but actually copied bytes viaFiles.copy()on the\\host\shareUNC path, i.e. through the client OS Windows SMB stack and its ambient credentials.Bug found and fixed along the way
WsmanClient.doneExitCodeusedInteger.valueOf(...): Windows reports HRESULT exit codes (e.g. certutil''s0x80070002for a missing file) as unsigned 32-bit values (2147942402) that overflowInteger.parseInt— the first remote hash probe of every fresh transfer crashed the whole command. Now parsed aslongand narrowed to the equivalent signed int; pinned by a newWsmanProtocolTestcase againstFakeWsmanServer.An important empirical finding is documented in the code: transfer legs are sent bare, because the WinRM shell already runs each command line through
cmd.exe, and a nestedCMD.EXE /C (...)wrapper mangles quoted redirection chains (firstechosilently dropped, closing paren echoed into the payload).Tests
ShellFileCopyTest(9 tests) +ScriptedWindowsRemoteExecutor, a hand-rolled fake (a first step toward Remove Mockito: replace mock-based tests with fakes and FakeWsmanServer #119 — no new Mockito).WinRMCommandExecutorTestrewritten against the fake (only theWinRMExecutorFactorymockStaticremains, to be removed in Remove Mockito: replace mock-based tests with fakes and FakeWsmanServer #119).SmbTempShareTestdeleted with its class.mvn verify sitegreen on JDK 17: 76 tests + 1 IT, Checkstyle clean, no new PMD warnings.Live verification
🤖 Generated with Claude Code