From 43087e89fe2acb57f5549bc67aeb2e2e1b98c63f Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 15:54:53 +0200 Subject: [PATCH 01/11] Replace SMB file copy with a transfer through the WinRM command shell 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 --- CHANGELOG.md | 30 +- README.md | 12 +- pom.xml | 13 - .../org/metricshub/winrm/ShellFileCopy.java | 456 ++++++++++++++++++ .../winrm/WindowsRemoteProcessUtils.java | 88 ---- .../winrm/command/WinRMCommandExecutor.java | 62 +-- .../metricshub/winrm/light/WsmanClient.java | 6 +- .../metricshub/winrm/shares/SmbTempShare.java | 314 ------------ .../winrm/ScriptedWindowsRemoteExecutor.java | 122 +++++ .../metricshub/winrm/ShellFileCopyTest.java | 300 ++++++++++++ .../metricshub/winrm/cli/StandaloneJarIT.java | 14 +- .../command/WinRMCommandExecutorTest.java | 221 +++++---- .../winrm/light/WsmanProtocolTest.java | 35 ++ .../winrm/shares/SmbTempShareTest.java | 139 ------ 14 files changed, 1116 insertions(+), 696 deletions(-) create mode 100644 src/main/java/org/metricshub/winrm/ShellFileCopy.java delete mode 100644 src/main/java/org/metricshub/winrm/shares/SmbTempShare.java create mode 100644 src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java create mode 100644 src/test/java/org/metricshub/winrm/ShellFileCopyTest.java delete mode 100644 src/test/java/org/metricshub/winrm/shares/SmbTempShareTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 675af42..f39014a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,28 @@ All notable changes to this project are documented in this file. ## [Unreleased] — 2.0.0 +### ⚠️ Breaking — SMB file copy replaced by a transfer through the WinRM channel + +Files passed to `WinRMCommandExecutor.execute(...)` in `localFileToCopyList` are no longer copied +over SMB: they are transferred **through the WinRM command shell** (chunked base64, decoded with +`certutil -decode`, verified with a `certutil -hashfile` digest against the locally computed one). +Consequences: + +- **Zero runtime dependencies**: `smbj` is gone, and with it BouncyCastle (`bcprov-jdk18on`, + 8.2 MB and a recurring source of CVE churn), `slf4j-api`, `mbassador`, and `asn-one`. The + standalone CLI JAR shrinks from ~9 MB to a few hundred kB, and the library no longer references + any logging API — problems are reported through exceptions only. +- **No SMB requirement**: TCP port 445 does not need to be reachable, no administrative/temporary + share is created on the remote host (`net share` is no longer issued), and the copy now works + from any client OS (the previous implementation wrote through a Windows UNC path, which only + worked from a Windows client with ambient access to the share). +- A file already present in the remote temporary directory with an identical digest is not + transferred again, preserving the caching behavior of repeated script executions. +- The transfer is designed for the small script files this API is meant for; base64 over SOAP is + not suited to bulk data. +- `SmbTempShare` (class) and `WindowsRemoteProcessUtils.copyLocalFilesToShare(...)` were removed. + `WindowsTempShare` is unchanged. + ### ⚠️ Breaking — the CXF backend was removed Version 2.0.0 removes the legacy Apache CXF backend. The dependency-free client introduced in the @@ -20,14 +42,18 @@ unaffected. Consequences: (**insecure — for testing only**). - Setting `-Dorg.metricshub.winrm.backend=cxf` now fails with a clear error instead of selecting the removed backend: remove the property (or stay on winrm-java 1.x). -- The jar shrinks dramatically: the Apache CXF / JAX-WS / JAXB stack is gone and the only runtime - dependency left is `smbj` (used for copying files to remote shares). +- The jar shrinks dramatically: the Apache CXF / JAX-WS / JAXB stack is gone, and with the SMB + file copy replaced by a WinRM-native transfer (see above), the library has **zero runtime + dependencies**. ### Removed - The Apache CXF-based backend (`WinRMService` and the `service.client` internals), the CXF / JAX-WS / JAXB / `jaxws-rt` dependencies, and the WSDL/XSD resources and code generation. - `KerberosCredentialsException` (was thrown only by CXF internals). +- The `smbj` dependency (and its transitive BouncyCastle, SLF4J, `mbassador`, and `asn-one`), + `SmbTempShare`, and `WindowsRemoteProcessUtils.copyLocalFilesToShare(...)` — replaced by the + file transfer through the WinRM command shell. ### Added diff --git a/README.md b/README.md index 7823aac..d5bbec0 100644 --- a/README.md +++ b/README.md @@ -29,10 +29,16 @@ The Windows Remote Management (WinRM) Java Client is a library that enables to: ## The WinRM client -The client is dependency-free (no Apache CXF / JAX-WS / JAXB — the only runtime dependency is -`smbj`, used for copying files to remote shares) and immune by construction to JAXP +The client has **zero runtime dependencies** (no Apache CXF / JAX-WS / JAXB, no BouncyCastle, no +SLF4J — problems are reported through exceptions only) and is immune by construction to JAXP `ServiceLoader` conflicts (it uses the JDK-default XML factories). It supports **NTLM over HTTP -(with message encryption) and HTTPS** and **Kerberos (SPNEGO) over HTTPS**. Over HTTPS it +(with message encryption) and HTTPS** and **Kerberos (SPNEGO) over HTTPS**. + +Files listed in `localFileToCopyList` are copied to the remote host **through the WinRM channel +itself** (chunked base64 through the command shell, decoded with `certutil` and verified with a +digest): no SMB, no TCP port 445, no administrative share — and it works from any client OS. A +file already present on the remote host with an identical digest is not transferred again. This +transport is designed for small script files, not bulk data. Over HTTPS it validates the certificate and verifies the hostname by default (see the upgrade warning above); `-Dorg.metricshub.winrm.tls.insecure=true` trusts all certificates (insecure, testing only). Kerberos uses the ambient Kerberos configuration (`krb5.conf` / `-Djava.security.krb5.*`) unless diff --git a/pom.xml b/pom.xml index ec978b9..00c8572 100644 --- a/pom.xml +++ b/pom.xml @@ -103,19 +103,6 @@ junit-jupiter-engine test - - com.hierynomus - smbj - 0.14.0 - - - - org.bouncycastle - bcprov-jdk18on - 1.85 - runtime - org.mockito mockito-inline diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java new file mode 100644 index 0000000..b96aa34 --- /dev/null +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -0,0 +1,456 @@ +package org.metricshub.winrm; + +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; +import java.util.Base64; +import java.util.List; +import java.util.Locale; +import java.util.Optional; +import java.util.concurrent.TimeoutException; +import org.metricshub.winrm.exceptions.WindowsRemoteException; + +/** + * Copies local files to the remote host through the WinRM command shell itself, without SMB. + *

+ * Each file is base64-encoded locally, appended to a remote temporary file with chunked + * {@code echo} commands, decoded with {@code certutil -decode}, and verified by comparing a + * remote {@code certutil -hashfile} digest with the locally computed one. The transfer rides + * the already-authenticated (and, over HTTP, encrypted) WinRM channel: no extra TCP port, + * no separate authentication, no dependency. + *

+ * A file that is already present on the remote host with an identical digest is not + * transferred again, so repeatedly executing the same script is cheap. + *

