ZAP related defect fixes#4286
Merged
Merged
Conversation
Contributor
WalkthroughUI changes wire ZAP toggle and add a fail-action combo; driver enums renamed and new FailActionOnSecurityIssue property added; security testing evaluation split into Web/API variants with API path conditionally propagating failure; Zap proxy UI and processing image visibility adjusted; helper FillComboWithYesNo added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as WebServicesDriverEditPage (UI)
participant Driver as WebServicesDriver
participant Sec as ActSecurityTesting
participant Zap as ZapProxyService
rect rgba(230,245,255,0.6)
note over UI: Configure ZAP Security Testing
User->>UI: Select Vulnerability + Fail Action
UI->>Driver: Bind FailActionOnSecurityIssue
end
rect rgba(240,255,240,0.6)
note over Driver: Execute API call with optional ZAP
Driver->>Sec: HandleAPISecurityTesting(endpoint, act)
Sec->>Zap: Run scan -> EvaluateScanResultAPI(endpoint, thresholds)
Zap-->>Sec: isPassed, alerts
alt failAction == true
Sec->>Sec: Set act.Status = Passed/Failed
else failAction == false
note over Sec: Do not modify act.Status (only report)
end
Sec-->>Driver: Completion (act updated per policy)
end
sequenceDiagram
autonumber
participant UI2 as WebAgentConfigEditPage (UI)
participant Proxy as Proxy Settings
User->>UI2: Toggle "Enable ZAP"
alt Checked
UI2->>Proxy: Set AutoDetectProxy = false and disable control
UI2-->>User: Reporter.ToUser info message
else Unchecked
UI2->>Proxy: Re-enable AutoDetectProxy control
end
UI2->>UI2: ProxyPnlVisbility() refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-28T09:27:18.793ZApplied to files:
🔇 Additional comments (5)
✨ 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}
Contributor
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 (5)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
263-285: EvaluateScanResultWeb logic is inverted; fails on below-threshold alerts.For threshold-based gating, the test should fail if any alert in allowedAlertNames has count > 0 (i.e., at/above threshold). Current code fails when encountering alerts not in the allowed list (i.e., below threshold). Fix as below and make comparison case-insensitive, guarding nulls.
- public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) + public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) { var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); var alertSummary = (ApiResponseSet)summaryResponse; - bool testPassed = true; + bool testPassed = true; foreach (var alertEntry in alertSummary.Dictionary) { string alertName = alertEntry.Key; string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); - if (!int.TryParse(valueString, out int count)) + if (!int.TryParse(valueString, out int count)) { continue; } - - if (!allowedAlertNames.Any(a => a.Value == alertName)) - { - testPassed = false; - } + if (count > 0 && (allowedAlertNames?.Any(a => string.Equals(a.Value, alertName, StringComparison.OrdinalIgnoreCase)) ?? false)) + { + testPassed = false; // at-or-above threshold present -> fail + } } return testPassed; }Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
226-236: Ensure evaluation logic matches threshold semantics (fail only when count>0 for severities at/above threshold).This switch to EvaluateScanResultWeb is fine, but current ZapProxyService implementations compare allowed lists incorrectly and ignore counts. Without fixing ZapProxyService, results will be wrong.
Proposed fix in ZapProxyService.cs (outside this file):
- public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) + public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) { ... - if (!allowedAlertNames.Any(a => a.Value == alertName)) - { - testPassed = false; - } + if (count > 0 && allowedAlertNames.Any(a => a.Value == alertName)) + { + testPassed = false; // fail if severity is at/above threshold + } } - public bool EvaluateScanResultAPI(string targetUrl, ObservableList<OperationValues> allowedAlertNames) + public bool EvaluateScanResultAPI(string targetUrl, ObservableList<OperationValues> allowedAlertNames) { ... - if (allowedAlertNames.Any(a => a.Value == alertName)) + if (count > 0 && allowedAlertNames.Any(a => a.Value == alertName)) { testPassed = false; } }Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (3)
373-380: Replace TODO with an actual ZAP session reset (prevents stale alerts).Start a fresh ZAP session or clear the sites tree before each action to avoid cross-test contamination.
Example (requires ZapProxyService support):
// before starting an action new ZapProxyService().StartNewSession(overwrite: true);If preferred, add a ClearSitesTree() helper calling ZAP core endpoints.
451-466: Passive scan should pass empty URL for session-wide evaluation.Passing endpointUrl scopes results; use empty string to aggregate, per prior guidance.
Apply:
- mActSecTest.ExecutePassiveZapScan(endpointUrl, mActWebAPI); + mActSecTest.ExecutePassiveZapScan(string.Empty, mActWebAPI);
1061-1068: ToHostPort helper: solid.Use it consistently when setting WebServicesProxy in HandleWebApiRequest for parity with CreateRawRequest.
Apply (outside changed lines):
- WebServicesProxy = zAPConfiguration.ZAPUrl; + WebServicesProxy = ToHostPort(zAPConfiguration.ZAPUrl);
📜 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 (8)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml(1 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs(2 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml(1 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs(1 hunks)Ginger/Ginger/ExternalConfigurations/ZAPConfigurationPage.xaml(1 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs(4 hunks)Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs(5 hunks)Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
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.
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.608Z
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.
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreCommon/External/Configurations/ZAPConfiguration.cs:37-58
Timestamp: 2025-08-25T09:30:08.106Z
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.
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-25T09:27:30.249Z
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.249Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs
📚 Learning: 2025-06-24T06:08:31.804Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4242
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:623-656
Timestamp: 2025-06-24T06:08:31.804Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs, the CSS selector construction by directly interpolating user input values in the GetElementLocator method is considered acceptable by the team and working as expected for their use cases.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs
📚 Learning: 2025-07-16T14:42:32.229Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs:70-73
Timestamp: 2025-07-16T14:42:32.229Z
Learning: In Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs, prashelke prefers to keep the current `!WorkSpace.Instance.BetaFeatures.ShowPOMForAI` logic for binding and showing AI POM controls, rather than changing it to positive logic.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml
📚 Learning: 2025-08-26T07:40:08.345Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4285
File: Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml:23-23
Timestamp: 2025-08-26T07:40:08.345Z
Learning: In the Ginger codebase ActSecurityTestingEditPage.xaml file, the large bottom margin of 530 on the "Acceptable Alerts" label (Margin="10,10,0,530") is intentional design choice, not an error.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml
📚 Learning: 2025-08-25T09:20:17.608Z
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.608Z
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.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.csGinger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs
🧬 Code graph analysis (5)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs (2)
Ginger/GingerCore/GeneralLib/General.cs (2)
FillComboItemsFromEnumType(250-266)FillComboFromEnumType(212-248)Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (2)
WebServicesDriver(48-1109)WebServicesDriver(244-247)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (2)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
ObservableList(1070-1108)Ginger/Ginger/Actions/ActionEditPages/ActSecurityTestingEditPage.xaml.cs (1)
ObservableList(94-103)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
ExecuteApiSecurityTestWithOpenApi(321-374)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (2)
EvaluateScanResultWeb(263-285)EvaluateScanResultAPI(287-309)
🔇 Additional comments (9)
Ginger/Ginger/ExternalConfigurations/ZAPConfigurationPage.xaml (1)
51-51: Spinner defaults to Collapsed — verify it’s toggled during “Test Connection”.With the processing image defaulting to Collapsed, ensure xTestConBtn_Click sets it Visible while testing and back to Collapsed on completion/failure; otherwise users get no progress feedback.
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (1)
20-20: Import looks correct.Adding Amdocs.Ginger.Common is needed for Reporter usage. LGTM.
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml (1)
150-157: Event wiring and helper hint: good addition.Checked/Unchecked both routed to the same handler is correct; tooltip clarifies behavior. LGTM.
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
263-309: Review resolved: EvaluateScanResult call sites and alert-name mapping confirmed
All usages of EvaluateScanResultWeb/EvaluateScanResultAPI have been updated (no lingering two-arg old calls), and BuildAlertListFromThreshold’s use of nameof(eAlertTypes.X) yields “High”, “Medium”, “Low”, “Informational” (and “FalsePositive” for that threshold), matching ZAP’s summary keys.Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
314-321: XML doc: Good context.Clear rationale for the optional parameter; no issues.
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (4)
181-183: Enum extension looks good.Adding None is sensible and default is wired to None below.
476-478: Copying execution attributes after security tests: good move.Ensures security-test artifacts and errors propagate to the model action.
495-503: Wiring failAction through to the API scan: looks correct.Call site aligns with the new signature.
1070-1108: Threshold builder comment vs. evaluator semantics – verify alignment.Builder returns severities “at/above” the threshold. Ensure EvaluateScanResultWeb/API fail when any of these have count>0 (see suggested ZapProxyService fix).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (4)
24-30: Guard null port/host; default sensibly to avoid InvalidCastException.Casting a null port to int will throw if ZAP URL has no explicit port. Default to 8080 and a safe host.
- _zapHost = GetHostFromUrl(ValueExpression.PasswordCalculation(zAPConfiguration.ZAPUrl)); - _zapPort = (int)GetPortFromUrl(ValueExpression.PasswordCalculation(zAPConfiguration.ZAPUrl)); + var url = ValueExpression.PasswordCalculation(zAPConfiguration.ZAPUrl); + _zapHost = GetHostFromUrl(url) ?? "localhost"; + var port = GetPortFromUrl(url); + _zapPort = port ?? 8080;
51-68: Remove async-without-await; keep synchronous semantics.Method runs synchronously yet declared async. Return a completed Task to remove CS1998 and preserve current blocking behavior.
- public async Task WaitTillPassiveScanCompleted() + public Task WaitTillPassiveScanCompleted() { try { IApiResponse apiResponse = _zapClient.pscan.recordsToScan(); string tempVal = ((ApiResponseElement)apiResponse).Value; while (!tempVal.Equals("0")) { - Thread.Sleep(1000); + Thread.Sleep(1000); apiResponse = _zapClient.pscan.recordsToScan(); tempVal = ((ApiResponseElement)apiResponse).Value; } + return Task.CompletedTask; } catch (Exception ex) { throw new InvalidOperationException("Error during passive scan: " + ex.Message, ex); } }
263-285: EvaluateScanResultWeb logic inverted; ignore count and case sensitivity — align with API variant.Fail only when count > 0 and the alert is within the allowed threshold list; compare case-insensitively and normalize “False Positive”/“FalsePositive”.
- public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) + public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) { var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); var alertSummary = (ApiResponseSet)summaryResponse; - - bool testPassed = true; - - foreach (var alertEntry in alertSummary.Dictionary) - { - string alertName = alertEntry.Key; - string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); - if (!int.TryParse(valueString, out int count)) - { - continue; - } - - if (!allowedAlertNames.Any(a => a.Value == alertName)) - { - testPassed = false; - } - } - return testPassed; + string Normalize(string s) => s?.Replace(" ", "").Replace("-", "").Trim(); + var allowed = new HashSet<string>( + (allowedAlertNames ?? []).Select(a => Normalize(a.Value)), + StringComparer.OrdinalIgnoreCase); + + foreach (var alertEntry in alertSummary.Dictionary) + { + string alertName = alertEntry.Key; + string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); + if (!int.TryParse(valueString, out int count) || count <= 0) continue; + if (allowed.Contains(Normalize(alertName))) return false; + } + return true; }
311-317: Legacy EvaluateScanResult always “true” for non-empty response; compute totals instead.Return pass only when all counts are zero.
public bool EvaluateScanResult(string targetUrl) { var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); var alertSummary = (ApiResponseSet)summaryResponse; - return !string.IsNullOrEmpty(alertSummary.ToString()); + int total = 0; + foreach (var e in alertSummary.Dictionary) + { + var v = e.Value is ApiResponseElement el ? el.Value : e.Value?.ToString(); + if (int.TryParse(v, out var c)) total += c; + } + return total == 0; }Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (2)
442-467: Compile-time bug (string.Equals with enum) and duplicated condition; also aggregate passive scans.Use enum comparison, drop duplicate inner check, and aggregate passive scans via empty URL.
- if (string.Equals(this.mZapScanTypeSetting, eZapScanType.Passive)) + if (this.mZapScanTypeSetting == eZapScanType.Passive) { string endpointUrl = mActWebAPI.GetInputParamValue(ActWebAPIBase.Fields.EndPointURL); if (string.IsNullOrWhiteSpace(endpointUrl)) { Reporter.ToLog(eLogLevel.WARN, "ZAP scan skipped: endpoint URL is empty"); } - else if (this.mZapScanTypeSetting == eZapScanType.Passive) - { - var mActSecTest = new ActSecurityTesting - { - AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) - }; - mActSecTest.ExecutePassiveZapScan(endpointUrl, mActWebAPI); - } + else + { + var mActSecTest = new ActSecurityTesting + { + AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) + }; + mActSecTest.ExecutePassiveZapScan(string.Empty, mActWebAPI); + } } else { var mActSecTest = new ActSecurityTesting { AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) }; HandleAPISecurityTesting(mActWebAPI, mActSecTest); }
603-607: Normalize ZAP proxy to host:port consistently.CreateRawRequest uses ToHostPort; HandleWebApiRequest should too to avoid passing a full URL where host:port is expected.
- ZAPConfiguration zAPConfiguration = WorkSpace.Instance.SolutionRepository.GetAllRepositoryItems<ZAPConfiguration>().Count == 0 ? new ZAPConfiguration() : WorkSpace.Instance.SolutionRepository.GetFirstRepositoryItem<ZAPConfiguration>(); - WebServicesProxy = zAPConfiguration.ZAPUrl; + ZAPConfiguration zAPConfiguration = WorkSpace.Instance.SolutionRepository.GetAllRepositoryItems<ZAPConfiguration>().Count == 0 ? new ZAPConfiguration() : WorkSpace.Instance.SolutionRepository.GetFirstRepositoryItem<ZAPConfiguration>(); + WebServicesProxy = ToHostPort(zAPConfiguration.ZAPUrl);
♻️ Duplicate comments (3)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (1)
556-571: Initialize persisted ZAP state on load; avoid showing the info modal during init.
On reload with UseSecurityTesting=true, the UI won’t reflect the disabled Auto Detect state unless the handler is invoked. Also, calling the handler during init would pop the info dialog. Extract the state application into a helper and call it without the dialog during init.Apply this focused diff within this block:
- private void xEnableZAPCheckBox_Checked(object sender, RoutedEventArgs e) - { - if (xEnableZAPCheckBox.IsChecked == true) - { - // Disable AutoDetect Proxy and set manual proxy as required - xAutoDetectProxyCB.IsChecked = false; - xAutoDetectProxyCB.IsEnabled = false; - Reporter.ToUser(eUserMsgKey.StaticInfoMessage, "Enabling Security Testing will disable the Auto Detect Proxy. All configured proxy settings will be overridden by the ZAP Proxy"); - ProxyPnlVisbility(); - } - else - { - xAutoDetectProxyCB.IsEnabled = true; - ProxyPnlVisbility(); - } - } + private void ApplyZAPUIState(bool showInfoMessage) + { + if (xEnableZAPCheckBox.IsChecked == true) + { + // Disable Auto Detect and require manual proxy (UI stays enabled) + xAutoDetectProxyCB.IsChecked = false; + xAutoDetectProxyCB.IsEnabled = false; + if (showInfoMessage) + { + Reporter.ToUser( + eUserMsgKey.StaticInfoMessage, + "Enabling Security Testing disables Auto Detect Proxy. All configured proxy settings will be overridden by the ZAP proxy." + ); + } + ProxyPnlVisbility(); + } + else + { + xAutoDetectProxyCB.IsEnabled = true; + ProxyPnlVisbility(); + } + } + + private void xEnableZAPCheckBox_Checked(object sender, RoutedEventArgs e) + { + ApplyZAPUIState(showInfoMessage: true); + }Then, after bindings complete in the constructor, call:
// Reflect persisted ZAP state without a modal on load ApplyZAPUIState(showInfoMessage: false);Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
279-285: Passive-scan scope: use empty URL to aggregate session alerts.Per team guidance, passive summaries should be session-wide, not scoped to endpointUrl.
- ProcessResultAsync(zapProxyService, act, testURL); - AddZapAlertOutputValues(zapProxyService, act, testURL); - isPassed = zapProxyService.EvaluateScanResultWeb(testURL, AlertList); + // Aggregate across the entire ZAP session + ProcessResultAsync(zapProxyService, act, string.Empty); + AddZapAlertOutputValues(zapProxyService, act, string.Empty); + isPassed = zapProxyService.EvaluateScanResultWeb(string.Empty, AlertList);Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs (1)
149-152: FillComboItemsFromEnumType sort bug; bind Text for string-backed params.FillComboItemsFromEnumType sorts by “text” on ComboBoxItem, which will throw; also SelectedValue (enum) ↔ string binding risks type mismatch. Use FillComboFromEnumType and bind Text.
- GingerCore.General.FillComboItemsFromEnumType(xVulnerabilityTypeComboBox, typeof(WebServicesDriver.eZapVulnerability)); + GingerCore.General.FillComboFromEnumType(xVulnerabilityTypeComboBox, typeof(WebServicesDriver.eZapVulnerability)); DriverConfigParam zapVulnerability = mAgent.GetOrCreateParam(nameof(WebServicesDriver.ZapVulnerabilitySetting)); - BindingHandler.ObjFieldBinding(xVulnerabilityTypeComboBox, ComboBox.SelectedValueProperty, zapVulnerability, nameof(DriverConfigParam.Value)); + BindingHandler.ObjFieldBinding(xVulnerabilityTypeComboBox, ComboBox.TextProperty, zapVulnerability, nameof(DriverConfigParam.Value));
📜 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 (5)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs(2 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs(1 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs(4 hunks)Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs(5 hunks)Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs:556-571
Timestamp: 2025-08-28T09:27:18.793Z
Learning: In Ginger's WebAgentConfigEditPage.xaml.cs ZAP configuration, when ZAP Security Testing is enabled, the manual proxy UI should remain enabled/accessible even though ZAP will override the proxy settings in the background. This allows users to see and modify their proxy configurations for transparency and future use when ZAP is disabled. Confirmed by AmanPrasad43 in PR #4286.
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.608Z
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-28T09:27:18.793Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs:556-571
Timestamp: 2025-08-28T09:27:18.793Z
Learning: In Ginger's WebAgentConfigEditPage.xaml.cs ZAP configuration, when ZAP Security Testing is enabled, the manual proxy UI should remain enabled/accessible even though ZAP will override the proxy settings in the background. This allows users to see and modify their proxy configurations for transparency and future use when ZAP is disabled. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T09:27:30.249Z
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.249Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-25T09:20:17.608Z
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.608Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-08-25T09:30:08.106Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreCommon/External/Configurations/ZAPConfiguration.cs:37-58
Timestamp: 2025-08-25T09:30:08.106Z
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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.csGinger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.cs
📚 Learning: 2025-07-16T14:42:32.229Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4254
File: Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs:70-73
Timestamp: 2025-07-16T14:42:32.229Z
Learning: In Ginger/Ginger/ApplicationModelsLib/POMModels/POMWizardLib/LearnWizard/POMLearnConfigWizardPage.xaml.cs, prashelke prefers to keep the current `!WorkSpace.Instance.BetaFeatures.ShowPOMForAI` logic for binding and showing AI POM controls, rather than changing it to positive logic.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.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/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs
📚 Learning: 2025-06-24T06:08:31.804Z
Learnt from: GokulBothe99
PR: Ginger-Automation/Ginger#4242
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs:623-656
Timestamp: 2025-06-24T06:08:31.804Z
Learning: In Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs, the CSS selector construction by directly interpolating user input values in the GetElementLocator method is considered acceptable by the team and working as expected for their use cases.
Applied to files:
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs
📚 Learning: 2025-06-19T05:08:45.192Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4222
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:2645-2656
Timestamp: 2025-06-19T05:08:45.192Z
Learning: In the Ginger mobile automation framework (GenericAppiumDriver.cs), XPath checks for "android" and "XCUIElement" should remain case-sensitive as currently implemented. The team prefers not to make these checks case-insensitive.
Applied to files:
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs
📚 Learning: 2025-08-28T09:29:59.108Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs:149-152
Timestamp: 2025-08-28T09:29:59.108Z
Learning: In Ginger's WebServicesDriverEditPage.xaml.cs, the FillComboItemsFromEnumType method should be used instead of FillComboFromEnumType when sorting by enum order is required. FillComboItemsFromEnumType preserves EnumValueDescription display through WPF's automatic ToString() calling on enum Content, and provides built-in sorting functionality via SortDescriptions.Add("text", Ascending). This was confirmed by AmanPrasad43 in PR #4286 for the ZAP vulnerability type dropdown.
Applied to files:
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
🧬 Code graph analysis (5)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (1)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (2)
EvaluateScanResultWeb(263-285)EvaluateScanResultAPI(287-309)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs (4)
Ginger/GingerCore/GeneralLib/InputBoxWindow.xaml.cs (1)
ObjFieldBinding(147-157)Ginger/GingerCoreNET/GeneralLib/General.cs (1)
General(58-1481)Ginger/GingerCore/GeneralLib/General.cs (2)
FillComboItemsFromEnumType(250-266)FillComboFromEnumType(212-248)Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (2)
WebServicesDriver(48-1102)WebServicesDriver(237-240)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
ExecuteApiSecurityTestWithOpenApi(321-374)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
ObservableList(1063-1101)
🔇 Additional comments (7)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs (3)
20-20: LGTM: required namespace for Reporter/eUserMsgKey.
Import is appropriate and used below.
560-565: Good: manual proxy UI remains accessible while ZAP is enabled.
This matches the team’s UX decision (retain visibility/editability even if ZAP overrides at runtime).
556-571: Resolved:xEnableZAPCheckBoxhas bothCheckedandUncheckedbound toxEnableZAPCheckBox_CheckedinWebAgentConfigEditPage.xaml.Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (2)
339-347: failAction handling — looks good.Case-insensitive comparison and conditional status assignment are correct.
222-238: Active path eval method alignment.Once ZapProxyService.EvaluateScanResultWeb is fixed (count>0 + case-insensitive), this call site is fine. No changes needed here.
To be safe, run an API scan producing a “False Positive” and confirm it is counted after normalization. Do you want a small harness?
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs (1)
154-157: Rename combobox and bind TextProperty
- In WebServicesDriverEditPage.xaml (≈line 177) and code-behind (lines 154–157), rename
xActionFaliureSecurityComboBox→xActionFailureSecurityComboBox.- In code-behind, change the value binding to use
ComboBox.TextProperty(notSelectedValueProperty) foractionFailOnSecurityIssue.⛔ Skipped due to learnings
Learnt from: AmanPrasad43 PR: Ginger-Automation/Ginger#4286 File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs:149-152 Timestamp: 2025-08-28T09:29:59.108Z Learning: In Ginger's WebServicesDriverEditPage.xaml.cs, the FillComboItemsFromEnumType method should be used instead of FillComboFromEnumType when sorting by enum order is required. FillComboItemsFromEnumType preserves EnumValueDescription display through WPF's automatic ToString() calling on enum Content, and provides built-in sorting functionality via SortDescriptions.Add("text", Ascending). This was confirmed by AmanPrasad43 in PR #4286 for the ZAP vulnerability type dropdown.Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
174-187: Enum ordering change — confirm default “None” remains last in UI lists.You moved None to the end; ensure UI population doesn’t re-sort alphabetically, otherwise the intended order may be lost.
Would you like a quick check across FillCombo* helpers to ensure order stability?
Maheshkale447
requested changes
Aug 29, 2025
added 2 commits
August 29, 2025 15:14
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
321-344: SetStatusafter scan to avoid hanging
ExecuteApiSecurityTestingsetsStatus = eRunStatus.Runningbut never updates it on completion, leaving the action stuck in “Running.” After computingisPassed, addStatus = isPassed ? eRunStatus.Passed : eRunStatus.Failed;to ensure the action reaches a terminal state.
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (3)
443-469: Fix enum comparison (compile-time issue) and correct passive-scan invocation.
- string.Equals is used with enums; this won’t compile. Use direct enum comparison.
- Passive scans should aggregate alerts across the entire session: call ExecutePassiveZapScan("") rather than scoping to the endpoint URL (per team guidance).
Apply:- if (string.Equals(this.mZapScanTypeSetting, eSecurityTestingScanType.Passive)) - { - string endpointUrl = mActWebAPI.GetInputParamValue(ActWebAPIBase.Fields.EndPointURL); - if (string.IsNullOrWhiteSpace(endpointUrl)) - { - Reporter.ToLog(eLogLevel.WARN, "ZAP scan skipped: endpoint URL is empty"); - } - else if (this.mZapScanTypeSetting == eSecurityTestingScanType.Passive) - { - var mActSecTest = new ActSecurityTesting - { - AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) - }; - mActSecTest.ExecutePassiveZapScan(endpointUrl, mActWebAPI); - } - } + if (mZapScanTypeSetting == eSecurityTestingScanType.Passive) + { + var mActSecTest = new ActSecurityTesting + { + AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) + }; + // Passive: aggregate across the ZAP session + mActSecTest.ExecutePassiveZapScan(string.Empty, mActWebAPI); + } else { var mActSecTest = new ActSecurityTesting { AlertList = BuildAlertListFromThreshold(mZapVulnerabilitySetting) }; HandleAPISecurityTesting(mActWebAPI, mActSecTest); }
604-612: Ensure proxy string is host:port and don’t bypass ZAP via OS proxy.
- Here you set WebServicesProxy to the full URL; RequestConstructor expects host:port (per property doc).
- If UseServerProxySettings is true, ZAP will be bypassed. Force local-proxy off when UseSecurityTesting is on.
Apply:
- if (this.UseSecurityTesting) - { - ZAPConfiguration zAPConfiguration = WorkSpace.Instance.SolutionRepository.GetAllRepositoryItems<ZAPConfiguration>().Count == 0 ? new ZAPConfiguration() : WorkSpace.Instance.SolutionRepository.GetFirstRepositoryItem<ZAPConfiguration>(); - WebServicesProxy = zAPConfiguration.ZAPUrl; - } - //Call for Request Construction - if (mWebAPI.RequestConstructor(act, WebServicesProxy, UseServerProxySettings)) + if (UseSecurityTesting) + { + ZAPConfiguration zAPConfiguration = WorkSpace.Instance.SolutionRepository.GetAllRepositoryItems<ZAPConfiguration>().Count == 0 ? new ZAPConfiguration() : WorkSpace.Instance.SolutionRepository.GetFirstRepositoryItem<ZAPConfiguration>(); + WebServicesProxy = ToHostPort(zAPConfiguration.ZAPUrl); + } + //Call for Request Construction + var useServerProxy = UseSecurityTesting ? false : UseServerProxySettings; + if (mWebAPI.RequestConstructor(act, WebServicesProxy, useServerProxy))
1064-1103: Threshold-to-alert list mapping is fine; optional simplification.Current mapping is explicit and clear. Optionally compress using a fall-through or array index approach to reduce repetition.
♻️ Duplicate comments (1)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml (1)
169-177: Grammar/UX: simplify label; remove awkward punctuation.Current: “Fail action if selected or higher vulnerability is found ? :”.
Suggested: concise, no stray spaces.- <Label Content="Fail action if selected or higher vulnerability is found ? :" Style="{StaticResource $LabelStyle}" Grid.Column="0" Grid.Row="2" HorizontalAlignment="Left"/> + <Label Content="Fail action on selected (or higher) vulnerability?" Style="{StaticResource $LabelStyle}" Grid.Column="0" Grid.Row="2" HorizontalAlignment="Left"/>
📜 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 (6)
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml(2 hunks)Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs(1 hunks)Ginger/GingerCore/GeneralLib/General.cs(2 hunks)Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs(4 hunks)Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs(6 hunks)Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs:556-571
Timestamp: 2025-08-28T09:27:18.793Z
Learning: In Ginger's WebAgentConfigEditPage.xaml.cs ZAP configuration, when ZAP Security Testing is enabled, the manual proxy UI should remain enabled/accessible even though ZAP will override the proxy settings in the background. This allows users to see and modify their proxy configurations for transparency and future use when ZAP is disabled. Confirmed by AmanPrasad43 in PR #4286.
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.608Z
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-25T09:20:17.608Z
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.608Z
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/ActionsLib/UI/Web/ActSecurityTesting.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-29T09:35:45.994Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs:287-309
Timestamp: 2025-08-29T09:35:45.994Z
Learning: In Ginger ZAP security testing, alert names should preserve their original formatting with spaces and hyphens. When comparing alert names in EvaluateScanResultAPI and similar methods, use StringComparison.OrdinalIgnoreCase for case-insensitive matching but do not normalize by removing spaces or hyphens, as alert names like "False Positive" need to remain as separate words. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-25T08:48:11.915Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCore/GingerCore.csproj:538-538
Timestamp: 2025-08-25T08:48:11.915Z
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/ActionsLib/UI/Web/ActSecurityTesting.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-06-19T05:08:45.192Z
Learnt from: prashelke
PR: Ginger-Automation/Ginger#4222
File: Ginger/GingerCoreNET/Drivers/CoreDrivers/Mobile/Appium/GenericAppiumDriver.cs:2645-2656
Timestamp: 2025-06-19T05:08:45.192Z
Learning: In the Ginger mobile automation framework (GenericAppiumDriver.cs), XPath checks for "android" and "XCUIElement" should remain case-sensitive as currently implemented. The team prefers not to make these checks case-insensitive.
Applied to files:
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs
📚 Learning: 2025-08-25T09:30:08.106Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4281
File: Ginger/GingerCoreCommon/External/Configurations/ZAPConfiguration.cs:37-58
Timestamp: 2025-08-25T09:30:08.106Z
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/ActionsLib/UI/Web/ActSecurityTesting.csGinger/GingerCoreNET/External/ZAP/ZapProxyService.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-28T09:29:59.108Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs:149-152
Timestamp: 2025-08-28T09:29:59.108Z
Learning: In Ginger's WebServicesDriverEditPage.xaml.cs, the FillComboItemsFromEnumType method should be used instead of FillComboFromEnumType when sorting by enum order is required. FillComboItemsFromEnumType preserves EnumValueDescription display through WPF's automatic ToString() calling on enum Content, and provides built-in sorting functionality via SortDescriptions.Add("text", Ascending). This was confirmed by AmanPrasad43 in PR #4286 for the ZAP vulnerability type dropdown.
Applied to files:
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-28T09:27:18.793Z
Learnt from: AmanPrasad43
PR: Ginger-Automation/Ginger#4286
File: Ginger/Ginger/Drivers/DriversConfigsEditPages/WebAgentConfigEditPage.xaml.cs:556-571
Timestamp: 2025-08-28T09:27:18.793Z
Learning: In Ginger's WebAgentConfigEditPage.xaml.cs ZAP configuration, when ZAP Security Testing is enabled, the manual proxy UI should remain enabled/accessible even though ZAP will override the proxy settings in the background. This allows users to see and modify their proxy configurations for transparency and future use when ZAP is disabled. Confirmed by AmanPrasad43 in PR #4286.
Applied to files:
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.csGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xamlGinger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.csGinger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2025-08-25T09:27:30.249Z
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.249Z
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/Drivers/WebServicesDriver/WebServicesDriver.cs
📚 Learning: 2024-07-08T13:56:50.209Z
Learnt from: IamRanjeetSingh
PR: Ginger-Automation/Ginger#3811
File: Ginger/GingerCoreCommon/UIElement/AppWindow.cs:28-28
Timestamp: 2024-07-08T13:56:50.209Z
Learning: When updating enum values, ensure to check both commented and uncommented code for consistency.
Applied to files:
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs
🧬 Code graph analysis (3)
Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (1)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (2)
EvaluateScanResultWeb(263-293)EvaluateScanResultAPI(295-325)
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (2)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (1)
ObservableList(1064-1102)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (2)
Ginger/GingerCoreCommon/Actions/Webservices/ActWebAPIBase.cs (3)
ActWebAPIBase(37-408)ActWebAPIBase(39-43)Fields(71-94)Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs (3)
Fields(43-46)ExecuteApiSecurityTesting(321-374)ActSecurityTesting(41-417)
🔇 Additional comments (7)
Ginger/GingerCore/GeneralLib/General.cs (1)
1571-1571: LGTM — formatting only.No behavioral change.
Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs (1)
295-325: API path hardening looks good.Count>0 check, null-guard, and OrdinalIgnoreCase match align with requirements.
Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml.cs (2)
154-158: Good: boolean SelectedValue binding for Yes/No.Binding SelectedValue to FailActionOnSecurityIssue with a Yes/No list avoids string parsing and aligns with the driver bool.
149-152: RetainFillComboItemsFromEnumType; it preserves the enum descriptions and provides correct enum‐order sorting.Likely an incorrect or invalid review comment.
Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs (3)
176-202: Vulnerability enum reordering and descriptions look correct.Ordering High→...→None with EnumValueDescription matches UI needs.
471-472: Good: copy execution attributes post security-testing.Keeps ActWebAPIModel in sync with underlying execution results.
496-497: API security test wiring is correct; behavior depends on the fixed default.Passing FailActionOnSecurityIssue through is right; ensure the default fix above lands to avoid surprising failures.
Comment on lines
+177
to
+183
| <ComboBox x:Name="xActionFailureSecurityComboBox" IsEditable="False" Style="{StaticResource $FlatInputComboBoxStyle}" Grid.Column="1" Grid.Row="2" Width="120" HorizontalAlignment="Left" SelectedValuePath="Value" SelectedValue="{Binding FailActionOnSecurityIssue, Mode=TwoWay}" > | ||
| <ComboBox.ItemTemplate> | ||
| <DataTemplate> | ||
| <TextBlock Text="{Binding Key}" /> | ||
| </DataTemplate> | ||
| </ComboBox.ItemTemplate> | ||
| </ComboBox> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer DisplayMemberPath over ItemTemplate for simple KeyValue pairs.
Since items are KeyValuePair<string,bool>, you can drop the template and rely on DisplayMemberPath/SelectedValuePath (already set in XAML or via helper) to reduce XAML noise.
- <ComboBox x:Name="xActionFailureSecurityComboBox" IsEditable="False" Style="{StaticResource $FlatInputComboBoxStyle}" Grid.Column="1" Grid.Row="2" Width="120" HorizontalAlignment="Left" SelectedValuePath="Value" SelectedValue="{Binding FailActionOnSecurityIssue, Mode=TwoWay}" >
- <ComboBox.ItemTemplate>
- <DataTemplate>
- <TextBlock Text="{Binding Key}" />
- </DataTemplate>
- </ComboBox.ItemTemplate>
- </ComboBox>
+ <ComboBox x:Name="xActionFailureSecurityComboBox" IsEditable="False" Style="{StaticResource $FlatInputComboBoxStyle}" Grid.Column="1" Grid.Row="2" Width="120" HorizontalAlignment="Left" DisplayMemberPath="Key" SelectedValuePath="Value" SelectedValue="{Binding FailActionOnSecurityIssue, Mode=TwoWay}" />📝 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
| <ComboBox x:Name="xActionFailureSecurityComboBox" IsEditable="False" Style="{StaticResource $FlatInputComboBoxStyle}" Grid.Column="1" Grid.Row="2" Width="120" HorizontalAlignment="Left" SelectedValuePath="Value" SelectedValue="{Binding FailActionOnSecurityIssue, Mode=TwoWay}" > | |
| <ComboBox.ItemTemplate> | |
| <DataTemplate> | |
| <TextBlock Text="{Binding Key}" /> | |
| </DataTemplate> | |
| </ComboBox.ItemTemplate> | |
| </ComboBox> | |
| <ComboBox x:Name="xActionFailureSecurityComboBox" | |
| IsEditable="False" | |
| Style="{StaticResource $FlatInputComboBoxStyle}" | |
| Grid.Column="1" | |
| Grid.Row="2" | |
| Width="120" | |
| HorizontalAlignment="Left" | |
| DisplayMemberPath="Key" | |
| SelectedValuePath="Value" | |
| SelectedValue="{Binding FailActionOnSecurityIssue, Mode=TwoWay}" /> |
🤖 Prompt for AI Agents
In Ginger/Ginger/Drivers/DriversConfigsEditPages/WebServicesDriverEditPage.xaml
around lines 177-183, the ComboBox uses an ItemTemplate to display KeyValuePair
items; replace the ItemTemplate with DisplayMemberPath="Key" (keeping
SelectedValuePath="Value") to simplify XAML. Remove the DataTemplate block and
add DisplayMemberPath="Key" to the ComboBox declaration so the Key is shown and
the Value remains the SelectedValue.
Comment on lines
+355
to
+364
| //Use this When You need Yes/No to show on UI but need to save boolean in backend | ||
| public static void FillComboWithYesNo(ComboBox combo) | ||
| { | ||
|
|
||
| combo.ItemsSource = new List<KeyValuePair<string, bool>> | ||
| { | ||
| new("Yes", true), | ||
| new("No", false) | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make helper self-contained: set DisplayMemberPath/SelectedValuePath.
Set these in the helper so callers don’t need to wire an ItemTemplate or paths every time; it’s safe even when a template is provided.
public static void FillComboWithYesNo(ComboBox combo)
{
-
- combo.ItemsSource = new List<KeyValuePair<string, bool>>
+ combo.DisplayMemberPath = "Key";
+ combo.SelectedValuePath = "Value";
+ combo.ItemsSource = new List<KeyValuePair<string, bool>>
{
new("Yes", true),
new("No", 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
| //Use this When You need Yes/No to show on UI but need to save boolean in backend | |
| public static void FillComboWithYesNo(ComboBox combo) | |
| { | |
| combo.ItemsSource = new List<KeyValuePair<string, bool>> | |
| { | |
| new("Yes", true), | |
| new("No", false) | |
| }; | |
| } | |
| //Use this When You need Yes/No to show on UI but need to save boolean in backend | |
| public static void FillComboWithYesNo(ComboBox combo) | |
| { | |
| combo.DisplayMemberPath = "Key"; | |
| combo.SelectedValuePath = "Value"; | |
| combo.ItemsSource = new List<KeyValuePair<string, bool>> | |
| { | |
| new("Yes", true), | |
| new("No", false) | |
| }; | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCore/GeneralLib/General.cs around lines 355 to 364, the
FillComboWithYesNo helper populates ItemsSource but does not set
DisplayMemberPath and SelectedValuePath; update the method to set
combo.DisplayMemberPath = "Key" and combo.SelectedValuePath = "Value" so callers
don’t need to wire an ItemTemplate or value paths each time and the combo will
display "Yes"/"No" while exposing the boolean as SelectedValue.
Comment on lines
281
to
285
| ProcessResultAsync(zapProxyService, act, testURL); | ||
| AddZapAlertOutputValues(zapProxyService, act, testURL); | ||
| isPassed = zapProxyService.EvaluateScanResult(testURL, AlertList); | ||
| isPassed = zapProxyService.EvaluateScanResultWeb(testURL, AlertList); | ||
|
|
||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Passive scan should aggregate at session level (empty URL).
Per team practice, pass empty string to aggregate passive alerts across the ZAP session; using testURL scopes results.
- ProcessResultAsync(zapProxyService, act, testURL);
- AddZapAlertOutputValues(zapProxyService, act, testURL);
- isPassed = zapProxyService.EvaluateScanResultWeb(testURL, AlertList);
+ ProcessResultAsync(zapProxyService, act, testURL);
+ AddZapAlertOutputValues(zapProxyService, act, string.Empty);
+ isPassed = zapProxyService.EvaluateScanResultWeb(string.Empty, AlertList);📝 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
| ProcessResultAsync(zapProxyService, act, testURL); | |
| AddZapAlertOutputValues(zapProxyService, act, testURL); | |
| isPassed = zapProxyService.EvaluateScanResult(testURL, AlertList); | |
| isPassed = zapProxyService.EvaluateScanResultWeb(testURL, AlertList); | |
| } | |
| ProcessResultAsync(zapProxyService, act, testURL); | |
| - AddZapAlertOutputValues(zapProxyService, act, testURL); | |
| AddZapAlertOutputValues(zapProxyService, act, string.Empty); | |
| isPassed = zapProxyService.EvaluateScanResultWeb(string.Empty, AlertList); |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/ActionsLib/UI/Web/ActSecurityTesting.cs around lines
281-285, the code passes testURL to the ZAP alert aggregation calls which scopes
alerts to that URL; to aggregate passive alerts at the session level, change the
calls that aggregate/evaluate alerts to use an empty string (e.g., string.Empty)
instead of testURL — specifically pass an empty string to
AddZapAlertOutputValues and to zapProxyService.EvaluateScanResultWeb so passive
alerts are aggregated across the session (leave ProcessResultAsync as-is if it
must remain URL-scoped).
Comment on lines
+157
to
174
| public enum eSecurityTestingScanType | ||
| { | ||
| Active = 0, | ||
| Passive = 1 | ||
| [EnumValueDescription("Active")] | ||
| Active, | ||
| [EnumValueDescription("Passive")] | ||
| Passive | ||
| } | ||
| [UserConfigured] | ||
| [UserConfiguredDefault("Passive")] | ||
| [UserConfiguredDescription("ZAP Scan Type: Active or Passive")] | ||
| public string ZapScanTypeSetting { get; set; } | ||
| private eZapScanType mZapScanTypeSetting | ||
| private eSecurityTestingScanType mZapScanTypeSetting | ||
| { | ||
| get | ||
| { | ||
| return Enum.TryParse<eZapScanType>(ZapScanTypeSetting, true, out var v) ? v : eZapScanType.Passive; | ||
| return Enum.TryParse<eSecurityTestingScanType>(ZapScanTypeSetting, true, out var v) ? v : eSecurityTestingScanType.Passive; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enum rename looks good; consider aligning the backing property name.
eSecurityTestingScanType is fine. Optional: rename ZapScanTypeSetting -> SecurityTestingScanTypeSetting for consistency (defer if it affects serialization/back-compat).
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs around
lines 157–174, the private backing property name (mZapScanTypeSetting) is
inconsistent with the suggested public setting rename; update names to be
consistent: if you rename the public ZapScanTypeSetting to
SecurityTestingScanTypeSetting, also rename the private property to
mSecurityTestingScanTypeSetting and update its getter to parse
SecurityTestingScanTypeSetting; otherwise rename the private property to
mZapScanTypeSetting -> mSecurityTestingScanTypeSetting to match the enum name
and adjust all internal references accordingly, ensuring the Enum.TryParse uses
the correct public property and the default remains Passive.
Comment on lines
+157
to
202
| public enum eSecurityTestingScanType | ||
| { | ||
| Active = 0, | ||
| Passive = 1 | ||
| [EnumValueDescription("Active")] | ||
| Active, | ||
| [EnumValueDescription("Passive")] | ||
| Passive | ||
| } | ||
| [UserConfigured] | ||
| [UserConfiguredDefault("Passive")] | ||
| [UserConfiguredDescription("ZAP Scan Type: Active or Passive")] | ||
| public string ZapScanTypeSetting { get; set; } | ||
| private eZapScanType mZapScanTypeSetting | ||
| private eSecurityTestingScanType mZapScanTypeSetting | ||
| { | ||
| get | ||
| { | ||
| return Enum.TryParse<eZapScanType>(ZapScanTypeSetting, true, out var v) ? v : eZapScanType.Passive; | ||
| return Enum.TryParse<eSecurityTestingScanType>(ZapScanTypeSetting, true, out var v) ? v : eSecurityTestingScanType.Passive; | ||
| } | ||
| } | ||
|
|
||
| public enum eZapVulnerability | ||
| public enum eSecurityTestingVulnerability | ||
| { | ||
| None, | ||
| [EnumValueDescription("High")] | ||
| High, | ||
| [EnumValueDescription("Medium")] | ||
| Medium, | ||
| [EnumValueDescription("Low")] | ||
| Low, | ||
| [EnumValueDescription("Informational")] | ||
| Informational, | ||
| [EnumValueDescription("False Positive")] | ||
| FalsePositive | ||
| FalsePositive, | ||
| [EnumValueDescription("None")] | ||
| None, | ||
| } | ||
|
|
||
| [UserConfigured] | ||
| [UserConfiguredDefault("None")] | ||
| [UserConfiguredDescription("ZAP Vulnerability Level: None, High, Medium, Low, Informational, False Positive")] | ||
| public string ZapVulnerabilitySetting { get; set; } | ||
| private eZapVulnerability mZapVulnerabilitySetting | ||
| private eSecurityTestingVulnerability mZapVulnerabilitySetting | ||
| { | ||
| get | ||
| { | ||
| return Enum.TryParse<eZapVulnerability>(ZapVulnerabilitySetting, true, out var v) ? v : eZapVulnerability.None; | ||
| return Enum.TryParse<eSecurityTestingVulnerability>(ZapVulnerabilitySetting, true, out var v) ? v : eSecurityTestingVulnerability.None; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm UI bindings and persisted values after enum/type changes.
- UI: ensure WebServicesDriverEditPage uses FillComboItemsFromEnumType for the new enums and Yes/No binding for the boolean.
- Back-compat: verify persisted strings still parse to the new enums (e.g., “FalsePositive”, “Informational”, “None”).
Run:
Also applies to: 604-612
🏁 Script executed:
#!/bin/bash
# Check for old enum usages left behind
rg -nP '\beZap(S|V)\w*' --type cs -C2
# Spot any boolean defaults specified as Yes/No (should be true/false)
rg -nP '\[UserConfiguredDefault\("(?i:yes|no)"\)\]' --type cs -C2
# Check that EditPages bind to the new enums and Yes/No UI helper
rg -nP 'WebServicesDriverEditPage|FillComboItemsFromEnumType|FillComboWithYesNo' --type cs -C2Length of output: 8096
Bind ScanType enum and correct boolean default
- WebServicesDriverEditPage.xaml.cs: insert
GingerCore.General.FillComboItemsFromEnumType(xScanTypeComboBox, typeof(WebServicesDriver.eSecurityTestingScanType));
immediately before bindingxScanTypeComboBox. - WebServicesDriver.cs (line 205): change
[UserConfiguredDefault("No")]→[UserConfiguredDefault("False")]
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs around
lines 157-202 and specifically at line ~205, and in
WebServicesDriverEditPage.xaml.cs where xScanTypeComboBox is bound, the ScanType
enum needs to be bound to the combo and the boolean default corrected; in the
XAML code-behind insert a call to
GingerCore.General.FillComboItemsFromEnumType(xScanTypeComboBox,
typeof(WebServicesDriver.eSecurityTestingScanType)) immediately before you bind
xScanTypeComboBox so the enum values populate the control, and in
WebServicesDriver.cs change the UserConfiguredDefault attribute value from "No"
to "False" for the relevant boolean setting on line ~205.
Comment on lines
+204
to
+208
| [UserConfigured] | ||
| [UserConfiguredDefault("No")] | ||
| [UserConfiguredDescription("Action Failed on Security Testing Vulnerability Found")] | ||
| public bool FailActionOnSecurityIssue { get; set; } = true; | ||
|
|
Contributor
There was a problem hiding this comment.
Boolean default mismatch will flip behavior at runtime. Fix default + attribute.
- [UserConfiguredDefault("No")] is not the boolean form used elsewhere (use "false"/"true").
- Property initializer sets true, contradicting default "No". This will unexpectedly fail actions by default.
Apply:
- [UserConfigured]
- [UserConfiguredDefault("No")]
- [UserConfiguredDescription("Action Failed on Security Testing Vulnerability Found")]
- public bool FailActionOnSecurityIssue { get; set; } = true;
+ [UserConfigured]
+ [UserConfiguredDefault("false")]
+ [UserConfiguredDescription("Fail action when security vulnerabilities are found")]
+ public bool FailActionOnSecurityIssue { get; set; }📝 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
| [UserConfigured] | |
| [UserConfiguredDefault("No")] | |
| [UserConfiguredDescription("Action Failed on Security Testing Vulnerability Found")] | |
| public bool FailActionOnSecurityIssue { get; set; } = true; | |
| [UserConfigured] | |
| [UserConfiguredDefault("false")] | |
| [UserConfiguredDescription("Fail action when security vulnerabilities are found")] | |
| public bool FailActionOnSecurityIssue { get; set; } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs around
lines 204 to 208, the UserConfiguredDefault attribute uses "No" while other code
expects "true"/"false", and the property initializer sets true which contradicts
the intended "No" default; change the attribute value to "false" (i.e.
UserConfiguredDefault("false")) and align the property initializer to false so
the declared default and the runtime default behavior match.
| public override void RunAction(Act act) | ||
| { | ||
| //TODO: add func to Act + Enum for switch | ||
| //TODO: Clear the Sites tree of ZAP here |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Resolve the ZAP Sites-tree TODO before merge.
Clear the Sites tree to avoid cross-test contamination. If ZapProxyService doesn’t expose it, add a helper (e.g., ClearSitesTree/ResetSession) and call it when UseSecurityTesting is true. I can wire this if you confirm the desired scope (per action vs per BF).
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/Drivers/WebServicesDriver/WebServicesDriver.cs around
line 373, there's a TODO to "Clear the Sites tree of ZAP": implement a call to
clear ZAP state when UseSecurityTesting is true by invoking a ZapProxyService
helper (e.g., ClearSitesTree() or ResetSession()) at this point; if the
ZapProxyService lacks such a method, add a public helper that clears the Sites
tree/session and call it here to avoid cross-test contamination (use per-action
clearing unless you want per-BF scope and tell the reviewer to wire that
instead).
Comment on lines
+263
to
+293
| public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) | ||
| { | ||
| var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); | ||
| var alertSummary = (ApiResponseSet)summaryResponse; | ||
| try | ||
| { | ||
| var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); | ||
| var alertSummary = (ApiResponseSet)summaryResponse; | ||
|
|
||
| bool testPassed = true; | ||
| bool testPassed = true; | ||
|
|
||
| foreach (var alertEntry in alertSummary.Dictionary) | ||
| { | ||
| string alertName = alertEntry.Key; | ||
| string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); | ||
| if (!int.TryParse(valueString, out int count)) | ||
| foreach (var alertEntry in alertSummary.Dictionary) | ||
| { | ||
| continue; | ||
| string alertName = alertEntry.Key; | ||
| string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); | ||
| if (!int.TryParse(valueString, out int count)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| if (!allowedAlertNames.Any(a => a.Value == alertName)) | ||
| { | ||
| testPassed = false; | ||
| } | ||
| } | ||
| return testPassed; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Reporter.ToLog(eLogLevel.ERROR, $"Error evaluating scan result for Web: {ex.Message}", ex); | ||
| return false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix Web evaluation logic: respect count>0 and invert condition.
Currently it fails when an alert is NOT in the list and ignores count; this flips semantics and yields false negatives/positives.
- public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames)
+ public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames)
{
try
{
var summaryResponse = _zapClient.alert.alertsSummary(targetUrl);
var alertSummary = (ApiResponseSet)summaryResponse;
bool testPassed = true;
foreach (var alertEntry in alertSummary.Dictionary)
{
string alertName = alertEntry.Key;
string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString();
- if (!int.TryParse(valueString, out int count))
+ if (!int.TryParse(valueString, out int count))
{
continue;
}
-
- if (!allowedAlertNames.Any(a => a.Value == alertName))
- {
- testPassed = false;
- }
+ // Fail only when a listed alert has positive count (case-insensitive match)
+ if (count > 0 && (allowedAlertNames?.Any(a => string.Equals(a.Value, alertName, StringComparison.OrdinalIgnoreCase)) ?? false))
+ {
+ testPassed = false;
+ break;
+ }
}
return testPassed;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, $"Error evaluating scan result for Web: {ex.Message}", ex);
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
| public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) | |
| { | |
| var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); | |
| var alertSummary = (ApiResponseSet)summaryResponse; | |
| try | |
| { | |
| var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); | |
| var alertSummary = (ApiResponseSet)summaryResponse; | |
| bool testPassed = true; | |
| bool testPassed = true; | |
| foreach (var alertEntry in alertSummary.Dictionary) | |
| { | |
| string alertName = alertEntry.Key; | |
| string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); | |
| if (!int.TryParse(valueString, out int count)) | |
| foreach (var alertEntry in alertSummary.Dictionary) | |
| { | |
| continue; | |
| string alertName = alertEntry.Key; | |
| string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); | |
| if (!int.TryParse(valueString, out int count)) | |
| { | |
| continue; | |
| } | |
| if (!allowedAlertNames.Any(a => a.Value == alertName)) | |
| { | |
| testPassed = false; | |
| } | |
| } | |
| return testPassed; | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Error evaluating scan result for Web: {ex.Message}", ex); | |
| return false; | |
| } | |
| } | |
| public bool EvaluateScanResultWeb(string targetUrl, ObservableList<OperationValues> allowedAlertNames) | |
| { | |
| try | |
| { | |
| var summaryResponse = _zapClient.alert.alertsSummary(targetUrl); | |
| var alertSummary = (ApiResponseSet)summaryResponse; | |
| bool testPassed = true; | |
| foreach (var alertEntry in alertSummary.Dictionary) | |
| { | |
| string alertName = alertEntry.Key; | |
| string valueString = alertEntry.Value is ApiResponseElement element ? element.Value : alertEntry.Value?.ToString(); | |
| if (!int.TryParse(valueString, out int count)) | |
| { | |
| continue; | |
| } | |
| // Fail only when a listed alert has positive count (case-insensitive match) | |
| if (count > 0 && (allowedAlertNames?.Any(a => string.Equals(a.Value, alertName, StringComparison.OrdinalIgnoreCase)) ?? false)) | |
| { | |
| testPassed = false; | |
| break; | |
| } | |
| } | |
| return testPassed; | |
| } | |
| catch (Exception ex) | |
| { | |
| Reporter.ToLog(eLogLevel.ERROR, $"Error evaluating scan result for Web: {ex.Message}", ex); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Ginger/GingerCoreNET/External/ZAP/ZapProxyService.cs around lines 263 to 293,
the current loop marks the test failed for any alert name not in
allowedAlertNames regardless of its count and does not check that count>0;
change the logic so you parse the alert count and only consider alerts with
count > 0, and mark testPassed = false only when count > 0 AND the alertName is
not present in allowedAlertNames (i.e., invert the existing condition). Keep the
existing TryParse guard (continue on parse failure) and return false on
exceptions as before.
Maheshkale447
approved these changes
Aug 29, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Style
Chores