[java] fix downloading files from Grid (when file name contains space)

[asolntsev]

User description

💥 What does this PR do?

Properly encode file name in URL

🔧 Implementation Notes

1. When adding value to URL, it has to be properly encoded.
   It's not enough to just call URLEncoder.encode(part), we also need to replace + characters by %20.

2. When extracting file name from URL, we don't need to call urlDecode because at that moment the name is already decoded (?).

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Fix file download from Grid when filename contains spaces or special characters

• Properly encode filenames in URLs using %20 instead of + for spaces

• Remove unnecessary URL decoding in filename extraction logic

• Add comprehensive test coverage for various filename formats

---

Diagram Walkthrough

flowchart LR
  A["File Download Request"] --> B["URL Encode Filename"]
  B --> C["Replace + with %20"]
  C --> D["Store in Grid URL"]
  D --> E["Extract Filename from URL"]
  E --> F["Return Decoded Filename"]
  F --> G["Match Against Local Files"]
  G --> H["Download File Successfully"]

Loading

File Walkthrough

	Relevant files
Bug fix	LocalNode.java Simplify filename extraction and improve error messages    java/src/org/openqa/selenium/grid/node/local/LocalNode.java Remove unused urlDecode import Simplify extractFileName() to return substring without decoding Change downloadedFiles() return type from List to List Map file objects to their names using File::getName Add Stream import for stream operations +5/-5      Urls.java Fix URL encoding to use %20 for spaces                                      java/src/org/openqa/selenium/net/Urls.java Modify urlEncode() to replace + characters with %20 Ensures proper URL encoding for filenames with spaces +1/-1
Tests	RemoteWebDriverDownloadTest.java Add parameterized tests for special character filenames    java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java Convert canDownloadFiles() from single test to parameterized test Add test cases for filenames with special characters (ø, Cyrillic, spaces) Expand canListDownloadedFiles() to verify 5 files with various formats Add downloadableFiles() method source for parameterized test data Import ParameterizedTest and MethodSource annotations +38/-9    download.html Add test files with special characters                                      common/src/web/downloads/download.html Add three new download links for testing special characters File 3: Norwegian character (ø) File 4: Cyrillic characters (Russian text) File 5: Space and special characters (&, _, ', ") File 5 href uses URL-encoded format +15/-0    file_ø.txt Add test file with Norwegian character                                      common/src/web/downloads/file_ø.txt New test file with Norwegian character in filename Contains test content for verification +1/-0      file-с-русским-названием.txt Add test file with Cyrillic characters                                      common/src/web/downloads/file-с-русским-названием.txt New test file with Cyrillic characters in filename Contains test content for verification +1/-0      file 0 & _ ` `.txt Add test file with spaces and special characters                  common/src/web/downloads/file 0 & _ .txt New test file with spaces and special characters in filename Contains test content for verification

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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: 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: Passed 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: Detailed error message: The newly constructed WebDriverException message includes the absolute downloads directory path and the list of filenames, which may be exposed to remote clients and leak local filesystem details depending on how the exception is surfaced. Referred Code List<String> files = downloadedFiles(downloadsDirectory); throw new WebDriverException( String.format( "Cannot find file [%s] in directory %s. Found %s files: %s.", filename, downloadsDirectory.getAbsolutePath(), files.size(), files)); } 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
Possible issue	Decode filename from request URI In the extractFileName method, URL-decode the extracted filename from the URI. This ensures that encoded characters are correctly handled, allowing files with special characters in their names to be found on the filesystem. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [781-788] String extractFileName(String uri) { String prefix = "/se/files/"; int index = uri.lastIndexOf(prefix); if (index < 0) { throw new IllegalArgumentException("Unexpected URL for downloading a file: " + uri); } - return uri.substring(index + prefix.length()); + return urlDecode(uri.substring(index + prefix.length())); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a regression in the extractFileName method where URL decoding was removed, which would break the file download functionality for filenames with special characters—the very feature this PR aims to improve.	High
	Correct expected filename in test Update the expected filename for file-5 in the downloadableFiles test data provider. The current expected name is missing a double quote that is present in the actual filename. java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java [163-169] static Stream<Arguments> downloadableFiles() { return Stream.of( Arguments.of(By.id("file-1"), "file_1.txt", "Hello, World!"), Arguments.of(By.id("file-3"), "file_ø.txt", "Hello, file with \"ø\" in name!"), Arguments.of(By.id("file-4"), "file-с-русским-названием.txt", "Hello, Cyrillic World!"), - Arguments.of(By.id("file-5"), "file 0 & _ ' _.txt", "Hello, filename with space!")); + Arguments.of(By.id("file-5"), "file 0 & _ ' \".txt", "Hello, filename with space!")); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies and fixes a bug in a new test case where the expected filename for file-5 is missing a double quote character, which would cause the test to fail.	Medium
High-level	Consider a more targeted encoding fix Instead of globally changing Urls.urlEncode to replace + with %20, create a new, specific encoding method for URL path segments. This isolates the fix for file download URLs and avoids potential side effects in other parts of the application. Examples: java/src/org/openqa/selenium/net/Urls.java [46] return URLEncoder.encode(value, UTF_8).replace("+", "%20"); Solution Walkthrough: Before: // In java/src/org/openqa/selenium/net/Urls.java public class Urls { /** * Encodes the text as an URL using UTF-8. */ public static String urlEncode(String value) { return URLEncoder.encode(value, UTF_8).replace("+", "%20"); } // ... other methods } After: // In java/src/org/openqa/selenium/net/Urls.java public class Urls { /** * Encodes the text as an URL using UTF-8 for query parameters. */ public static String urlEncode(String value) { return URLEncoder.encode(value, UTF_8); } /** * Encodes the text for a URL path segment. */ public static String urlPathSegmentEncode(String value) { return URLEncoder.encode(value, UTF_8).replace("+", "%20"); } // ... other methods } // At the call site for creating download URLs (hypothetical): // String downloadUrl = "/se/files/" + Urls.urlPathSegmentEncode(fileName); Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a significant risk of regression by modifying a global utility method (Urls.urlEncode) and proposes a safer, more localized solution, which is a best practice.	Medium
Learned best practice	Clean up temporary directories Ensure the temporary directory (and any downloaded file) is cleaned up in a finally block so test failures don’t leak files/directories. 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 { + Files.walk(targetLocation) + .sorted((a, b) -> b.getNameCount() - a.getNameCount()) + .forEach( + p -> { + try { + Files.deleteIfExists(p); + } catch (IOException e) { + // best-effort cleanup + } + }); +} Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Pattern 2: When creating external resources (e.g., temporary directories), wrap usage in try/finally and always perform cleanup in finally so failures don’t leak resources.	Low
Update

[asolntsev]

@titusfortner I recall why I wanted to pass file name in URL.
Because JSON format allows putting multiple filenames, but this endpoint can only print out contents of one file.

[asolntsev]

This PR was overseeded by #16844