16612 download large files

[asolntsev]

User description

🔗 Related Issues

Fixes #16612

💥 What does this PR do?

This PR allows downloading very large files (I tried up to 4GB) from Selenium Grid, while consuming very low memory.

🔄 Types of changes

• Performance improvement (backwards compatible)

---

PR Type

Enhancement, Performance

---

Description

• Add new Grid endpoint /se/files/:name for direct file downloads without Base64 encoding

• Optimize RemoteWebDriver.downloadFile() to stream files directly instead of loading into memory

• Extract anonymous Contents.Supplier implementations into separate named classes for better debugging

• Improve HTTP response handling to support binary streams and large file transfers

• Fix flaky tests by adding proper wait conditions and increased timeouts

---

Diagram Walkthrough

flowchart LR
  A["Grid Node"] -->|"GET /se/files/:name"| B["New Direct Download Endpoint"]
  B -->|"Stream file"| C["FileContentSupplier"]
  C -->|"InputStream"| D["RemoteWebDriver"]
  D -->|"Files.copy"| E["Target Location"]
  F["Old Method"] -->|"Base64 + JSON"| G["Memory Issues"]
  style B fill:#90EE90
  style C fill:#90EE90
  style E fill:#87CEEB

Loading

File Walkthrough

	Relevant files
Enhancement	20 files Node.java Add new GET endpoint for direct file downloads                      +3/-0      LocalNode.java Implement direct file streaming and URL parsing logic        +77/-16  RemoteWebDriver.java Optimize file download to stream directly to disk                +10/-4    Contents.java Add file and stream content suppliers, deprecate memory-intensive methods +31/-27  FileContentSupplier.java New supplier for streaming file content without loading to memory +80/-0    InputStreamContentSupplier.java New supplier for binary stream content with length tracking +67/-0    BytesContentSupplier.java Extract bytes supplier to separate class for clarity          +65/-0    FileBackedOutputStreamContentSupplier.java New supplier for file-backed output stream content              +74/-0    JdkHttpClient.java Switch to InputStream response handler for large file support +5/-2      JdkHttpMessages.java Handle binary streams separately from JSON responses          +36/-8    W3CHttpResponseCodec.java Add binary stream detection and avoid loading large files to memory +15/-6    AbstractHttpCommandCodec.java Add new GET_DOWNLOADED_FILE command mapping                            +2/-0      DriverCommand.java Add GET_DOWNLOADED_FILE command constant                                  +1/-0      NodeStatus.java Add toString method to prevent debug log spam                        +6/-0      Session.java Add toString method to prevent debug log spam                        +5/-0      Urls.java Add URL decoding utility method                                                    +8/-2      RequestConverter.java Use atomic long for thread-safe length tracking                    +6/-31    HttpMessage.java Add toString and contentAsString methods                                  +9/-0      HttpRequest.java Improve toString to include content information                    +5/-1      HttpResponse.java Improve toString to avoid loading large content                    +4/-2
Tests	5 files NodeTest.java Add wait conditions for async directory deletion                  +7/-3      LocalNewSessionQueueTest.java Increase timeout to fix flaky test                                              LocalNodeTest.java Add wait conditions and test for filename extraction          +55/-30  HttpClientTestBase.java Handle HttpTimeoutException in interrupt detection              +3/-1      ReverseProxyEndToEndTest.java Remove debug logging statement                                                      +0/-2
Configuration changes	3 files BUILD.bazel Add jspecify dependency                                                                    +1/-0      BUILD.bazel Add guava dependency for media type handling                          +2/-0      bazel.yml Add test logs upload on failure                                                    +8/-0
Additional files	2 files ResponseConverter.java +4/-6      LocalNewSessionQueueTest.java +1/-1

---

