feat/CUS-9990-Added class to get latest downloaded zip, extract files and store filepaths inside zip #298
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…epaths inside zip
📝 WalkthroughWalkthroughIntroduces a new Maven module Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/ExtractLatestDownloadedZipFilePaths.java:
- Around line 69-74: The code in ExtractLatestDownloadedZipFilePaths uses
entry.getName() directly to build File(fullPath, entry.getName()) and create
parent directories, which allows Zip Slip path traversal; fix by resolving
canonical paths: compute the canonical path of the extraction base directory and
the canonical path of the constructed target file (after joining zipBaseDir with
entry.getName()), reject any entry whose canonical target does not start with
the canonical base directory (or contains ".." / is absolute), and skip or throw
on such invalid entries before calling parent.mkdirs() or writing the file;
update the logic around File fullPath = new File(zipBaseDir, entry.getName()) /
parent.mkdirs() to perform this validation first.
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/UnzipCSVGZFile.java:
- Around line 61-75: The unzip method (unzip) currently opens FileInputStream,
GZIPInputStream and FileOutputStream manually and closes them in finally-like
code, which leaks resources if an exception happens; refactor the method to use
try-with-resources to auto-close FileInputStream, GZIPInputStream and
FileOutputStream, change the read loop to check for -1 (while ((len =
gis.read(buffer)) != -1) ) and remove manual close() calls so streams are always
closed even on exceptions.
In @unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfile.java:
- Around line 60-96: The unzip method has resource leaks, a Zip Slip
vulnerability, silent failures, and unnecessary Thread.sleep calls; fix by
removing all Thread.sleep invocations, use try-with-resources for
FileInputStream/ZipInputStream and each FileOutputStream so streams are always
closed, validate extracted paths inside unzip by resolving
newFile.getCanonicalPath() and ensuring it startsWith destDir.getCanonicalPath()
(reject or throw if not), and stop swallowing IOExceptions—either propagate the
IOException from unzip or wrap and throw a runtime exception so callers know
extraction failed instead of returning partial/null results; refer to the unzip
method, variables zipFilePath, destDir, fileName, fis, zis, fos and ZipEntry
usage when applying these changes.
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfilelatest.java:
- Line 46: getLastModified(...) can return null when the target directory is
empty or missing; currently the returned value is assigned to zipFilePath and
passed into unzip(...) without a null check causing a NullPointerException. Add
a null check after the assignment to zipFilePath (the variable from
getLastModified) and handle the empty/missing-directory case by logging an error
or returning/throwing early instead of calling unzip; apply the same guard
around uses of getLastModified results in the other block noted (the code around
the 102-122 range) so unzip(...) and any downstream usage never receives a null
File reference.
- Around line 63-99: The unzip method in unzipfilelatest.java has resource
leaks, a Zip Slip vulnerability, unnecessary Thread.sleep calls, and swallows
IOExceptions; fix it by replacing manual stream handling with try-with-resources
around FileInputStream and ZipInputStream (and FileOutputStream), validate each
ZipEntry name to prevent path traversal (reject or normalize entries that
resolve outside destDir) and create directories for entries before writing,
remove all Thread.sleep(1000) calls, and propagate or throw IOExceptions instead
of only printing stack traces; also consider moving this logic into a shared
utility class (e.g., ZipUtils.extract or similar) to reuse the secure
implementation across unzip and unzipfilelatest.
In @unzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunzip.java:
- Line 56: The success message is being set unconditionally; move the
setSuccessMessage("File was unzipped and uploaded successfully on the specific
element location") call into the try block inside the Uploadunzip class method
so it only executes after the unzip-and-upload operations complete without
throwing, and ensure the catch block logs/sets an appropriate failure message
instead (refer to the existing try/catch and setSuccessMessage usage to locate
where to relocate the call).
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunziplatest.java:
- Line 41: getLastModified may return null (no files found), so avoid passing a
null File into unzip; in Uploadunziplatest where you assign File zipFilePath =
getLastModified(testData1.getValue().toString()), add a null-check for
zipFilePath and handle it (log an error via the addon logger or processLogger,
throw a clear exception, or return/skip further processing) before calling
unzip(zipFilePath); ensure the handling provides context (include the directory
value from testData1.getValue()) so callers know why operation was aborted.
- Line 56: The success message call setSuccessMessage("File was unzipped and
uploaded successfully on the specific element location") is currently executed
even when exceptions occur; move this call into the try block inside the
Uploadunziplatest class (where the unzip-and-upload logic runs) so it only
executes after all operations complete successfully, and ensure the catch block
sets an appropriate failure message (e.g., via setFailureMessage) instead of
leaving success set on error.
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/util/ChromeDownloadUtilities.java:
- Around line 65-76: The getFilePathByFileNameInDownloads method currently
concatenates desiredFileName into the JavaScript string, allowing JS injection;
instead pass the filename as a safe argument to the JavascriptExecutor and
reference it via arguments[0] (or use JSON.stringify on the arg in JS) so the
value is not interpreted as code; update the script variable to use arguments[0]
and call js.executeScript(script, desiredFileName), keep the existing exception
handling and ensure you null-check the returned Object before calling toString
to avoid NPEs.
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/util/EdgeDownloadUtilities.java:
- Around line 93-97: The JavaScript executed in EdgeDownloadUtilities (via
((JavascriptExecutor) driver).executeScript(edgeJavaScript)) returns an array of
primitive values, but the result is incorrectly cast to List<WebElement>; change
the cast to List<Object> (or List<String> if you normalize to strings) for the
variable progressValues, update subsequent null/size checks to use that type,
and handle element value conversion accordingly so the code correctly inspects
the returned progress values instead of WebElement instances.
In @unzipfileandupload/src/main/resources/testsigma-sdk.properties:
- Line 1: The properties file contains a hardcoded secret in the
testsigma-sdk.api.key property; remove the literal JWT from the file, load the
API key at runtime from a secure source (e.g., environment variable or a secret
manager) and update the code that reads testsigma-sdk.api.key to fall back to
that runtime secret, add the local properties file to .gitignore (or use a
template like testsigma-sdk.properties.template without secrets) so secrets are
not committed, and ensure the exposed key is rotated immediately.
🧹 Nitpick comments (12)
unzipfileandupload/src/main/java/com/testsigma/addons/web/util/DownloadUtilitiesFactory.java (1)
9-18: Consider defensive casting and Firefox support.The cast to
RemoteWebDriveron line 10 may throwClassCastExceptionif the driver is not aRemoteWebDriverinstance. Additionally, Firefox is a common browser that's not currently supported.Suggested improvement
public static DownloadUtilities create(WebDriver driver, Logger logger) { + if (!(driver instanceof RemoteWebDriver)) { + throw new RuntimeException("Driver must be an instance of RemoteWebDriver"); + } String browserName = ((RemoteWebDriver) driver).getCapabilities().getBrowserName().toLowerCase(); if (browserName.contains("chrome")) { return new ChromeDownloadUtilities(driver, logger); } else if (browserName.contains("edge")) { return new EdgeDownloadUtilities(driver, logger); + } else if (browserName.contains("firefox")) { + // TODO: Add FirefoxDownloadUtilities implementation + throw new RuntimeException("Firefox support not yet implemented"); } else { throw new RuntimeException("Unsupported browser: " + browserName); } }unzipfileandupload/src/main/java/com/testsigma/addons/web/ExtractLatestDownloadedZipFilePaths.java (1)
108-117: Exception handling structure is reasonable but may mask unexpected errors.Catching
RuntimeExceptionseparately fromExceptioncan inadvertently mask unexpected runtime errors (e.g.,NullPointerException). Consider logging the stack trace forRuntimeExceptionas well, or restructuring to catch only specific expected exceptions.unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfile.java (2)
24-24: Class name should follow Java naming conventions.The class name
unzipfileshould beUnzipFile(PascalCase) per Java naming conventions.
50-50: Remove unnecessary Thread.sleep in execute method.The 1-second sleep on line 50 appears unnecessary and slows down execution without apparent benefit.
unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfilelatest.java (2)
7-7: Remove unused imports.
ElementandWebElementare imported but never used in this class.Also applies to: 14-14
27-27: Class name should follow Java naming conventions.The class name
unzipfilelatestshould beUnzipFileLatest(PascalCase) per Java naming conventions.unzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunziplatest.java (2)
47-49: Remove unnecessary Thread.sleep() calls or document their purpose.These sleep calls add 2 seconds of unnecessary delay. If they're meant to wait for UI updates, consider using explicit waits instead.
♻️ Proposed refactor
String filePath = destDir + File.separator + fileNameupload; -Thread.sleep(1000); webElement.sendKeys(filePath); -Thread.sleep(1000);
98-118: Consider filtering for ZIP files only and documenting null return.The method name suggests it gets the latest modified file, but doesn't filter by file type. Consider adding a filter for
.zipfiles to prevent selecting non-ZIP files.♻️ Proposed refactor
public static File getLastModified(String directoryFilePath) { File directory = new File(directoryFilePath); - File[] files = directory.listFiles(File::isFile); + File[] files = directory.listFiles(f -> f.isFile() && f.getName().toLowerCase().endsWith(".zip")); long lastModifiedTime = Long.MIN_VALUE; File chosenFile = null; if (files != null) { for (File file : files) { if (file.lastModified() > lastModifiedTime) { chosenFile = file; lastModifiedTime = file.lastModified(); } } } return chosenFile; }unzipfileandupload/src/main/java/com/testsigma/addons/web/util/EdgeDownloadUtilities.java (1)
34-40: Consider reducing the 60-second wait timeout.A 60-second wait for download completion may be excessive for small files and could slow down test execution. Consider making this configurable or reducing it to a more reasonable default (e.g., 30 seconds).
unzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunzip.java (1)
47-49: Remove unnecessary Thread.sleep() calls.These sleep calls add 2 seconds of delay without clear justification. Consider removing them or using explicit waits if synchronization is needed.
unzipfileandupload/src/main/java/com/testsigma/addons/web/util/ChromeDownloadUtilities.java (1)
39-50: Consider using explicit wait instead of immediate check.The code immediately checks if the file is downloaded without waiting. If the download starts after navigation, this could fail. Consider adding a small explicit wait before checking.
unzipfileandupload/src/main/java/com/testsigma/addons/web/UnzipCSVGZFile.java (1)
45-46: Potential filename collision with timestamp-based naming.While unlikely, using
System.currentTimeMillis()for unique filenames could result in collisions if multiple actions execute within the same millisecond. Consider usingUUID.randomUUID()for guaranteed uniqueness.♻️ Proposed refactor
-String fileNameUpload = "output" + System.currentTimeMillis() + ".csv"; +String fileNameUpload = "output_" + UUID.randomUUID() + ".csv";Add import:
import java.util.UUID;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
unzipfileandupload/pom.xmlunzipfileandupload/src/main/java/com/testsigma/addons/web/ExtractLatestDownloadedZipFilePaths.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/UnzipCSVGZFile.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunzip.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunziplatest.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfile.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfilelatest.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/util/ChromeDownloadUtilities.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/util/DownloadUtilities.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/util/DownloadUtilitiesFactory.javaunzipfileandupload/src/main/java/com/testsigma/addons/web/util/EdgeDownloadUtilities.javaunzipfileandupload/src/main/resources/testsigma-sdk.properties
🔇 Additional comments (6)
unzipfileandupload/src/main/java/com/testsigma/addons/web/util/DownloadUtilities.java (1)
5-10: Interface design looks good.The interface provides a clear contract for browser-specific download utilities. Consider adding Javadoc to clarify the expected behavior of each method, particularly
copyFileFromDownloadsregarding whatfileFormatandfileNameparameters represent when null.unzipfileandupload/pom.xml (1)
36-46: Use stable versions for test dependencies instead of pre-release and outdated versions.
junit-jupiter-apiversion5.8.0-M1is a milestone (pre-release) version. Use a stable release like5.10.5or newer.testngversion6.14.3is significantly outdated. Current stable versions are in the 7.10+ series (latest is 7.11.0).- Clarify whether both JUnit Jupiter and TestNG are actively used in the test suite, as maintaining both test frameworks increases complexity.
unzipfileandupload/src/main/java/com/testsigma/addons/web/util/EdgeDownloadUtilities.java (2)
52-56: Good interrupt handling pattern.The code correctly restores the interrupt flag when
InterruptedExceptionis caught, which is a best practice for interrupt handling.
245-285: Creative approach using FileReader API for file copying.The implementation cleverly uses JavaScript's FileReader API to read the downloaded file content and create a local copy. This is a sound approach for handling files in remote browser sessions.
unzipfileandupload/src/main/java/com/testsigma/addons/web/util/ChromeDownloadUtilities.java (1)
78-118: Solid implementation of file copying from downloads.The use of FileReader API to create a local copy of the downloaded file is well-implemented with proper error handling.
unzipfileandupload/src/main/java/com/testsigma/addons/web/UnzipCSVGZFile.java (1)
38-60: Good use of runtime variables for storing extracted file path.The implementation correctly stores the extracted file path in a runtime variable, making it available for subsequent test steps. This is a good pattern for test data management.
Comment on lines
+69
to
+74
| File fullPath = new File(zipBaseDir, entry.getName()); | ||
|
|
||
| File parent = fullPath.getParentFile(); | ||
| if (!parent.exists()) { | ||
| parent.mkdirs(); | ||
| } |
There was a problem hiding this comment.
Zip Slip vulnerability: Validate entry paths before extraction.
Malicious ZIP files can contain entries with paths like ../../../etc/passwd. The code directly uses entry.getName() to construct the output path without validating that it stays within the intended extraction directory.
Proposed fix to prevent path traversal
+ // Prevent Zip Slip vulnerability
+ String canonicalDestPath = zipBaseDir.getCanonicalPath();
File fullPath = new File(zipBaseDir, entry.getName());
+ if (!fullPath.getCanonicalPath().startsWith(canonicalDestPath + File.separator)) {
+ throw new SecurityException("ZIP entry is outside of the target dir: " + entry.getName());
+ }
File parent = fullPath.getParentFile();
if (!parent.exists()) {
parent.mkdirs();
}🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/ExtractLatestDownloadedZipFilePaths.java
around lines 69 - 74, The code in ExtractLatestDownloadedZipFilePaths uses
entry.getName() directly to build File(fullPath, entry.getName()) and create
parent directories, which allows Zip Slip path traversal; fix by resolving
canonical paths: compute the canonical path of the extraction base directory and
the canonical path of the constructed target file (after joining zipBaseDir with
entry.getName()), reject any entry whose canonical target does not start with
the canonical base directory (or contains ".." / is absolute), and skip or throw
on such invalid entries before calling parent.mkdirs() or writing the file;
update the logic around File fullPath = new File(zipBaseDir, entry.getName()) /
parent.mkdirs() to perform this validation first.
Comment on lines
+61
to
+75
| private void unzip(String zipFilePath, String destFilepath) throws IOException { | ||
| logger.info("Extracting the file"); | ||
| FileInputStream fis = new FileInputStream(zipFilePath); | ||
| GZIPInputStream gis = new GZIPInputStream(fis); | ||
| FileOutputStream fos = new FileOutputStream(destFilepath); | ||
| byte[] buffer = new byte[1024]; | ||
| int len; | ||
| while ((len = gis.read(buffer)) > 0) { | ||
| fos.write(buffer, 0, len); | ||
| } | ||
| fos.close(); | ||
| gis.close(); | ||
| fis.close(); | ||
| logger.info("Successfully extracted to location: " + destFilepath); | ||
| } |
There was a problem hiding this comment.
Resource leak in unzip method.
The streams are not closed with try-with-resources. If an exception occurs during decompression, the streams may remain open, leading to resource leaks.
🛡️ Proposed fix
private void unzip(String zipFilePath, String destFilepath) throws IOException {
logger.info("Extracting the file");
- FileInputStream fis = new FileInputStream(zipFilePath);
- GZIPInputStream gis = new GZIPInputStream(fis);
- FileOutputStream fos = new FileOutputStream(destFilepath);
- byte[] buffer = new byte[1024];
- int len;
- while ((len = gis.read(buffer)) > 0) {
- fos.write(buffer, 0, len);
+ try (FileInputStream fis = new FileInputStream(zipFilePath);
+ GZIPInputStream gis = new GZIPInputStream(fis);
+ FileOutputStream fos = new FileOutputStream(destFilepath)) {
+ byte[] buffer = new byte[1024];
+ int len;
+ while ((len = gis.read(buffer)) > 0) {
+ fos.write(buffer, 0, len);
+ }
}
- fos.close();
- gis.close();
- fis.close();
logger.info("Successfully extracted to location: " + destFilepath);
}🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/UnzipCSVGZFile.java
around lines 61 - 75, The unzip method (unzip) currently opens FileInputStream,
GZIPInputStream and FileOutputStream manually and closes them in finally-like
code, which leaks resources if an exception happens; refactor the method to use
try-with-resources to auto-close FileInputStream, GZIPInputStream and
FileOutputStream, change the read loop to check for -1 (while ((len =
gis.read(buffer)) != -1) ) and remove manual close() calls so streams are always
closed even on exceptions.
Comment on lines
+60
to
+96
| private static String unzip(String zipFilePath, String destDir) throws InterruptedException { | ||
| String fileName = null; | ||
| Thread.sleep(1000); | ||
| File dir = new File(destDir); | ||
| // create output directory if it doesn't exist | ||
| if (!dir.exists()) dir.mkdirs(); | ||
| FileInputStream fis; | ||
| // buffer for read and write data to file | ||
| byte[] buffer = new byte[1024]; | ||
| try { | ||
| fis = new FileInputStream(zipFilePath); | ||
| ZipInputStream zis = new ZipInputStream(fis); | ||
| ZipEntry ze = zis.getNextEntry(); | ||
| while (ze != null) { | ||
| fileName = ze.getName(); | ||
| File newFile = new File(destDir + File.separator + fileName); | ||
| // create directories for sub directories in zip | ||
| new File(newFile.getParent()).mkdirs(); | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| Thread.sleep(1000); | ||
| int len; | ||
| while ((len = zis.read(buffer)) > 0) { | ||
| fos.write(buffer, 0, len); | ||
| } | ||
| fos.close(); | ||
| zis.closeEntry(); | ||
| ze = zis.getNextEntry(); | ||
| Thread.sleep(1000); | ||
| } | ||
| zis.closeEntry(); | ||
| zis.close(); | ||
| fis.close(); | ||
| Thread.sleep(1000); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return fileName; |
There was a problem hiding this comment.
Critical: Resource leaks and missing Zip Slip protection in unzip method.
Multiple issues in this method:
- Resource leaks:
FileInputStream,ZipInputStream, andFileOutputStreamare not in try-with-resources blocks. If an exception occurs, streams won't be closed. - Zip Slip vulnerability: No validation that extracted paths stay within
destDir. - Silent failure:
IOExceptionis caught but only prints stack trace—the method still returns potentially partial/null data. - Excessive Thread.sleep: Multiple 1-second sleeps (lines 62, 79, 87, 92) add 3+ seconds per file, significantly slowing extraction without clear benefit.
Proposed fix with proper resource management and security
-private static String unzip(String zipFilePath, String destDir) throws InterruptedException {
+private static String unzip(String zipFilePath, String destDir) throws IOException {
String fileName = null;
- Thread.sleep(1000);
File dir = new File(destDir);
- // create output directory if it doesn't exist
if (!dir.exists()) dir.mkdirs();
- FileInputStream fis;
- // buffer for read and write data to file
+ String canonicalDestPath = dir.getCanonicalPath();
byte[] buffer = new byte[1024];
- try {
- fis = new FileInputStream(zipFilePath);
- ZipInputStream zis = new ZipInputStream(fis);
+
+ try (FileInputStream fis = new FileInputStream(zipFilePath);
+ ZipInputStream zis = new ZipInputStream(fis)) {
ZipEntry ze = zis.getNextEntry();
while (ze != null) {
fileName = ze.getName();
File newFile = new File(destDir + File.separator + fileName);
- // create directories for sub directories in zip
+ // Prevent Zip Slip
+ if (!newFile.getCanonicalPath().startsWith(canonicalDestPath + File.separator)) {
+ throw new SecurityException("ZIP entry outside target dir: " + fileName);
+ }
new File(newFile.getParent()).mkdirs();
- FileOutputStream fos = new FileOutputStream(newFile);
- Thread.sleep(1000);
- int len;
- while ((len = zis.read(buffer)) > 0) {
- fos.write(buffer, 0, len);
+ try (FileOutputStream fos = new FileOutputStream(newFile)) {
+ int len;
+ while ((len = zis.read(buffer)) > 0) {
+ fos.write(buffer, 0, len);
+ }
}
- fos.close();
zis.closeEntry();
ze = zis.getNextEntry();
- Thread.sleep(1000);
}
- zis.closeEntry();
- zis.close();
- fis.close();
- Thread.sleep(1000);
- } catch (IOException e) {
- e.printStackTrace();
}
return fileName;
}🤖 Prompt for AI Agents
In @unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfile.java
around lines 60 - 96, The unzip method has resource leaks, a Zip Slip
vulnerability, silent failures, and unnecessary Thread.sleep calls; fix by
removing all Thread.sleep invocations, use try-with-resources for
FileInputStream/ZipInputStream and each FileOutputStream so streams are always
closed, validate extracted paths inside unzip by resolving
newFile.getCanonicalPath() and ensuring it startsWith destDir.getCanonicalPath()
(reject or throw if not), and stop swallowing IOExceptions—either propagate the
IOException from unzip or wrap and throw a runtime exception so callers know
extraction failed instead of returning partial/null results; refer to the unzip
method, variables zipFilePath, destDir, fileName, fis, zis, fos and ZipEntry
usage when applying these changes.
| logger.info("Initiating execution"); | ||
| com.testsigma.sdk.Result result = com.testsigma.sdk.Result.SUCCESS; | ||
| try { | ||
| File zipFilePath = getLastModified(testData1.getValue().toString()); |
There was a problem hiding this comment.
Potential NullPointerException when directory is empty.
getLastModified() returns null if the directory is empty or doesn't exist. This value is used directly on line 46 without a null check, which will cause an NPE in the unzip() method.
Proposed fix
try {
File zipFilePath = getLastModified(testData1.getValue().toString());
+ if (zipFilePath == null) {
+ throw new Exception("No files found in directory: " + testData1.getValue().toString());
+ }
String destDir = testData2.getValue().toString();Also applies to: 102-122
🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfilelatest.java
at line 46, getLastModified(...) can return null when the target directory is
empty or missing; currently the returned value is assigned to zipFilePath and
passed into unzip(...) without a null check causing a NullPointerException. Add
a null check after the assignment to zipFilePath (the variable from
getLastModified) and handle the empty/missing-directory case by logging an error
or returning/throwing early instead of calling unzip; apply the same guard
around uses of getLastModified results in the other block noted (the code around
the 102-122 range) so unzip(...) and any downstream usage never receives a null
File reference.
Comment on lines
+63
to
+99
| private static String unzip(File zipFilePath, String destDir) throws InterruptedException { | ||
| String fileName = null; | ||
| Thread.sleep(1000); | ||
| File dir = new File(destDir); | ||
| // create output directory if it doesn't exist | ||
| if (!dir.exists()) dir.mkdirs(); | ||
| FileInputStream fis; | ||
| // buffer for read and write data to file | ||
| byte[] buffer = new byte[1024]; | ||
| try { | ||
| fis = new FileInputStream(zipFilePath); | ||
| ZipInputStream zis = new ZipInputStream(fis); | ||
| ZipEntry ze = zis.getNextEntry(); | ||
| while (ze != null) { | ||
| fileName = ze.getName(); | ||
| File newFile = new File(destDir + File.separator + fileName); | ||
| // create directories for sub directories in zip | ||
| new File(newFile.getParent()).mkdirs(); | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| Thread.sleep(1000); | ||
| int len; | ||
| while ((len = zis.read(buffer)) > 0) { | ||
| fos.write(buffer, 0, len); | ||
| } | ||
| fos.close(); | ||
| zis.closeEntry(); | ||
| ze = zis.getNextEntry(); | ||
| Thread.sleep(1000); | ||
| } | ||
| zis.closeEntry(); | ||
| zis.close(); | ||
| fis.close(); | ||
| Thread.sleep(1000); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return fileName; |
There was a problem hiding this comment.
Same critical issues as unzipfile.java: resource leaks, Zip Slip, and excessive sleeps.
This unzip method has identical issues to the one in unzipfile.java:
- Resource leaks with streams not properly closed
- Zip Slip vulnerability allowing path traversal
- Multiple unnecessary
Thread.sleep(1000)calls - Silent exception handling that returns potentially invalid data
Consider extracting a shared utility class to avoid code duplication and apply fixes in one place.
🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/unzipfilelatest.java
around lines 63 - 99, The unzip method in unzipfilelatest.java has resource
leaks, a Zip Slip vulnerability, unnecessary Thread.sleep calls, and swallows
IOExceptions; fix it by replacing manual stream handling with try-with-resources
around FileInputStream and ZipInputStream (and FileOutputStream), validate each
ZipEntry name to prevent path traversal (reject or normalize entries that
resolve outside destDir) and create directories for entries before writing,
remove all Thread.sleep(1000) calls, and propagate or throw IOExceptions instead
of only printing stack traces; also consider moving this logic into a shared
utility class (e.g., ZipUtils.extract or similar) to reuse the secure
implementation across unzip and unzipfilelatest.
| setErrorMessage(errorMessage); | ||
| logger.warn(errorMessage); | ||
| } | ||
| setSuccessMessage("File was unzipped and uploaded successfully on the specific element location"); |
There was a problem hiding this comment.
Move success message inside try block.
The success message is set even when an exception occurs, which is misleading. It should only be set on successful execution.
🐛 Proposed fix
String filePath = destDir + File.separator + fileNameupload;
- Thread.sleep(1000);
webElement.sendKeys(filePath);
- Thread.sleep(1000);
+ setSuccessMessage("File was unzipped and uploaded successfully on the specific element location");
}catch (Exception e) {
String errorMessage = ExceptionUtils.getStackTrace(e);
result = com.testsigma.sdk.Result.FAILED;
setErrorMessage(errorMessage);
logger.warn(errorMessage);
}
- setSuccessMessage("File was unzipped and uploaded successfully on the specific element location");
return result;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/Uploadunziplatest.java
at line 56, The success message call setSuccessMessage("File was unzipped and
uploaded successfully on the specific element location") is currently executed
even when exceptions occur; move this call into the try block inside the
Uploadunziplatest class (where the unzip-and-upload logic runs) so it only
executes after all operations complete successfully, and ensure the catch block
sets an appropriate failure message (e.g., via setFailureMessage) instead of
leaving success set on error.
Comment on lines
+59
to
+96
| private static String unzip(File zipFilePath, String destDir) throws InterruptedException { | ||
| String fileName = null; | ||
| Thread.sleep(1000); | ||
| File dir = new File(destDir); | ||
| // create output directory if it doesn't exist | ||
| if (!dir.exists()) dir.mkdirs(); | ||
| FileInputStream fis; | ||
| // buffer for read and write data to file | ||
| byte[] buffer = new byte[1024]; | ||
| try { | ||
| fis = new FileInputStream(zipFilePath); | ||
| ZipInputStream zis = new ZipInputStream(fis); | ||
| ZipEntry ze = zis.getNextEntry(); | ||
| while (ze != null) { | ||
| fileName = ze.getName(); | ||
| File newFile = new File(destDir + File.separator + fileName); | ||
| // create directories for sub directories in zip | ||
| new File(newFile.getParent()).mkdirs(); | ||
| FileOutputStream fos = new FileOutputStream(newFile); | ||
| Thread.sleep(1000); | ||
| int len; | ||
| while ((len = zis.read(buffer)) > 0) { | ||
| fos.write(buffer, 0, len); | ||
| } | ||
| fos.close(); | ||
| zis.closeEntry(); | ||
| ze = zis.getNextEntry(); | ||
| Thread.sleep(1000); | ||
| } | ||
| zis.closeEntry(); | ||
| zis.close(); | ||
| fis.close(); | ||
| Thread.sleep(1000); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return fileName; | ||
| } |
There was a problem hiding this comment.
Critical security and resource management issues in unzip method.
This method has multiple critical issues:
- Path Traversal Vulnerability (Line 74): ZIP entries are not validated, allowing malicious ZIP files to write outside the destination directory using
../in entry names. - Resource Leaks: Streams are not closed with try-with-resources. If an exception occurs,
FileOutputStreamandZipInputStreammay remain open. - Silent Exception Swallowing (Lines 92-93):
IOExceptionis caught and only printed to stderr, not propagated to the caller. - Zip Bomb Risk: No validation of uncompressed file sizes.
🛡️ Proposed comprehensive fix
-private static String unzip(File zipFilePath, String destDir) throws InterruptedException {
+private static String unzip(File zipFilePath, String destDir) throws IOException {
String fileName = null;
- Thread.sleep(1000);
File dir = new File(destDir);
+ String canonicalDestPath = dir.getCanonicalPath();
// create output directory if it doesn't exist
if (!dir.exists()) dir.mkdirs();
- FileInputStream fis;
// buffer for read and write data to file
byte[] buffer = new byte[1024];
- try {
- fis = new FileInputStream(zipFilePath);
- ZipInputStream zis = new ZipInputStream(fis);
+ try (FileInputStream fis = new FileInputStream(zipFilePath);
+ ZipInputStream zis = new ZipInputStream(fis)) {
ZipEntry ze = zis.getNextEntry();
while (ze != null) {
+ if (ze.isDirectory()) {
+ ze = zis.getNextEntry();
+ continue;
+ }
fileName = ze.getName();
File newFile = new File(destDir + File.separator + fileName);
+ // Prevent path traversal
+ String canonicalFilePath = newFile.getCanonicalPath();
+ if (!canonicalFilePath.startsWith(canonicalDestPath + File.separator)) {
+ throw new IOException("Entry is outside of the target dir: " + fileName);
+ }
// create directories for sub directories in zip
new File(newFile.getParent()).mkdirs();
- FileOutputStream fos = new FileOutputStream(newFile);
- Thread.sleep(1000);
- int len;
- while ((len = zis.read(buffer)) > 0) {
- fos.write(buffer, 0, len);
+ try (FileOutputStream fos = new FileOutputStream(newFile)) {
+ int len;
+ while ((len = zis.read(buffer)) > 0) {
+ fos.write(buffer, 0, len);
+ }
}
- fos.close();
zis.closeEntry();
ze = zis.getNextEntry();
- Thread.sleep(1000);
}
- zis.closeEntry();
- zis.close();
- fis.close();
- Thread.sleep(1000);
- } catch (IOException e) {
- e.printStackTrace();
}
return fileName;
}
Comment on lines
+65
to
+76
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | ||
| try { | ||
| String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | ||
| " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;"; | ||
| JavascriptExecutor js = (JavascriptExecutor) driver; | ||
| Object obj = js.executeScript(script); | ||
| return obj.toString(); | ||
| } catch(Exception e){ | ||
| logger.info("There is no file with name:"+desiredFileName); | ||
| throw new RuntimeException("There is no file with name:"+desiredFileName); | ||
| } | ||
| } |
There was a problem hiding this comment.
JavaScript injection vulnerability in file name search.
The desiredFileName parameter is directly concatenated into the JavaScript string without escaping (Line 68), which could allow JavaScript injection if the filename contains special characters like quotes.
🛡️ Proposed fix
public String getFilePathByFileNameInDownloads(String desiredFileName) {
try {
+ // Escape special characters to prevent injection
+ String escapedFileName = desiredFileName.replace("\\", "\\\\").replace("'", "\\'");
String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" +
- " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;";
+ " .find(item => item.fileName && item.fileName.includes('" + escapedFileName + "')).filePath;";
JavascriptExecutor js = (JavascriptExecutor) driver;
Object obj = js.executeScript(script);
return obj.toString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | |
| try { | |
| String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | |
| " .find(item => item.fileName && item.fileName.includes('" + desiredFileName + "')).filePath;"; | |
| JavascriptExecutor js = (JavascriptExecutor) driver; | |
| Object obj = js.executeScript(script); | |
| return obj.toString(); | |
| } catch(Exception e){ | |
| logger.info("There is no file with name:"+desiredFileName); | |
| throw new RuntimeException("There is no file with name:"+desiredFileName); | |
| } | |
| } | |
| public String getFilePathByFileNameInDownloads(String desiredFileName) { | |
| try { | |
| // Escape special characters to prevent injection | |
| String escapedFileName = desiredFileName.replace("\\", "\\\\").replace("'", "\\'"); | |
| String script = "return Array.from(document.querySelector('downloads-manager').shadowRoot.querySelector('#downloadsList').items)" + | |
| " .find(item => item.fileName && item.fileName.includes('" + escapedFileName + "')).filePath;"; | |
| JavascriptExecutor js = (JavascriptExecutor) driver; | |
| Object obj = js.executeScript(script); | |
| return obj.toString(); | |
| } catch(Exception e){ | |
| logger.info("There is no file with name:"+desiredFileName); | |
| throw new RuntimeException("There is no file with name:"+desiredFileName); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/util/ChromeDownloadUtilities.java
around lines 65 - 76, The getFilePathByFileNameInDownloads method currently
concatenates desiredFileName into the JavaScript string, allowing JS injection;
instead pass the filename as a safe argument to the JavascriptExecutor and
reference it via arguments[0] (or use JSON.stringify on the arg in JS) so the
value is not interpreted as code; update the script variable to use arguments[0]
and call js.executeScript(script, desiredFileName), keep the existing exception
handling and ensure you null-check the returned Object before calling toString
to avoid NPEs.
Comment on lines
+93
to
+97
| List<WebElement> progressValues = (List<WebElement>) ((JavascriptExecutor) driver).executeScript(edgeJavaScript); | ||
| if (progressValues != null && progressValues.size() > 0) { | ||
| logger.info("Download is still in progress."); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Incorrect type casting for JavaScript execution result.
The JavaScript returns an array of progress values (strings/numbers), but the code casts the result to List<WebElement>, which is incorrect. It should be List<Object> or List<String>.
🐛 Proposed fix
- List<WebElement> progressValues = (List<WebElement>) ((JavascriptExecutor) driver).executeScript(edgeJavaScript);
+ List<Object> progressValues = (List<Object>) ((JavascriptExecutor) driver).executeScript(edgeJavaScript);
if (progressValues != null && progressValues.size() > 0) {
logger.info("Download is still in progress.");
return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<WebElement> progressValues = (List<WebElement>) ((JavascriptExecutor) driver).executeScript(edgeJavaScript); | |
| if (progressValues != null && progressValues.size() > 0) { | |
| logger.info("Download is still in progress."); | |
| return false; | |
| } | |
| List<Object> progressValues = (List<Object>) ((JavascriptExecutor) driver).executeScript(edgeJavaScript); | |
| if (progressValues != null && progressValues.size() > 0) { | |
| logger.info("Download is still in progress."); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In
@unzipfileandupload/src/main/java/com/testsigma/addons/web/util/EdgeDownloadUtilities.java
around lines 93 - 97, The JavaScript executed in EdgeDownloadUtilities (via
((JavascriptExecutor) driver).executeScript(edgeJavaScript)) returns an array of
primitive values, but the result is incorrectly cast to List<WebElement>; change
the cast to List<Object> (or List<String> if you normalize to strings) for the
variable progressValues, update subsequent null/size checks to use that type,
and handle element value conversion accordingly so the code correctly inspects
the returned progress values instead of WebElement instances.
| @@ -0,0 +1 @@ | |||
| testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9.eyJzdWIiOiJiODUzOWIxMi0wOWM1LTc0YTktNTZjMy0xMTQxZjFmYjY0M2UiLCJ1bmlxdWVJZCI6IjI5OTAiLCJleHRlcm5hbFRlbmFudElkIjoiMjgxNyJ9.lo3N7NyRGoukg7ag9nx8ZG9fsg17ZnOs5hddk2TLzazFY9Kh5uEmXcOoPUY9AxBA8__2pzmsnE_2SY8kXh9Hag No newline at end of file | |||
There was a problem hiding this comment.
Critical: Remove hardcoded API key from source control.
This file contains a plaintext API key that will be committed to the repository and bundled into the JAR. The JWT payload contains identifiable information (externalTenantId: 2817, uniqueId: 2990). Secrets should be:
- Loaded from environment variables or secure secret managers at runtime
- Added to
.gitignoreif required locally - Rotated immediately since this key is now exposed
🤖 Prompt for AI Agents
In @unzipfileandupload/src/main/resources/testsigma-sdk.properties at line 1,
The properties file contains a hardcoded secret in the testsigma-sdk.api.key
property; remove the literal JWT from the file, load the API key at runtime from
a secure source (e.g., environment variable or a secret manager) and update the
code that reads testsigma-sdk.api.key to fall back to that runtime secret, add
the local properties file to .gitignore (or use a template like
testsigma-sdk.properties.template without secrets) so secrets are not committed,
and ensure the exposed key is rotated immediately.
vigneshtestsigma
approved these changes
Jan 11, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Publish this addon as public
Addon Name: UnzipFileAndUpload
Jarvis Link: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira : https://testsigma.atlassian.net/browse/CUS-9990
Added class to get latest downloaded zip, extract files and store filepaths inside zip
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.