+ * This transport is intended for the small script files that + * {@link org.metricshub.winrm.command.WinRMCommandExecutor} copies before execution; base64 + * over SOAP is not suited to bulk data. + */ +public class ShellFileCopy { + + private ShellFileCopy() {} + + /** + * cmd.exe rejects command lines longer than 8191 characters; stay well under it, the + * redirection targets count toward the limit. The transfer commands are sent bare (no + * {@code CMD.EXE /C (...)} wrapper): the WinRM shell already runs each command line + * through cmd.exe, and a second nesting level mangles quoted redirection chains. + */ + private static final int MAX_COMMAND_LENGTH = 8000; + + /** PEM-style base64 line length, accepted by every certutil version. */ + private static final int BASE64_LINE_LENGTH = 76; + + /** + * Digest algorithms in order of preference, as certutil spells them. SHA1 is only a + * fallback for old certutil versions without SHA256 support; the digest is a transfer + * integrity check on an already-encrypted channel, not a security control. + */ + private static final String[] CERTUTIL_ALGORITHMS = { "SHA256", "SHA1" }; + + /** + * Copy the specified local files to a temporary directory on the remote host through the + * WinRM command shell, and return the command updated so that each reference to a local + * file path points to the corresponding remote copy. + * + * @param windowsRemoteExecutor Executor connected to the remote host (mandatory) + * @param command The command referencing the local files (mandatory) + * @param localFiles The list of local files to copy (may be null or empty: no-op) + * @param timeout Timeout in milliseconds (throws an IllegalArgumentException if negative or zero) + * @return The command updated with the remote paths of the copied files + * @throws IOException If a local file cannot be read + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException For any problem encountered on the remote host + */ + public static String copyLocalFilesToRemote( + final WindowsRemoteExecutor windowsRemoteExecutor, + final String command, + final List localFiles, + final long timeout + ) throws IOException, TimeoutException, WindowsRemoteException { + Utils.checkNonNull(windowsRemoteExecutor, "windowsRemoteExecutor"); + Utils.checkNonNull(command, "command"); + Utils.checkArgumentNotZeroOrNegative(timeout, "timeout"); + + if (localFiles == null || localFiles.isEmpty()) { + return command; + } + + final long start = Utils.getCurrentTimeMillis(); + + final String windowsDirectory = WindowsTempShare.getWindowsDirectory( + windowsRemoteExecutor, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to locate the remote Windows directory") + ); + + final String remoteDirectory = WindowsTempShare.buildRemotePath( + windowsDirectory, + WindowsTempShare.buildShareName() + ); + + WindowsTempShare.createRemoteDirectory( + windowsRemoteExecutor, + remoteDirectory, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to create the remote temporary directory"), + start + ); + + String updatedCommand = command; + for (final String localFile : localFiles) { + final String remoteFile = copyFile(windowsRemoteExecutor, Paths.get(localFile), remoteDirectory, timeout, start); + + updatedCommand = WindowsRemoteProcessUtils.caseInsensitiveReplace(updatedCommand, localFile, remoteFile); + } + + return updatedCommand; + } + + /** + * Copy one local file to the remote directory, skipping the transfer when an identical + * copy is already present. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param localPath The local file to copy + * @param remoteDirectory The existing remote directory receiving the file + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @return the path of the file on the remote host + * @throws IOException If the local file cannot be read + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException For any problem encountered on the remote host + */ + static String copyFile( + final WindowsRemoteExecutor windowsRemoteExecutor, + final Path localPath, + final String remoteDirectory, + final long timeout, + final long start + ) throws IOException, TimeoutException, WindowsRemoteException { + final String fileName = localPath.getFileName().toString(); + checkTransferableFileName(fileName); + + final String remoteFile = remoteDirectory + "\\" + fileName; + final byte[] content = Files.readAllBytes(localPath); + + // Skip the transfer if the remote host already has an identical copy + final Optional existing = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); + if (existing.isPresent() && existing.get().matches(content)) { + return remoteFile; + } + + upload(windowsRemoteExecutor, content, remoteFile, timeout, start); + + final Optional uploaded = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); + if (!uploaded.isPresent() || !uploaded.get().matches(content)) { + bestEffortDelete(windowsRemoteExecutor, timeout, start, remoteFile); + + throw new WindowsRemoteException( + String.format( + "Integrity check failed after copying %s to %s on %s.", + localPath, + remoteFile, + windowsRemoteExecutor.getHostname() + ) + ); + } + + return remoteFile; + } + + /** + * Transfer the file content to the remote path: chunked base64 {@code echo} legs, then a + * single {@code certutil -decode} that also removes the intermediate base64 file. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param content The file content + * @param remoteFile The target path on the remote host + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException For any problem encountered on the remote host + */ + private static void upload( + final WindowsRemoteExecutor windowsRemoteExecutor, + final byte[] content, + final String remoteFile, + final long timeout, + final long start + ) throws TimeoutException, WindowsRemoteException { + if (content.length == 0) { + runChecked( + windowsRemoteExecutor, + String.format("TYPE NUL >\"%s\"", remoteFile), + "create an empty file", + timeout, + start + ); + + return; + } + + final String base64File = String.format( + "%s.%s.b64", + remoteFile, + WindowsRemoteProcessUtils.buildNewOutputFileName() + ); + + try { + for (final String uploadCommand : buildUploadCommands( + Base64.getEncoder().encodeToString(content), + base64File + )) { + runChecked(windowsRemoteExecutor, uploadCommand, "upload the file content", timeout, start); + } + + runChecked( + windowsRemoteExecutor, + String.format("certutil -f -decode \"%s\" \"%s\" && DEL /F /Q \"%s\"", base64File, remoteFile, base64File), + "decode the transferred file", + timeout, + start + ); + } catch (final TimeoutException | WindowsRemoteException | RuntimeException e) { + bestEffortDelete(windowsRemoteExecutor, timeout, start, base64File, remoteFile); + + throw e; + } + } + + /** + * Split the base64 payload into PEM-length lines and group them into as few + * {@code CMD.EXE /C} command legs as possible, each below the cmd.exe line-length limit. + * The first {@code echo} of the first leg truncates the target file, all others append. + * + * @param base64 The base64-encoded file content (non-empty) + * @param base64File The remote path of the intermediate base64 file + * @return the list of commands to execute in order + */ + static List buildUploadCommands(final String base64, final String base64File) { + final List commands = new ArrayList<>(); + + StringBuilder leg = null; + for (int position = 0; position < base64.length(); position += BASE64_LINE_LENGTH) { + final String line = base64.substring(position, Math.min(position + BASE64_LINE_LENGTH, base64.length())); + + // ">" (truncate) for the very first line of the file, ">>" (append) afterward + final String piece = String.format("%s\"%s\" echo %s", position == 0 ? ">" : ">>", base64File, line); + + if (leg == null) { + leg = new StringBuilder(piece); + } else if (leg.length() + piece.length() + 2 <= MAX_COMMAND_LENGTH) { + leg.append("& ").append(piece); + } else { + commands.add(leg.toString()); + leg = new StringBuilder(piece); + } + } + commands.add(leg.toString()); + + return commands; + } + + /** + * Get the digest of a remote file with {@code certutil -hashfile}, trying each supported + * algorithm in order. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param remoteFile The remote file to hash + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @return the digest of the remote file, or an empty Optional if it couldn't be computed + * (typically because the file doesn't exist) + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException For any problem encountered on the remote host + */ + private static Optional remoteDigest( + final WindowsRemoteExecutor windowsRemoteExecutor, + final String remoteFile, + final long timeout, + final long start + ) throws TimeoutException, WindowsRemoteException { + for (final String algorithm : CERTUTIL_ALGORITHMS) { + final WindowsRemoteCommandResult result = run( + windowsRemoteExecutor, + String.format("certutil -hashfile \"%s\" %s", remoteFile, algorithm), + "hash the remote file", + timeout, + start + ); + + if (result.getStatusCode() == 0) { + final Optional digest = parseCertutilDigest(result.getStdout(), algorithm); + if (digest.isPresent()) { + return Optional.of(new RemoteDigest(algorithm, digest.get())); + } + } + } + + return Optional.empty(); + } + + /** + * Extract the digest from a {@code certutil -hashfile} output: the line that is nothing + * but hexadecimal digits of the expected length, ignoring the spaces older certutil + * versions insert between bytes. + * + * @param output The certutil standard output + * @param algorithm The certutil algorithm name the output was produced with + * @return the lowercase digest, or an empty Optional if none was found + */ + static Optional parseCertutilDigest(final String output, final String algorithm) { + final int expectedLength = "SHA256".equals(algorithm) ? 64 : 40; + + return output == null + ? Optional.empty() + : output + .lines() + .map(line -> line.replaceAll("\\s", Utils.EMPTY).toLowerCase(Locale.ROOT)) + .filter(line -> line.length() == expectedLength && line.matches("[0-9a-f]+")) + .findFirst(); + } + + /** + * Execute a transfer command and fail if its exit code is not zero. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param command The command to execute + * @param description What the command does, for the timeout and failure messages + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException When the command fails or reports a non-zero exit code + */ + private static void runChecked( + final WindowsRemoteExecutor windowsRemoteExecutor, + final String command, + final String description, + final long timeout, + final long start + ) throws TimeoutException, WindowsRemoteException { + final WindowsRemoteCommandResult result = run(windowsRemoteExecutor, command, description, timeout, start); + + if (result.getStatusCode() != 0) { + throw new WindowsRemoteException( + String.format( + "Failed to %s on %s (exit code %d): %s", + description, + windowsRemoteExecutor.getHostname(), + result.getStatusCode(), + Utils.isNotBlank(result.getStderr()) ? result.getStderr().trim() : result.getStdout().trim() + ) + ); + } + } + + private static WindowsRemoteCommandResult run( + final WindowsRemoteExecutor windowsRemoteExecutor, + final String command, + final String description, + final long timeout, + final long start + ) throws TimeoutException, WindowsRemoteException { + return windowsRemoteExecutor.executeCommand( + command, + null, + null, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to " + description) + ); + } + + /** + * Delete remote files, ignoring any failure: used to clean up after a failed transfer, + * where the original exception must not be masked. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @param remoteFiles The remote files to delete + */ + private static void bestEffortDelete( + final WindowsRemoteExecutor windowsRemoteExecutor, + final long timeout, + final long start, + final String... remoteFiles + ) { + final StringBuilder files = new StringBuilder(); + for (final String remoteFile : remoteFiles) { + files.append(String.format(" \"%s\"", remoteFile)); + } + + try { + run(windowsRemoteExecutor, "DEL /F /Q" + files, "clean up", timeout, start); + } catch (final Exception ignored) { + // Cleanup is best-effort: the exception that triggered it matters more + } + } + + /** + * Reject file names that cannot be embedded safely in a quoted cmd.exe argument. + * Windows already forbids most cmd metacharacters in file names; the remaining dangerous + * one is {@code %}, which cmd.exe expands as a variable reference even between quotes. + * + * @param fileName The name of the file to transfer + */ + static void checkTransferableFileName(final String fileName) { + if (fileName.contains("%") || fileName.contains("\"") || fileName.chars().anyMatch(c -> c < 0x20)) { + throw new IllegalArgumentException( + String.format("File name %s contains characters that cannot be transferred safely.", fileName) + ); + } + } + + /** The digest of a remote file, with the certutil algorithm that produced it. */ + private static final class RemoteDigest { + + private final String algorithm; + private final String digest; + + private RemoteDigest(final String algorithm, final String digest) { + this.algorithm = algorithm; + this.digest = digest; + } + + /** Whether this remote digest matches the digest of the given local content. */ + private boolean matches(final byte[] content) { + try { + final MessageDigest messageDigest = MessageDigest.getInstance( + "SHA256".equals(algorithm) ? "SHA-256" : "SHA-1" + ); + + final StringBuilder hex = new StringBuilder(); + for (final byte b : messageDigest.digest(content)) { + hex.append(String.format("%02x", b)); + } + + return hex.toString().equals(digest); + } catch (final NoSuchAlgorithmException e) { + // Cannot happen: every JVM is required to provide SHA-1 and SHA-256 + throw new IllegalStateException(e); + } + } + } +} diff --git a/src/main/java/org/metricshub/winrm/WindowsRemoteProcessUtils.java b/src/main/java/org/metricshub/winrm/WindowsRemoteProcessUtils.java index 9420f79..c2fb68d 100644 --- a/src/main/java/org/metricshub/winrm/WindowsRemoteProcessUtils.java +++ b/src/main/java/org/metricshub/winrm/WindowsRemoteProcessUtils.java @@ -20,14 +20,8 @@ * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ */ -import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.nio.file.StandardCopyOption; -import java.nio.file.attribute.FileTime; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -131,88 +125,6 @@ public static String buildNewOutputFileName() { ); } - /** - * Copy the local files to the share and update the command with their path as seen in the remote system. - * - * @param command The command (mandatory) - * @param localFiles The local files to copy list - * @param uncSharePath The UNC path of the share - * @param remotePath The remote path - * @return The updated command. - * @throws IOException If an I/O error occurs. - */ - public static String copyLocalFilesToShare( - final String command, - final List localFiles, - final String uncSharePath, - final String remotePath - ) throws IOException { - Utils.checkNonNull(command, "command"); - - if (localFiles == null || localFiles.isEmpty()) { - return command; - } - - Utils.checkNonNull(uncSharePath, "uncSharePath"); - Utils.checkNonNull(remotePath, "remotePath"); - - try { - return localFiles - .stream() - .reduce( - command, - (cmd, localFile) -> { - try { - final Path localFilePath = Paths.get(localFile); - final Path remoteFilePath = copyToShare(localFilePath, uncSharePath, remotePath); - - return caseInsensitiveReplace(cmd, localFile, remoteFilePath.toString()); - } catch (final IOException e) { - throw new RuntimeException(e); - } - } - ); - } catch (final Exception e) { - if (e.getCause() instanceof IOException) { - throw (IOException) e.getCause(); - } - throw e; - } - } - - /** - * Copy a file to the share. - * If the same file is already present on the share, the copy is not performed. - * The "last-modified" time is used to determine whether the file needs to be - * copied or not. - * - * @param localFilePath The path to the file to copy - * @param uncSharePath The UNC path of the share - * @param remotePath The remote path - * @return the path to the copied file, as seen in the remote system - * @throws IOException If an I/O error occurs. - */ - static Path copyToShare(final Path localFilePath, final String uncSharePath, final String remotePath) - throws IOException { - final Path targetUncPath = Paths.get(uncSharePath, localFilePath.getFileName().toString()); - final Path targetRemotePath = Paths.get(remotePath, localFilePath.getFileName().toString()); - - if (Files.exists(targetUncPath)) { - final FileTime sourceFileTime = Files.getLastModifiedTime(localFilePath); - final FileTime targetFileTime = Files.getLastModifiedTime(targetUncPath); - if (sourceFileTime.compareTo(targetFileTime) <= 0) { - // File is already present on the target, simply skip the copy operation - return targetRemotePath; - } - } - - // Copy - Files.copy(localFilePath, targetUncPath, StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING); - - // Return the path to the copied file, as seen in the remote system - return targetRemotePath; - } - /** * Perform a case-insensitive replace of all occurrences of target string with * specified replacement diff --git a/src/main/java/org/metricshub/winrm/command/WinRMCommandExecutor.java b/src/main/java/org/metricshub/winrm/command/WinRMCommandExecutor.java index a7f8585..f518142 100644 --- a/src/main/java/org/metricshub/winrm/command/WinRMCommandExecutor.java +++ b/src/main/java/org/metricshub/winrm/command/WinRMCommandExecutor.java @@ -23,9 +23,11 @@ import java.io.IOException; import java.nio.charset.Charset; import java.nio.file.Path; +import java.util.Collections; import java.util.List; import java.util.concurrent.TimeoutException; import java.util.stream.Collectors; +import org.metricshub.winrm.ShellFileCopy; import org.metricshub.winrm.TimeoutHelper; import org.metricshub.winrm.Utils; import org.metricshub.winrm.WinRMHttpProtocolEnum; @@ -37,7 +39,6 @@ import org.metricshub.winrm.service.WinRMEndpoint; import org.metricshub.winrm.service.WinRMExecutorFactory; import org.metricshub.winrm.service.client.auth.AuthenticationEnum; -import org.metricshub.winrm.shares.SmbTempShare; public class WinRMCommandExecutor { @@ -95,60 +96,45 @@ public static WindowsRemoteCommandResult execute( final WinRMEndpoint winRMEndpoint = new WinRMEndpoint(protocol, hostname, port, username, password, null); - if (localFileToCopyList == null || localFileToCopyList.isEmpty()) { - try ( - final WindowsRemoteExecutor winRMService = WinRMExecutorFactory.createInstance( - winRMEndpoint, - timeout, - ticketCache, - authentications - )) { - final Charset charset = WindowsRemoteProcessUtils.getWindowsEncodingCharset( - winRMService, - TimeoutHelper.getRemainingTime(timeout, start, "No time left to retrieve the code set") - ); - - return winRMService.executeCommand(command, workingDirectory, charset, timeout); - } catch (final WqlQuerySyntaxException e) { - throw new IOException(e); - } - } + final List localFiles = localFileToCopyList == null + ? Collections.emptyList() + : localFileToCopyList.stream().filter(Utils::isNotBlank).collect(Collectors.toList()); try ( - final SmbTempShare smbTempShare = SmbTempShare.createInstance( + final WindowsRemoteExecutor winRMService = WinRMExecutorFactory.createInstance( winRMEndpoint, timeout, ticketCache, authentications )) { - smbTempShare.checkConnectedFirst(); + if (localFiles.isEmpty()) { + final Charset charset = WindowsRemoteProcessUtils.getWindowsEncodingCharset( + winRMService, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to retrieve the code set") + ); - final List localFiles = localFileToCopyList - .stream() - .filter(Utils::isNotBlank) - .collect(Collectors.toList()); + return winRMService.executeCommand(command, workingDirectory, charset, timeout); + } - // Copy the list specified list of files, and update the command accordingly - final String localFilesUpdatedCommand = WindowsRemoteProcessUtils.copyLocalFilesToShare( + // Copy the specified list of files through the command shell, and update the command accordingly + final String localFilesUpdatedCommand = ShellFileCopy.copyLocalFilesToRemote( + winRMService, command, localFiles, - smbTempShare.getUncSharePath(), - smbTempShare.getRemotePath() + TimeoutHelper.getRemainingTime(timeout, start, "No time left to copy the local files") ); final Charset charset = WindowsRemoteProcessUtils.getWindowsEncodingCharset( - smbTempShare.getWindowsRemoteExecutor(), + winRMService, TimeoutHelper.getRemainingTime(timeout, start, "No time left to retrieve the code set") ); - return smbTempShare - .getWindowsRemoteExecutor() - .executeCommand( - String.format("CMD.EXE /C (%s)", localFilesUpdatedCommand), - null, - charset, - TimeoutHelper.getRemainingTime(timeout, start, "No time left to execute command") - ); + return winRMService.executeCommand( + String.format("CMD.EXE /C (%s)", localFilesUpdatedCommand), + null, + charset, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to execute command") + ); } catch (final WqlQuerySyntaxException e) { throw new IOException(e); } diff --git a/src/main/java/org/metricshub/winrm/light/WsmanClient.java b/src/main/java/org/metricshub/winrm/light/WsmanClient.java index 587584a..a7a4bb6 100644 --- a/src/main/java/org/metricshub/winrm/light/WsmanClient.java +++ b/src/main/java/org/metricshub/winrm/light/WsmanClient.java @@ -70,7 +70,7 @@ final class WsmanClient implements AutoCloseable { private String shellId; // A single NTLM connection is a serial channel: one socket, stateful RC4 ciphers with sequence - // numbers, and a single shellId. Concurrent callers (e.g. a cached SmbTempShare shared across + // numbers, and a single shellId. Concurrent callers (e.g. one executor shared across // threads) MUST NOT interleave, or they read each other's responses and desync the cipher streams. // Every high-level operation (wql/executeCommand) runs while holding this lock; close() only // tries it, so it can still hard-close the transport to unblock an abandoned, timed-out worker. @@ -420,7 +420,9 @@ private static Integer doneExitCode(final Document doc) { final Element state = (Element) states.item(i); if (Envelopes.COMMAND_STATE_DONE.equals(state.getAttribute("State"))) { final NodeList exit = state.getElementsByTagNameNS("*", "ExitCode"); - return exit.getLength() > 0 ? Integer.valueOf(exit.item(0).getTextContent().trim()) : 0; + // Parse as long, then narrow: Windows reports HRESULT exit codes (e.g. certutil's + // 0x80070002) as unsigned 32-bit values that overflow Integer.parseInt. + return exit.getLength() > 0 ? (int) Long.parseLong(exit.item(0).getTextContent().trim()) : 0; } } return null; diff --git a/src/main/java/org/metricshub/winrm/shares/SmbTempShare.java b/src/main/java/org/metricshub/winrm/shares/SmbTempShare.java deleted file mode 100644 index 4327db5..0000000 --- a/src/main/java/org/metricshub/winrm/shares/SmbTempShare.java +++ /dev/null @@ -1,314 +0,0 @@ -package org.metricshub.winrm.shares; - -/*- - * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ - * WinRM Java Client - * ჻჻჻჻჻჻ - * Copyright 2023 - 2026 MetricsHub - * ჻჻჻჻჻჻ - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ - */ - -import com.hierynomus.security.bc.BCSecurityProvider; -import com.hierynomus.smbj.SMBClient; -import com.hierynomus.smbj.SmbConfig; -import com.hierynomus.smbj.auth.AuthenticationContext; -import com.hierynomus.smbj.connection.Connection; -import com.hierynomus.smbj.session.Session; -import com.hierynomus.smbj.share.DiskShare; -import java.io.IOException; -import java.nio.file.Path; -import java.util.List; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicInteger; -import org.metricshub.winrm.Utils; -import org.metricshub.winrm.WindowsRemoteExecutor; -import org.metricshub.winrm.WindowsTempShare; -import org.metricshub.winrm.exceptions.WinRMException; -import org.metricshub.winrm.exceptions.WindowsRemoteException; -import org.metricshub.winrm.service.WinRMEndpoint; -import org.metricshub.winrm.service.WinRMExecutorFactory; -import org.metricshub.winrm.service.client.auth.AuthenticationEnum; - -public class SmbTempShare extends WindowsTempShare implements AutoCloseable { - - private final WinRMEndpoint winRMEndpoint; - private final SMBClient smbClient; - private final Connection connection; - private final Session session; - private final DiskShare diskShare; - - /** - * The SmbTempShare constructor. - * - * @param windowsRemoteExecutor WinRM executor (CXF or light backend) - * @param winRMEndpoint Endpoint with credentials - * @param smbClient The SMB client - * @param connection The SMB connection - * @param session The SMB session - * @param diskShare The SMB disk share - * @param shareNameOrUnc The name of the share, or its full UNC path - * @param remotePath The path on the remote system of the directory being shared - */ - private SmbTempShare( - final WindowsRemoteExecutor windowsRemoteExecutor, - final WinRMEndpoint winRMEndpoint, - final SMBClient smbClient, - final Connection connection, - final Session session, - final DiskShare diskShare, - final String shareNameOrUnc, - final String remotePath - ) { - super(windowsRemoteExecutor, shareNameOrUnc, remotePath); - this.winRMEndpoint = winRMEndpoint; - this.smbClient = smbClient; - this.connection = connection; - this.session = session; - this.diskShare = diskShare; - } - - private static final ConcurrentHashMap CONNECTIONS_CACHE = new ConcurrentHashMap<>(); - - private final AtomicInteger useCount = new AtomicInteger(1); - - /** - * Create a SmbTempShare instance. - * Get or create a temp share and connect to it with SMB. - * - * @param winRMEndpoint Endpoint with credentials (mandatory) - * @param timeout Timeout in milliseconds (throws an IllegalArgumentException if negative or zero) - * @param ticketCache The Ticket Cache path - * @param authentications List of authentications. only NTLM if absent - * @return SmbTempShare instance - * @throws IOException If an I/O error occurred - * @throws WinRMException For any problem encountered - * @throws TimeoutException To notify userName of timeout. - */ - public static SmbTempShare createInstance( - final WinRMEndpoint winRMEndpoint, - final long timeout, - final Path ticketCache, - final List authentications - ) throws IOException, WinRMException, TimeoutException { - Utils.checkNonNull(winRMEndpoint, "winRMEndpoint"); - Utils.checkNonNull(winRMEndpoint.getPassword(), "password"); - Utils.checkArgumentNotZeroOrNegative(timeout, "timeout"); - - try { - return CONNECTIONS_CACHE.compute( - winRMEndpoint, - (key, smb) -> { - if (smb == null) { - WindowsRemoteExecutor windowsRemoteExecutor = null; - SMBClient smbClient = null; - Connection connection = null; - Session session = null; - DiskShare diskShare = null; - - try { - // Honour the backend toggle: SMB file transfer is always smbj, but the WinRM command - // orchestration follows the selected backend (so "light" does not fall back to CXF). - windowsRemoteExecutor = WinRMExecutorFactory - .createInstance(winRMEndpoint, timeout, ticketCache, authentications); - - final WindowsTempShare windowsTempShare = getOrCreateShare( - windowsRemoteExecutor, - timeout, - (w, r, s, t) -> { - try { - shareRemoteDirectory(w, r, s, t); - } catch (final TimeoutException | WindowsRemoteException e) { - throw new RuntimeException(e); - } - } - ); - - final SmbConfig smbConfig = SmbConfig - .builder() - .withSecurityProvider(new BCSecurityProvider()) - .withTimeout(timeout, TimeUnit.SECONDS) - .build(); - - final AuthenticationContext authenticationContext = new AuthenticationContext( - winRMEndpoint.getUsername(), - winRMEndpoint.getPassword(), - winRMEndpoint.getDomain() - ); - - smbClient = createSmbClient(smbConfig); - connection = smbClient.connect(winRMEndpoint.getHostname()); - session = connection.authenticate(authenticationContext); - diskShare = (DiskShare) session.connectShare(windowsTempShare.getShareName()); - - return new SmbTempShare( - windowsRemoteExecutor, - winRMEndpoint, - smbClient, - connection, - session, - diskShare, - windowsTempShare.getUncSharePath(), - windowsTempShare.getRemotePath() - ); - } catch (final RuntimeException e) { - closeResources(windowsRemoteExecutor, smbClient, connection, session, diskShare); - - throw e; - } catch (final Exception e) { - closeResources(windowsRemoteExecutor, smbClient, connection, session, diskShare); - - throw new RuntimeException(e); - } - } else { - synchronized (smb) { - smb.incrementUseCount(); - - return smb; - } - } - } - ); - } catch (final RuntimeException e) { - final Throwable cause = e.getCause(); - - if (cause instanceof IOException) { - throw (IOException) cause; - } - - if (cause instanceof TimeoutException) { - throw (TimeoutException) cause; - } - - if (cause instanceof WindowsRemoteException) { - throw (WinRMException) cause; - } - - throw e; - } - } - - private static void closeResources( - final WindowsRemoteExecutor windowsRemoteExecutor, - final SMBClient smbClient, - final Connection connection, - final Session session, - final DiskShare diskShare - ) { - try { - if (diskShare != null) { - diskShare.close(); - } - - if (session != null) { - session.close(); - } - - if (connection != null) { - connection.close(); - } - } catch (final IOException ioe) { - throw new RuntimeException(ioe); - } - - if (smbClient != null) { - smbClient.close(); - } - - if (windowsRemoteExecutor != null) { - windowsRemoteExecutor.close(); - } - } - - int getUseCount() { - return useCount.get(); - } - - void incrementUseCount() { - useCount.incrementAndGet(); - } - - /** - * @return whether this WbemServices instance is connected and usable - */ - boolean isConnected() { - return getUseCount() > 0; - } - - /** - * Check if it's connected. If not, throw an IllegalStateException. - */ - public void checkConnectedFirst() { - if (!isConnected()) { - throw new IllegalStateException("This instance has been closed and a new one must be created."); - } - } - - @Override - public synchronized void close() throws IOException { - if (useCount.decrementAndGet() == 0) { - CONNECTIONS_CACHE.remove(winRMEndpoint); - - if (diskShare != null) { - diskShare.close(); - } - - if (session != null) { - session.close(); - } - - if (connection != null) { - connection.close(); - } - - if (smbClient != null) { - smbClient.close(); - } - - getWindowsRemoteExecutor().close(); - } - } - - /** - * Share the remote directory on the host. - * - * @param windowsRemoteExecutor WinRM executor (CXF or light backend). - * @param remotePath The remote path. - * @param shareName The Share Name. - * @param timeout Timeout in milliseconds. - * @throws TimeoutException To notify userName of timeout. - * @throws WindowsRemoteException For any problem encountered - */ - private static void shareRemoteDirectory( - final WindowsRemoteExecutor windowsRemoteExecutor, - final String remotePath, - final String shareName, - final long timeout - ) throws TimeoutException, WindowsRemoteException { - final String command = String.format( - "net share %s=%s /grant:%s,Full", - shareName, - remotePath, - windowsRemoteExecutor.getUsername() - ); - - windowsRemoteExecutor.executeCommand(command, null, null, timeout); - } - - static SMBClient createSmbClient(final SmbConfig smbConfig) { - return new SMBClient(smbConfig); - } -} diff --git a/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java b/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java new file mode 100644 index 0000000..baddeea --- /dev/null +++ b/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java @@ -0,0 +1,122 @@ +package org.metricshub.winrm; + +import java.nio.charset.Charset; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Deque; +import java.util.List; +import java.util.Map; + +/** + * Hand-rolled {@link WindowsRemoteExecutor} fake: tests register canned responses matched by + * substring, in registration order, and can then assert on the commands that were executed. + * A response queue that runs out keeps repeating its last element. + */ +public class ScriptedWindowsRemoteExecutor implements WindowsRemoteExecutor { + + private static final class CommandHandler { + + private final String substring; + private final Deque results; + + private CommandHandler(final String substring, final WindowsRemoteCommandResult[] results) { + this.substring = substring; + this.results = new ArrayDeque<>(List.of(results)); + } + + private WindowsRemoteCommandResult next() { + return results.size() > 1 ? results.poll() : results.peek(); + } + } + + private static final class WqlHandler { + + private final String substring; + private final List> rows; + + private WqlHandler(final String substring, final List> rows) { + this.substring = substring; + this.rows = rows; + } + } + + private final List commandHandlers = new ArrayList<>(); + private final List wqlHandlers = new ArrayList<>(); + + private final List executedCommands = new ArrayList<>(); + private boolean closed; + + /** + * Register command responses: each executed command containing the substring consumes the + * next result of the queue (the last result repeats once the queue is exhausted). + */ + public ScriptedWindowsRemoteExecutor expectCommand( + final String substring, + final WindowsRemoteCommandResult... results + ) { + commandHandlers.add(new CommandHandler(substring, results)); + return this; + } + + /** Register the result rows of any WQL query containing the substring. */ + public ScriptedWindowsRemoteExecutor expectWql(final String substring, final List> rows) { + wqlHandlers.add(new WqlHandler(substring, rows)); + return this; + } + + /** All the commands executed so far, in order. */ + public List getExecutedCommands() { + return executedCommands; + } + + public boolean isClosed() { + return closed; + } + + @Override + public List> executeWql(final String wqlQuery, final long timeout) { + return wqlHandlers + .stream() + .filter(handler -> wqlQuery.contains(handler.substring)) + .findFirst() + .map(handler -> handler.rows) + .orElseThrow(() -> new AssertionError("Unexpected WQL query: " + wqlQuery)); + } + + @Override + public WindowsRemoteCommandResult executeCommand( + final String command, + final String workingDirectory, + final Charset charset, + final long timeout + ) { + executedCommands.add(command); + + return commandHandlers + .stream() + .filter(handler -> command.contains(handler.substring)) + .findFirst() + .map(CommandHandler::next) + .orElseThrow(() -> new AssertionError("Unexpected command: " + command)); + } + + @Override + public String getHostname() { + return "host"; + } + + @Override + public String getUsername() { + return "user"; + } + + @Override + public char[] getPassword() { + return "pass".toCharArray(); + } + + @Override + public void close() { + closed = true; + } +} diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java new file mode 100644 index 0000000..950eb44 --- /dev/null +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -0,0 +1,300 @@ +package org.metricshub.winrm; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.MessageDigest; +import java.util.Base64; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.metricshub.winrm.exceptions.WindowsRemoteException; + +class ShellFileCopyTest { + + private static final long TIMEOUT = 30 * 1000L; + private static final String WINDOWS_DIRECTORY = "C:\\Windows"; + private static final Pattern ECHO_PAYLOAD = Pattern.compile("echo ([A-Za-z0-9+/=]+)"); + + @TempDir + Path tempDir; + + private static final WindowsRemoteCommandResult SUCCESS = new WindowsRemoteCommandResult("", "", 0.1f, 0); + private static final WindowsRemoteCommandResult FAILURE = new WindowsRemoteCommandResult( + "", + "CertUtil: -hashfile command FAILED: 0x80070002", + 0.1f, + 1 + ); + + private static String expectedRemoteDirectory() { + return WINDOWS_DIRECTORY + "\\Temp\\" + WindowsTempShare.buildShareName(); + } + + private static String sha256Hex(final byte[] content) throws Exception { + final StringBuilder hex = new StringBuilder(); + for (final byte b : MessageDigest.getInstance("SHA-256").digest(content)) { + hex.append(String.format("%02x", b)); + } + return hex.toString(); + } + + private static String sha1Hex(final byte[] content) throws Exception { + final StringBuilder hex = new StringBuilder(); + for (final byte b : MessageDigest.getInstance("SHA-1").digest(content)) { + hex.append(String.format("%02x", b)); + } + return hex.toString(); + } + + private static WindowsRemoteCommandResult hashOutput(final String algorithm, final String hex) { + return new WindowsRemoteCommandResult( + String.format( + "%s hash of file C:\\whatever:\r\n%s\r\nCertUtil: -hashfile command completed successfully.\r\n", + algorithm, + hex + ), + "", + 0.1f, + 0 + ); + } + + private static ScriptedWindowsRemoteExecutor executorWithTempDirectory() { + return new ScriptedWindowsRemoteExecutor() + .expectWql("WindowsDirectory", List.of(Map.of("WindowsDirectory", WINDOWS_DIRECTORY))) + .expectCommand("MKDIR", SUCCESS); + } + + /** Reassemble the file bytes from the base64 payloads echoed by the recorded upload legs. */ + private static byte[] echoedContent(final List commands) { + final String base64 = commands + .stream() + .filter(command -> command.contains(" echo ")) + .flatMap(command -> { + final Matcher matcher = ECHO_PAYLOAD.matcher(command); + final StringBuilder payload = new StringBuilder(); + while (matcher.find()) { + payload.append(matcher.group(1)); + } + return payload.length() > 0 ? java.util.stream.Stream.of(payload.toString()) : java.util.stream.Stream.empty(); + }) + .collect(Collectors.joining()); + + return Base64.getDecoder().decode(base64); + } + + @Test + void uploadsNewFileAndRewritesCommand() throws Exception { + final byte[] content = "Résultat: héllo wörld\r\nsecond line\n".getBytes(UTF_8); + final Path localFile = tempDir.resolve("My Script.vbs"); + Files.write(localFile, content); + + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex(content))) + .expectCommand(" SHA1", FAILURE) + .expectCommand(" echo ", SUCCESS) + .expectCommand("certutil -f -decode", SUCCESS); + + final String remoteFile = expectedRemoteDirectory() + "\\My Script.vbs"; + + // The command references the local file with a different case: the replacement is case-insensitive + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + executor, + "CSCRIPT " + localFile.toString().toUpperCase(), + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals("CSCRIPT " + remoteFile, updatedCommand); + + // The echoed base64, reassembled, is exactly the file content (multibyte UTF-8 intact) + assertArrayEquals(content, echoedContent(executor.getExecutedCommands())); + + // The decode leg produces the target file and removes the intermediate base64 file + final String decodeCommand = executor + .getExecutedCommands() + .stream() + .filter(command -> command.contains("certutil -f -decode")) + .findFirst() + .orElseThrow(); + assertTrue(decodeCommand.contains("\"" + remoteFile + "\"")); + assertTrue(decodeCommand.contains("DEL /F /Q")); + assertTrue(decodeCommand.contains(".b64")); + } + + @Test + void skipsUploadWhenRemoteCopyIsIdentical() throws Exception { + final byte[] content = "some script".getBytes(UTF_8); + final Path localFile = tempDir.resolve("script.bat"); + Files.write(localFile, content); + + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" SHA256", hashOutput("SHA256", sha256Hex(content))); + + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + executor, + "CMD /C " + localFile, + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals("CMD /C " + expectedRemoteDirectory() + "\\script.bat", updatedCommand); + assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains(" echo "))); + assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains("-decode"))); + } + + @Test + void fallsBackToSha1WhenSha256IsUnavailable() throws Exception { + final byte[] content = "legacy host".getBytes(UTF_8); + final Path localFile = tempDir.resolve("legacy.vbs"); + Files.write(localFile, content); + + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" SHA256", FAILURE) + .expectCommand(" SHA1", FAILURE, hashOutput("SHA1", sha1Hex(content))) + .expectCommand(" echo ", SUCCESS) + .expectCommand("certutil -f -decode", SUCCESS); + + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + executor, + localFile.toString(), + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals(expectedRemoteDirectory() + "\\legacy.vbs", updatedCommand); + assertArrayEquals(content, echoedContent(executor.getExecutedCommands())); + } + + @Test + void throwsAndCleansUpOnIntegrityMismatch() throws Exception { + final byte[] content = "expected content".getBytes(UTF_8); + final Path localFile = tempDir.resolve("corrupted.txt"); + Files.write(localFile, content); + + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex("tampered".getBytes(UTF_8)))) + .expectCommand(" SHA1", FAILURE) + .expectCommand(" echo ", SUCCESS) + .expectCommand("certutil -f -decode", SUCCESS) + .expectCommand("DEL /F /Q", SUCCESS); + + final WindowsRemoteException exception = assertThrows( + WindowsRemoteException.class, + () -> ShellFileCopy.copyLocalFilesToRemote(executor, localFile.toString(), List.of(localFile.toString()), TIMEOUT) + ); + + assertTrue(exception.getMessage().contains("Integrity check failed")); + + final String remoteFile = expectedRemoteDirectory() + "\\corrupted.txt"; + assertTrue( + executor + .getExecutedCommands() + .stream() + .anyMatch(command -> command.startsWith("DEL /F /Q") && command.contains(remoteFile)) + ); + } + + @Test + void transfersEmptyFile() throws Exception { + final byte[] content = new byte[0]; + final Path localFile = tempDir.resolve("empty.txt"); + Files.write(localFile, content); + + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex(content))) + .expectCommand(" SHA1", FAILURE) + .expectCommand("TYPE NUL", SUCCESS); + + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + executor, + localFile.toString(), + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals(expectedRemoteDirectory() + "\\empty.txt", updatedCommand); + assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains(" echo "))); + } + + @Test + void returnsCommandUnchangedWithoutFiles() throws Exception { + // No handler registered: any remote interaction would fail the test + final ScriptedWindowsRemoteExecutor executor = new ScriptedWindowsRemoteExecutor(); + + assertEquals("dir", ShellFileCopy.copyLocalFilesToRemote(executor, "dir", null, TIMEOUT)); + assertEquals("dir", ShellFileCopy.copyLocalFilesToRemote(executor, "dir", List.of(), TIMEOUT)); + assertTrue(executor.getExecutedCommands().isEmpty()); + } + + @Test + void rejectsFileNamesUnsafeForTheCommandShell() { + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("we%ird.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("quo\"te.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("ctrl\u0001.txt")); + + // Legal Windows file names pass, including cmd metacharacters neutralized by quoting + ShellFileCopy.checkTransferableFileName("My Script (v2) & more!.vbs"); + } + + @Test + void buildUploadCommandsChunksBelowTheCommandLineLimit() { + final String base64File = expectedRemoteDirectory() + "\\big.bin.SEN_X_1_2.b64"; + final byte[] content = new byte[15000]; + for (int i = 0; i < content.length; i++) { + content[i] = (byte) i; + } + final String base64 = Base64.getEncoder().encodeToString(content); + + final List commands = ShellFileCopy.buildUploadCommands(base64, base64File); + + assertTrue(commands.size() > 1, "A 15 kB file must not fit in a single command leg"); + + // Only the very first echo truncates; every other one appends + assertTrue(commands.get(0).startsWith(">\"" + base64File + "\" echo ")); + for (final String command : commands) { + assertTrue(command.length() <= 8000, () -> "Command leg exceeds the cmd.exe limit: " + command.length()); + } + for (int i = 1; i < commands.size(); i++) { + assertTrue(commands.get(i).startsWith(">>\"" + base64File + "\" echo ")); + } + + // Reassembling every echoed payload yields the original base64, in order + assertArrayEquals(content, echoedContent(commands)); + } + + @Test + void parsesCertutilDigestOutputs() { + final String modern = "SHA256 hash of file C:\\x:\r\nAB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34\r\n" + + + "CertUtil: -hashfile command completed successfully.\r\n"; + assertEquals( + Optional.of("ab12cd34ab12cd34ab12cd34ab12cd34ab12cd34ab12cd34ab12cd34ab12cd34"), + ShellFileCopy.parseCertutilDigest(modern, "SHA256") + ); + + // Older certutil versions separate every byte with a space + final String legacy = "SHA1 hash of file C:\\x:\r\nab 12 cd 34 ab 12 cd 34 ab 12 cd 34 ab 12 cd 34 ab 12 cd 34\r\n" + + + "CertUtil: -hashfile command completed successfully.\r\n"; + assertEquals( + Optional.of("ab12cd34ab12cd34ab12cd34ab12cd34ab12cd34"), + ShellFileCopy.parseCertutilDigest(legacy, "SHA1") + ); + + assertEquals(Optional.empty(), ShellFileCopy.parseCertutilDigest("no digest here", "SHA256")); + assertEquals(Optional.empty(), ShellFileCopy.parseCertutilDigest(null, "SHA256")); + } +} diff --git a/src/test/java/org/metricshub/winrm/cli/StandaloneJarIT.java b/src/test/java/org/metricshub/winrm/cli/StandaloneJarIT.java index e409b53..a94398c 100644 --- a/src/test/java/org/metricshub/winrm/cli/StandaloneJarIT.java +++ b/src/test/java/org/metricshub/winrm/cli/StandaloneJarIT.java @@ -1,7 +1,6 @@ package org.metricshub.winrm.cli; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; @@ -29,7 +28,18 @@ void packagedJarHasManifestAndLaunchesInSeparateJvm() throws Exception { final Attributes attributes = jar.getManifest().getMainAttributes(); assertEquals("org.metricshub.winrm.cli.WinRmCli", attributes.getValue(Attributes.Name.MAIN_CLASS)); assertEquals(projectVersion, attributes.getValue("Implementation-Version")); - assertNotNull(jar.getEntry("com/hierynomus/smbj/SMBClient.class")); + + // winrm-java is dependency-free: no third-party classes may leak into the standalone JAR + jar + .stream() + .map(entry -> entry.getName()) + .filter(name -> name.endsWith(".class")) + .forEach( + name -> assertTrue( + name.startsWith("org/metricshub/winrm/"), + () -> "Unexpected third-party class in the standalone JAR: " + name + ) + ); } final ProcessResult help = launch(standaloneJar, "--help"); diff --git a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java index be69dda..11c091f 100644 --- a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java +++ b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java @@ -5,212 +5,243 @@ import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.metricshub.winrm.WinRMHttpProtocolEnum.HTTPS; -import static org.metricshub.winrm.WindowsRemoteProcessUtils.copyLocalFilesToShare; -import static org.metricshub.winrm.WindowsRemoteProcessUtils.getWindowsEncodingCharset; import static org.metricshub.winrm.command.WinRMCommandExecutor.execute; import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.NTLM; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.security.MessageDigest; import java.util.List; +import java.util.Map; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.metricshub.winrm.ScriptedWindowsRemoteExecutor; import org.metricshub.winrm.WindowsRemoteCommandResult; -import org.metricshub.winrm.WindowsRemoteExecutor; -import org.metricshub.winrm.WindowsRemoteProcessUtils; import org.metricshub.winrm.service.WinRMEndpoint; import org.metricshub.winrm.service.WinRMExecutorFactory; import org.metricshub.winrm.service.client.auth.AuthenticationEnum; -import org.metricshub.winrm.shares.SmbTempShare; import org.mockito.MockedStatic; class WinRMCommandExecutorTest { + private static final String COMMAND = "launch"; + private static final String HOSTNAME = "host"; + private static final String USERNAME = "domain\\user"; + private static final char[] PASSWORD = "pass".toCharArray(); + private static final long TIMEOUT = 30 * 1000L; + private static final Path TICKET_CACHE = Paths.get("path"); + private static final List AUTHENTICATIONS = singletonList(NTLM); + + @TempDir + Path tempDir; + @Test - void testExecute() throws Exception { - final String command = "launch"; - final String hostname = "host"; - final String username = "domain\\user"; - final char[] password = "pass".toCharArray(); + void testExecuteArgumentChecks() { final String workingDirectory = " \t\r\n dir \t\r\n "; - final long timeout = 30 * 1000L; final List localFileToCopyList = singletonList(" \r\t\n localFile \t\r\n "); - final WindowsRemoteCommandResult expected = new WindowsRemoteCommandResult("stdout", "stderr", 1.0f, 0); - final Path ticketCache = Paths.get("path"); - final List authentications = singletonList(NTLM); - // check arguments assertThrows( IllegalArgumentException.class, () -> execute( null, HTTPS, - hostname, + HOSTNAME, 5986, - username, - password, + USERNAME, + PASSWORD, workingDirectory, - timeout, + TIMEOUT, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); assertThrows( IllegalArgumentException.class, () -> execute( - command, + COMMAND, HTTPS, null, 5986, - username, - password, + USERNAME, + PASSWORD, workingDirectory, - timeout, + TIMEOUT, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); assertThrows( IllegalArgumentException.class, () -> execute( - command, + COMMAND, HTTPS, - hostname, + HOSTNAME, 5986, null, - password, + PASSWORD, workingDirectory, - timeout, + TIMEOUT, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); assertThrows( IllegalArgumentException.class, () -> execute( - command, + COMMAND, HTTPS, - hostname, + HOSTNAME, 5986, - username, + USERNAME, null, workingDirectory, - timeout, + TIMEOUT, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); assertThrows( IllegalArgumentException.class, () -> execute( - command, + COMMAND, HTTPS, - hostname, + HOSTNAME, 5986, - username, - password, + USERNAME, + PASSWORD, workingDirectory, -1L, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); assertThrows( IllegalArgumentException.class, () -> execute( - command, + COMMAND, HTTPS, - hostname, + HOSTNAME, 5986, - username, - password, + USERNAME, + PASSWORD, workingDirectory, 0L, localFileToCopyList, - ticketCache, - authentications + TICKET_CACHE, + AUTHENTICATIONS ) ); + } - // case localFileToCopyList null or empty - try ( - final MockedStatic mockedWindowsRemoteProcessUtils = mockStatic( - WindowsRemoteProcessUtils.class - ); - final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class)) { - mockedWindowsRemoteProcessUtils.when(() -> getWindowsEncodingCharset(any(), anyLong())).thenReturn(UTF_8); + @Test + void testExecuteWithoutFilesToCopy() throws Exception { + final WindowsRemoteCommandResult expected = new WindowsRemoteCommandResult("stdout", "stderr", 1.0f, 0); - final WindowsRemoteExecutor executor = mock(WindowsRemoteExecutor.class); + final ScriptedWindowsRemoteExecutor executor = new ScriptedWindowsRemoteExecutor() + .expectWql("CodeSet", List.of(Map.of("CodeSet", "65001"))) + .expectCommand(COMMAND, expected); + try (final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class)) { mockedFactory .when(() -> WinRMExecutorFactory.createInstance(any(WinRMEndpoint.class), anyLong(), isNull(), isNull())) .thenReturn(executor); - doReturn(expected).when(executor).executeCommand(eq(command), isNull(), eq(UTF_8), anyLong()); - assertEquals( expected, - execute(command, null, hostname, null, username, password, null, timeout, null, null, null) + execute(COMMAND, null, HOSTNAME, null, USERNAME, PASSWORD, null, TIMEOUT, null, null, null) ); assertEquals( expected, - execute(command, null, hostname, null, username, password, null, timeout, emptyList(), null, null) + execute(COMMAND, null, HOSTNAME, null, USERNAME, PASSWORD, null, TIMEOUT, emptyList(), null, null) ); - } - // Case with localFileToCopyList - try ( - final MockedStatic mockedWindowsRemoteProcessUtils = mockStatic( - WindowsRemoteProcessUtils.class + // A list of blank names behaves like no list at all + assertEquals( + expected, + execute(COMMAND, null, HOSTNAME, null, USERNAME, PASSWORD, null, TIMEOUT, singletonList(" \r\t\n "), null, null) ); - final MockedStatic mockedSmbTempShare = mockStatic(SmbTempShare.class)) { - mockedWindowsRemoteProcessUtils.when(() -> getWindowsEncodingCharset(any(), anyLong())).thenReturn(UTF_8); - - mockedWindowsRemoteProcessUtils - .when(() -> copyLocalFilesToShare(anyString(), anyList(), anyString(), anyString())) - .thenReturn("launch remote/localFile"); - final SmbTempShare smbTempShare = mock(SmbTempShare.class); - final WindowsRemoteExecutor winRMService = mock(WindowsRemoteExecutor.class); + assertEquals(List.of(COMMAND, COMMAND, COMMAND), executor.getExecutedCommands()); + assertTrue(executor.isClosed()); + } + } - mockedSmbTempShare - .when(() -> SmbTempShare.createInstance(any(WinRMEndpoint.class), anyLong(), isNull(), isNull())) - .thenReturn(smbTempShare); + @Test + void testExecuteWithFileToCopy() throws Exception { + final byte[] content = "WScript.Echo \"Hello é\"".getBytes(UTF_8); + final Path localFile = tempDir.resolve("MyScript.vbs"); + Files.write(localFile, content); - doNothing().when(smbTempShare).checkConnectedFirst(); - doReturn(winRMService).when(smbTempShare).getWindowsRemoteExecutor(); - doReturn("\\\\2001-db8--85b-3c51-f5ff-ffdb.ipv6-literal.net\\SEN_ShareFor_PC-TEST$") - .when(smbTempShare) - .getUncSharePath(); - doReturn("Windows\\Temp\\SEN_ShareFor_TEST$").when(smbTempShare).getRemotePath(); + final WindowsRemoteCommandResult expected = new WindowsRemoteCommandResult("stdout", "stderr", 1.0f, 0); - doReturn(expected).when(winRMService).executeCommand(anyString(), isNull(), eq(UTF_8), anyLong()); + final StringBuilder hex = new StringBuilder(); + for (final byte b : MessageDigest.getInstance("SHA-256").digest(content)) { + hex.append(String.format("%02x", b)); + } + final WindowsRemoteCommandResult remoteHash = new WindowsRemoteCommandResult( + "SHA256 hash of file:\r\n" + hex + "\r\nCertUtil: -hashfile command completed successfully.\r\n", + "", + 0.1f, + 0 + ); + final WindowsRemoteCommandResult failure = new WindowsRemoteCommandResult("", "not found", 0.1f, 1); + final WindowsRemoteCommandResult success = new WindowsRemoteCommandResult("", "", 0.1f, 0); + + final ScriptedWindowsRemoteExecutor executor = new ScriptedWindowsRemoteExecutor() + .expectWql("WindowsDirectory", List.of(Map.of("WindowsDirectory", "C:\\Windows"))) + .expectWql("CodeSet", List.of(Map.of("CodeSet", "65001"))) + .expectCommand("MKDIR", success) + .expectCommand(" SHA256", failure, remoteHash) + .expectCommand(" SHA1", failure) + .expectCommand(" echo ", success) + .expectCommand("certutil -f -decode", success) + .expectCommand("CSCRIPT", expected); + + try (final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class)) { + mockedFactory + .when(() -> WinRMExecutorFactory.createInstance(any(WinRMEndpoint.class), anyLong(), isNull(), isNull())) + .thenReturn(executor); - assertEquals( - expected, - execute(command, null, hostname, null, username, password, null, timeout, localFileToCopyList, null, null) + final WindowsRemoteCommandResult actual = execute( + "CSCRIPT " + localFile, + null, + HOSTNAME, + null, + USERNAME, + PASSWORD, + null, + TIMEOUT, + singletonList(localFile.toString()), + null, + null ); + + assertEquals(expected, actual); + + // The executed command references the remote copy, wrapped in CMD.EXE /C (...) + final String finalCommand = executor.getExecutedCommands().get(executor.getExecutedCommands().size() - 1); + assertTrue(finalCommand.startsWith("CMD.EXE /C (CSCRIPT ")); + assertTrue(finalCommand.contains("\\Temp\\")); + assertTrue(finalCommand.contains("MyScript.vbs")); + assertTrue(executor.isClosed()); } } } diff --git a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java index 9a275ba..7ad934c 100644 --- a/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java +++ b/src/test/java/org/metricshub/winrm/light/WsmanProtocolTest.java @@ -218,6 +218,41 @@ void receiveRetriesOnOperationTimeoutFault() throws Exception { assertEquals(2, receives); } + @Test + void commandExitCodeAboveIntegerMaxIsNarrowedNotRejected() throws Exception { + // Windows reports HRESULT exit codes (e.g. certutil's 0x80070002 for a missing file) as + // unsigned 32-bit values like 2147942402, which overflow Integer.parseInt: the client + // must narrow them to the equivalent signed int instead of failing the whole command. + server + .enqueue(200, envelope(resourceCreated("SHELL-1"))) + .enqueue(200, envelope(commandResponse("CMD-1"))) + .enqueue( + 200, + envelope( + receiveResponse( + "CMD-1", + stream("stdout", "CertUtil: -hashfile command FAILED: 0x80070002".getBytes(StandardCharsets.UTF_8)), + "2147942402" + ) + ) + ) + .enqueue(200, envelope("")); + + try (LightWinRMService service = client(PASSWORD)) { + final WindowsRemoteCommandResult result = service.executeCommand( + "certutil -hashfile \"C:\\missing\" SHA256", + null, + StandardCharsets.UTF_8, + TIMEOUT + ); + + assertEquals((int) 2147942402L, result.getStatusCode()); + assertTrue(result.getStdout().contains("0x80070002")); + } + } + @Test void terminateSignalToleratesShellNotFoundFault() throws Exception { // The command finished and the shell may already be gone: fault 2150858843 on the terminate diff --git a/src/test/java/org/metricshub/winrm/shares/SmbTempShareTest.java b/src/test/java/org/metricshub/winrm/shares/SmbTempShareTest.java deleted file mode 100644 index f27b481..0000000 --- a/src/test/java/org/metricshub/winrm/shares/SmbTempShareTest.java +++ /dev/null @@ -1,139 +0,0 @@ -package org.metricshub.winrm.shares; - -import static java.nio.file.Paths.get; -import static java.util.Collections.singletonList; -import static java.util.concurrent.TimeUnit.SECONDS; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.metricshub.winrm.WindowsTempShare.getOrCreateShare; -import static org.metricshub.winrm.service.client.auth.AuthenticationEnum.NTLM; -import static org.metricshub.winrm.shares.SmbTempShare.createInstance; -import static org.metricshub.winrm.shares.SmbTempShare.createSmbClient; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isNull; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockStatic; - -import com.hierynomus.security.bc.BCSecurityProvider; -import com.hierynomus.smbj.SMBClient; -import com.hierynomus.smbj.SmbConfig; -import com.hierynomus.smbj.SmbConfig.Builder; -import com.hierynomus.smbj.auth.AuthenticationContext; -import com.hierynomus.smbj.connection.Connection; -import com.hierynomus.smbj.session.Session; -import com.hierynomus.smbj.share.DiskShare; -import java.nio.file.Path; -import java.util.List; -import org.junit.jupiter.api.Test; -import org.metricshub.winrm.ShareRemoteDirectoryConsumer; -import org.metricshub.winrm.WindowsRemoteExecutor; -import org.metricshub.winrm.WindowsTempShare; -import org.metricshub.winrm.service.WinRMEndpoint; -import org.metricshub.winrm.service.WinRMExecutorFactory; -import org.metricshub.winrm.service.client.auth.AuthenticationEnum; -import org.mockito.MockedStatic; - -class SmbTempShareTest { - - @SuppressWarnings("unchecked") - @Test - void testCreateInstance() throws Exception { - final String hostname = "host"; - final String username = "user"; - final char[] password = "pwd".toCharArray(); - final WinRMEndpoint winRMEndpoint = new WinRMEndpoint(null, hostname, null, "domain\\" + username, password, null); - final long timeout = 30 * 1000L; - final Path ticketCache = get("path"); - final List authentications = singletonList(NTLM); - - // check arguments - assertThrows(IllegalArgumentException.class, () -> createInstance(null, timeout, ticketCache, authentications)); - - assertThrows( - IllegalArgumentException.class, - () -> createInstance(winRMEndpoint, -1L, ticketCache, authentications) - ); - - assertThrows(IllegalArgumentException.class, () -> createInstance(winRMEndpoint, 0L, ticketCache, authentications)); - - try ( - final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class); - final MockedStatic mockedSmbTempShare = mockStatic(SmbTempShare.class); - final MockedStatic mockedWindowsTempShare = mockStatic(WindowsTempShare.class); - final MockedStatic mockedSmbConfig = mockStatic(SmbConfig.class)) { - final WindowsRemoteExecutor executor = mock(WindowsRemoteExecutor.class); - mockedFactory - .when(() -> WinRMExecutorFactory.createInstance(winRMEndpoint, timeout, null, null)) - .thenReturn(executor); - - final WindowsTempShare windowsTempShare = mock(WindowsTempShare.class); - mockedWindowsTempShare - .when(() -> getOrCreateShare(eq(executor), anyLong(), any(ShareRemoteDirectoryConsumer.class))) - .thenReturn(windowsTempShare); - doReturn("\\\\2001-db8--85b-3c51-f5ff-ffdb.ipv6-literal.net\\SEN_ShareFor_PC-TEST$") - .when(windowsTempShare) - .getUncSharePath(); - doReturn("Windows\\Temp\\SEN_ShareFor_TEST$").when(windowsTempShare).getRemotePath(); - doReturn("SEN_ShareFor_PC-TEST$").when(windowsTempShare).getShareName(); - - final Builder smbConfigBuilder = mock(Builder.class); - mockedSmbConfig.when(SmbConfig::builder).thenReturn(smbConfigBuilder); - - doReturn(smbConfigBuilder).when(smbConfigBuilder).withSecurityProvider(any(BCSecurityProvider.class)); - - doReturn(smbConfigBuilder).when(smbConfigBuilder).withTimeout(anyLong(), eq(SECONDS)); - - final SmbConfig smbConfig = mock(SmbConfig.class); - doReturn(smbConfig).when(smbConfigBuilder).build(); - - final SMBClient smbClient = mock(SMBClient.class); - mockedSmbTempShare.when(() -> createSmbClient(smbConfig)).thenReturn(smbClient); - - final Connection connection = mock(Connection.class); - doReturn(connection).when(smbClient).connect(anyString()); - - final Session session = mock(Session.class); - doReturn(session).when(connection).authenticate(any(AuthenticationContext.class)); - - final DiskShare diskShare = mock(DiskShare.class); - doReturn(diskShare).when(session).connectShare(anyString()); - - mockedSmbTempShare - .when(() -> createInstance(any(WinRMEndpoint.class), anyLong(), isNull(), isNull())) - .thenCallRealMethod(); - - final SmbTempShare smbTempShare1 = createInstance(winRMEndpoint, timeout, null, null); - assertNotNull(smbTempShare1); - assertEquals(1, smbTempShare1.getUseCount()); - assertEquals(executor, smbTempShare1.getWindowsRemoteExecutor()); - assertTrue(smbTempShare1.isConnected()); - - final SmbTempShare smbTempShare2 = createInstance(winRMEndpoint, timeout, null, null); - assertNotNull(smbTempShare2); - assertEquals(2, smbTempShare1.getUseCount()); - assertEquals(2, smbTempShare2.getUseCount()); - assertEquals(executor, smbTempShare2.getWindowsRemoteExecutor()); - assertTrue(smbTempShare1.isConnected()); - assertTrue(smbTempShare2.isConnected()); - - smbTempShare1.close(); - assertTrue(smbTempShare1.isConnected()); - assertTrue(smbTempShare2.isConnected()); - assertEquals(1, smbTempShare1.getUseCount()); - assertEquals(1, smbTempShare2.getUseCount()); - - smbTempShare2.close(); - assertFalse(smbTempShare1.isConnected()); - assertFalse(smbTempShare2.isConnected()); - assertEquals(0, smbTempShare1.getUseCount()); - assertEquals(0, smbTempShare2.getUseCount()); - } - } -} From d6ed73b3414d34de2fea8e36b4a7d8f07e00c070 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 16:11:12 +0200 Subject: [PATCH 02/11] Content-address remote file names to prevent cross-client overwrites 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 --- CHANGELOG.md | 4 ++ .../org/metricshub/winrm/ShellFileCopy.java | 61 ++++++++++++++----- src/main/java/org/metricshub/winrm/Utils.java | 24 +++++++- .../metricshub/winrm/ShellFileCopyTest.java | 38 ++++++++++-- .../command/WinRMCommandExecutorTest.java | 3 +- 5 files changed, 105 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f39014a..e8181dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ Consequences: worked from a Windows client with ambient access to the share). - A file already present in the remote temporary directory with an identical digest is not transferred again, preserving the caching behavior of repeated script executions. +- The remote copy is **content-addressed**: a fragment of the content digest is inserted before + the file extension (e.g. `script.1a2b3c4d5e6f.vbs`), so same-named files with different content + from concurrent clients can never overwrite each other. Scripts that inspect their own file + name (e.g. `WScript.ScriptName`) will see the digest fragment. - The transfer is designed for the small script files this API is meant for; base64 over SOAP is not suited to bulk data. - `SmbTempShare` (class) and `WindowsRemoteProcessUtils.copyLocalFilesToShare(...)` were removed. diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index b96aa34..94b74aa 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -153,9 +153,14 @@ static String copyFile( final String fileName = localPath.getFileName().toString(); checkTransferableFileName(fileName); - final String remoteFile = remoteDirectory + "\\" + fileName; final byte[] content = Files.readAllBytes(localPath); + // Content-addressed remote name: same-named files with different content get different + // remote paths, so concurrent clients — including clients whose computer names collide + // in the shared temporary directory — can never overwrite each other's payload between + // the digest verification and the command execution. + final String remoteFile = remoteDirectory + "\\" + contentAddressedName(fileName, content); + // Skip the transfer if the remote host already has an identical copy final Optional existing = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); if (existing.isPresent() && existing.get().matches(content)) { @@ -408,6 +413,44 @@ private static void bestEffortDelete( } } + /** + * Build the remote name of a transferred file: the local file name with a fragment of the + * content digest inserted before the extension (e.g. {@code script.1a2b3c4d5e6f.vbs}), so + * the remote path identifies both the name and the content of the file. + * + * @param fileName The local file name + * @param content The file content + * @return the content-addressed remote file name + */ + static String contentAddressedName(final String fileName, final byte[] content) { + final int dot = fileName.lastIndexOf('.'); + final String base = dot > 0 ? fileName.substring(0, dot) : fileName; + final String extension = dot > 0 ? fileName.substring(dot) : Utils.EMPTY; + + return base + "." + digestHex("SHA-256", content).substring(0, 12) + extension; + } + + /** + * Compute the hexadecimal digest of the given content. + * + * @param algorithm The {@link MessageDigest} algorithm name + * @param content The content to hash + * @return the lowercase hexadecimal digest + */ + static String digestHex(final String algorithm, final byte[] content) { + try { + final StringBuilder hex = new StringBuilder(); + for (final byte b : MessageDigest.getInstance(algorithm).digest(content)) { + hex.append(String.format("%02x", b)); + } + + return hex.toString(); + } catch (final NoSuchAlgorithmException e) { + // Cannot happen: every JVM is required to provide SHA-1 and SHA-256 + throw new IllegalStateException(e); + } + } + /** * Reject file names that cannot be embedded safely in a quoted cmd.exe argument. * Windows already forbids most cmd metacharacters in file names; the remaining dangerous @@ -436,21 +479,7 @@ private RemoteDigest(final String algorithm, final String digest) { /** Whether this remote digest matches the digest of the given local content. */ private boolean matches(final byte[] content) { - try { - final MessageDigest messageDigest = MessageDigest.getInstance( - "SHA256".equals(algorithm) ? "SHA-256" : "SHA-1" - ); - - final StringBuilder hex = new StringBuilder(); - for (final byte b : messageDigest.digest(content)) { - hex.append(String.format("%02x", b)); - } - - return hex.toString().equals(digest); - } catch (final NoSuchAlgorithmException e) { - // Cannot happen: every JVM is required to provide SHA-1 and SHA-256 - throw new IllegalStateException(e); - } + return digestHex("SHA256".equals(algorithm) ? "SHA-256" : "SHA-1", content).equals(digest); } } } diff --git a/src/main/java/org/metricshub/winrm/Utils.java b/src/main/java/org/metricshub/winrm/Utils.java index cf7e320..c2abf39 100644 --- a/src/main/java/org/metricshub/winrm/Utils.java +++ b/src/main/java/org/metricshub/winrm/Utils.java @@ -75,11 +75,29 @@ public static boolean isNotBlank(final String value) { * @return the name of the local computer (or "localhost" if it can't be determined) */ public static String getComputerName() { + // Windows sets COMPUTERNAME; on other platforms fall back to HOSTNAME, then to the + // resolver, so distinct clients don't all end up named "localhost" (their transfer + // directories on the remote host are keyed by this name). final String computerName = System.getenv("COMPUTERNAME"); - if (computerName == null) { - return "localhost"; + if (isNotBlank(computerName)) { + return computerName; } - return computerName; + + final String hostName = System.getenv("HOSTNAME"); + if (isNotBlank(hostName)) { + return hostName; + } + + try { + final String localName = java.net.InetAddress.getLocalHost().getHostName(); + if (isNotBlank(localName)) { + return localName; + } + } catch (final java.net.UnknownHostException ignored) { + // Fall through to the default + } + + return "localhost"; } /** diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 950eb44..1b7ca2e 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -107,7 +107,8 @@ void uploadsNewFileAndRewritesCommand() throws Exception { .expectCommand(" echo ", SUCCESS) .expectCommand("certutil -f -decode", SUCCESS); - final String remoteFile = expectedRemoteDirectory() + "\\My Script.vbs"; + final String remoteFile = expectedRemoteDirectory() + "\\" + + ShellFileCopy.contentAddressedName("My Script.vbs", content); // The command references the local file with a different case: the replacement is case-insensitive final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( @@ -150,7 +151,10 @@ void skipsUploadWhenRemoteCopyIsIdentical() throws Exception { TIMEOUT ); - assertEquals("CMD /C " + expectedRemoteDirectory() + "\\script.bat", updatedCommand); + assertEquals( + "CMD /C " + expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("script.bat", content), + updatedCommand + ); assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains(" echo "))); assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains("-decode"))); } @@ -174,7 +178,10 @@ void fallsBackToSha1WhenSha256IsUnavailable() throws Exception { TIMEOUT ); - assertEquals(expectedRemoteDirectory() + "\\legacy.vbs", updatedCommand); + assertEquals( + expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("legacy.vbs", content), + updatedCommand + ); assertArrayEquals(content, echoedContent(executor.getExecutedCommands())); } @@ -198,7 +205,8 @@ void throwsAndCleansUpOnIntegrityMismatch() throws Exception { assertTrue(exception.getMessage().contains("Integrity check failed")); - final String remoteFile = expectedRemoteDirectory() + "\\corrupted.txt"; + final String remoteFile = expectedRemoteDirectory() + "\\" + + ShellFileCopy.contentAddressedName("corrupted.txt", content); assertTrue( executor .getExecutedCommands() @@ -225,7 +233,10 @@ void transfersEmptyFile() throws Exception { TIMEOUT ); - assertEquals(expectedRemoteDirectory() + "\\empty.txt", updatedCommand); + assertEquals( + expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("empty.txt", content), + updatedCommand + ); assertFalse(executor.getExecutedCommands().stream().anyMatch(command -> command.contains(" echo "))); } @@ -275,6 +286,23 @@ void buildUploadCommandsChunksBelowTheCommandLineLimit() { assertArrayEquals(content, echoedContent(commands)); } + @Test + void buildsContentAddressedRemoteNames() { + // SHA-256("abc") = ba7816bf8f01cfea...: the first 12 hex chars go into the remote name + final byte[] content = "abc".getBytes(UTF_8); + + assertEquals("script.ba7816bf8f01.vbs", ShellFileCopy.contentAddressedName("script.vbs", content)); + assertEquals("no-extension.ba7816bf8f01", ShellFileCopy.contentAddressedName("no-extension", content)); + assertEquals(".hidden.ba7816bf8f01", ShellFileCopy.contentAddressedName(".hidden", content)); + + // Same name, different content: different remote path (no cross-client overwrite) + assertFalse( + ShellFileCopy + .contentAddressedName("script.vbs", content) + .equals(ShellFileCopy.contentAddressedName("script.vbs", "abd".getBytes(UTF_8))) + ); + } + @Test void parsesCertutilDigestOutputs() { final String modern = "SHA256 hash of file C:\\x:\r\nAB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34\r\n" diff --git a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java index 11c091f..75fd111 100644 --- a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java +++ b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java @@ -240,7 +240,8 @@ void testExecuteWithFileToCopy() throws Exception { final String finalCommand = executor.getExecutedCommands().get(executor.getExecutedCommands().size() - 1); assertTrue(finalCommand.startsWith("CMD.EXE /C (CSCRIPT ")); assertTrue(finalCommand.contains("\\Temp\\")); - assertTrue(finalCommand.contains("MyScript.vbs")); + // The remote name is content-addressed: MyScript..vbs + assertTrue(finalCommand.matches("(?s).*MyScript\\.[0-9a-f]{12}\\.vbs.*")); assertTrue(executor.isClosed()); } } From 1c98e81654547deaf8aea71388b99506f49dce3a Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 16:44:50 +0200 Subject: [PATCH 03/11] Add license headers and Javadoc to the new test helper 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 --- .../winrm/ScriptedWindowsRemoteExecutor.java | 23 +++++++++++++++++++ .../metricshub/winrm/ShellFileCopyTest.java | 20 ++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java b/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java index baddeea..6b52ac1 100644 --- a/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java +++ b/src/test/java/org/metricshub/winrm/ScriptedWindowsRemoteExecutor.java @@ -1,5 +1,25 @@ package org.metricshub.winrm; +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + import java.nio.charset.Charset; import java.util.ArrayDeque; import java.util.ArrayList; @@ -69,6 +89,9 @@ public List getExecutedCommands() { return executedCommands; } + /** + * @return whether {@link #close()} has been called on this executor + */ public boolean isClosed() { return closed; } diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 1b7ca2e..a188f89 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -1,5 +1,25 @@ package org.metricshub.winrm; +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; From 9abd970963c5b72734f0c344a424e3909356162a Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 16:56:24 +0200 Subject: [PATCH 04/11] Stage transfers in unique files and bound remote name lengths 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 "..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 --- CHANGELOG.md | 7 +- .../org/metricshub/winrm/ShellFileCopy.java | 161 ++++++++++++++---- .../metricshub/winrm/ShellFileCopyTest.java | 31 +++- .../command/WinRMCommandExecutorTest.java | 2 + 4 files changed, 164 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8181dc..7704179 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,12 @@ Consequences: - The remote copy is **content-addressed**: a fragment of the content digest is inserted before the file extension (e.g. `script.1a2b3c4d5e6f.vbs`), so same-named files with different content from concurrent clients can never overwrite each other. Scripts that inspect their own file - name (e.g. `WScript.ScriptName`) will see the digest fragment. + name (e.g. `WScript.ScriptName`) will see the digest fragment. Overlong names are truncated to + stay below the NTFS path-component limit (the digest keeps them unique). +- The transfer is decoded into an operation-unique staging file, verified there, and only then + published as the content-addressed destination — never replacing an existing file — so + concurrent transfers of the same content cannot invalidate a copy already verified by another + operation. - The transfer is designed for the small script files this API is meant for; base64 over SOAP is not suited to bulk data. - `SmbTempShare` (class) and `WindowsRemoteProcessUtils.copyLocalFilesToShare(...)` were removed. diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 94b74aa..2372f62 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -65,6 +65,16 @@ private ShellFileCopy() {} /** PEM-style base64 line length, accepted by every certutil version. */ private static final int BASE64_LINE_LENGTH = 76; + /** + * Maximum length of a content-addressed remote file name: leaves ample room for the + * ".<unique>.part" and ".b64" staging suffixes under the NTFS 255-character + * path-component limit. + */ + private static final int MAX_REMOTE_NAME_LENGTH = 180; + + /** Maximum extension length preserved when a remote file name must be truncated. */ + private static final int MAX_EXTENSION_LENGTH = 30; + /** * Digest algorithms in order of preference, as certutil spells them. SHA1 is only a * fallback for old certutil versions without SHA256 support; the digest is a transfer @@ -167,25 +177,113 @@ static String copyFile( return remoteFile; } - upload(windowsRemoteExecutor, content, remoteFile, timeout, start); + if (content.length == 0) { + // Nothing to stage: create the empty file only if absent, and verify it + runChecked( + windowsRemoteExecutor, + String.format("IF NOT EXIST \"%s\" TYPE NUL >\"%s\"", remoteFile, remoteFile), + "create an empty file", + timeout, + start + ); + + final Optional uploaded = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); + if (!uploaded.isPresent() || !uploaded.get().matches(content)) { + bestEffortDelete(windowsRemoteExecutor, timeout, start, remoteFile); - final Optional uploaded = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); - if (!uploaded.isPresent() || !uploaded.get().matches(content)) { - bestEffortDelete(windowsRemoteExecutor, timeout, start, remoteFile); + throw integrityCheckFailure(localPath, remoteFile, windowsRemoteExecutor); + } - throw new WindowsRemoteException( - String.format( - "Integrity check failed after copying %s to %s on %s.", - localPath, - remoteFile, - windowsRemoteExecutor.getHostname() - ) - ); + return remoteFile; + } + + // Upload and verify in an operation-unique staging file, then publish: the shared, + // content-addressed destination is never rewritten once it exists, so a concurrent + // operation can never invalidate a copy another operation has already verified. + final String stagingFile = String.format("%s.%s.part", remoteFile, uniqueSuffix()); + try { + upload(windowsRemoteExecutor, content, stagingFile, timeout, start); + + final Optional staged = remoteDigest(windowsRemoteExecutor, stagingFile, timeout, start); + if (!staged.isPresent() || !staged.get().matches(content)) { + throw integrityCheckFailure(localPath, stagingFile, windowsRemoteExecutor); + } + + publish(windowsRemoteExecutor, stagingFile, remoteFile, timeout, start); + } catch (final TimeoutException | WindowsRemoteException | RuntimeException e) { + bestEffortDelete(windowsRemoteExecutor, timeout, start, stagingFile); + + throw e; } return remoteFile; } + private static WindowsRemoteException integrityCheckFailure( + final Path localPath, + final String remoteFile, + final WindowsRemoteExecutor windowsRemoteExecutor + ) { + return new WindowsRemoteException( + String.format( + "Integrity check failed after copying %s to %s on %s.", + localPath, + remoteFile, + windowsRemoteExecutor.getHostname() + ) + ); + } + + /** + * Publish the verified staging file as the content-addressed destination without ever + * replacing an existing file: if a concurrent operation already published the destination + * (whose content is identical by construction of the name), the staging copy is simply + * discarded. The publication is confirmed with an existence check, so the loser of a + * publish race succeeds as long as the destination is there. + * + * @param windowsRemoteExecutor Executor connected to the remote host + * @param stagingFile The verified, operation-unique staging file + * @param remoteFile The content-addressed destination + * @param timeout Timeout in milliseconds + * @param start Operation start time in milliseconds + * @throws TimeoutException To notify userName of timeout + * @throws WindowsRemoteException When the destination is missing after publication + */ + private static void publish( + final WindowsRemoteExecutor windowsRemoteExecutor, + final String stagingFile, + final String remoteFile, + final long timeout, + final long start + ) throws TimeoutException, WindowsRemoteException { + // Unchecked: in a publish race, the loser's MOVE may fail — the confirmation below decides + run( + windowsRemoteExecutor, + String.format( + "IF EXIST \"%2$s\" (DEL /F /Q \"%1$s\") ELSE (MOVE /Y \"%1$s\" \"%2$s\")", + stagingFile, + remoteFile + ), + "publish the transferred file", + timeout, + start + ); + + runChecked( + windowsRemoteExecutor, + String.format("IF NOT EXIST \"%s\" EXIT /B 1", remoteFile), + "confirm the published file", + timeout, + start + ); + } + + /** Compact operation-unique suffix for staging file names. */ + private static String uniqueSuffix() { + return (Long.toHexString(Utils.getCurrentTimeMillis()) + "-" + + Integer.toHexString((int) (Math.random() * 0x10000))); + } + /** * Transfer the file content to the remote path: chunked base64 {@code echo} legs, then a * single {@code certutil -decode} that also removes the intermediate base64 file. @@ -205,23 +303,8 @@ private static void upload( final long timeout, final long start ) throws TimeoutException, WindowsRemoteException { - if (content.length == 0) { - runChecked( - windowsRemoteExecutor, - String.format("TYPE NUL >\"%s\"", remoteFile), - "create an empty file", - timeout, - start - ); - - return; - } - - final String base64File = String.format( - "%s.%s.b64", - remoteFile, - WindowsRemoteProcessUtils.buildNewOutputFileName() - ); + // The target is already operation-unique (staging), so the base64 sidecar is too + final String base64File = remoteFile + ".b64"; try { for (final String uploadCommand : buildUploadCommands( @@ -424,10 +507,24 @@ private static void bestEffortDelete( */ static String contentAddressedName(final String fileName, final byte[] content) { final int dot = fileName.lastIndexOf('.'); - final String base = dot > 0 ? fileName.substring(0, dot) : fileName; - final String extension = dot > 0 ? fileName.substring(dot) : Utils.EMPTY; + String base = dot > 0 ? fileName.substring(0, dot) : fileName; + String extension = dot > 0 ? fileName.substring(dot) : Utils.EMPTY; + + final String digest = digestHex("SHA-256", content).substring(0, 12); + + // Bound the name so that even with the "..part.b64" staging suffixes the remote + // path component stays well below the NTFS 255-character limit. Truncating never causes + // collisions: the digest fragment keeps the name unique per content. + if (extension.length() > MAX_EXTENSION_LENGTH) { + extension = extension.substring(0, MAX_EXTENSION_LENGTH); + } + + final int maxBaseLength = MAX_REMOTE_NAME_LENGTH - digest.length() - 1 - extension.length(); + if (base.length() > maxBaseLength) { + base = base.substring(0, maxBaseLength); + } - return base + "." + digestHex("SHA-256", content).substring(0, 12) + extension; + return base + "." + digest + extension; } /** diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index a188f89..5b8a507 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -125,7 +125,9 @@ void uploadsNewFileAndRewritesCommand() throws Exception { .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex(content))) .expectCommand(" SHA1", FAILURE) .expectCommand(" echo ", SUCCESS) - .expectCommand("certutil -f -decode", SUCCESS); + .expectCommand("certutil -f -decode", SUCCESS) + .expectCommand("MOVE /Y", SUCCESS) + .expectCommand("EXIT /B 1", SUCCESS); final String remoteFile = expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("My Script.vbs", content); @@ -143,16 +145,25 @@ void uploadsNewFileAndRewritesCommand() throws Exception { // The echoed base64, reassembled, is exactly the file content (multibyte UTF-8 intact) assertArrayEquals(content, echoedContent(executor.getExecutedCommands())); - // The decode leg produces the target file and removes the intermediate base64 file + // The decode leg produces the operation-unique staging file and removes the base64 sidecar final String decodeCommand = executor .getExecutedCommands() .stream() .filter(command -> command.contains("certutil -f -decode")) .findFirst() .orElseThrow(); - assertTrue(decodeCommand.contains("\"" + remoteFile + "\"")); + assertTrue(decodeCommand.contains(remoteFile + ".")); + assertTrue(decodeCommand.contains(".part")); assertTrue(decodeCommand.contains("DEL /F /Q")); assertTrue(decodeCommand.contains(".b64")); + + // The verified staging file is then published as the content-addressed destination + assertTrue( + executor + .getExecutedCommands() + .stream() + .anyMatch(command -> command.contains("MOVE /Y") && command.contains("\"" + remoteFile + "\"")) + ); } @Test @@ -189,7 +200,9 @@ void fallsBackToSha1WhenSha256IsUnavailable() throws Exception { .expectCommand(" SHA256", FAILURE) .expectCommand(" SHA1", FAILURE, hashOutput("SHA1", sha1Hex(content))) .expectCommand(" echo ", SUCCESS) - .expectCommand("certutil -f -decode", SUCCESS); + .expectCommand("certutil -f -decode", SUCCESS) + .expectCommand("MOVE /Y", SUCCESS) + .expectCommand("EXIT /B 1", SUCCESS); final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( executor, @@ -315,6 +328,16 @@ void buildsContentAddressedRemoteNames() { assertEquals("no-extension.ba7816bf8f01", ShellFileCopy.contentAddressedName("no-extension", content)); assertEquals(".hidden.ba7816bf8f01", ShellFileCopy.contentAddressedName(".hidden", content)); + // Very long names are truncated (digest keeps them unique) so that even with the + // "..part.b64" staging suffixes the NTFS 255-character component limit holds + final String longName = ShellFileCopy.contentAddressedName("x".repeat(300) + ".vbs", content); + assertTrue(longName.length() <= 180); + assertTrue(longName.endsWith(".ba7816bf8f01.vbs")); + + final String longExtension = ShellFileCopy.contentAddressedName("f." + "e".repeat(300), content); + assertTrue(longExtension.length() <= 180); + assertTrue(longExtension.contains(".ba7816bf8f01.")); + // Same name, different content: different remote path (no cross-client overwrite) assertFalse( ShellFileCopy diff --git a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java index 75fd111..379f5ec 100644 --- a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java +++ b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java @@ -213,6 +213,8 @@ void testExecuteWithFileToCopy() throws Exception { .expectCommand(" SHA1", failure) .expectCommand(" echo ", success) .expectCommand("certutil -f -decode", success) + .expectCommand("MOVE /Y", success) + .expectCommand("EXIT /B 1", success) .expectCommand("CSCRIPT", expected); try (final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class)) { From 747029be585fc4b30ccbab95dcfe679700405d95 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 17:30:11 +0200 Subject: [PATCH 05/11] Repair mismatched cached destinations and verify the published digest 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 --- CHANGELOG.md | 12 +- .../org/metricshub/winrm/ShellFileCopy.java | 264 +++++++++++++----- .../metricshub/winrm/ShellFileCopyTest.java | 191 +++++++++++-- .../command/WinRMCommandExecutorTest.java | 8 +- 4 files changed, 386 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7704179..42f130d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,11 +27,17 @@ Consequences: name (e.g. `WScript.ScriptName`) will see the digest fragment. Overlong names are truncated to stay below the NTFS path-component limit (the digest keeps them unique). - The transfer is decoded into an operation-unique staging file, verified there, and only then - published as the content-addressed destination — never replacing an existing file — so - concurrent transfers of the same content cannot invalidate a copy already verified by another - operation. + published as the content-addressed destination. A destination that already carries the + expected digest is never rewritten (so concurrent transfers of the same content cannot + invalidate a copy already verified by another operation), while a mismatched pre-existing + copy (e.g. corrupted in place) is repaired by replacement. In every case, the destination's + digest is verified last: the operation fails rather than execute unproven bytes. - The transfer is designed for the small script files this API is meant for; base64 over SOAP is not suited to bulk data. +- Transfer steps are batched to minimize WinRM operations (the digest probe rides the same + command leg as the decode and publish steps), and a command rejected by the server-side + concurrent-operation quota — very low on old hosts (15 per user on Windows 2008 R2) — is + retried with a delay when the rejection happened before the command could run. - `SmbTempShare` (class) and `WindowsRemoteProcessUtils.copyLocalFilesToShare(...)` were removed. `WindowsTempShare` is unchanged. diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 2372f62..1faea8f 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -82,6 +82,19 @@ private ShellFileCopy() {} */ private static final String[] CERTUTIL_ALGORITHMS = { "SHA256", "SHA1" }; + /** WSManFault code for "the maximum number of concurrent operations for this user has been exceeded". */ + private static final String FAULT_OPERATION_QUOTA = "2150859174"; + + /** How many times a transfer command is retried after an operation-quota rejection. */ + private static final int QUOTA_RETRIES = 4; + + /** + * Base delay before retrying after an operation-quota rejection; each retry waits one step + * longer. Measured on Windows 2008 R2 (quota 15 per user): the budget fully recovers within + * 30 seconds, so the escalating delays (5+10+15+20 s) comfortably bridge it. + */ + private static final long QUOTA_RETRY_DELAY_MILLIS = 5_000L; + /** * Copy the specified local files to a temporary directory on the remote host through the * WinRM command shell, and return the command updated so that each reference to a local @@ -122,10 +135,12 @@ public static String copyLocalFilesToRemote( WindowsTempShare.buildShareName() ); - WindowsTempShare.createRemoteDirectory( + // Through the local, quota-retrying runChecked rather than WindowsTempShare.createRemoteDirectory + runChecked( windowsRemoteExecutor, - remoteDirectory, - TimeoutHelper.getRemainingTime(timeout, start, "No time left to create the remote temporary directory"), + WindowsTempShare.buildCreateRemoteDirectoryCommand(remoteDirectory), + "create the remote temporary directory", + timeout, start ); @@ -171,26 +186,35 @@ static String copyFile( // the digest verification and the command execution. final String remoteFile = remoteDirectory + "\\" + contentAddressedName(fileName, content); - // Skip the transfer if the remote host already has an identical copy + // Skip the transfer if the remote host already has an identical copy. A destination that + // exists with a DIFFERENT digest (e.g. a cached copy corrupted or modified in place) is + // remembered: it must be repaired by replacement, not trusted. final Optional existing = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); if (existing.isPresent() && existing.get().matches(content)) { return remoteFile; } + final boolean mismatchedDestination = existing.isPresent(); if (content.length == 0) { - // Nothing to stage: create the empty file only if absent, and verify it - runChecked( + // Nothing to stage: create the empty file (truncating a mismatched pre-existing copy) + // and verify its digest in the same command leg + final WindowsRemoteCommandResult created = run( windowsRemoteExecutor, - String.format("IF NOT EXIST \"%s\" TYPE NUL >\"%s\"", remoteFile, remoteFile), + (mismatchedDestination + ? String.format("TYPE NUL >\"%s\"", remoteFile) + : String.format("IF NOT EXIST \"%s\" TYPE NUL >\"%s\"", remoteFile, remoteFile)) + + " & " + + digestProbe(remoteFile), "create an empty file", timeout, start ); - final Optional uploaded = remoteDigest(windowsRemoteExecutor, remoteFile, timeout, start); - if (!uploaded.isPresent() || !uploaded.get().matches(content)) { - bestEffortDelete(windowsRemoteExecutor, timeout, start, remoteFile); - + // The destination itself must carry the expected digest — never return (and let the + // caller execute) a file whose content wasn't proven. On failure the destination is + // left in place: the next transfer detects the mismatch and repairs it. + final Optional published = parseAnyDigest(created.getStdout()); + if (!published.isPresent() || !published.get().matches(content)) { throw integrityCheckFailure(localPath, remoteFile, windowsRemoteExecutor); } @@ -198,18 +222,22 @@ static String copyFile( } // Upload and verify in an operation-unique staging file, then publish: the shared, - // content-addressed destination is never rewritten once it exists, so a concurrent - // operation can never invalidate a copy another operation has already verified. + // content-addressed destination is never rewritten once it carries the right content, + // so a concurrent operation can never invalidate a copy another operation verified. final String stagingFile = String.format("%s.%s.part", remoteFile, uniqueSuffix()); try { - upload(windowsRemoteExecutor, content, stagingFile, timeout, start); + upload(windowsRemoteExecutor, content, stagingFile, localPath, timeout, start); - final Optional staged = remoteDigest(windowsRemoteExecutor, stagingFile, timeout, start); - if (!staged.isPresent() || !staged.get().matches(content)) { - throw integrityCheckFailure(localPath, stagingFile, windowsRemoteExecutor); - } - - publish(windowsRemoteExecutor, stagingFile, remoteFile, timeout, start); + publish( + windowsRemoteExecutor, + stagingFile, + remoteFile, + mismatchedDestination, + content, + localPath, + timeout, + start + ); } catch (final TimeoutException | WindowsRemoteException | RuntimeException e) { bestEffortDelete(windowsRemoteExecutor, timeout, start, stagingFile); @@ -235,47 +263,58 @@ private static WindowsRemoteException integrityCheckFailure( } /** - * Publish the verified staging file as the content-addressed destination without ever - * replacing an existing file: if a concurrent operation already published the destination - * (whose content is identical by construction of the name), the staging copy is simply - * discarded. The publication is confirmed with an existence check, so the loser of a - * publish race succeeds as long as the destination is there. + * Publish the verified staging file as the content-addressed destination and verify the + * destination digest in the same command leg. When the destination was seen with a + * mismatched digest, it is force-replaced (repair); otherwise an existing destination is + * left untouched — a concurrent operation already published the identical content — and the + * staging copy is discarded. The exit code is deliberately ignored: in a publish race the + * loser's {@code MOVE} may fail, and only the destination digest decides success. Never + * return (and let the caller execute) a file whose content wasn't proven; on failure the + * destination is left in place, so the next transfer detects the mismatch and repairs it. * * @param windowsRemoteExecutor Executor connected to the remote host * @param stagingFile The verified, operation-unique staging file * @param remoteFile The content-addressed destination + * @param replaceMismatched Whether the destination pre-existed with a mismatched digest and + * must be replaced + * @param content The expected file content + * @param localPath The local file, for the failure message * @param timeout Timeout in milliseconds * @param start Operation start time in milliseconds * @throws TimeoutException To notify userName of timeout - * @throws WindowsRemoteException When the destination is missing after publication + * @throws WindowsRemoteException When the published destination does not carry the digest + * of the local file */ private static void publish( final WindowsRemoteExecutor windowsRemoteExecutor, final String stagingFile, final String remoteFile, + final boolean replaceMismatched, + final byte[] content, + final Path localPath, final long timeout, final long start ) throws TimeoutException, WindowsRemoteException { - // Unchecked: in a publish race, the loser's MOVE may fail — the confirmation below decides - run( + final WindowsRemoteCommandResult result = run( windowsRemoteExecutor, - String.format( - "IF EXIST \"%2$s\" (DEL /F /Q \"%1$s\") ELSE (MOVE /Y \"%1$s\" \"%2$s\")", - stagingFile, - remoteFile - ), + (replaceMismatched + ? String.format("MOVE /Y \"%s\" \"%s\"", stagingFile, remoteFile) + : String.format( + "IF EXIST \"%2$s\" (DEL /F /Q \"%1$s\") ELSE (MOVE /Y \"%1$s\" \"%2$s\")", + stagingFile, + remoteFile + )) + + " & " + + digestProbe(remoteFile), "publish the transferred file", timeout, start ); - runChecked( - windowsRemoteExecutor, - String.format("IF NOT EXIST \"%s\" EXIT /B 1", remoteFile), - "confirm the published file", - timeout, - start - ); + final Optional published = parseAnyDigest(result.getStdout()); + if (!published.isPresent() || !published.get().matches(content)) { + throw integrityCheckFailure(localPath, remoteFile, windowsRemoteExecutor); + } } /** Compact operation-unique suffix for staging file names. */ @@ -285,12 +324,15 @@ private static String uniqueSuffix() { } /** - * Transfer the file content to the remote path: chunked base64 {@code echo} legs, then a - * single {@code certutil -decode} that also removes the intermediate base64 file. + * Transfer the file content to the remote (staging) path: chunked base64 {@code echo} legs, + * then a single leg that decodes with {@code certutil -decode}, removes the intermediate + * base64 file, and reports the digest of the decoded file — which is verified against the + * local content before returning. * * @param windowsRemoteExecutor Executor connected to the remote host * @param content The file content * @param remoteFile The target path on the remote host + * @param localPath The local file, for the failure message * @param timeout Timeout in milliseconds * @param start Operation start time in milliseconds * @throws TimeoutException To notify userName of timeout @@ -300,6 +342,7 @@ private static void upload( final WindowsRemoteExecutor windowsRemoteExecutor, final byte[] content, final String remoteFile, + final Path localPath, final long timeout, final long start ) throws TimeoutException, WindowsRemoteException { @@ -314,13 +357,30 @@ private static void upload( runChecked(windowsRemoteExecutor, uploadCommand, "upload the file content", timeout, start); } - runChecked( + final WindowsRemoteCommandResult decoded = run( windowsRemoteExecutor, - String.format("certutil -f -decode \"%s\" \"%s\" && DEL /F /Q \"%s\"", base64File, remoteFile, base64File), + String.format("certutil -f -decode \"%s\" \"%s\" && DEL /F /Q \"%s\"", base64File, remoteFile, base64File) + + " & " + + digestProbe(remoteFile), "decode the transferred file", timeout, start ); + + final Optional staged = parseAnyDigest(decoded.getStdout()); + if (!staged.isPresent()) { + throw new WindowsRemoteException( + String.format( + "Failed to decode the transferred file %s on %s: %s", + remoteFile, + windowsRemoteExecutor.getHostname(), + Utils.isNotBlank(decoded.getStderr()) ? decoded.getStderr().trim() : decoded.getStdout().trim() + ) + ); + } + if (!staged.get().matches(content)) { + throw integrityCheckFailure(localPath, remoteFile, windowsRemoteExecutor); + } } catch (final TimeoutException | WindowsRemoteException | RuntimeException e) { bestEffortDelete(windowsRemoteExecutor, timeout, start, base64File, remoteFile); @@ -362,8 +422,9 @@ static List buildUploadCommands(final String base64, final String base64 } /** - * Get the digest of a remote file with {@code certutil -hashfile}, trying each supported - * algorithm in order. + * Get the digest of a remote file: a single command leg runs {@code certutil -hashfile} for + * every supported algorithm (one round trip; old certutil versions without SHA256 simply + * fail that part), and the output is parsed by algorithm preference. * * @param windowsRemoteExecutor Executor connected to the remote host * @param remoteFile The remote file to hash @@ -380,20 +441,51 @@ private static Optional remoteDigest( final long timeout, final long start ) throws TimeoutException, WindowsRemoteException { + final WindowsRemoteCommandResult result = run( + windowsRemoteExecutor, + digestProbe(remoteFile), + "hash the remote file", + timeout, + start + ); + + return parseAnyDigest(result.getStdout()); + } + + /** + * Build the command that prints the digest of the given remote file with every supported + * algorithm in one go. Appending the probe (with {@code " & "}) to a transfer command saves + * a WinRM operation per step, which both speeds the transfer up and relieves the + * server-side concurrent-operation quota that old Windows versions set very low (15 per + * user on Windows 2008 R2). + * + * @param remoteFile The remote file to hash + * @return the digest-probing command + */ + static String digestProbe(final String remoteFile) { + final StringBuilder probe = new StringBuilder(); for (final String algorithm : CERTUTIL_ALGORITHMS) { - final WindowsRemoteCommandResult result = run( - windowsRemoteExecutor, - String.format("certutil -hashfile \"%s\" %s", remoteFile, algorithm), - "hash the remote file", - timeout, - start - ); + if (probe.length() > 0) { + probe.append(" & "); + } + probe.append(String.format("certutil -hashfile \"%s\" %s", remoteFile, algorithm)); + } - if (result.getStatusCode() == 0) { - final Optional digest = parseCertutilDigest(result.getStdout(), algorithm); - if (digest.isPresent()) { - return Optional.of(new RemoteDigest(algorithm, digest.get())); - } + return probe.toString(); + } + + /** + * Extract the first digest found in a {@code certutil -hashfile} output, trying each + * supported algorithm in preference order. + * + * @param output The command standard output + * @return the digest, or an empty Optional if none was found + */ + static Optional parseAnyDigest(final String output) { + for (final String algorithm : CERTUTIL_ALGORITHMS) { + final Optional digest = parseCertutilDigest(output, algorithm); + if (digest.isPresent()) { + return Optional.of(new RemoteDigest(algorithm, digest.get())); } } @@ -461,12 +553,56 @@ private static WindowsRemoteCommandResult run( final long timeout, final long start ) throws TimeoutException, WindowsRemoteException { - return windowsRemoteExecutor.executeCommand( - command, - null, - null, - TimeoutHelper.getRemainingTime(timeout, start, "No time left to " + description) - ); + for (int attempt = 0;; attempt++) { + try { + return windowsRemoteExecutor.executeCommand( + command, + null, + null, + TimeoutHelper.getRemainingTime(timeout, start, "No time left to " + description) + ); + } catch (final WindowsRemoteException e) { + if (attempt >= QUOTA_RETRIES || !isRetryableQuotaRejection(e)) { + throw e; + } + + // The quota rejection happened while the operation was being CREATED — before the + // command could run — so retrying cannot duplicate a side effect. Old Windows + // versions cap concurrent operations very low (15 per user on 2008 R2) and reap + // completed ones lazily: give the server increasingly more time to recover. + try { + Utils.sleep( + Math.min( + QUOTA_RETRY_DELAY_MILLIS * (attempt + 1), + TimeoutHelper.getRemainingTime(timeout, start, "No time left to retry after a quota rejection") + ) + ); + } catch (final InterruptedException interrupted) { + Thread.currentThread().interrupt(); + throw e; + } + } + } + } + + /** + * Whether the exception is a server-side operation-quota rejection that occurred while the + * operation was being created (shell creation or command start), i.e. before the command + * could produce any side effect — the only situation where a retry is safe. A quota fault + * on a later protocol step (e.g. Receive) means the command may already be running and is + * never retried. + * + * @param exception The exception reported by the executor + * @return whether the failed command can safely be retried + */ + static boolean isRetryableQuotaRejection(final Exception exception) { + final String message = exception.getMessage(); + + return (message != null + && + message.contains(FAULT_OPERATION_QUOTA) + && + (message.contains("Command failed") || message.contains("Create shell failed"))); } /** diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 5b8a507..645b5c4 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -122,12 +122,10 @@ void uploadsNewFileAndRewritesCommand() throws Exception { Files.write(localFile, content); final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() - .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex(content))) - .expectCommand(" SHA1", FAILURE) .expectCommand(" echo ", SUCCESS) - .expectCommand("certutil -f -decode", SUCCESS) - .expectCommand("MOVE /Y", SUCCESS) - .expectCommand("EXIT /B 1", SUCCESS); + .expectCommand("certutil -f -decode", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("MOVE /Y", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("certutil -hashfile", FAILURE); final String remoteFile = expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("My Script.vbs", content); @@ -173,7 +171,7 @@ void skipsUploadWhenRemoteCopyIsIdentical() throws Exception { Files.write(localFile, content); final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() - .expectCommand(" SHA256", hashOutput("SHA256", sha256Hex(content))); + .expectCommand("certutil -hashfile", hashOutput("SHA256", sha256Hex(content))); final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( executor, @@ -197,12 +195,10 @@ void fallsBackToSha1WhenSha256IsUnavailable() throws Exception { Files.write(localFile, content); final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() - .expectCommand(" SHA256", FAILURE) - .expectCommand(" SHA1", FAILURE, hashOutput("SHA1", sha1Hex(content))) .expectCommand(" echo ", SUCCESS) - .expectCommand("certutil -f -decode", SUCCESS) - .expectCommand("MOVE /Y", SUCCESS) - .expectCommand("EXIT /B 1", SUCCESS); + .expectCommand("certutil -f -decode", hashOutput("SHA1", sha1Hex(content))) + .expectCommand("MOVE /Y", hashOutput("SHA1", sha1Hex(content))) + .expectCommand("certutil -hashfile", FAILURE); final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( executor, @@ -225,10 +221,9 @@ void throwsAndCleansUpOnIntegrityMismatch() throws Exception { Files.write(localFile, content); final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() - .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex("tampered".getBytes(UTF_8)))) - .expectCommand(" SHA1", FAILURE) .expectCommand(" echo ", SUCCESS) - .expectCommand("certutil -f -decode", SUCCESS) + .expectCommand("certutil -f -decode", hashOutput("SHA256", sha256Hex("tampered".getBytes(UTF_8)))) + .expectCommand("certutil -hashfile", FAILURE) .expectCommand("DEL /F /Q", SUCCESS); final WindowsRemoteException exception = assertThrows( @@ -248,6 +243,66 @@ void throwsAndCleansUpOnIntegrityMismatch() throws Exception { ); } + @Test + void repairsAMismatchedCachedDestination() throws Exception { + final byte[] content = "good content".getBytes(UTF_8); + final Path localFile = tempDir.resolve("repair.vbs"); + Files.write(localFile, content); + + // The destination pre-exists with a DIFFERENT digest (e.g. a cached copy corrupted in + // place): the transfer must replace it, never trust it. + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" echo ", SUCCESS) + .expectCommand("certutil -f -decode", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("MOVE /Y", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("certutil -hashfile", hashOutput("SHA256", sha256Hex("corrupted".getBytes(UTF_8)))); + + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + executor, + localFile.toString(), + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals( + expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("repair.vbs", content), + updatedCommand + ); + + // The file was re-uploaded and force-published: a bare MOVE, not guarded by IF EXIST + assertArrayEquals(content, echoedContent(executor.getExecutedCommands())); + final String publishCommand = executor + .getExecutedCommands() + .stream() + .filter(command -> command.contains("MOVE /Y")) + .findFirst() + .orElseThrow(); + assertTrue(publishCommand.startsWith("MOVE /Y")); + } + + @Test + void failsWhenThePublishedDestinationDigestMismatches() throws Exception { + final byte[] content = "expected content".getBytes(UTF_8); + final Path localFile = tempDir.resolve("unlucky.txt"); + Files.write(localFile, content); + + // Staging verifies fine, but the published destination reports a different digest: + // the operation must fail rather than let the caller execute unproven bytes. + final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() + .expectCommand(" echo ", SUCCESS) + .expectCommand("certutil -f -decode", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("MOVE /Y", hashOutput("SHA256", sha256Hex("something else".getBytes(UTF_8)))) + .expectCommand("certutil -hashfile", FAILURE) + .expectCommand("DEL /F /Q", SUCCESS); + + final WindowsRemoteException exception = assertThrows( + WindowsRemoteException.class, + () -> ShellFileCopy.copyLocalFilesToRemote(executor, localFile.toString(), List.of(localFile.toString()), TIMEOUT) + ); + + assertTrue(exception.getMessage().contains("Integrity check failed")); + } + @Test void transfersEmptyFile() throws Exception { final byte[] content = new byte[0]; @@ -255,9 +310,8 @@ void transfersEmptyFile() throws Exception { Files.write(localFile, content); final ScriptedWindowsRemoteExecutor executor = executorWithTempDirectory() - .expectCommand(" SHA256", FAILURE, hashOutput("SHA256", sha256Hex(content))) - .expectCommand(" SHA1", FAILURE) - .expectCommand("TYPE NUL", SUCCESS); + .expectCommand("TYPE NUL", hashOutput("SHA256", sha256Hex(content))) + .expectCommand("certutil -hashfile", FAILURE); final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( executor, @@ -346,6 +400,103 @@ void buildsContentAddressedRemoteNames() { ); } + @Test + void classifiesQuotaRejectionsForRetry() { + // Retryable: the operation was rejected at creation (shell or command), nothing ran yet + assertTrue( + ShellFileCopy.isRetryableQuotaRejection( + new WindowsRemoteException( + "Command failed: HTTP 500 (WSManFault 2150859174): The WS-Management service cannot process the request." + ) + ) + ); + assertTrue( + ShellFileCopy.isRetryableQuotaRejection( + new WindowsRemoteException("Create shell failed: HTTP 500 (WSManFault 2150859174): quota exceeded") + ) + ); + + // Not retryable: the command may already be running (Receive), or it's another fault + assertFalse( + ShellFileCopy.isRetryableQuotaRejection( + new WindowsRemoteException("Receive failed: HTTP 500 (WSManFault 2150859174): quota exceeded") + ) + ); + assertFalse( + ShellFileCopy.isRetryableQuotaRejection( + new WindowsRemoteException("Command failed: HTTP 500 (WSManFault 2150858793): timeout") + ) + ); + assertFalse(ShellFileCopy.isRetryableQuotaRejection(new WindowsRemoteException((String) null))); + } + + @Test + void retriesACommandRejectedByTheOperationQuota() throws Exception { + final byte[] content = "quota".getBytes(UTF_8); + final Path localFile = tempDir.resolve("quota.bat"); + Files.write(localFile, content); + + // Delegate scripted for the cheap skip path (remote copy already identical) + final ScriptedWindowsRemoteExecutor delegate = executorWithTempDirectory() + .expectCommand("certutil -hashfile", hashOutput("SHA256", sha256Hex(content))); + + // The first command attempt is rejected by the server operation quota; the retry succeeds + final java.util.concurrent.atomic.AtomicInteger rejections = new java.util.concurrent.atomic.AtomicInteger(1); + final WindowsRemoteExecutor flaky = new WindowsRemoteExecutor() { + @Override + public List> executeWql(final String wqlQuery, final long timeout) { + return delegate.executeWql(wqlQuery, timeout); + } + + @Override + public WindowsRemoteCommandResult executeCommand( + final String command, + final String workingDirectory, + final java.nio.charset.Charset charset, + final long timeout + ) throws WindowsRemoteException { + if (command.contains("certutil -hashfile") && rejections.getAndDecrement() > 0) { + throw new WindowsRemoteException( + "Command failed: HTTP 500 (WSManFault 2150859174): maximum number of concurrent operations exceeded" + ); + } + return delegate.executeCommand(command, workingDirectory, charset, timeout); + } + + @Override + public String getHostname() { + return delegate.getHostname(); + } + + @Override + public String getUsername() { + return delegate.getUsername(); + } + + @Override + public char[] getPassword() { + return delegate.getPassword(); + } + + @Override + public void close() { + delegate.close(); + } + }; + + final String updatedCommand = ShellFileCopy.copyLocalFilesToRemote( + flaky, + localFile.toString(), + List.of(localFile.toString()), + TIMEOUT + ); + + assertEquals( + expectedRemoteDirectory() + "\\" + ShellFileCopy.contentAddressedName("quota.bat", content), + updatedCommand + ); + } + @Test void parsesCertutilDigestOutputs() { final String modern = "SHA256 hash of file C:\\x:\r\nAB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34AB12cd34\r\n" @@ -367,5 +518,11 @@ void parsesCertutilDigestOutputs() { assertEquals(Optional.empty(), ShellFileCopy.parseCertutilDigest("no digest here", "SHA256")); assertEquals(Optional.empty(), ShellFileCopy.parseCertutilDigest(null, "SHA256")); + + // The combined probe hashes with every supported algorithm in a single command leg + assertEquals( + "certutil -hashfile \"C:\\f\" SHA256 & certutil -hashfile \"C:\\f\" SHA1", + ShellFileCopy.digestProbe("C:\\f") + ); } } diff --git a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java index 379f5ec..20645e5 100644 --- a/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java +++ b/src/test/java/org/metricshub/winrm/command/WinRMCommandExecutorTest.java @@ -209,12 +209,10 @@ void testExecuteWithFileToCopy() throws Exception { .expectWql("WindowsDirectory", List.of(Map.of("WindowsDirectory", "C:\\Windows"))) .expectWql("CodeSet", List.of(Map.of("CodeSet", "65001"))) .expectCommand("MKDIR", success) - .expectCommand(" SHA256", failure, remoteHash) - .expectCommand(" SHA1", failure) .expectCommand(" echo ", success) - .expectCommand("certutil -f -decode", success) - .expectCommand("MOVE /Y", success) - .expectCommand("EXIT /B 1", success) + .expectCommand("certutil -f -decode", remoteHash) + .expectCommand("MOVE /Y", remoteHash) + .expectCommand("certutil -hashfile", failure) .expectCommand("CSCRIPT", expected); try (final MockedStatic mockedFactory = mockStatic(WinRMExecutorFactory.class)) { From 07ff4836194afcf0dba79591016f234dfe9d0fd7 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 17:39:12 +0200 Subject: [PATCH 06/11] Reject file names Windows cannot create 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 --- .../org/metricshub/winrm/ShellFileCopy.java | 32 ++++++++++++++++--- .../metricshub/winrm/ShellFileCopyTest.java | 20 ++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 1faea8f..154c847 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -32,6 +32,7 @@ import java.util.Locale; import java.util.Optional; import java.util.concurrent.TimeoutException; +import java.util.regex.Pattern; import org.metricshub.winrm.exceptions.WindowsRemoteException; /** @@ -82,6 +83,13 @@ private ShellFileCopy() {} */ private static final String[] CERTUTIL_ALGORITHMS = { "SHA256", "SHA1" }; + /** Characters that are forbidden in Windows file names (a non-Windows client may produce them locally). */ + private static final String WINDOWS_FORBIDDEN_CHARACTERS = "<>:\"/\\|?*"; + + /** Windows reserved device names, with or without an extension (e.g. {@code CON}, {@code CON.ps1}). */ + private static final Pattern RESERVED_DEVICE_NAME = Pattern + .compile("(?i)(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])(\\..*)?"); + /** WSManFault code for "the maximum number of concurrent operations for this user has been exceeded". */ private static final String FAULT_OPERATION_QUOTA = "2150859174"; @@ -685,16 +693,30 @@ static String digestHex(final String algorithm, final byte[] content) { } /** - * Reject file names that cannot be embedded safely in a quoted cmd.exe argument. - * Windows already forbids most cmd metacharacters in file names; the remaining dangerous - * one is {@code %}, which cmd.exe expands as a variable reference even between quotes. + * Reject file names that cannot be transferred: names that cannot be embedded safely in a + * quoted cmd.exe argument ({@code %} expands as a variable reference even between quotes, + * {@code "} and control characters break the quoting), and names Windows cannot create — + * relevant when the client runs on an OS whose local file names may legally contain + * Windows-forbidden characters ({@code < > : " / \ | ? *}), end with a dot or a space, or + * collide with a reserved device name ({@code CON}, {@code NUL}, {@code COM1}…, with or + * without an extension). * * @param fileName The name of the file to transfer */ static void checkTransferableFileName(final String fileName) { - if (fileName.contains("%") || fileName.contains("\"") || fileName.chars().anyMatch(c -> c < 0x20)) { + if (fileName.isEmpty() + || + fileName.contains("%") + || + fileName.chars().anyMatch(c -> c < 0x20 || WINDOWS_FORBIDDEN_CHARACTERS.indexOf(c) >= 0) + || + fileName.endsWith(".") + || + fileName.endsWith(" ") + || + RESERVED_DEVICE_NAME.matcher(fileName).matches()) { throw new IllegalArgumentException( - String.format("File name %s contains characters that cannot be transferred safely.", fileName) + String.format("File name %s cannot be transferred to a Windows host safely.", fileName) ); } } diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 645b5c4..e26424b 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -343,8 +343,28 @@ void rejectsFileNamesUnsafeForTheCommandShell() { assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("quo\"te.txt")); assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("ctrl\u0001.txt")); + // Windows-forbidden characters, legal in file names on other client platforms + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("wild*card.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("que?ry.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("col:on.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("back\\slash.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("pi|pe.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("angle.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("trailingdot.")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("trailing space ")); + + // Windows reserved device names, with or without an extension, case-insensitive + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("CON")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("CON.ps1")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("nul.txt")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("com3.vbs")); + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("Lpt9")); + // Legal Windows file names pass, including cmd metacharacters neutralized by quoting ShellFileCopy.checkTransferableFileName("My Script (v2) & more!.vbs"); + ShellFileCopy.checkTransferableFileName("CONSOLE.vbs"); + ShellFileCopy.checkTransferableFileName("COM10.txt"); + ShellFileCopy.checkTransferableFileName("null.txt"); } @Test From 2c82c96fbf1b12d69ac8f9d5fefd47cc575ec862 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 17:48:57 +0200 Subject: [PATCH 07/11] Sanitize the computer name and purge aged transfer-directory entries 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 --- .../org/metricshub/winrm/ShellFileCopy.java | 18 ++++++- src/main/java/org/metricshub/winrm/Utils.java | 25 ++++++++-- .../metricshub/winrm/WindowsTempShare.java | 2 +- .../metricshub/winrm/ShellFileCopyTest.java | 11 ++++ .../java/org/metricshub/winrm/UtilsTest.java | 50 +++++++++++++++++++ 5 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 src/test/java/org/metricshub/winrm/UtilsTest.java diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 154c847..92c6df7 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -76,6 +76,15 @@ private ShellFileCopy() {} /** Maximum extension length preserved when a remote file name must be truncated. */ private static final int MAX_EXTENSION_LENGTH = 30; + /** + * Transfer-directory entries not modified for this many days are purged before a transfer: + * content-addressing means every revision of a changing file gets a new remote name, and + * without a lifecycle the directory would grow without bound. Also reclaims staging or + * base64 files orphaned by an interrupted transfer. The rare downside: a cached script + * used unmodified for that long is re-uploaded once after the purge. + */ + private static final int CLEANUP_AGE_DAYS = 30; + /** * Digest algorithms in order of preference, as certutil spells them. SHA1 is only a * fallback for old certutil versions without SHA256 support; the digest is a transfer @@ -143,10 +152,15 @@ public static String copyLocalFilesToRemote( WindowsTempShare.buildShareName() ); - // Through the local, quota-retrying runChecked rather than WindowsTempShare.createRemoteDirectory + // One leg (through the local, quota-retrying runChecked): first purge entries older than + // CLEANUP_AGE_DAYS (best-effort, stderr suppressed — reclaims obsolete content-addressed + // revisions and orphaned staging files), then create the directory — last command, so the + // leg's exit code is the MKDIR's. runChecked( windowsRemoteExecutor, - WindowsTempShare.buildCreateRemoteDirectoryCommand(remoteDirectory), + String + .format("forfiles /P \"%s\" /D -%d /C \"cmd /c del /f /q @path\" 2>NUL & ", remoteDirectory, CLEANUP_AGE_DAYS) + + WindowsTempShare.buildCreateRemoteDirectoryCommand(remoteDirectory), "create the remote temporary directory", timeout, start diff --git a/src/main/java/org/metricshub/winrm/Utils.java b/src/main/java/org/metricshub/winrm/Utils.java index c2abf39..23334fd 100644 --- a/src/main/java/org/metricshub/winrm/Utils.java +++ b/src/main/java/org/metricshub/winrm/Utils.java @@ -80,18 +80,18 @@ public static String getComputerName() { // directories on the remote host are keyed by this name). final String computerName = System.getenv("COMPUTERNAME"); if (isNotBlank(computerName)) { - return computerName; + return sanitizeComputerName(computerName); } final String hostName = System.getenv("HOSTNAME"); if (isNotBlank(hostName)) { - return hostName; + return sanitizeComputerName(hostName); } try { final String localName = java.net.InetAddress.getLocalHost().getHostName(); if (isNotBlank(localName)) { - return localName; + return sanitizeComputerName(localName); } } catch (final java.net.UnknownHostException ignored) { // Fall through to the default @@ -100,6 +100,25 @@ public static String getComputerName() { return "localhost"; } + /** + * Keep only characters that are safe both in a Windows directory name and in a cmd.exe + * command line. Real host names only contain letters, digits, dots, and hyphens, but the + * name may come from an environment variable, which is not constrained at all — and it + * ends up embedded in remote shell commands (temporary directory names). + * + * @param name The raw computer name + * @return the sanitized name, or "localhost" if nothing safe remains + */ + static String sanitizeComputerName(final String name) { + String sanitized = name.trim().replaceAll("[^A-Za-z0-9._-]", "-"); + + if (sanitized.length() > 64) { + sanitized = sanitized.substring(0, 64); + } + + return sanitized.replaceAll("[-.]", EMPTY).isEmpty() ? "localhost" : sanitized; + } + /** * Wrapper for Thread.sleep(millis) * diff --git a/src/main/java/org/metricshub/winrm/WindowsTempShare.java b/src/main/java/org/metricshub/winrm/WindowsTempShare.java index 5bb74d4..fbcd70e 100644 --- a/src/main/java/org/metricshub/winrm/WindowsTempShare.java +++ b/src/main/java/org/metricshub/winrm/WindowsTempShare.java @@ -204,7 +204,7 @@ static String buildUncPath(final String hostname, final String share) { static String buildCreateRemoteDirectoryCommand(final String remotePath) { Utils.checkNonBlank(remotePath, "remotePath"); - return String.format("CMD.EXE /C IF NOT EXIST \"%s\" MKDIR %s", remotePath, remotePath); + return String.format("CMD.EXE /C IF NOT EXIST \"%1$s\" MKDIR \"%1$s\"", remotePath); } /** diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index e26424b..4f02395 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -162,6 +162,17 @@ void uploadsNewFileAndRewritesCommand() throws Exception { .stream() .anyMatch(command -> command.contains("MOVE /Y") && command.contains("\"" + remoteFile + "\"")) ); + + // The age-based purge rides the directory-creation leg (exit code stays MKDIR's) + final String directoryCommand = executor + .getExecutedCommands() + .stream() + .filter(command -> command.contains("MKDIR")) + .findFirst() + .orElseThrow(); + assertTrue(directoryCommand.startsWith("forfiles /P ")); + assertTrue(directoryCommand.contains("del /f /q @path")); + assertTrue(directoryCommand.endsWith("\"")); } @Test diff --git a/src/test/java/org/metricshub/winrm/UtilsTest.java b/src/test/java/org/metricshub/winrm/UtilsTest.java new file mode 100644 index 0000000..a5c02be --- /dev/null +++ b/src/test/java/org/metricshub/winrm/UtilsTest.java @@ -0,0 +1,50 @@ +package org.metricshub.winrm; + +/*- + * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ + * WinRM Java Client + * ჻჻჻჻჻჻ + * Copyright 2023 - 2026 MetricsHub + * ჻჻჻჻჻჻ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ + */ + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +class UtilsTest { + + @Test + void sanitizesComputerNames() { + // Real host names pass through unchanged + assertEquals("MY-PC", Utils.sanitizeComputerName("MY-PC")); + assertEquals("host.domain.example.net", Utils.sanitizeComputerName("host.domain.example.net")); + assertEquals("host_01", Utils.sanitizeComputerName("host_01")); + + // The name may come from an unconstrained environment variable and ends up embedded in + // remote shell commands: cmd metacharacters must never survive + assertEquals("bad---del--q-c--", Utils.sanitizeComputerName("bad & del /q c:\\")); + assertEquals("a-b-c--d-e-f-g", Utils.sanitizeComputerName("a\"b c^%d|e>f Date: Fri, 24 Jul 2026 17:58:01 +0200 Subject: [PATCH 08/11] Bound the complete staging path to the Windows MAX_PATH limit 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 --- CHANGELOG.md | 5 ++- .../org/metricshub/winrm/ShellFileCopy.java | 44 +++++++++++++++++-- .../metricshub/winrm/ShellFileCopyTest.java | 11 +++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42f130d..165a794 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,8 +24,9 @@ Consequences: - The remote copy is **content-addressed**: a fragment of the content digest is inserted before the file extension (e.g. `script.1a2b3c4d5e6f.vbs`), so same-named files with different content from concurrent clients can never overwrite each other. Scripts that inspect their own file - name (e.g. `WScript.ScriptName`) will see the digest fragment. Overlong names are truncated to - stay below the NTFS path-component limit (the digest keeps them unique). + name (e.g. `WScript.ScriptName`) will see the digest fragment. Overlong names are truncated so + that both the NTFS path-component limit and the traditional Windows `MAX_PATH` (260) full-path + limit hold, staging suffixes included (the digest keeps truncated names unique). - The transfer is decoded into an operation-unique staging file, verified there, and only then published as the content-addressed destination. A destination that already carries the expected digest is never rewritten (so concurrent transfers of the same content cannot diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 92c6df7..5ebcbf6 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -76,6 +76,12 @@ private ShellFileCopy() {} /** Maximum extension length preserved when a remote file name must be truncated. */ private static final int MAX_EXTENSION_LENGTH = 30; + /** The traditional Windows MAX_PATH limit (260 including the terminator) still enforced on old hosts. */ + private static final int MAX_WINDOWS_PATH_LENGTH = 259; + + /** Space reserved for the ".<unique>.part" and ".b64" staging suffixes (at most 26 characters). */ + private static final int STAGING_SUFFIX_BUDGET = 30; + /** * Transfer-directory entries not modified for this many days are purged before a transfer: * content-addressing means every revision of a changing file gets a new remote name, and @@ -205,8 +211,10 @@ static String copyFile( // Content-addressed remote name: same-named files with different content get different // remote paths, so concurrent clients — including clients whose computer names collide // in the shared temporary directory — can never overwrite each other's payload between - // the digest verification and the command execution. - final String remoteFile = remoteDirectory + "\\" + contentAddressedName(fileName, content); + // the digest verification and the command execution. The name budget accounts for the + // actual directory prefix, so the COMPLETE staging path stays under MAX_PATH. + final String remoteFile = remoteDirectory + "\\" + + contentAddressedName(fileName, content, maxRemoteNameLength(remoteDirectory)); // Skip the transfer if the remote host already has an identical copy. A destination that // exists with a DIFFERENT digest (e.g. a cached copy corrupted or modified in place) is @@ -664,6 +672,20 @@ private static void bestEffortDelete( * @return the content-addressed remote file name */ static String contentAddressedName(final String fileName, final byte[] content) { + return contentAddressedName(fileName, content, MAX_REMOTE_NAME_LENGTH); + } + + /** + * Same as {@link #contentAddressedName(String, byte[])} with an explicit length bound, + * derived by the caller from the length of the directory the file goes to, so the complete + * path (staging suffixes included) honors the traditional Windows MAX_PATH limit. + * + * @param fileName The local file name + * @param content The file content + * @param maxLength Maximum length of the generated name + * @return the content-addressed remote file name + */ + static String contentAddressedName(final String fileName, final byte[] content, final int maxLength) { final int dot = fileName.lastIndexOf('.'); String base = dot > 0 ? fileName.substring(0, dot) : fileName; String extension = dot > 0 ? fileName.substring(dot) : Utils.EMPTY; @@ -677,7 +699,7 @@ static String contentAddressedName(final String fileName, final byte[] content) extension = extension.substring(0, MAX_EXTENSION_LENGTH); } - final int maxBaseLength = MAX_REMOTE_NAME_LENGTH - digest.length() - 1 - extension.length(); + final int maxBaseLength = Math.max(1, maxLength - digest.length() - 1 - extension.length()); if (base.length() > maxBaseLength) { base = base.substring(0, maxBaseLength); } @@ -685,6 +707,22 @@ static String contentAddressedName(final String fileName, final byte[] content) return base + "." + digest + extension; } + /** + * Maximum length of a content-addressed name in the given remote directory: the component + * bound, further reduced so that the COMPLETE path of the longest transfer artifact + * (destination + "." + unique suffix + ".part" + ".b64") stays within the traditional + * Windows MAX_PATH limit that old hosts still enforce. + * + * @param remoteDirectory The directory receiving the transferred files + * @return the maximum name length + */ + static int maxRemoteNameLength(final String remoteDirectory) { + return Math.min( + MAX_REMOTE_NAME_LENGTH, + MAX_WINDOWS_PATH_LENGTH - remoteDirectory.length() - 1 - STAGING_SUFFIX_BUDGET + ); + } + /** * Compute the hexadecimal digest of the given content. * diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 4f02395..f59ba10 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -423,6 +423,17 @@ void buildsContentAddressedRemoteNames() { assertTrue(longExtension.length() <= 180); assertTrue(longExtension.contains(".ba7816bf8f01.")); + // The explicit bound derived from the directory keeps the COMPLETE staging path (with + // the "..part.b64" suffixes) within the traditional Windows MAX_PATH limit, + // even for the longest allowed (64-character) client computer name + final String longDirectory = "C:\\Windows\\Temp\\SEN_ShareFor_" + "h".repeat(64) + "$"; + final int budget = ShellFileCopy.maxRemoteNameLength(longDirectory); + final String bounded = ShellFileCopy.contentAddressedName("x".repeat(300) + ".vbs", content, budget); + assertTrue( + longDirectory.length() + 1 + bounded.length() + ".0123456789a-bcde.part".length() + ".b64".length() <= 259 + ); + assertTrue(bounded.endsWith(".ba7816bf8f01.vbs")); + // Same name, different content: different remote path (no cross-client overwrite) assertFalse( ShellFileCopy From 87a6379a4beafdd9ddac26597e21a5460242e152 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 18:37:47 +0200 Subject: [PATCH 09/11] Truncate remote names at Unicode code-point boundaries 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 --- .../org/metricshub/winrm/ShellFileCopy.java | 25 ++++++++++++++----- .../metricshub/winrm/ShellFileCopyTest.java | 8 ++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index 5ebcbf6..cf20171 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -695,18 +695,31 @@ static String contentAddressedName(final String fileName, final byte[] content, // Bound the name so that even with the "..part.b64" staging suffixes the remote // path component stays well below the NTFS 255-character limit. Truncating never causes // collisions: the digest fragment keeps the name unique per content. - if (extension.length() > MAX_EXTENSION_LENGTH) { - extension = extension.substring(0, MAX_EXTENSION_LENGTH); - } + extension = truncateAtCodePoint(extension, MAX_EXTENSION_LENGTH); final int maxBaseLength = Math.max(1, maxLength - digest.length() - 1 - extension.length()); - if (base.length() > maxBaseLength) { - base = base.substring(0, maxBaseLength); - } + base = truncateAtCodePoint(base, maxBaseLength); return base + "." + digest + extension; } + /** + * Truncate a string on a Unicode code-point boundary: cutting between the two UTF-16 chars + * of a surrogate pair (e.g. in the middle of an emoji) would leave a malformed character + * that turns into {@code ?} — illegal, and a wildcard — when the command is UTF-8 encoded. + * + * @param value The string to truncate + * @param maxLength Maximum length, in UTF-16 chars + * @return the truncated string + */ + private static String truncateAtCodePoint(final String value, final int maxLength) { + if (value.length() <= maxLength) { + return value; + } + + return value.substring(0, Character.isHighSurrogate(value.charAt(maxLength - 1)) ? maxLength - 1 : maxLength); + } + /** * Maximum length of a content-addressed name in the given remote directory: the component * bound, further reduced so that the COMPLETE path of the longest transfer artifact diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index f59ba10..25a2c82 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -423,6 +423,14 @@ void buildsContentAddressedRemoteNames() { assertTrue(longExtension.length() <= 180); assertTrue(longExtension.contains(".ba7816bf8f01.")); + // Truncation never splits a surrogate pair: a malformed half would become "?" (illegal + // and a wildcard in Windows paths) once the command is UTF-8 encoded + final String emojiExtension = ShellFileCopy.contentAddressedName("f." + "😀".repeat(15), content); + assertEquals(emojiExtension, new String(emojiExtension.getBytes(UTF_8), UTF_8)); + final String emojiBase = ShellFileCopy.contentAddressedName("😀".repeat(300) + ".vbs", content); + assertEquals(emojiBase, new String(emojiBase.getBytes(UTF_8), UTF_8)); + assertTrue(emojiBase.length() <= 180); + // The explicit bound derived from the directory keeps the COMPLETE staging path (with // the "..part.b64" suffixes) within the traditional Windows MAX_PATH limit, // even for the longest allowed (64-character) client computer name From 4fe30e5c6d7b147908ea048b71168a6802fbea03 Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 18:47:30 +0200 Subject: [PATCH 10/11] Reject exclamation marks in transferred file names 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 --- src/main/java/org/metricshub/winrm/ShellFileCopy.java | 3 +++ src/test/java/org/metricshub/winrm/ShellFileCopyTest.java | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index cf20171..cc8c4dd 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -760,6 +760,7 @@ static String digestHex(final String algorithm, final byte[] content) { /** * Reject file names that cannot be transferred: names that cannot be embedded safely in a * quoted cmd.exe argument ({@code %} expands as a variable reference even between quotes, + * {@code !} does too on hosts with delayed expansion enabled, * {@code "} and control characters break the quoting), and names Windows cannot create — * relevant when the client runs on an OS whose local file names may legally contain * Windows-forbidden characters ({@code < > : " / \ | ? *}), end with a dot or a space, or @@ -773,6 +774,8 @@ static void checkTransferableFileName(final String fileName) { || fileName.contains("%") || + fileName.contains("!") + || fileName.chars().anyMatch(c -> c < 0x20 || WINDOWS_FORBIDDEN_CHARACTERS.indexOf(c) >= 0) || fileName.endsWith(".") diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index 25a2c82..d9e6ca5 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -364,6 +364,9 @@ void rejectsFileNamesUnsafeForTheCommandShell() { assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("trailingdot.")); assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("trailing space ")); + // "!" expands like a variable reference on hosts with cmd delayed expansion enabled + assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("script!TEMP!.vbs")); + // Windows reserved device names, with or without an extension, case-insensitive assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("CON")); assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("CON.ps1")); @@ -372,7 +375,7 @@ void rejectsFileNamesUnsafeForTheCommandShell() { assertThrows(IllegalArgumentException.class, () -> ShellFileCopy.checkTransferableFileName("Lpt9")); // Legal Windows file names pass, including cmd metacharacters neutralized by quoting - ShellFileCopy.checkTransferableFileName("My Script (v2) & more!.vbs"); + ShellFileCopy.checkTransferableFileName("My Script (v2) & more.vbs"); ShellFileCopy.checkTransferableFileName("CONSOLE.vbs"); ShellFileCopy.checkTransferableFileName("COM10.txt"); ShellFileCopy.checkTransferableFileName("null.txt"); From d2c613a85fde8fa7439b9df8abf0210f4195d49c Mon Sep 17 00:00:00 2001 From: Bertrand Martin Date: Fri, 24 Jul 2026 18:55:52 +0200 Subject: [PATCH 11/11] Strengthen staging-suffix uniqueness 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 --- .../org/metricshub/winrm/ShellFileCopy.java | 17 +++++++++++++---- .../org/metricshub/winrm/ShellFileCopyTest.java | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/metricshub/winrm/ShellFileCopy.java b/src/main/java/org/metricshub/winrm/ShellFileCopy.java index cc8c4dd..0ef8819 100644 --- a/src/main/java/org/metricshub/winrm/ShellFileCopy.java +++ b/src/main/java/org/metricshub/winrm/ShellFileCopy.java @@ -79,7 +79,7 @@ private ShellFileCopy() {} /** The traditional Windows MAX_PATH limit (260 including the terminator) still enforced on old hosts. */ private static final int MAX_WINDOWS_PATH_LENGTH = 259; - /** Space reserved for the ".<unique>.part" and ".b64" staging suffixes (at most 26 characters). */ + /** Space reserved for the ".<unique>.part" and ".b64" staging suffixes (at most 30 characters). */ private static final int STAGING_SUFFIX_BUDGET = 30; /** @@ -347,10 +347,19 @@ private static void publish( } } - /** Compact operation-unique suffix for staging file names. */ + /** Process-wide counter distinguishing concurrent staging files from the same JVM. */ + private static final java.util.concurrent.atomic.AtomicLong STAGING_COUNTER = new java.util.concurrent.atomic.AtomicLong(); + + /** Random source for the cross-process part of the staging suffix (thread-safe). */ + private static final java.security.SecureRandom STAGING_RANDOM = new java.security.SecureRandom(); + + /** + * Compact operation-unique suffix for staging file names: a process-wide counter makes + * same-JVM collisions impossible, and 64 random bits make cross-process collisions + * negligible — at most 20 characters, fitting the {@link #STAGING_SUFFIX_BUDGET}. + */ private static String uniqueSuffix() { - return (Long.toHexString(Utils.getCurrentTimeMillis()) + "-" - + Integer.toHexString((int) (Math.random() * 0x10000))); + return String.format("%x-%016x", STAGING_COUNTER.incrementAndGet() & 0xFFF, STAGING_RANDOM.nextLong()); } /** diff --git a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java index d9e6ca5..515a29b 100644 --- a/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java +++ b/src/test/java/org/metricshub/winrm/ShellFileCopyTest.java @@ -441,7 +441,7 @@ void buildsContentAddressedRemoteNames() { final int budget = ShellFileCopy.maxRemoteNameLength(longDirectory); final String bounded = ShellFileCopy.contentAddressedName("x".repeat(300) + ".vbs", content, budget); assertTrue( - longDirectory.length() + 1 + bounded.length() + ".0123456789a-bcde.part".length() + ".b64".length() <= 259 + longDirectory.length() + 1 + bounded.length() + ".fff-0123456789abcdef.part".length() + ".b64".length() <= 259 ); assertTrue(bounded.endsWith(".ba7816bf8f01.vbs"));