[java] Allow downloading files from old Grid server

[asolntsev]

User description

The problem

Users with

• Selenium client 4.39 or newer
• Selenium Grid 4.38 or older

cannot download files from Grid.

🔧 Implementation Notes

Endpoint "/se/files/:name" was added in Selenium 4.39.0. If user has upgraded only Selenium client version (but not the server!) then method remoteDriver.downloadFile() fails with "unsupported command" exception.

Now the download method will work again, falling back to the old endpoint POST /se/files (which is slow for large files).

🔗 Related Issues

See issue #16612 and PR #16627.

💡 Additional Considerations

After some time, we can remove this fallback. Probably starting from Selenium 5.0.0

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Add fallback to old Grid endpoint for file downloads

• Support clients with newer Selenium but older Grid versions

• Gracefully handle UnsupportedCommandException with warning log

• Maintain backwards compatibility for file download functionality

---

Diagram Walkthrough

flowchart LR
  A["downloadFile request"] --> B{"Try new endpoint<br/>/se/files/:name"}
  B -->|Success| C["Stream file content"]
  B -->|UnsupportedCommandException| D["Log warning about<br/>old Grid version"]
  D --> E["Fallback to old endpoint<br/>POST /se/files"]
  E --> F["Unzip file contents"]
  C --> G["Save to target location"]
  F --> G

Loading

File Walkthrough

Relevant files

Bug fix

RemoteWebDriver.java Add fallback for legacy Grid file download endpoint            java/src/org/openqa/selenium/remote/RemoteWebDriver.java Added try-catch block to handle UnsupportedCommandException when new endpoint fails Implemented fallback to legacy POST /se/files endpoint for older Grid servers Added warning log message when old Grid version is detected Imported UnsupportedCommandException and Zip utility classes Refactored file download logic to support both new and old Grid endpoints +20/-6

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Path traversal Description: The new fallback path writes files using user-controlled fileName via targetLocation.resolve(fileName) and also unzips server-provided contents with Zip.unzip(...), which can enable path traversal/Zip-Slip (e.g., fileName like ../evil or zip entries containing ../ or absolute paths) to write outside targetLocation if inputs are not strictly sanitized by upstream components. RemoteWebDriver.java [732-753] Referred Code public void downloadFile(String fileName, Path targetLocation) throws IOException { requireDownloadsEnabled(capabilities); try { Response response = execute(DriverCommand.GET_DOWNLOADED_FILE, Map.of("name", fileName)); Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { Files.createDirectories(targetLocation); Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } } catch (UnsupportedCommandException e) { String error = requireNonNull(e.getMessage(), e.toString()).split("\n", 2)[0]; LOG.log( Level.WARNING, "You have too old Selenium Grid version, please upgrade it. Caused by: {0}", error); Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); String contents = ((Map<String, String>) response.getValue()).get("contents"); Zip.unzip(contents, targetLocation.toFile()); ... (clipped 1 lines)
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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Fallback edge cases: The fallback path does not ensure targetLocation exists and does not validate the presence/format of contents, which can lead to runtime failures (e.g., missing directory or null/invalid zip payload). Referred Code Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); String contents = ((Map<String, String>) response.getValue()).get("contents"); Zip.unzip(contents, targetLocation.toFile()); } 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: fileName is used directly in targetLocation.resolve(fileName) without validation, allowing path traversal or writing outside the intended directory if fileName contains separators/... Referred Code Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } 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: Exception message leakage: The warning log includes the first line of UnsupportedCommandException (e.getMessage()), which may contain internal server details and should be reviewed to ensure no sensitive information is exposed. Referred Code String error = requireNonNull(e.getMessage(), e.toString()).split("\n", 2)[0]; LOG.log( Level.WARNING, "You have too old Selenium Grid version, please upgrade it. Caused by: {0}", error); 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: Unstructured warning log: The added LOG.log(Level.WARNING, ...) statement is unstructured and includes raw exception text, so confirm it aligns with project logging standards and cannot include sensitive data. Referred Code LOG.log( Level.WARNING, "You have too old Selenium Grid version, please upgrade it. Caused by: {0}", error); Learn more about managing compliance generic rules or creating your own custom rules

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
Possible issue	Add validation for fallback response Add validation to the fallback logic to check the response value's type and ensure the 'contents' key exists before use, preventing potential runtime exceptions. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [750-752] Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); -String contents = ((Map<String, String>) response.getValue()).get("contents"); +Object responseValue = response.getValue(); +if (!(responseValue instanceof Map)) { + throw new WebDriverException("Unexpected response value: " + responseValue); +} +String contents = ((Map<String, String>) responseValue).get("contents"); +if (contents == null) { + throw new WebDriverException("Response from old Grid did not contain 'contents' key: " + responseValue); +} Zip.unzip(contents, targetLocation.toFile()); Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out the lack of validation for the response payload in the fallback logic, and the proposed change improves robustness by preventing potential ClassCastException or NullPointerException.	Medium
General	Ensure target directory is created Move the Files.createDirectories(targetLocation) call to before the try-catch block to ensure the target directory is created for both the new and legacy download logic. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [735-753] +Files.createDirectories(targetLocation); try { Response response = execute(DriverCommand.GET_DOWNLOADED_FILE, Map.of("name", fileName)); Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { - Files.createDirectories(targetLocation); Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); } } catch (UnsupportedCommandException e) { String error = requireNonNull(e.getMessage(), e.toString()).split("\n", 2)[0]; LOG.log( Level.WARNING, "You have too old Selenium Grid version, please upgrade it. Caused by: {0}", error); Response response = execute(DriverCommand.DOWNLOAD_FILE, Map.of("name", fileName)); String contents = ((Map<String, String>) response.getValue()).get("contents"); Zip.unzip(contents, targetLocation.toFile()); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: This suggestion correctly identifies that Files.createDirectories is only called in the try block and proposes moving it out to serve both code paths, which improves code structure and robustness by making directory creation explicit.	Low
	Log full exception stack trace Modify the logging call to pass the entire exception object e instead of just the error message, enabling the full stack trace to be logged. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [745-748] LOG.log( Level.WARNING, - "You have too old Selenium Grid version, please upgrade it. Caused by: {0}", - error); + "You have too old Selenium Grid version, please upgrade it.", + e); Apply / Chat Suggestion importance[1-10]: 5 __ Why: This is a good suggestion for improving debuggability by logging the full stack trace of the exception, which provides more context than just the message.	Low