[qodo-merge-pro]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Path parsing ambiguity Description: The URL-decoding in extractFileName replaces spaces with '+' (urlDecode(...).replace(' ', '+')), which can be abused to bypass exact filename matching and access unexpected files whose real names contain spaces and plus signs, potentially enabling ambiguous or unintended file access. LocalNode.java [771-783] Referred Code private String extractFileName(HttpRequest req) { return extractFileName(req.getUri()); } 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); }
	Insecure direct object access Description: The file download endpoint streams arbitrary files from the per-session downloads directory based solely on client-supplied names without enforcing content-type restrictions or download safeguards, allowing retrieval of any file present in that directory (including potentially sensitive artifacts) if an attacker can write to it via the browser. LocalNode.java [839-851] Referred Code 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); BasicFileAttributes attributes = readAttributes(file.toPath(), BasicFileAttributes.class); return new HttpResponse() .setHeader("Content-Type", MediaType.OCTET_STREAM.toString()) .setHeader("Content-Length", String.valueOf(attributes.size())) .setHeader("Last-Modified", lastModifiedHeader(attributes.lastModifiedTime())) .setContent(Contents.file(file)); }
	Memory exhaustion risk Description: contentAsString reads the entire stream into memory if length ≤ 256MB, which could still be abused to trigger high memory usage by returning large "text" responses near the threshold; this is risky for untrusted endpoints and contradicts the streaming intent. InputStreamContentSupplier.java [61-69] Referred Code public String contentAsString(Charset charset) { if (length > MAX_TEXT_RESPONSE_SIZE) { throw new UnsupportedOperationException("Cannot print out too large stream content"); } try { return new String(stream.readAllBytes(), UTF_8); } catch (IOException e) { throw new RuntimeException(e); }
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: 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: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Info Exposure: Error when file not found exposes absolute downloads directory path and full file list in the message, which may leak internal system details to the client. Referred Code throw new WebDriverException( String.format( "Cannot find file [%s] in directory %s. Found %s files: %s.", filename, downloadsDirectory.getAbsolutePath(), files.size(), files)); } if (matchingFiles.size() != 1) { throw new WebDriverException( String.format( Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Action Logging: New download endpoints and file operations (listing, streaming, deleting) do not include explicit audit logging of user/session, action, target file, and outcome in the added code hunks. Referred Code + id + " — ensure downloads are enabled in the options class when requesting a session."; throw new WebDriverException(msg); } File downloadsDirectory = Optional.ofNullable(tempFS.getBaseDir().listFiles()).orElse(new File[] {})[0]; try { if (req.getMethod().equals(HttpMethod.GET) && req.getUri().endsWith("/se/files")) { return listDownloadedFiles(downloadsDirectory); } if (req.getMethod().equals(HttpMethod.GET)) { return getDownloadedFile(downloadsDirectory, extractFileName(req)); } if (req.getMethod().equals(HttpMethod.DELETE)) { return deleteDownloadedFile(downloadsDirectory); } return getDownloadedFile(req, downloadsDirectory); } catch (IOException e) { throw new UncheckedIOException(e); } ... (clipped 117 lines) 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: Edge Cases: Streaming download path sets Content-Length from attributes but may not handle missing length or concurrent file changes, and GET without file name relies on URL parsing that throws IllegalArgumentException without standardized error response mapping. Referred Code throws IOException { if (fileName.isEmpty()) { throw new WebDriverException("Please specify file to download in URL"); } File file = findDownloadedFile(downloadsDirectory, fileName); BasicFileAttributes attributes = readAttributes(file.toPath(), BasicFileAttributes.class); return new HttpResponse() .setHeader("Content-Type", MediaType.OCTET_STREAM.toString()) .setHeader("Content-Length", String.valueOf(attributes.size())) .setHeader("Last-Modified", lastModifiedHeader(attributes.lastModifiedTime())) .setContent(Contents.file(file)); } private String lastModifiedHeader(FileTime fileTime) { return HTTP_DATE_FORMAT.format(fileTime.toInstant().atZone(UTC)); } 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 Handling: Filename extraction and matching rely on equals against names found in the downloads directory, but there is no explicit normalization/validation beyond urlDecode to prevent traversal or special-name attacks, requiring verification of directory confinement. Referred Code 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); } /** User wants to list files that can be downloaded */ private HttpResponse listDownloadedFiles(File downloadsDirectory) { File[] files = Optional.ofNullable(downloadsDirectory.listFiles()).orElse(new File[] {}); List<String> fileNames = Arrays.stream(files).map(File::getName).collect(Collectors.toList()); List<DownloadedFile> fileInfos = Arrays.stream(files).map(this::getFileInfo).collect(Collectors.toList()); Map<String, Object> data = Map.of( "names", fileNames, "files", fileInfos); Map<String, Map<String, Object>> result = Map.of("value", data); ... (clipped 88 lines) 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-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect filename extraction logic Remove the incorrect .replace(' ', '+') from the extractFileName method to correctly handle filenames containing spaces. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [775-782] 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); + return urlDecode(uri.substring(index + prefix.length())); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a bug in the new extractFileName method where spaces in filenames are incorrectly converted to plus signs, which would cause file downloads to fail for such files.	High
	Allow multiple stream reads from file Update FileContentSupplier.get() to return a new InputStream on each call to adhere to the Contents.Supplier contract, removing the single-use restriction. java/src/org/openqa/selenium/remote/http/FileContentSupplier.java [38-50] @Override public synchronized InputStream get() { - if (inputStream != null) { - throw new IllegalStateException("File input stream has been opened before"); - } try { - inputStream = Files.newInputStream(file.toPath()); + return Files.newInputStream(file.toPath()); } catch (IOException e) { throw new IllegalStateException("File not readable: " + file.getAbsolutePath(), e); } - - return inputStream; } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies that the get() method in the new FileContentSupplier class violates the Contents.Supplier contract by not allowing multiple invocations, which could cause issues with features like HTTP retries.	Medium
	Overwrite existing files on download Modify the downloadFile method to use StandardCopyOption.REPLACE_EXISTING with Files.copy to allow overwriting existing files. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [729-739] @Override public void downloadFile(String fileName, Path targetLocation) throws IOException { requireDownloadsEnabled(capabilities); Response response = execute(DriverCommand.GET_DOWNLOADED_FILE, Map.of("name", fileName)); Contents.Supplier content = (Contents.Supplier) response.getValue(); try (InputStream fileContent = content.get()) { - Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); + Files.copy( + new BufferedInputStream(fileContent), + targetLocation.resolve(fileName), + StandardCopyOption.REPLACE_EXISTING); } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that Files.copy will fail if the target file exists and proposes adding StandardCopyOption.REPLACE_EXISTING to fix it, which is a valid and useful improvement.	Medium
