Bug fix of value expression#4285
Conversation
WalkthroughRefactors the ActSecurityTesting edit page layout to a two-row Grid, converts GetAlertList to a static method that populates entries from the enum descriptions, and updates ZapProxyService to resolve ZAP URL/API key with ValueExpression.PasswordCalculation before creating the client. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as ActSecurityTestingEditPage
participant Logic as GetAlertList (static)
participant Enum as eAlertTypes
participant Util as General.GetEnumValueDescription
User->>UI: Open Edit Page
UI->>Logic: GetAlertList()
Logic->>Enum: Iterate values()
loop per enum value
Logic->>Util: GetEnumValueDescription(value)
Util-->>Logic: description
Logic-->>Logic: Add OperationValues(description)
end
Logic-->>UI: ObservableList<OperationValues>
UI-->>User: Alert items bound
sequenceDiagram
autonumber
participant C as ZapProxyService
participant Cfg as zAPConfiguration
participant VE as ValueExpression.PasswordCalculation
participant Url as GetHostFromUrl/GetPortFromUrl
participant API as ClientApi
C->>Cfg: Read ZAPUrl, ZAPApiKey
C->>VE: PasswordCalculation(ZAPUrl)
VE-->>C: processedUrl
C->>Url: GetHostFromUrl(processedUrl)
Url-->>C: host
C->>Url: GetPortFromUrl(processedUrl)
Url-->>C: port
C->>VE: PasswordCalculation(ZAPApiKey)
VE-->>C: apiKey
C->>API: new ClientApi(host, port, apiKey)
API-->>C: instance ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (8)
51-68: Make the async method truly asynchronous.The method is declared async but uses Thread.Sleep, blocking the thread and triggering analyzer warnings. Use Task.Delay.
Apply this diff:
- Thread.Sleep(1000); + await Task.Delay(1000).ConfigureAwait(false);Optional follow-up: add a cancellation token and timeout to avoid indefinite waits; I can propose that refactor if useful.
245-251: Reduce log spam and improve observability during active scan.Logging “Active scan is in progress” every second is noisy. Log periodically and include progress.
Apply this diff:
- while (!status.Equals("100")) - { - Thread.Sleep(1000); - apiResponse = _zapClient.ascan.status(scanId); - status = ((ApiResponseElement)apiResponse).Value; - Reporter.ToLog(eLogLevel.INFO, "Active scan is in progress"); - } + int tick = 0; + while (!status.Equals("100")) + { + Thread.Sleep(1000); + apiResponse = _zapClient.ascan.status(scanId); + status = ((ApiResponseElement)apiResponse).Value; + if (++tick % 5 == 0) + { + Reporter.ToLog(eLogLevel.INFO, $"Active scan is in progress: {status}%"); + } + }
90-92: Fix XML doc return type for GetPortFromUrl.The documentation says “string” but the method returns int?.
Apply this diff:
- /// <returns>The port number as string, or null if not found.</returns> + /// <returns>The port number as int?, or null if not found.</returns>
270-283: False negatives/positives in EvaluateScanResult: count is ignored.The code marks the test as failed if an alert isn’t in the allowed list, even when its count is “0”. This will fail runs with zero occurrences. Also, comparison should be case-insensitive and null-safe.
Apply this diff:
- if (!allowedAlertNames.Any(a => a.Value == alertName)) - { - testPassed = false; - } + bool isAllowed = allowedAlertNames != null && + allowedAlertNames.Any(a => string.Equals(a.Value, alertName, StringComparison.OrdinalIgnoreCase)); + if (count > 0 && !isAllowed) + { + testPassed = false; + }
287-293: EvaluateScanResult(string) uses ToString(), not actual counts.ToString() on ApiResponseSet isn’t a reliable indicator of alert presence. Parse counts and return true only when any count > 0.
Apply this diff:
- return !string.IsNullOrEmpty(alertSummary.ToString()); + foreach (var entry in alertSummary.Dictionary) + { + var val = entry.Value is ApiResponseElement e ? e.Value : entry.Value?.ToString(); + if (int.TryParse(val, out int count) && count > 0) + { + return true; + } + } + return false;
216-234: Optional: expose async active-scan wait to avoid blocking call chains.WaitTillActiveScanIsCompleted is synchronous with Thread.Sleep. Consider an async variant to integrate better with async flows, similar to the passive scan method. This is optional but improves scalability.
I can provide a cohesive async version (including cancellation support) for PerformActiveScan/WaitTillActiveScanIsCompleted if you’re open to widening the surface.
86-177: Unit tests for URL parsing helpers would pay off.Given the breadth of URL/host:port/IPv6 formats handled, unit tests will prevent regressions (and validate the constructor fallback changes).
I can add tests covering:
- "http://zap:8080", "https://zap", "zap:8080", "zap", "[::1]:8080", "http://[::1]:8080", "", null
Let me know and I’ll draft them.
263-285: Ensure EmptytargetUrlIs Used for Passive ZAP Scan AggregationThe passive‐scan summary methods
EvaluateScanResultandGetAlertSummarymust be invoked with an emptytargetUrl("") to aggregate results across the entire ZAP session. Currently, all calls inActSecurityTesting.cspass a scoped URL (testURLorapiEndpointURL), which limits the summary to that single endpoint.Please update the following call sites in
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs:
- Line 226
- bool isPassed = zapProxyService.EvaluateScanResult(testURL, AlertList);
- bool isPassed = zapProxyService.EvaluateScanResult("", AlertList);
- Line 241
- if (zapProxyService.EvaluateScanResult(testURL))
- if (zapProxyService.EvaluateScanResult(""))
- Line 283
- isPassed = zapProxyService.EvaluateScanResult(testURL, AlertList);
- isPassed = zapProxyService.EvaluateScanResult("", AlertList);
- Lines 332–333
? zapProxyService.EvaluateScanResult(apiEndpointURL, AlertList): zapProxyService.EvaluateScanResult(apiEndpointURL);
? zapProxyService.EvaluateScanResult("", AlertList): zapProxyService.EvaluateScanResult("");
- Line 346
- var alertSummary = zapProxyService.GetAlertSummary(apiEndpointURL);
- var alertSummary = zapProxyService.GetAlertSummary("");
- Line 392
- var alertSummary = zapProxyService.GetAlertSummary(testURL);
- var alertSummary = zapProxyService.GetAlertSummary("");
These changes ensure that passive‐scan alerts are aggregated for the entire session, not just a single URL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml(1 hunks)Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml.cs(1 hunks)Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1958-1970
Timestamp: 2025-08-25T09:20:17.589Z
Learning: In Ginger security testing, passive ZAP scans should be invoked with an empty testURL (ActSecurityTesting.ExecutePassiveZapScan("", act)) to aggregate alerts across the entire ZAP session. Passing a URL scopes results to that base URL. Only the active scan path should pass Driver.Url, with null/empty checks. Context: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs :: ActSecurity. Confirmed by AmanPrasad43 in PR #4281.
📚 Learning: 2025-08-25T08:48:11.895Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.895Z
Learning: In the Ginger codebase, the OWASPZAPDotNetAPI package reference in GingerCore.csproj is required for architectural reasons, even when direct usage is not immediately visible in the codebase. This was confirmed by AmanPrasad43 during the security testing feature implementation.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T09:30:08.066Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreCommon/External/Configurations/ZAPConfiguration.cs:37-58
Timestamp: 2025-08-25T09:30:08.066Z
Learning: In the Ginger codebase, URL validation in configuration classes like ZAPConfiguration should not throw exceptions for invalid URLs. The team prefers simple normalization (like trimming trailing slashes) over strict validation with exception throwing. This was confirmed by AmanPrasad43 in PR #4281 for the ZAPConfiguration.ZAPUrl property setter.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T09:27:30.226Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1433-1441
Timestamp: 2025-08-25T09:27:30.226Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs, SetProxy(dynamic options) should not force options.AcceptInsecureCertificates = false. Only set AcceptInsecureCertificates = true when UseZAP is enabled to avoid breaking setups that permit self-signed/dev certificates.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-06-16T10:37:13.073Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4232
File: Ginger/GingerCoreNET/ActionsLib/UI/VisualTesting/VRTAnalyzer.cs:19-22
Timestamp: 2025-06-16T10:37:13.073Z
Learning: In Ginger codebase, both `using amdocs.ginger.GingerCoreNET;` and `using Amdocs.Ginger.CoreNET;` are valid and serve different purposes. The first (lowercase) contains the WorkSpace class and related workspace functionality, while the second (proper case) contains drivers like GenericAppiumDriver and other core functionality. Both may be required in files that use types from both namespaces.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it provides access to the `ObservableList<>` class which is used throughout the file for collections of RunSetConfig, AnalyzerItemBase, and ApplicationPOMModel objects.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-03-20T11:10:30.816Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4137
File: Ginger/Ginger/SourceControl/SourceControlProjectsPage.xaml.cs:21-21
Timestamp: 2025-03-20T11:10:30.816Z
Learning: The `Amdocs.Ginger.Common.SourceControlLib` namespace is required in files that reference the `GingerSolution` class for source control operations in the Ginger automation framework.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-04-25T13:29:45.059Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4188
File: Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs:22-23
Timestamp: 2025-04-25T13:29:45.059Z
Learning: The `using Amdocs.Ginger.Repository;` statement in Ginger/GingerCoreNET/RunLib/CLILib/DoOptionsHanlder.cs is necessary as it imports the ObservableList<> class which is used throughout the file.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T09:20:17.589Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1958-1970
Timestamp: 2025-08-25T09:20:17.589Z
Learning: In Ginger security testing, passive ZAP scans should be invoked with an empty testURL (ActSecurityTesting.ExecutePassiveZapScan("", act)) to aggregate alerts across the entire ZAP session. Passing a URL scopes results to that base URL. Only the active scan path should pass Driver.Url, with null/empty checks. Context: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs :: ActSecurity. Confirmed by AmanPrasad43 in PR #4281.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs
🧬 Code graph analysis (2)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
Ginger/GingerCoreNET/ValueExpressionLib/ValueExpression.cs (1)
PasswordCalculation(2039-2064)
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml.cs (2)
Ginger/GingerCoreNET/GeneralLib/General.cs (2)
General(58-1481)GetEnumValueDescription(72-105)Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
ActSecurityTesting(41-410)
🔇 Additional comments (1)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
5-5: Import looks acceptable; no issues.The added import aligns with recent UI/enum refactors mentioned in the PR description. No concerns here.
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Ginger/Ginger/ExternalConfigurations/ZAPConfigurationPage.xaml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1958-1970
Timestamp: 2025-08-25T09:20:17.589Z
Learning: In Ginger security testing, passive ZAP scans should be invoked with an empty testURL (ActSecurityTesting.ExecutePassiveZapScan("", act)) to aggregate alerts across the entire ZAP session. Passing a URL scopes results to that base URL. Only the active scan path should pass Driver.Url, with null/empty checks. Context: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs :: ActSecurity. Confirmed by AmanPrasad43 in PR #4281.
🔇 Additional comments (2)
Ginger/Ginger/ExternalConfigurations/ZAPConfigurationPage.xaml (2)
19-19: Third row added — OKAdding a second star-sized content row (Grid.Row="2") aligns with the layout below. No issues noted.
55-55: No action — whitespace onlyThis appears to be only whitespace/formatting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml (2)
22-22: Remove hard-coded right margin; let the Grid’s star column handle width (responsiveness fix).The 916px right margin forces clipping on smaller widths and wastes space on larger screens. Stretch within the star column and use small, symmetric margins. Also,
Width="Auto"is redundant and can prevent full stretch.Apply:
- <ComboBox x:Name="xScanTypeComboBox" Width="Auto" Grid.Row="0" Grid.Column="1" Style="{StaticResource $FlatInputComboBoxStyle}" SelectionChanged="xScanTypeComboBox_SelectionChanged" Margin="0,8,916,8" /> + <ComboBox x:Name="xScanTypeComboBox" + Grid.Row="0" Grid.Column="1" + Style="{StaticResource $FlatInputComboBoxStyle}" + SelectionChanged="xScanTypeComboBox_SelectionChanged" + HorizontalAlignment="Stretch" + Margin="0,8,10,8" />
24-24: Let MultiSelect grow with the star row; replace fixed Height and massive margins with stretch + MinHeight.
Height="26"prevents growth;Margin="0,0,916,528"breaks layout at most sizes. Use stretch both ways, keep a small padding, and rely onMinHeightfor ergonomics.- <UserControlsLib:MultiSelectComboBox x:Name="xSeverityCB" Width="Auto" Grid.Row="1" Grid.Column="1" Height="26" HorizontalAlignment="Stretch" Margin="0,0,916,528"/> + <UserControlsLib:MultiSelectComboBox x:Name="xSeverityCB" + Grid.Row="1" Grid.Column="1" + HorizontalAlignment="Stretch" + VerticalAlignment="Stretch" + MinHeight="26" + Margin="0,8,10,10"/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs:1958-1970
Timestamp: 2025-08-25T09:20:17.589Z
Learning: In Ginger security testing, passive ZAP scans should be invoked with an empty testURL (ActSecurityTesting.ExecutePassiveZapScan("", act)) to aggregate alerts across the entire ZAP session. Passing a URL scopes results to that base URL. Only the active scan path should pass Driver.Url, with null/empty checks. Context: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Selenium/SeleniumDriver.cs :: ActSecurity. Confirmed by AmanPrasad43 in PR #4281.
📚 Learning: 2025-08-26T06:51:51.852Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4285
File: Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml:21-23
Timestamp: 2025-08-26T06:51:51.852Z
Learning: In the Ginger codebase, labels should have an extra space before colons in their Content strings (e.g., "Operation Type :" instead of "Operation Type:"). This is an intentional UI design choice.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml
📚 Learning: 2024-07-26T22:04:12.930Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#3429
File: Ginger/Ginger/AutomatePageLib/AddActionMenu/WindowExplorer/Common/WindowExplorerPage.xaml.cs:1123-1126
Timestamp: 2024-07-26T22:04:12.930Z
Learning: The user has removed an unnecessary empty line at 1123 in `WindowExplorerPage.xaml.cs` for better code compactness.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml
📚 Learning: 2025-06-16T10:36:01.993Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4228
File: Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs:83-84
Timestamp: 2025-06-16T10:36:01.993Z
Learning: For the `UnLockType` property in `Ginger/Ginger/Actions/ActionEditPages/ActMobileDeviceEditPage.xaml.cs`, the current naming convention with capital 'L' in the middle should be maintained per project preferences.
Applied to files:
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml
🔇 Additional comments (2)
Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml (2)
21-21: Label text is correct per project UI convention (intentional space before colon).Keeping "Operation Type :" with the extra space matches the Ginger UI standard captured in learnings. No change needed.
22-24: DPI/resize sanity check requested for layout after margin/height fixes.Please run a quick manual verification:
- Resize the window from 800px to ~1200px width and confirm both ComboBox and MultiSelect stretch without clipping.
- Test at 125% and 150% Windows scaling; ensure labels don’t overlap inputs and the MultiSelect shows all selected items without scrollbars unless content exceeds reasonable bounds.
If issues persist, consider adding Grid row/column shared size groups or a container-level Padding instead of per-control large margins.
not required to be handled
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Style