More

[titusfortner]

Wait, why did we add a new endpoint for this? It makes a lot more sense for file name to be in the payload instead of in the endpoint.

[asolntsev]

Wait, why did we add a new endpoint for this? It makes a lot more sense for file name to be in the payload instead of in the endpoint.

Why do you prefer to put file name in the payload instead of URL? I don't understand.

But the new payload was needed not because of file name.
It was needed because we need to use different response format. Instead of putting the file content into ZIP (which is very slow and memory-consuming in case of large files), we want to print out the raw file content right to the response. This is much faster.

P.S. Just in case, it was already done in Selenium 4.39.0.

[titusfortner]

because the name can have spaces? Plus introducing new endpoints affects all the languages, not just Java. And this isn't the convention used everywhere else?

[titusfortner]

At least for this, it is an implementation detail. I think it would make more sense to provide a toggle in existing endpoint if we need different behavior? But I didn't really read through everything you did in the PR. Just introducing a new endpoint should be a big deal, though. 😂

[asolntsev]

because the name can have spaces?
Usually it's not a problem. File name just should be encoded. E.g. space replaced by '+' etc.

introducing new endpoints affects all the languages, not just Java.

No matter if it was new endpoint or new parameter/header/whatever else, it doesn't immediately affect other languages.
Other languages can continue using the old endpoint, everything will work as previously.
At any moment, anyone can update other bindings to use the new endpoint/parameter/header.

And this isn't the convention used everywhere else?

What convention do you mean? @titusfortner

it would make more sense to provide a toggle in existing endpoint if we need different behaviour?

It's doable. We can replace new "new" endpoint with a parameter/header in the old endpoint (e.g. http header Accept: binary/octet-stream).

[asolntsev]

Anyway, we can merge this PR and then discuss how to improve the mechanism for downloading files.