Learned best practice	Validate filename to prevent traversal Validate the extracted name against path traversal and reject names containing separators or parent segments; avoid mutating decoded names unexpectedly. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [775-851] 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); + String decoded = urlDecode(uri.substring(index + prefix.length())); + // reject path traversal or nested paths + if (decoded.contains("/") || decoded.contains("\\") || decoded.contains("..")) { + throw new IllegalArgumentException("Invalid file name"); + } + return decoded; } private HttpResponse getDownloadedFile(File downloadsDirectory, String fileName) throws IOException { - if (fileName.isEmpty()) { + if (fileName == null || fileName.isBlank()) { throw new WebDriverException("Please specify file to download in URL"); } + // rest unchanged File file = findDownloadedFile(downloadsDirectory, fileName); BasicFileAttributes attributes = readAttributes(file.toPath(), BasicFileAttributes.class); return new HttpResponse() .setHeader("Content-Type", MediaType.OCTET_STREAM.toString()) .setHeader("Content-Length", String.valueOf(attributes.size())) .setHeader("Last-Modified", lastModifiedHeader(attributes.lastModifiedTime())) .setContent(Contents.file(file)); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard external I/O and user-supplied inputs with validation to avoid crashes or security issues; validate and sanitize path-derived file names and method-specific routes.	Low
Update

[diemol]

Please ping when this is ready for review.

[asolntsev]

Please ping when this is ready for review.

Sure. I think that the PR is already finished, but there are some tests failing.
Now I am trying to understand if these tests are just flaky, or broken by my changes...

