[java] rework downloading files from Grid

[asolntsev]

User description

• remove the "new" endpoint GET /se/files/:name (that was recently added in 4.39.0)
• use the "old" endpoint POST /se/files, but with optional "format" parameter in payload. Possible values: "application/octet-stream" or "application/json" (default).

Endpoint POST /se/files can respond either in JSON or binary format depending on the "format" parameter.

• Currently only Java binding sends "format=application/octet-stream"
• Other bindings don't yet send "format" parameter, thus downloading file using the old method (which encodes file content into Base64 and Json).
  • Other bindings can gradually migrate to "format=application/octet-stream" as well in following releases.

As a side effect, this PR fixes downloading files with space in name. :)

🔄 Types of changes

• Bug fix (backwards compatible)
• Breaking change (fix or feature that would cause existing functionality to change)

---

PR Type

Bug fix, Enhancement

---

Description

• Remove deprecated GET /se/files/:name endpoint, use POST /se/files with Accept header

• Support binary file downloads via Accept: application/octet-stream header

• Fix downloading files with special characters (spaces, unicode, etc.)

• Add backward compatibility for older Grid versions (4.38.0 and earlier)

• Expand test coverage with parameterized tests for various filename types

---

Diagram Walkthrough

flowchart LR
  Client["Client<br/>RemoteWebDriver"]
  OldEndpoint["Old Endpoint<br/>GET /se/files/:name"]
  NewEndpoint["New Endpoint<br/>POST /se/files<br/>+ Accept Header"]
  BinaryResponse["Binary Response<br/>application/octet-stream"]
  JsonResponse["JSON Response<br/>for older Grid"]
  
  Client -->|Remove| OldEndpoint
  Client -->|Use| NewEndpoint
  NewEndpoint -->|Accept: octet-stream| BinaryResponse
  NewEndpoint -->|Fallback| JsonResponse

Loading

File Walkthrough

	Relevant files
