Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Jan 17, 2026

User description

🔗 Related Issues

Fixes #16927

💥 What does this PR do?

When uploading a file remotely, the uploaded file is packed to ZIP, and unpacked on the server side.
This PR fixes ZIP algorithm, so that it preserves the files' "lastModified" value when zipping/unzipping.

🔧 Implementation Notes

I touched only Java (tested myself) and JS (don't know how to test it).
Other bindings didn't have this problem (according to #16927).

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Preserve file modification time when zipping/unzipping in Java and JavaScript

  • Add FileTime handling to ZipEntry during compression and decompression

  • Update tests to verify modification time preservation across zip operations


Diagram Walkthrough

flowchart LR
  A["Original File<br/>with lastModified"] -->|zip with FileTime| B["ZipEntry<br/>with lastModifiedTime"]
  B -->|unzip and restore| C["Extracted File<br/>with original lastModified"]
  D["Test Verification"] -->|assert lastModified| E["Matches Original Value"]
Loading

File Walkthrough

Relevant files
Bug fix
Zip.java
Add FileTime preservation to zip/unzip operations               

java/src/org/openqa/selenium/io/Zip.java

  • Import FileTime from java.nio.file.attribute package
  • Set lastModifiedTime on ZipEntry when adding files to preserve
    original modification time
  • Restore file modification time from ZipEntry when extracting files
    during unzip operation
+3/-0     
zip.js
Preserve file modification time in JavaScript zip               

javascript/selenium-webdriver/io/zip.js

  • Import fs/promises module for file stat operations
  • Modify addFile method to retrieve file stats and preserve mtime in zip
    options
  • Pass date parameter to jszip's file() method to maintain original
    modification time
+6/-4     
Tests
ZipTest.java
Add modification time preservation tests                                 

java/test/org/openqa/selenium/io/ZipTest.java

  • Import FileTime and stream utilities for test assertions
  • Add explicit modification time (100_000L) to test file in
    testCanZipASingleFile
  • Verify unzipped file preserves original modification time with
    assertion
  • Refactor writeTestZip to accept lastModifiedStep parameter for varied
    timestamps
  • Update writeTestZipEntry to set lastModifiedTime on ZipEntry with
    calculated values
  • Enhance testCanUnzip to verify multiple files preserve different
    modification times
+15/-6   

@selenium-ci selenium-ci added C-java Java Bindings C-nodejs JavaScript Bindings labels Jan 17, 2026
@asolntsev asolntsev added this to the 4.40.0 milestone Jan 17, 2026
@asolntsev asolntsev self-assigned this Jan 17, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 17, 2026

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
🟡
🎫 #16927
🟢 Java binding: when creating the ZIP payload for remote file upload, preserve the original
file's last modified time by setting it on each ZipEntry.
Java Grid/server side: when unzipping uploaded files, restore the extracted file's last
modified time from the ZipEntry metadata.
JavaScript binding: when creating the ZIP payload for remote file upload, preserve the
original file's last modified time in the ZIP entry (e.g., via JSZip date option).
Add/adjust automated tests to verify modification time is preserved across zip/unzip
operations.
Ensure remote upload behavior preserves File.lastModified when used against real remote
providers (e.g., SauceLabs) or Selenium Grid.
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 Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null timestamp risk: The unzip path assumes entry.getLastModifiedTime() is always non-null and calls
.toMillis() without a null/edge-case fallback, which can fail for zip entries without a
stored modification time.

Referred Code
  unzipFile(outputDir, zis, entry.getName());
  file.setLastModified(entry.getLastModifiedTime().toMillis());
}

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:
Zip path safety: The new unzip behavior sets timestamps on extracted files (file.setLastModified(...)) but
the diff does not show whether entry.getName() is validated against path traversal (e.g.,
../), so a human should confirm extraction remains constrained to outputDir.

Referred Code
  File file = new File(outputDir, entry.getName());
  if (entry.isDirectory()) {
    FileHandler.createDir(file);
    continue;
  }

  unzipFile(outputDir, zis, entry.getName());
  file.setLastModified(entry.getLastModifiedTime().toMillis());
}

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

@asolntsev asolntsev added B-grid Everything grid and server related Review effort [1-5]: 1 and removed Review effort 3/5 labels Jan 17, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 17, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent null pointer on missing timestamp

Add a null check for entry.getLastModifiedTime() before calling .toMillis() to
prevent a potential NullPointerException.

java/src/org/openqa/selenium/io/Zip.java [111-112]

 unzipFile(outputDir, zis, entry.getName());
-file.setLastModified(entry.getLastModifiedTime().toMillis());
+FileTime lastModifiedTime = entry.getLastModifiedTime();
+if (lastModifiedTime != null) {
+  file.setLastModified(lastModifiedTime.toMillis());
+}
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential NullPointerException on a new line of code if a zip entry lacks a modification time. Adding the proposed null check prevents a crash and makes the new functionality more robust.

Medium
Learned
best practice
Ensure streams are always closed

Wrap the ZipOutputStream in try-with-resources so it's always closed even if
writeTestZipEntry or assertions fail.

java/test/org/openqa/selenium/io/ZipTest.java [108-113]

-ZipOutputStream out = new ZipOutputStream(new FileOutputStream(file));
-for (int i = 0; i < files; i++) {
-  writeTestZipEntry(out, i * lastModifiedStep);
+try (ZipOutputStream out = new ZipOutputStream(new FileOutputStream(file))) {
+  for (int i = 0; i < files; i++) {
+    writeTestZipEntry(out, i * lastModifiedStep);
+  }
 }
-out.close();
 file.deleteOnExit();
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - In tests that create external resources, use try/finally (or try-with-resources) to ensure cleanup even on failures.

Low
General
Set legacy DOS timestamp

Call entry.setTime(toAdd.lastModified()) to set the legacy DOS timestamp for
improved compatibility with older zip tools.

java/src/org/openqa/selenium/io/Zip.java [64-66]

 ZipEntry entry = new ZipEntry(name.replace('\\', '/'));
 entry.setLastModifiedTime(FileTime.fromMillis(toAdd.lastModified()));
+entry.setTime(toAdd.lastModified());
 zos.putNextEntry(entry);
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion improves backward compatibility by setting the legacy DOS timestamp in addition to the modern extended timestamp. This is a useful enhancement for interoperability with older zip tools.

Low
  • Update

@VietND96
Copy link
Member

@asolntsev I think the review of Prevent null pointer on missing timestamp is valid to make CI-RBE can pass, then the PR can merge

@asolntsev asolntsev force-pushed the fix/preserve-uploaded-file-modification-time branch from 02b17c5 to b12d49d Compare January 17, 2026 19:14
@asolntsev asolntsev force-pushed the fix/preserve-uploaded-file-modification-time branch from b12d49d to a41f728 Compare January 17, 2026 20:19
@asolntsev
Copy link
Contributor Author

@asolntsev I think the review of Prevent null pointer on missing timestamp is valid to make CI-RBE can pass, then the PR can merge

I don't think it can be null. As much as I tried, it was never null - if not set, Zip treats entry modification time as "now".
But anyway, I've added the null check.

P.S. Merging this PR because only DefaultMouseTest which is flaky, and will be fixed in PR #16906

@asolntsev asolntsev merged commit bb84614 into SeleniumHQ:trunk Jan 17, 2026
12 of 13 checks passed
@asolntsev asolntsev deleted the fix/preserve-uploaded-file-modification-time branch January 17, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related C-java Java Bindings C-nodejs JavaScript Bindings Review effort [1-5]: 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: File upload to remote browser may fail to preserve last modified time

3 participants