[qodo-merge-pro]

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: 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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Download Access Logging: New direct file download endpoints and code paths do not include explicit audit logging of who downloaded which file and when, which may be required for audit trails. Referred Code + id + " — ensure downloads are enabled in the options class when requesting a session."; throw new WebDriverException(msg); } File downloadsDirectory = Optional.ofNullable(tempFS.getBaseDir().listFiles()).orElse(new File[] {})[0]; try { if (req.getMethod().equals(HttpMethod.GET) && req.getUri().endsWith("/se/files")) { return listDownloadedFiles(downloadsDirectory); } if (req.getMethod().equals(HttpMethod.GET)) { return getDownloadedFile(downloadsDirectory, extractFileName(req)); } if (req.getMethod().equals(HttpMethod.DELETE)) { return deleteDownloadedFile(downloadsDirectory); } return getDownloadedFile(req, downloadsDirectory); } catch (IOException e) { throw new UncheckedIOException(e); } ... (clipped 84 lines) 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 Error Details: Error messages for missing or duplicate files include full directory paths and file listings which could expose internal details if surfaced to end users. Referred Code () -> new File[0])); if (matchingFiles.isEmpty()) { List<File> 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)); } if (matchingFiles.size() != 1) { throw new WebDriverException( String.format( "Expected there to be only 1 file. Found %s files: %s.", matchingFiles.size(), matchingFiles)); } return matchingFiles.get(0); 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 new file download by name uses URL-decoded fileName without explicit sanitization against path traversal, relying on exact name match which may be safe but warrants verification. Referred Code private String extractFileName(HttpRequest req) { return extractFileName(req.getUri()); } 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); } /** User wants to list files that can be downloaded */ private HttpResponse listDownloadedFiles(File downloadsDirectory) { File[] files = Optional.ofNullable(downloadsDirectory.listFiles()).orElse(new File[] {}); List<String> fileNames = Arrays.stream(files).map(File::getName).collect(Collectors.toList()); List<DownloadedFile> fileInfos = Arrays.stream(files).map(this::getFileInfo).collect(Collectors.toList()); Map<String, Object> data = ... (clipped 92 lines) 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-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect space encoding in filenames Remove the incorrect .replace(' ', '+') from extractFileName as urlDecode already handles space conversion. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [775-782] 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 urlDecode(uri.substring(index + prefix.length())).replace(' ', '+'); + return urlDecode(uri.substring(index + prefix.length())); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug in extractFileName where spaces in filenames are incorrectly converted to +, which would cause file lookup to fail for names containing spaces.	Medium
	Correctly handle file and directory paths In downloadFile, check if targetLocation is a directory before resolving the filename to correctly handle both file and directory paths as the destination. java/src/org/openqa/selenium/remote/RemoteWebDriver.java [733-738] Response response = execute(DriverCommand.GET_DOWNLOADED_FILE, Map.of("name", fileName)); Contents.Supplier content = (Contents.Supplier) response.getValue(); +Path destination = + Files.isDirectory(targetLocation) ? targetLocation.resolve(fileName) : targetLocation; try (InputStream fileContent = content.get()) { - Files.copy(new BufferedInputStream(fileContent), targetLocation.resolve(fileName)); + Files.copy(new BufferedInputStream(fileContent), destination); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the PR's implementation of downloadFile does not adhere to its documented contract, as it fails to handle cases where targetLocation is a file path.	Medium
Learned best practice	Close stream in try-with-resources Use try-with-resources to ensure the InputStream from the buffer is closed after copying, preventing potential resource leaks on exceptions. java/src/org/openqa/selenium/netty/server/FileBackedOutputStreamContentSupplier.java [64-73] @Override public String contentAsString(Charset charset) { - ByteArrayOutputStream out = new ByteArrayOutputStream(); - try { - buffer.asByteSource().copyTo(out); + try (InputStream in = buffer.asByteSource().openBufferedStream(); + ByteArrayOutputStream out = new ByteArrayOutputStream()) { + in.transferTo(out); + return out.toString(charset); } catch (IOException e) { - throw new RuntimeException(e); + throw new UncheckedIOException(e); } - return out.toString(charset); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Ensure resource cleanup by closing or disposing I/O resources in a finally/try-with-resources path, even on exceptions.	Low
More

[asolntsev]

Please ping when this is ready for review.

@diemol Ping. This PR is ready for review, as well as few others.