Bug fix	2 files LocalNode.java Remove GET endpoint, add binary response support                  +7/-13    RemoteWebDriver.java Implement backward compatibility for file downloads            +7/-13
Refactoring	1 files DriverCommand.java Remove deprecated GET_DOWNLOADED_FILE command                        +0/-1
Enhancement	2 files AbstractHttpCommandCodec.java Add HTTP header support to command specifications                +20/-5    HttpHeader.java Add Accept header enum constant                                                    +1/-0
Tests	5 files RemoteWebDriverDownloadTest.java Add parameterized tests for special character filenames    +40/-9    download.html Add download links for special character test files            +15/-0    file-with-scandinavian-ø.txt Add test file with scandinavian characters                              +1/-0      file-with-cyrillic-серцеєдність.txt Add test file with cyrillic characters                                      +1/-0      file-with-space 0 & _ ` ~.txt Add test file with spaces and special characters

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Path traversal write Description: Potential path traversal on the client machine: downloadFile writes to targetLocation.resolve(fileName) using fileName originating from the remote end/browser download metadata, so a crafted name containing path separators (e.g., ../.ssh/authorized_keys or absolute paths) could cause writes outside targetLocation unless the name is normalized/validated. RemoteWebDriver.java [733-747] Referred Code Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); if (response.getValue() instanceof Contents.Supplier) { // Selenium Grid 4.39.0 or newer Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { Files.createDirectories(targetLocation); Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } } else if (response.getValue() instanceof Map) { // Selenium Grid 4.38.0 or older String contents = ((Map<String, String>) response.getValue()).get("contents"); Zip.unzip(contents, targetLocation.toFile()); } else { throw new UnsupportedOperationException("Unexpected grid response: " + response); }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Path traversal risk: The code writes downloaded content to targetLocation.resolve(fileName) without sanitizing fileName, enabling path traversal (e.g., ../) to escape the target directory if a malicious/compromised Grid returns an unexpected filename. Referred Code public void downloadFile(String fileName, Path targetLocation) throws IOException { requireDownloadsEnabled(capabilities); Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); if (response.getValue() instanceof Contents.Supplier) { // Selenium Grid 4.39.0 or newer Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { Files.createDirectories(targetLocation); Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } } else if (response.getValue() instanceof Map) { Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing input validation: The new downloadFile flow does not validate fileName (e.g., null/empty/invalid characters) before using it for file operations, which may lead to unclear failures depending on upstream inputs. Referred Code public void downloadFile(String fileName, Path targetLocation) throws IOException { requireDownloadsEnabled(capabilities); Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); if (response.getValue() instanceof Contents.Supplier) { // Selenium Grid 4.39.0 or newer Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { Files.createDirectories(targetLocation); Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } } else if (response.getValue() instanceof Map) { // Selenium Grid 4.38.0 or older String contents = ((Map<String, String>) response.getValue()).get("contents"); Zip.unzip(contents, targetLocation.toFile()); } else { throw new UnsupportedOperationException("Unexpected grid response: " + response); } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Verbose exception detail: Throwing new UnsupportedOperationException("Unexpected grid response: " + response) may expose internal response details (potentially including headers/body) to callers instead of keeping details to internal logs. Referred Code } else { throw new UnsupportedOperationException("Unexpected grid response: " + response); } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ Create a defensive copy of map Suggestion Impact: Instead of defensively copying httpHeaders, the commit removed the httpHeaders field entirely (along with its constructor parameter and usage when building requests), thereby eliminating the mutability/immutability concern the suggestion raised. code diff: @@ -363,36 +356,28 @@ private final HttpMethod method; private final String path; private final List<String> pathSegments; - private final Map<String, String> httpHeaders; private CommandSpec(HttpMethod method, String path) { - this(method, path, emptyMap()); - } - - private CommandSpec(HttpMethod method, String path, Map<String, String> httpHeaders) { this.method = Require.nonNull("HTTP method", method); this.path = path; this.pathSegments = Arrays.stream(path.split("/")) .filter(e -> !e.isEmpty()) .collect(Collectors.toUnmodifiableList()); - this.httpHeaders = httpHeaders; } @Override public boolean equals(@Nullable Object o) { if (o instanceof CommandSpec) { CommandSpec that = (CommandSpec) o; - return this.method.equals(that.method) - && this.path.equals(that.path) - && this.httpHeaders.equals(that.httpHeaders); + return this.method.equals(that.method) && this.path.equals(that.path); } return false; } @Override public int hashCode() { - return Objects.hash(method, path, httpHeaders); + return Objects.hash(method, path); } Make the CommandSpec class more robust by creating a defensive, unmodifiable copy of the httpHeaders map in its constructor to ensure immutability. java/src/org/openqa/selenium/remote/codec/AbstractHttpCommandCodec.java [372-380] private CommandSpec(HttpMethod method, String path, Map<String, String> httpHeaders) { this.method = Require.nonNull("HTTP method", method); this.path = path; this.pathSegments = Arrays.stream(path.split("/")) .filter(e -> !e.isEmpty()) .collect(Collectors.toUnmodifiableList()); - this.httpHeaders = httpHeaders; + this.httpHeaders = Map.copyOf(httpHeaders); } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential mutability issue and proposes using Map.copyOf to create an immutable defensive copy, which improves the robustness and immutability of CommandSpec.	Low
Learned best practice	Ensure temp resources are cleaned up Wrap the temp directory usage in a try/finally and delete it in the finally block to avoid leaking files on failures. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [154-160] Path targetLocation = Files.createTempDirectory("download"); -((HasDownloads) driver).downloadFile(file.getName(), targetLocation); +try { + ((HasDownloads) driver).downloadFile(file.getName(), targetLocation); -File localFile = targetLocation.resolve(expectedFileName).toFile(); -assertThat(localFile).hasName(expectedFileName); -assertThat(localFile).hasSize(file.getSize()); -assertThat(localFile).content().isEqualToIgnoringNewLines(expectedFileContent); + File localFile = targetLocation.resolve(expectedFileName).toFile(); + assertThat(localFile).hasName(expectedFileName); + assertThat(localFile).hasSize(file.getSize()); + assertThat(localFile).content().isEqualToIgnoringNewLines(expectedFileContent); +} finally { + try (var paths = Files.walk(targetLocation)) { + paths.sorted((a, b) -> b.compareTo(a)).forEach(p -> p.toFile().delete()); + } +} Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - In tests that create external resources (e.g., temporary directories), always use try/finally to ensure cleanup even when assertions fail.	Low
	✅ Validate inputs and parse headers robustly Suggestion Impact: The commit changed the binary response selection logic away from using the HTTP Accept header to instead use a payload field ("format") with a default, and still uses an exact octet-stream equality check on that value. It also added an explicit empty-filename guard in a new backward-compatibility GET path. However, it did not implement trimming/filtering of the "name" value nor the suggested tolerant Accept-header parsing (contains check). code diff: @@ -831,8 +840,11 @@ "Please specify file to download in payload as {\"name\":" + " \"fileToDownload\"}")); File file = findDownloadedFile(downloadsDirectory, filename); - - if (MediaType.OCTET_STREAM.toString().equalsIgnoreCase(req.getHeader(HttpHeader.Accept))) { + String contentType = + requireNonNullElseGet( + (String) incoming.get("format"), () -> MediaType.JSON_UTF_8.toString()); + + if (MediaType.OCTET_STREAM.toString().equalsIgnoreCase(contentType)) { return fileAsBinaryResponse(file); } @@ -844,6 +856,17 @@ "contents", content); Map<String, Map<String, Object>> result = Map.of("value", data); return new HttpResponse().setContent(asJson(result)); + } + + /** Left here for backward compatibility. Remove this IF in Selenium 4.41, 4.42 or 4.43 */ + @Deprecated + private HttpResponse getDownloadedFile(File downloadsDirectory, String fileName) + throws IOException { + if (fileName.isEmpty()) { + throw new WebDriverException("Please specify file to download in URL"); + } + File file = findDownloadedFile(downloadsDirectory, fileName); + return fileAsBinaryResponse(file); } Trim and reject blank name values, and make the Accept check tolerant of compound header values (e.g., multiple media types) instead of exact string equality. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [825-837] String filename = Optional.ofNullable(incoming.get("name")) .map(Object::toString) + .map(String::trim) + .filter(name -> !name.isEmpty()) .orElseThrow( () -> new WebDriverException( "Please specify file to download in payload as {\"name\":" + " \"fileToDownload\"}")); File file = findDownloadedFile(downloadsDirectory, filename); -if (MediaType.OCTET_STREAM.toString().equalsIgnoreCase(req.getHeader(HttpHeader.Accept))) { +String accept = Optional.ofNullable(req.getHeader(HttpHeader.Accept)).orElse(""); +if (accept.toLowerCase(US).contains(MediaType.OCTET_STREAM.toString())) { return fileAsBinaryResponse(file); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Add explicit validation and guards at integration boundaries by trimming inputs and validating presence/format before use.	Low
Update

[titusfortner]

I'm confused by what you are doing changing this around. We want the same pattern for all the bindings:

GET with GET_DOWNLOADED_FILES returns a list of file names that can be downloaded
POST with DOWNLOAD_FILE allows you to send a payload requesting the name of the files to download.

My suggestion elsewhere was to add an optional parameter to the payload for DOWNLOAD_FILE endpoint specifying whether it should come back with base64 or octet-stream and bindings can update to use octet stream handling as desired. That keeps everything backwards compatible and making it easy (one parameter) to update.

The pattern for "download all" was to iterate over the list from GET_DOWNLOADED_FIELS.

If we want to allow DOWNLOAD_FILE to accept a list, that could work as well, but needs to be backwards compatible.

[asolntsev]

@titusfortner Yes, this PR does exactly what you described:

1. We now have only one endpoint for downloading files (POST /se/files).
2. Its body contains two parameters:

• (required) "name" - name of the file to download
• (optional) "format" - either "application/octet-stream" or "application/json" (default)

3. Java binding sends "format: application/octet-stream" which causes downloading a raw file (which is fast)
4. Other bindings (and older java clients) don't send "format" parameter which causes downloading in zip/json format (which is slow for large files, but works).

[asolntsev]

I don't understand why the build is failing. I see in logs only this:

ERROR: D:/a/selenium/selenium/java/test/org/openqa/selenium/firefox/BUILD.bazel:53:25: Testing //java/test/org/openqa/selenium/firefox:FirefoxDriverBuilderTest failed: build-runfiles.exe failed: error executing <shell command> command 
  cd /d D:\b\execroot\_main
  SET CI=true
    SET EXPERIMENTAL_SPLIT_XML_GENERATION=1
    SET GITHUB_ACTIONS=true
    SET JAVA_RUNFILES=bazel-out/x64_windows-fastbuild/bin/java/test/org/openqa/selenium/firefox/FirefoxDriverBuilderTest.exe.runfiles
    SET JRUBY_OPTS=--dev
    SET LOCALAPPDATA=C:\Users\runneradmin\AppData\Local
    SET PATH= ... 
    ...
    SET XML_OUTPUT_FILE=bazel-out/x64_windows-fastbuild/testlogs/java/test/org/openqa/selenium/firefox/FirefoxDriverBuilderTest/test.xml
  C:\users\runneradmin\_bazel_runneradmin\install\0e888f1e2ba674a2c24f59f5119d0b41\build-runfiles.exe --allow_relative bazel-out/x64_windows-fastbuild/bin/java/test/org/openqa/selenium/firefox/FirefoxDriverBuilderTest.exe.runfiles_manifest bazel-out/x64_windows-fastbuild/bin/java/test/org/openqa/selenium/firefox/FirefoxDriverBuilderTest.exe.runfiles: Process exited with status 1: Process exited with status 1

[2,649 / 2,649] 1 / 6 tests; no actions running
INFO: Elapsed time: 761.371s, Critical Path: 136.37s
INFO: 2407 processes: 919 internal, 1276 local, 212 worker.
ERROR: Build did NOT complete successfully

No reasons why the build failed... :(
Just "Process exited with status 1" :(

UPD Seems the build failure was caused by two test files: "file-with-cyrillic-серцеєдність.txt" and "file-with-scandinavian-ø.txt". Looks like Windows build scripts contain some error which don't handle cyrillic/scandinavian characters correctly. Needs to be investigated.

By now, I've deleted these two files.