Devassist | Hover solution (AST-16742^) - #268
Conversation
- Remove McpInstallService from PreferencesPage (common-lib) - Create AuthenticationListener in devassist-lib configuration - Register listener in McpInstallService static block - Create IProjectLifecycleListener interface in common-lib - ProjectLifecycleListener implements interface - Update PluginStartup.getProjectListener() to return interface - MCP auto-install now triggered by authentication event (devassist-lib) - Workspace scan triggered after login via interface Architecture: common-lib has no devassist imports, clean separation.
- Create IAuthenticationSuccessHandler interface in common-lib - Move welcome dialog logic to AuthenticationSuccessHandler in devassist-lib - PreferencesPage delegates to handler via Preferences registry - Removes WelcomeDialog import from common-lib PreferencesPage - Handlers registered in McpInstallService static block Architecture: common-lib has NO devassist imports, clean separation.
- Create ISettingsChangeNotifier interface in common-lib - Create SettingsChangeNotifier implementation in main plugin - Register notifier in PluginStartup static block - Remove PluginStartup and PluginUtils imports from common-lib PreferencesPage - Use notifier instead of direct event broker calls Architecture: common-lib has NO main plugin imports, clean separation.
devassist-lib should depend on common-lib for JAR access, not duplicate them. - Remove lib/ references from devassist-lib MANIFEST.MF and build.properties - Remove lib/ references from devassist-lib .classpath - devassist-lib Require-Bundle: common-lib provides JAR access - Revert .gitignore to only track main plugin lib/
Major improvements to HTML rich hover display: 1. Fixed O(n²) duplicate detection using HashSet instead of ArrayList 2. Added performance monitoring to detect slow hover operations (>100ms) 3. Improved error handling with proper exception catching during annotation iteration 4. Centralized HTML escaping to HtmlEscapeUtil utility class 5. Enhanced HTML styling with severity-based colors, proper spacing, and fonts 6. Added visual improvements: separator styling, font sizes, color hierarchy 7. Optimized annotation model access with better error recovery Performance fixes: - Replaced O(n) contains() checks with O(1) HashSet lookups - Added timeout monitoring (logs if hover takes >100ms) - Proper exception handling without blocking UI thread HTML/UX improvements: - Severity colors: Malicious (red), Critical (dark red), High (orange), Medium (yellow), Low (green) - Better visual hierarchy with font sizes and weights - Improved spacing and divider styling - Action links now styled with blue color and cursor pointer indicator - Font family and size defaults for consistent rendering Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Hover now shows: - Title (orange/red) - Description - Informational action links (Fix, View Details, Ignore, Copy Details) - Helper text: "Press Ctrl+1 for Quick Fix actions" The action links are text-only (Eclipse hovers can't capture clicks). Actual implementations are in Quick Fix system via CheckmarxMarkerResolutionGenerator. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Hover now displays: - Title (orange) + Description - Four styled action buttons: Fix with AI, Details, Ignore, Copy - Buttons are clickable and trigger corresponding actions - Uses HTML button elements with action: protocol URL handlers - LocationListener intercepts clicks via reflection on internal Browser Next: Hook action handlers to actual Quick Fix implementations. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Removed button styling (background, borders, padding) - Now displays as simple text links: blue + underlined - Links are clickable via LocationListener on action: protocol URLs - Simpler, cleaner appearance matching typical hover link styles Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The HoverControlCreator (small preview) had the action handler set up, but the PresenterControlCreator (large interactive popup) did not. When the user moved the mouse into the hover popup, JFace replaced it with the PresenterControlCreator, which had no LocationListener to intercept action: protocol URLs. Now both control creators set up the handler, so clicks work on both the preview and the interactive popup. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Changed action links from href-based to onclick-based with window.location assignment. This ensures LocationListener receives location change events for action: protocol URLs. Added debug logging to verify LocationListener setup and invocation. Fixes: Links now trigger handleHoverAction() when clicked on both preview and interactive popups. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Changed from custom protocol (action:) to URL fragment (#action:). SWT Browser navigates to about:blank#action:fix on link click, and LocationListener can now parse the fragment to extract the action name. Fixes: Clicks on action links now trigger handleHoverAction() correctly. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…nto feature/hover
Original alert (resolved)Security Policy Alert: Actions Policy ViolationThis workflow run has been blocked by StepSecurity's actions policy. Disallowed Actions:
To fix this issue, please modify the workflow to use only allowed actions. Contact your organization administrator to request changes to the allowed actions list if needed. For more information, see StepSecurity's Actions Policy documentation. |
| import com.checkmarx.ast.wrapper.CxException; | ||
| import com.checkmarx.eclipse.common.events.SettingsTopics; | ||
| import com.checkmarx.eclipse.common.preferences.Preferences; | ||
| import com.checkmarx.eclipse.common.events.SettingsTopics; |
| /** | ||
| * Checks whether a severity is enabled (only actual severity levels) | ||
| */ | ||
| public static boolean isSeverityEnabled(String severity) { |
There was a problem hiding this comment.
This PR adds MALICIOUS to the shared common-lib Severity enum, but FilterState.setState/isSeverityEnabled have no case for it (falls to return false), and DataProvider.buildResults() calls removeIf(!isSeverityEnabled(...)), so every malicious-package result is unconditionally removed from the tree with no toolbar toggle to re-enable it — inconsistent with devassist-lib's own VulnerabilityFilterAction.MaliciousFilter added in the same PR.
Suggested fix: Add a MALICIOUS case (backing boolean + preference key) in FilterState, and a matching toolbar filter action in ActionFilters.createFilterActions().
Evidence: FilterState.java:73-96,207-231 no MALICIOUS case; DataProvider.java:354 strips disabled severities; ActionFilters.java:45-60 never builds a MALICIOUS toggle.
| return false; | ||
| } | ||
|
|
||
| String engineName = extractEngineName(linkData); |
There was a problem hiding this comment.
"View details"/"Ignore" hover action links are completely dead due to a link-format/parsing mismatch
extractEngineName requires a 3rd, non-empty split segment or the link is rejected outright. CheckmarxProblemDescriptionFormatter.buildRemediationActionsSection only appends the engine-name segment for the copyfixprompt (Fix) action — viewdetails/ignorethis/ignoreallofthis build action:scanIssueId: with nothing after the trailing separator, and Java's String.split drops trailing empty strings, so extractEngineName always returns "" for these three actions. Result: clicking "View details" (a real, shipped DevAssistConstants.VIEW_DETAILS_FIX_NAME feature) silently does nothing, for every engine, every time — only "Fix" ever works.
Suggested fix: Append engineName for all four action link types (matching copyfixprompt's shape), or make RemediationLinkHandler tolerate a missing engine segment for actions that don't need it.
Evidence: RemediationLinkHandler.java:63-67,167-169; CheckmarxProblemDescriptionFormatter.java:352-370 only copyfixprompt appends engineName.
| * @param actionId the action ID for vulnerability-specific fixes | ||
| * @return true if the action is successfully handled, false otherwise | ||
| */ | ||
| private boolean handleActions(@NonNull String action, @NonNull ScanIssue scanIssue, @NonNull String actionId) { |
There was a problem hiding this comment.
No verification that a clicked link's scanIssueId matches the ScanIssue used for the action
handleActions(action, scanIssue, actionId) never checks scanIssue.getScanIssueId().equals(actionId). Combined with CheckmarxAnnotationHover's shared static currentFinding field (see below), this means a mismatched ScanIssue can be used with zero detection for OSS/SECRETS/CONTAINERS remediation prompts. A safer handleLink(String) overload exists that re-resolves the issue from ProblemHolderService by ID, but it's never called from the only live call site.
Suggested fix: Verify actionId against scanIssue.getScanIssueId() before dispatching, or always re-resolve via ProblemHolderService using the link's own ID, as the unused safer overload already does.
Evidence: RemediationLinkHandler.java:129; RemediationManager.java:211-230 ignores actionId entirely; handleLink(String) (:83-117) has zero callers.
| * Default fallback description. | ||
| */ | ||
| private void buildDefaultDescription(StringBuilder descBuilder, ScanIssue scanIssue) { | ||
| descBuilder.append("<div><b>").append(scanIssue.getTitle()).append("</b> -").append(scanIssue.getDescription()); |
There was a problem hiding this comment.
Unescaped finding text in the default (unknown-engine) hover HTML branch
Every other branch (OSS/ASCA/SECRETS/IAC/CONTAINERS) escapes scanner-derived strings via HtmlEscapeUtil.escape() before appending to the HTML rendered in the SWT Browser hover; buildDefaultDescription() is the sole exception, appending raw title/description text. Currently unreachable (the switch is exhaustive over the 5-value model enum), but a sibling utils.ScanEngine enum already has a 6th value (ALL) not present in the model enum, showing the two are already drifting — the next added engine silently falls into this unescaped, attacker-influenceable HTML injection sink.
Suggested fix: Escape title/description in buildDefaultDescription() for defense-in-depth even though currently unreachable.
Evidence: CheckmarxProblemDescriptionFormatter.java:289-291 vs. every other branch's HtmlEscapeUtil.escape() usage; model/ScanEngine.java vs utils/ScanEngine.java enum drift.
| // Shared severity icon instances | ||
| private static final Image MALICIOUS_ICON = IconRegistry.getIcon(DevAssistConstants.MALICIOUS, IconRegistry.Size.SMALL); | ||
| private static final Image CRITICAL_ICON = IconRegistry.getIcon(DevAssistConstants.CRITICAL, IconRegistry.Size.SMALL); | ||
| private static final Image MALICIOUS_ICON = IconRegistry.getIcon(DevAssistConstants.MALICIOUS, |
There was a problem hiding this comment.
Findings tree loses expand/selection state on every refresh — tree nodes lack equals/hashCode
getElements()/getChildren() allocate brand-new FileNodeLabel/ScanDetailWithPath instances on every call, and neither overrides equals()/hashCode() (identity comparison). JFace's AbstractTreeViewer keys its internal element map on identity/equals, so CxFindingsView's existing workaround (getExpandedElements()/setExpandedElements() around setInput()) silently fails because the old instances can't be matched in the new tree. Every filter toggle, ignore action, or scan refresh collapses whatever nodes the user had expanded — a frequently-triggered UX regression.
Suggested fix: Override equals()/hashCode() on both model classes based on stable keys (file path; file path + issue ID), or install a custom IElementComparer on the TreeViewer.
Evidence: FindingsContentProvider.java:38-42,70-71; no equals/hashCode on either model class; CxFindingsView.java:1256-1281 expand-state workaround that can't succeed against fresh instances.
| String countStr = String.valueOf(counts.get(severity)); | ||
| int textWidth = event.gc.textExtent(countStr).x; | ||
| // 16px (Icon) + 4px (Gap between icon & number) + number length + gap to next badge | ||
| extraWidth += 16 + 0 + textWidth + BETWEEN_BADGE_SPACING; |
There was a problem hiding this comment.
Layout under-reserves width for severity badges, risking clipped counts
measure() reserves 16px per badge icon, but paint() actually draws the 20px (MEDIUM) icon set and advances 20px — a 4px-per-badge shortfall that compounds when several severities appear on one file node, clipping the last badge's count text.
Suggested fix: Use the same icon-width constant in measure() as paint() uses.
Evidence: FindingsLabelProvider.java:93,126,131.
| @@ -1 +1 @@ | |||
| package com.checkmarx.eclipse.devassist.backend.listener; | |||
There was a problem hiding this comment.
ui/findings/realtime is a ~750-line, fully commented-out duplicate of the live backend/listener real-time scan pipeline
Three files here (RealTimeScanJob.java, CheckmarxEditorListener.java, CheckmarxDocumentListener.java) are entirely comment-prefixed line-for-line — they compile to nothing. The equivalently-named live classes in backend/listener are the ones actually referenced from PluginStartup, WorkspaceScanService, DevAssistScanScheduler, and CxFindingsView. Not a live correctness risk (it can never execute), but shipping 750 lines of an abandoned parallel package with identical class names strongly suggests an incomplete migration, and risks a future engineer un-commenting or extending the stale copy — which lacks a fix already present in the live version (a specific "CRITICAL FIX" comment for stale-annotation handling).
Suggested fix: Delete the ui/findings/realtime directory entirely.
Evidence: All three files are 100% comment-prefixed (grep -cv '^//' returns 0); zero live references anywhere in the repo (confirmed independently by architecture-reviewer and reliability-reviewer).
| @@ -204,12 +202,12 @@ protected void createButtonsForButtonBar(Composite parent) { | |||
| } | |||
|
|
|||
| private void onQuickFixClick() { | |||
There was a problem hiding this comment.
Finding-details dialog's Quick Fix / Ignore / Open Window buttons are unwired no-op stubs
Only the "Copy" button does anything; onQuickFixClick, onIgnoreClick, and onOpenWindowClick are all empty // TODO: Implement... stubs wired to real SelectionAdapters. A user reaching this dialog via Ctrl+1 → "View Finding Details" and clicking "Ignore" or "⚡ Quick Fix" gets no feedback and no effect, even though functionally-identical, working sibling resolutions exist in the same Ctrl+1 menu.
Suggested fix: Wire these buttons to the same logic as IgnoreVulnerabilityResolution.run()/QuickFixRemediationResolution.run(), or remove the buttons until implemented.
Evidence: ViewFindingDetailsResolution.java:155-193,204-231.
…269) - Added addition SCA package manager support - Publish plugin version and refactor existing wrapper call code - Centralize CxWrapper construction with agent version reporting and architectural cleanup - Added agent name + plugin version stamping in CxWrapperFactory to report "Eclipse_<version>" in all API calls - Moved CxWrapperFactory from devassist-lib/factory to common-lib/wrapper (shared location) - Added comprehensive unit tests (CxWrapperFactoryTest, WrapperProviderTest) - Enhance Checkmarx One preferences page UI and add logout confirmation - Persist the connected state and success message across page reopens, lock/unlock the API key field and Connect/Logout buttons based on connection state, add a logout confirmation dialog, and focus the API key field on open. * Decouple auth-state checks from API key presence; keep key after logout * Add Checkmarx MCP configuration UI with install/edit links and status display * Implement MCP uninstall with handler and callback pattern - New IMcpUninstallHandler interface for logout page to trigger uninstall - New IMcpUninstallCallback with onSuccess() / onNotFound() / onFailure() - Enables bidirectional MCP lifecycle (install on login, uninstall on logout) with symmetric callback-based result reporting for both operations
Security Policy Alert: Secret Policy ViolationThis workflow run has been blocked by StepSecurity's secrets policy because it accesses secrets and the workflow file differs from the default branch. Secret references detected:
To approve this workflow, please add the Note: The label must be added by someone other than the PR author (cx-anand-nandeshwar) or automation bots to ensure proper security review. After the label is added, you can re-run the blocked workflow to proceed. This workflow will be automatically approved once merged into the default branch. For more information, see StepSecurity's Secret Exfiltration Policy documentation. |
2fc9aca
into
feature/devassist_integration
No description provided.