[CUS-9936] Added 2 new NLP's to check if the sheet name is present in excel file #297
Conversation
📝 WalkthroughWalkthroughThis pull request introduces two new WebAction classes for Excel sheet verification ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
🤖 Fix all issues with AI Agents
In
@excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameAtIndex.java:
- Around line 107-138: getExcelFile and downloadFile are duplicated in
VerifySheetNameAtIndex (and VerifySheetNameExists); extract them into a shared
utility (e.g., ExcelFileUtils) and replace the local implementations with calls
to ExcelFileUtils.getExcelFile(filePath, logger) (and
ExcelFileUtils.downloadFile if needed). Move the logic from the private methods
getExcelFile and downloadFile into public static methods in ExcelFileUtils
(accepting Logger), update both action classes to call the utility, and remove
the duplicated private methods from VerifySheetNameAtIndex and
VerifySheetNameExists.
- Around line 124-138: The downloadFile method in VerifySheetNameAtIndex has the
same SSRF and resource issues as VerifySheetNameExists; replace its raw
URL.openStream usage with a secure downloader: validate and whitelist the URL
scheme/host (or disallow private IPs), use HttpURLConnection (or a shared
utility method) with connection and read timeouts, check/limit Content-Length
and enforce a maximum streamed byte limit while copying, ensure
try-with-resources and deleteOnExit or explicit temp-file cleanup on failure,
and factor the logic into a shared utility (e.g.,
SecureFileDownloader.downloadFile) so both VerifySheetNameAtIndex and
VerifySheetNameExists reuse the hardened implementation.
In
@excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameExists.java:
- Around line 113-127: The downloadFile method has multiple security and
robustness issues; update downloadFile to: validate the input URL scheme (only
allow http/https), resolve and reject hosts that map to
local/private/loopback/CIDR ranges to mitigate SSRF, and enforce a maximum
allowed content size before streaming (check Content-Length and abort if too
large or use a streaming counter and abort at limit). Replace direct
url.openStream() with an HTTP client configured with connect/read timeouts and
redirect limits, validate the Content-Type against allowed Excel MIME types, and
handle missing filename safely (fallback to a generated name instead of relying
on Paths.get(url.getPath()).getFileName()). Ensure tempFile is deleted on
failures and document caller responsibility to delete it on success (use
try-with-resources and finally cleanup), and throw clear exceptions when
validation or download limits are violated; reference symbols: downloadFile,
fileName, tempFile, logger.info.
🧹 Nitpick comments (1)
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameExists.java (1)
96-108: Add file extension validation.The method should verify that the file has a valid Excel extension (.xlsx or .xls) before attempting to open it. This would provide clearer error messages when users provide incorrect file types.
🔎 Proposed enhancement
private File getExcelFile(String filePath) throws IOException { if (filePath.startsWith("http://") || filePath.startsWith("https://")) { logger.info("Downloading file from URL: " + filePath); return downloadFile(filePath); } else { logger.info("Using local file: " + filePath); File file = new File(filePath); if (!file.exists()) { throw new IOException("File not found: " + filePath); } + String lowerPath = filePath.toLowerCase(); + if (!lowerPath.endsWith(".xlsx") && !lowerPath.endsWith(".xls")) { + throw new IOException("Invalid file type. Expected .xlsx or .xls file: " + filePath); + } return file; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
excelActions_cloud/pom.xmlexcelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameAtIndex.javaexcelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameExists.java
🔇 Additional comments (3)
excelActions_cloud/pom.xml (1)
9-9: LGTM!The version bump to 1.0.20 is appropriate for adding new NLP actions.
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameExists.java (1)
35-91: LGTM with minor inefficiency note.The execute method correctly uses try-with-resources for proper resource management and handles exceptions appropriately. The iteration logic at lines 57-64 continues checking all sheets even after finding a match, which is slightly inefficient but allows comprehensive logging.
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameAtIndex.java (1)
36-102: Excellent implementation with proper validation.The execute method demonstrates good defensive programming with index range validation (lines 58-65) and appropriate exception handling for NumberFormatException (lines 89-93). The error messages are clear and helpful.
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameAtIndex.java
Show resolved
Hide resolved
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameAtIndex.java
Show resolved
Hide resolved
excelActions_cloud/src/main/java/com/testsigma/addons/web/VerifySheetNameExists.java
Show resolved
Hide resolved
please review this addon and publish as PUBLIC
Addon name : ExcelActions_cloud
Addon accont: https://jarvis.testsigma.com/ui/tenants/2817/addons
Jira: https://testsigma.atlassian.net/browse/CUS-9936
fix
Added 2 new NLP's to check if the sheet name is present in excel file
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.