feat/CUS-8108-Modified the addon, supported upload section and sheet index #241
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
akhil-testsigma
changed the title
WalkthroughThis PR introduces a new Maven-based Excel validation addon for XLS files with three WebAction classes that read, verify, and write cell values. The project includes Maven build configuration with Apache POI dependencies, file download support for remote URLs, formula evaluation capabilities, and comprehensive error handling with runtime variable storage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as WebAction
participant FileHandler as File Handler
participant POI as Apache POI
participant Runtime as Runtime Data
User->>FileHandler: Receive file path (URL or local)
alt URL detected
FileHandler->>FileHandler: Download to temp file
end
FileHandler->>POI: Load HSSFWorkbook
POI-->>FileHandler: Workbook ready
FileHandler->>POI: Validate sheet index & row
POI-->>FileHandler: Cell reference valid
rect rgb(220, 240, 220)
Note over FileHandler,POI: Read/Verify Operation
FileHandler->>POI: Read cell value
POI-->>FileHandler: Return cell data
FileHandler->>FileHandler: Evaluate formulas if needed
FileHandler->>FileHandler: Convert to string
end
alt StoreCellvalue or Verify
FileHandler->>Runtime: Store/Compare value
else WriteCellvalue
FileHandler->>POI: Write cell value
POI-->>FileHandler: Write complete
FileHandler->>FileHandler: Save workbook to disk
FileHandler->>Runtime: Store file path
end
FileHandler-->>User: Return Result (SUCCESS/FAILED)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The PR introduces three new WebAction classes with overlapping but distinct logic for Excel operations. Review requires careful examination of file handling, cell reading with formula evaluation, URL download capabilities, and error handling patterns across multiple classes. While patterns repeat, each implementation has independent execution paths warranting separate reasoning. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (10)
excelvalidation_xls/pom.xml (1)
65-68: Useorg.apache.poi:poifor HSSF (.xls);poi-ooxmlis unnecessary.All code imports
org.apache.poi.hssf.*. Switch topoito slim the artifact.- <dependency> - <groupId>org.apache.poi</groupId> - <artifactId>poi-ooxml</artifactId> - <version>5.2.5</version> - </dependency> + <dependency> + <groupId>org.apache.poi</groupId> + <artifactId>poi</artifactId> + <version>5.2.5</version> + </dependency>excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java (3)
120-155: Unify numeric formatting with Verify action to avoid “.0” surprises.Format integer numerics without trailing
.0as done in VerifyExcelXLS.- case NUMERIC: - cellValue = String.valueOf(cell.getNumericCellValue()); + case NUMERIC: + double num = cell.getNumericCellValue(); + cellValue = (num == (long) num) ? String.valueOf((long) num) : String.valueOf(num); break; @@ - switch (evaluator.evaluateInCell(cell).getCellType()) { + switch (evaluator.evaluateInCell(cell).getCellType()) { case NUMERIC: - cellValue = String.valueOf(cell.getNumericCellValue()); + double n = cell.getNumericCellValue(); + cellValue = (n == (long) n) ? String.valueOf((long) n) : String.valueOf(n); break;
96-100: Nit: stray;;and noisy log.- cellValue = getCellValue(cell, workbook).trim();; + cellValue = getCellValue(cell, workbook).trim();
120-174: DRY: duplicate helpers across 3 classes. Extract to a shared util.Create
com.testsigma.addons.web.util.ExcelXlsUtilswithgetCellValue(HSSFCell,HSSFWorkbook)anddownloadFile(String), and reuse from all actions to keep behavior consistent.excelvalidation_xls/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java (3)
63-69: URLs are downloaded and edited locally; clarify this in UX/messages.The action doesn’t upload back to the source URL; it writes a temp local copy and returns its path. Make this explicit in
actionText/success message to avoid confusion.- actionText = "Write the data datavalue into Excel(xls) filepath ... and store the path in runtime variable variable-name", + actionText = "Write datavalue into Excel (xls) at filepath (or download URL to a temp local copy), rowNo,columnNo, sheet-index (1-based); store the resulting local file path in runtime variable variable-name",
21-27: Polish: clarify messages.Make success message explicit when source was a URL and we wrote to a local temp path.
- setSuccessMessage("Successfully written value '" + valueToWrite + - "' to Excel file at row " + rowIdx + ", column " + colIdx + - ", sheet index " + sheetIdx + ". File path: " + excelFile.getAbsolutePath()); + setSuccessMessage("Successfully wrote '" + valueToWrite + + "' at row " + rowIdx + ", column " + colIdx + + ", sheet index " + sheetIdx + ". Local file path: " + excelFile.getAbsolutePath());Also applies to: 102-107
120-133: DRY: duplicate helpers.Extract
downloadFileinto a shared util used by all 3 actions.excelvalidation_xls/src/main/java/com/testsigma/addons/web/VerifyExcelXLS.java (3)
86-94: Minor: noisy log + stray;;.- logger.info("Successfully read cell value from cell sheet index"); + logger.info("Blank cell at row " + rowIdx + ", column " + colIdx); @@ - actual = getCellValue(cell, workbook).trim();; + actual = getCellValue(cell, workbook).trim();
116-161: Improve cell value extraction: handle dates and unify numeric formatting.Excel dates are numeric; consider
DateUtil.isCellDateFormatted(cell)to avoid confusing outputs.+import org.apache.poi.ss.usermodel.DateUtil; @@ - case NUMERIC: - double num = cell.getNumericCellValue(); - if (num == (long) num) { - cellValue = String.valueOf((long) num); // format integers without .0 - } else { - cellValue = String.valueOf(num); - } + case NUMERIC: + if (DateUtil.isCellDateFormatted(cell)) { + cellValue = cell.getDateCellValue().toString(); // or format with SimpleDateFormat + } else { + double num = cell.getNumericCellValue(); + cellValue = (num == (long) num) ? String.valueOf((long) num) : String.valueOf(num); + } break; @@ - case NUMERIC: - double n = cell.getNumericCellValue(); - cellValue = (n == (long) n) ? String.valueOf((long) n) : String.valueOf(n); + case NUMERIC: + if (DateUtil.isCellDateFormatted(cell)) { + cellValue = cell.getDateCellValue().toString(); + } else { + double n = cell.getNumericCellValue(); + cellValue = (n == (long) n) ? String.valueOf((long) n) : String.valueOf(n); + } break;
116-161: DRY: duplicate helpers. Extract and reuse.Create a shared utility for
getCellValue/downloadFileto ensure parity across actions and ease future fixes.Also applies to: 163-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
excelvalidation_xls/pom.xml(1 hunks)excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java(1 hunks)excelvalidation_xls/src/main/java/com/testsigma/addons/web/VerifyExcelXLS.java(1 hunks)excelvalidation_xls/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java(1 hunks)excelvalidation_xls/src/main/resources/testsigma-sdk.properties(1 hunks)
🔇 Additional comments (6)
excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java (1)
82-93: Row/column indexing semantics unclear (0‑ vs 1‑based).Sheet index is 1‑based; row/column are not adjusted. Confirm expected contract; if 1‑based, subtract 1 before access.
Proposed fix if 1‑based:
- int intRow = Integer.parseInt(rowNo.getValue().toString()); - int intColumn = Integer.parseInt(columnNo.getValue().toString()); + int intRow = Integer.parseInt(rowNo.getValue().toString()) - 1; + int intColumn = Integer.parseInt(columnNo.getValue().toString()) - 1;excelvalidation_xls/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java (1)
55-59: Validate indices and confirm 0‑/1‑based contract.Currently, row/column used as-is. If users enter 1‑based indices, subtract 1; also guard negatives.
- int rowIdx = Integer.parseInt(rowNumber.getValue().toString()); - int colIdx = Integer.parseInt(columnNumber.getValue().toString()); + int rowIdx = Integer.parseInt(rowNumber.getValue().toString()); + int colIdx = Integer.parseInt(columnNumber.getValue().toString()); int sheetIdx = Integer.parseInt(sheetIndex.getValue().toString()); + if (rowIdx < 0 || colIdx < 0) { + setErrorMessage("Row/Column indexes must be >= 0"); + return Result.FAILED; + }If 1‑based is desired:
rowIdx--; colIdx--;.excelvalidation_xls/src/main/java/com/testsigma/addons/web/VerifyExcelXLS.java (1)
49-54: Trim expected; confirm index semantics.Avoid whitespace issues and confirm 0‑/1‑based row/column contract.
- String expected = expectedValue.getValue().toString(); + String expected = expectedValue.getValue().toString().trim();If row/col are 1‑based, subtract 1 before use.
excelvalidation_xls/pom.xml (2)
47-58: Likely unnecessary heavy deps (Selenium/Appium) for an Excel-only addon.They bloat the shaded jar and increase CVE surface. If not used here, drop them.
Run to confirm usage:
37-46: Dual test frameworks and pre-release JUnit.You include both JUnit 5 and TestNG, and JUnit 5.8.0-M1 (milestone). Prefer one stable framework or scope them only where used.
Quick check:
excelvalidation_xls/src/main/resources/testsigma-sdk.properties (1)
1-1: Critical: Hardcoded API key committed; rotate and remove immediately.This file exposes a live-looking token in VCS and will be bundled into the shaded jar.
Actions:
- Revoke/rotate the leaked key now.
- Replace with a non-secret template and inject via CI/env at runtime.
- Exclude this file from packaging.
Apply one of these minimal diffs right away:
- testsigma-sdk.api.key=eyJhbGciOiJIUzUxMiJ9... + # Intentionally blank. Inject via env or -Dtestsigma-sdk.api.key at runtime. + testsigma-sdk.api.key=And exclude from the shaded jar:
@@ <plugin> <artifactId>maven-shade-plugin</artifactId> @@ - <filters> + <filters> <filter> <artifact>*:*</artifact> <excludes> <exclude>META-INF/*.SF</exclude> <exclude>META-INF/*.DSA</exclude> <exclude>META-INF/*.RSA</exclude> + <exclude>testsigma-sdk.properties</exclude> </excludes> </filter> </filters>Optionally, move this to
testsigma-sdk.properties.exampleand add the real file to.gitignore.Would you like a PR-ready follow-up that rotates the key and adds packaging/resource exclusions across modules?
Comment on lines
+79
to
+105
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>3.2.4</version> | ||
| <executions> | ||
| <execution> | ||
| <phase>package</phase> | ||
| <goals> | ||
| <goal>shade</goal> | ||
| </goals> | ||
| <configuration> | ||
| <minimizeJar>true</minimizeJar> | ||
| <filters> | ||
| <filter> | ||
| <artifact>*:*</artifact> | ||
| <excludes> | ||
| <exclude>META-INF/*.SF</exclude> | ||
| <exclude>META-INF/*.DSA</exclude> | ||
| <exclude>META-INF/*.RSA</exclude> | ||
| </excludes> | ||
| </filter> | ||
| </filters> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> | ||
| <plugin> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Exclude sensitive resources during shading.
To prevent accidental bundling of credentials, exclude testsigma-sdk.properties.
@@
- <filters>
+ <filters>
<filter>
<artifact>*:*</artifact>
<excludes>
<exclude>META-INF/*.SF</exclude>
<exclude>META-INF/*.DSA</exclude>
<exclude>META-INF/*.RSA</exclude>
+ <exclude>testsigma-sdk.properties</exclude>
</excludes>
</filter>
</filters>📝 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
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-shade-plugin</artifactId> | |
| <version>3.2.4</version> | |
| <executions> | |
| <execution> | |
| <phase>package</phase> | |
| <goals> | |
| <goal>shade</goal> | |
| </goals> | |
| <configuration> | |
| <minimizeJar>true</minimizeJar> | |
| <filters> | |
| <filter> | |
| <artifact>*:*</artifact> | |
| <excludes> | |
| <exclude>META-INF/*.SF</exclude> | |
| <exclude>META-INF/*.DSA</exclude> | |
| <exclude>META-INF/*.RSA</exclude> | |
| </excludes> | |
| </filter> | |
| </filters> | |
| </configuration> | |
| </execution> | |
| </executions> | |
| </plugin> | |
| <plugin> | |
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-shade-plugin</artifactId> | |
| <version>3.2.4</version> | |
| <executions> | |
| <execution> | |
| <phase>package</phase> | |
| <goals> | |
| <goal>shade</goal> | |
| </goals> | |
| <configuration> | |
| <minimizeJar>true</minimizeJar> | |
| <filters> | |
| <filter> | |
| <artifact>*:*</artifact> | |
| <excludes> | |
| <exclude>META-INF/*.SF</exclude> | |
| <exclude>META-INF/*.DSA</exclude> | |
| <exclude>META-INF/*.RSA</exclude> | |
| <exclude>testsigma-sdk.properties</exclude> | |
| </excludes> | |
| </filter> | |
| </filters> | |
| </configuration> | |
| </execution> | |
| </executions> | |
| </plugin> | |
| <plugin> |
🤖 Prompt for AI Agents
In excelvalidation_xls/pom.xml around lines 79-105, the maven-shade-plugin
filter needs to explicitly exclude the credentials file to avoid bundling
sensitive resources; add an <exclude> entry for testsigma-sdk.properties (and if
it may appear under META-INF, also add META-INF/testsigma-sdk.properties) to the
existing <excludes> list inside the plugin configuration so the shade process
omits that file from the packaged JAR.
Comment on lines
+39
to
+41
| @TestData(reference = "testdata") | ||
| private com.testsigma.sdk.TestData runtimeKey; | ||
|
|
There was a problem hiding this comment.
Runtime variable param missing isRuntimeVariable = true.
Write action uses it; this one doesn’t. Align to allow proper variable selection in UI.
- @TestData(reference = "testdata")
+ @TestData(reference = "testdata", isRuntimeVariable = true)
private com.testsigma.sdk.TestData runtimeKey;📝 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
| @TestData(reference = "testdata") | |
| private com.testsigma.sdk.TestData runtimeKey; | |
| @TestData(reference = "testdata", isRuntimeVariable = true) | |
| private com.testsigma.sdk.TestData runtimeKey; |
🤖 Prompt for AI Agents
In
excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java
around lines 39 to 41, the TestData field annotation is missing
isRuntimeVariable = true which prevents proper runtime variable selection in the
UI; update the @TestData annotation to include isRuntimeVariable = true (e.g.,
@TestData(reference = "testdata", isRuntimeVariable = true)) so the action can
accept runtime variables in the UI, and ensure imports and compilation succeed
after the change.
Comment on lines
+57
to
+76
| FileInputStream inputStream = null; | ||
| HSSFWorkbook workbook = null; | ||
|
|
||
| try { | ||
| logger.info("Excel file path: " + fileLocation); | ||
|
|
||
| if (fileLocation.startsWith("http://") || fileLocation.startsWith("https://")) { | ||
| excelFile = downloadFile(fileLocation); | ||
| } else { | ||
| excelFile = new File(fileLocation); | ||
| } | ||
|
|
||
| if (!excelFile.exists() || !excelFile.isFile()) { | ||
| setErrorMessage("Invalid file path or file not found: " + fileLocation); | ||
| return Result.FAILED; | ||
| } | ||
|
|
||
| inputStream = new FileInputStream(excelFile); | ||
| workbook = new HSSFWorkbook(inputStream); | ||
|
|
There was a problem hiding this comment.
Resource leak: workbook and stream not closed.
Use try‑with‑resources to avoid file handle leaks.
- FileInputStream inputStream = null;
- HSSFWorkbook workbook = null;
@@
- inputStream = new FileInputStream(excelFile);
- workbook = new HSSFWorkbook(inputStream);
+ try (FileInputStream inputStream = new FileInputStream(excelFile);
+ HSSFWorkbook workbook = new HSSFWorkbook(inputStream)) {
+ // ... move the following logic inside this block until cell read/store ...
+
+ if (sheetNum < 1 || sheetNum > workbook.getNumberOfSheets()) {
+ setErrorMessage("Invalid sheet index: " + sheetNum);
+ return Result.FAILED;
+ }
+
+ HSSFSheet sheet = workbook.getSheetAt(sheetNum - 1);
+ int intRow = Integer.parseInt(rowNo.getValue().toString());
+ int intColumn = Integer.parseInt(columnNo.getValue().toString());
+
+ if (intRow < 0 || intColumn < 0) {
+ setErrorMessage("Row/Column indexes must be >= 0");
+ return Result.FAILED;
+ }
+
+ HSSFRow row = sheet.getRow(intRow);
+ if (row == null) {
+ setErrorMessage("Row " + intRow + " does not exist in sheet index " + sheetNum);
+ return Result.FAILED;
+ }
+ HSSFCell cell = row.getCell(intColumn);
+ String cellValue = "";
+ if (cell == null || cell.getCellType() == CellType.BLANK) {
+ cellValue = "";
+ setSuccessMessage("Successfully read an empty/blank cell at row " + intRow + ", column " + intColumn);
+ } else {
+ cellValue = getCellValue(cell, workbook).trim();
+ }
+
+ logger.info("Read cell value: " + cellValue);
+ runTimeData.setKey(runtimeKey.getValue().toString());
+ runTimeData.setValue(cellValue);
+ setSuccessMessage("Successfully stored cell value '" + cellValue +
+ "' from row " + intRow + ", column " + intColumn + " (Sheet index " + sheetNum + ") into runtime variable '" +
+ runtimeKey.getValue() + "'");
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java
around lines 57 to 76, the FileInputStream and HSSFWorkbook are opened but never
closed; replace the current manual open with a try-with-resources block that
declares the FileInputStream and constructs the HSSFWorkbook from that stream
(e.g., try (FileInputStream inputStream = new FileInputStream(excelFile);
HSSFWorkbook workbook = new HSSFWorkbook(inputStream)) { ... }), remove the
external variables and any manual close calls, and ensure any code that uses
workbook stays inside the try block so both the stream and workbook are
auto-closed to prevent resource leaks.
Comment on lines
+161
to
+174
| private File downloadFile(String fileUrl) throws IOException { | ||
| URL url = new URL(fileUrl); | ||
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | ||
| File tempFile = File.createTempFile("downloaded-", "-" + fileName); | ||
| try (InputStream in = url.openStream(); | ||
| OutputStream out = new FileOutputStream(tempFile)) { | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = in.read(buffer)) != -1) { | ||
| out.write(buffer, 0, bytesRead); | ||
| } | ||
| } | ||
| return tempFile; | ||
| } |
There was a problem hiding this comment.
Remote download without timeouts or SSRF guard.
Unbounded URL.openStream() can hang; accepting arbitrary URLs risks SSRF.
Minimal safer replacement:
- private File downloadFile(String fileUrl) throws IOException {
- URL url = new URL(fileUrl);
- String fileName = Paths.get(url.getPath()).getFileName().toString();
- File tempFile = File.createTempFile("downloaded-", "-" + fileName);
- try (InputStream in = url.openStream();
- OutputStream out = new FileOutputStream(tempFile)) {
+ private File downloadFile(String fileUrl) throws IOException {
+ URL url = new URL(fileUrl);
+ String fileName = Paths.get(url.getPath()).getFileName().toString();
+ File tempFile = File.createTempFile("downloaded-", "-" + fileName);
+ tempFile.deleteOnExit();
+ java.net.HttpURLConnection conn = (java.net.HttpURLConnection) url.openConnection();
+ conn.setInstanceFollowRedirects(false);
+ conn.setConnectTimeout(10_000);
+ conn.setReadTimeout(20_000);
+ conn.connect();
+ try (InputStream in = conn.getInputStream();
+ OutputStream out = new FileOutputStream(tempFile)) {
byte[] buffer = new byte[1024];
int bytesRead;
while ((bytesRead = in.read(buffer)) != -1) {
out.write(buffer, 0, bytesRead);
}
- }
+ } finally {
+ conn.disconnect();
+ }
return tempFile;
}Optionally, add an allowlist or block private/loopback ranges before connecting. I can provide a hardened helper if needed.
🤖 Prompt for AI Agents
In
excelvalidation_xls/src/main/java/com/testsigma/addons/web/StoreCellvalueXLS.java
around lines 161 to 174, the downloadFile method uses URL.openStream() without
timeouts or SSRF protections; replace it with a safer implementation that (1)
validates the input URL (scheme must be http/https), (2) resolves the host and
rejects private/loopback/link-local/unspecified addresses (or enforce an
allowlist) to prevent SSRF, (3) open an HttpURLConnection instead of
URL.openStream(), set reasonable connect and read timeouts and limit redirects,
and (4) keep try-with-resources for streams and return the temp file; implement
these checks and timeouts before opening the connection.
Comment on lines
+163
to
+176
| private File downloadFile(String fileUrl) throws IOException { | ||
| URL url = new URL(fileUrl); | ||
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | ||
| File tempFile = File.createTempFile("downloaded-", "-" + fileName); | ||
| try (InputStream in = url.openStream(); | ||
| OutputStream out = new FileOutputStream(tempFile)) { | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = in.read(buffer)) != -1) { | ||
| out.write(buffer, 0, bytesRead); | ||
| } | ||
| } | ||
| return tempFile; | ||
| } |
There was a problem hiding this comment.
Remote download without timeouts or SSRF guard.
Same concern as other actions; apply the hardened downloader or shared util.
- private File downloadFile(String fileUrl) throws IOException {
- URL url = new URL(fileUrl);
- String fileName = Paths.get(url.getPath()).getFileName().toString();
- File tempFile = File.createTempFile("downloaded-", "-" + fileName);
- try (InputStream in = url.openStream();
- OutputStream out = new FileOutputStream(tempFile)) {
+ private File downloadFile(String fileUrl) throws IOException {
+ URL url = new URL(fileUrl);
+ String fileName = Paths.get(url.getPath()).getFileName().toString();
+ File tempFile = File.createTempFile("downloaded-", "-" + fileName);
+ tempFile.deleteOnExit();
+ java.net.HttpURLConnection conn = (java.net.HttpURLConnection) url.openConnection();
+ conn.setInstanceFollowRedirects(false);
+ conn.setConnectTimeout(10_000);
+ conn.setReadTimeout(20_000);
+ conn.connect();
+ try (InputStream in = conn.getInputStream();
+ OutputStream out = new FileOutputStream(tempFile)) {
byte[] buffer = new byte[1024];
int bytesRead;
while ((bytesRead = in.read(buffer)) != -1) {
out.write(buffer, 0, bytesRead);
}
- }
+ } finally {
+ conn.disconnect();
+ }
return tempFile;
}🤖 Prompt for AI Agents
In
excelvalidation_xls/src/main/java/com/testsigma/addons/web/VerifyExcelXLS.java
around lines 163 to 176, the downloadFile method performs an unguarded remote
download (no timeouts, no input validation/SSRF protection); replace this ad-hoc
downloader with the shared/hardened downloader utility or implement equivalent
protections: validate and restrict the URL host (deny internal IPs/localhost),
enforce a whitelist or allowed-scheme check, set connection and read timeouts on
the HTTP request, follow a strict max download size and streaming limits, and
propagate meaningful exceptions; use the existing shared util if available to
ensure consistent SSRF protections and timeout/config handling.
Comment on lines
+120
to
+133
| private File downloadFile(String fileUrl) throws IOException { | ||
| URL url = new URL(fileUrl); | ||
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | ||
| File tempFile = File.createTempFile("downloaded-", "-" + fileName); | ||
| try (InputStream in = url.openStream(); | ||
| OutputStream out = new FileOutputStream(tempFile)) { | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = in.read(buffer)) != -1) { | ||
| out.write(buffer, 0, bytesRead); | ||
| } | ||
| } | ||
| return tempFile; | ||
| } |
There was a problem hiding this comment.
Remote download without timeouts or SSRF guard. Same fix as Store.
Apply the hardened downloadFile from StoreCellvalueXLS here as well (timeouts + deleteOnExit), or centralize via a shared util.
- private File downloadFile(String fileUrl) throws IOException {
- URL url = new URL(fileUrl);
- String fileName = Paths.get(url.getPath()).getFileName().toString();
- File tempFile = File.createTempFile("downloaded-", "-" + fileName);
- try (InputStream in = url.openStream();
- OutputStream out = new FileOutputStream(tempFile)) {
+ private File downloadFile(String fileUrl) throws IOException {
+ URL url = new URL(fileUrl);
+ String fileName = Paths.get(url.getPath()).getFileName().toString();
+ File tempFile = File.createTempFile("downloaded-", "-" + fileName);
+ tempFile.deleteOnExit();
+ java.net.HttpURLConnection conn = (java.net.HttpURLConnection) url.openConnection();
+ conn.setInstanceFollowRedirects(false);
+ conn.setConnectTimeout(10_000);
+ conn.setReadTimeout(20_000);
+ conn.connect();
+ try (InputStream in = conn.getInputStream();
+ OutputStream out = new FileOutputStream(tempFile)) {
byte[] buffer = new byte[1024];
int bytesRead;
while ((bytesRead = in.read(buffer)) != -1) {
out.write(buffer, 0, bytesRead);
}
- }
+ } finally {
+ conn.disconnect();
+ }
return tempFile;
}📝 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
| private File downloadFile(String fileUrl) throws IOException { | |
| URL url = new URL(fileUrl); | |
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | |
| File tempFile = File.createTempFile("downloaded-", "-" + fileName); | |
| try (InputStream in = url.openStream(); | |
| OutputStream out = new FileOutputStream(tempFile)) { | |
| byte[] buffer = new byte[1024]; | |
| int bytesRead; | |
| while ((bytesRead = in.read(buffer)) != -1) { | |
| out.write(buffer, 0, bytesRead); | |
| } | |
| } | |
| return tempFile; | |
| } | |
| private File downloadFile(String fileUrl) throws IOException { | |
| URL url = new URL(fileUrl); | |
| String fileName = Paths.get(url.getPath()).getFileName().toString(); | |
| File tempFile = File.createTempFile("downloaded-", "-" + fileName); | |
| tempFile.deleteOnExit(); | |
| java.net.HttpURLConnection conn = (java.net.HttpURLConnection) url.openConnection(); | |
| conn.setInstanceFollowRedirects(false); | |
| conn.setConnectTimeout(10_000); | |
| conn.setReadTimeout(20_000); | |
| conn.connect(); | |
| try (InputStream in = conn.getInputStream(); | |
| OutputStream out = new FileOutputStream(tempFile)) { | |
| byte[] buffer = new byte[1024]; | |
| int bytesRead; | |
| while ((bytesRead = in.read(buffer)) != -1) { | |
| out.write(buffer, 0, bytesRead); | |
| } | |
| } finally { | |
| conn.disconnect(); | |
| } | |
| return tempFile; | |
| } |
🤖 Prompt for AI Agents
In
excelvalidation_xls/src/main/java/com/testsigma/addons/web/WriteCellvalueWithSheet.java
around lines 120-133, the downloadFile method performs an unguarded remote
download without timeouts or cleanup; replace it with the hardened
implementation used in StoreCellvalueXLS: validate the input URL (ensure scheme
is http/https and reject private/loopback/internal IPs or enforce a host
allowlist to mitigate SSRF), open a URLConnection and set connect and read
timeouts (e.g. setConnectTimeout and setReadTimeout), stream the response to a
temp file created via File.createTempFile and call tempFile.deleteOnExit(), and
propagate IOExceptions; alternatively extract this logic to a shared util and
call it from both classes to avoid duplication.
Ganesh-Testsigma
approved these changes
Oct 22, 2025
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.
Addon Name: ExcelValidation_XLS
Jarvis Link: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira : https://testsigma.atlassian.net/browse/CUS-8108
Modified the addon, supported upload section and sheet index
Summary by CodeRabbit
Release Notes