feat: Add vulnerable functions to CLI issue/finding OD-296 - #34
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 24 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Surfaces advisoryInformation (advisory ID, vulnerable functions, published date) across issue, issues, pull-request --issue, finding, and findings: compact one-liners on list/card views, full blocks on detail views. finding skips its own block when a linked Codacy issue already renders the same data via printIssueCodeContext, so SCA/dependency findings (which have no linked issue) are the case this closes out, now that SrmItem carries advisoryInformation directly (server-side, API 57.3.9).
Drop the vulnerable-functions/advisoryInformation qualifiers from the pull-request/issues/issue/findings/finding rows in the command inventory.
1903748 to
5838a71
Compare
There was a problem hiding this comment.
Pull Request Overview
The PR successfully updates the CLI to version 57.3.9 of the Codacy API to render vulnerable functions. However, there is a high-risk security flaw: externally sourced advisory strings (names, IDs, and functions) are rendered to the terminal without sanitization, exposing users to potential terminal control character injection (CWE-150). Additionally, while the PR is generally up to standards, a specific acceptance criterion regarding the suppression of redundant advisory blocks in the 'finding' command (when a linked issue is present) appears unaddressed or untested. Finally, the complexity of the printFindingCard function now exceeds project line-count limits.
About this PR
- The requirement to suppress the advisory block in the 'finding' command when a linked Codacy issue is present appears to be missing or unverified. Ensure logic is implemented to avoid duplicate information in detail views.
Test suggestions
- Verify compact vulnerable functions line on issues/findings cards
- Verify truncation logic ('+N more') when more than 3 functions are present
- Verify full advisory block in detail views (issue/finding/pr --issue)
- Verify suppression of redundant advisory block in finding detail when linked issue is present
- Verify advisoryInformation is present in JSON output for all commands
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify suppression of redundant advisory block in finding detail when linked issue is present
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| export function summarizeFunctions(fns: string[], limit = 3): string { | ||
| const shown = fns.slice(0, limit).join(", "); | ||
| const more = fns.length > limit ? ` (+${fns.length - limit} more)` : ""; | ||
| return `${shown}${more}`; | ||
| } |
There was a problem hiding this comment.
🔴 HIGH RISK
Neutralize terminal control characters in external advisory data to prevent terminal injection attacks.
| export function summarizeFunctions(fns: string[], limit = 3): string { | |
| const shown = fns.slice(0, limit).join(", "); | |
| const more = fns.length > limit ? ` (+${fns.length - limit} more)` : ""; | |
| return `${shown}${more}`; | |
| } | |
| export function summarizeFunctions(fns: string[], limit = 3): string { | |
| const shown = fns.slice(0, limit).map(sanitizeText).join(", "); | |
| const more = fns.length > limit ? ` (+${fns.length - limit} more)` : ""; | |
| return `${shown}${more}`; | |
| } |
| } | ||
|
|
||
| // Vulnerable functions (findings with an OSV-linked advisory), compact form | ||
| if (item.advisoryInformation) { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The printFindingCard function has exceeded the 50-line limit (currently 53 lines). Additionally, the new vulnerable functions line should only be rendered if there are functions to display to avoid empty labels.
Consider refactoring the status and version metadata construction into a dedicated helper function to reduce complexity, and gate the rendering logic:
| if (item.advisoryInformation) { | |
| if (item.advisoryInformation?.vulnerableFunctions?.length) { | |
| console.log( | |
| ansis.dim(`Vulnerable functions: ${summarizeFunctions(item.advisoryInformation.vulnerableFunctions)}`), | |
| ); | |
| } |
| export function printAdvisoryBlock(advisory: AdvisoryInformation): void { | ||
| console.log(); | ||
| console.log(ansis.bold(`Vulnerable Functions (${advisory.advisoryId})`)); | ||
| if (advisory.publishedAt) { | ||
| console.log(ansis.dim(`Published: ${formatDueDate(advisory.publishedAt)}`)); | ||
| } | ||
| console.log(); | ||
| for (const fn of advisory.vulnerableFunctions) { | ||
| console.log(` • ${fn}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Sanitize external advisory fields and handle empty function lists gracefully to avoid empty headers and stray newlines.
| export function printAdvisoryBlock(advisory: AdvisoryInformation): void { | |
| console.log(); | |
| console.log(ansis.bold(`Vulnerable Functions (${advisory.advisoryId})`)); | |
| if (advisory.publishedAt) { | |
| console.log(ansis.dim(`Published: ${formatDueDate(advisory.publishedAt)}`)); | |
| } | |
| console.log(); | |
| for (const fn of advisory.vulnerableFunctions) { | |
| console.log(` • ${fn}`); | |
| } | |
| } | |
| export function printAdvisoryBlock(advisory: AdvisoryInformation): void { | |
| console.log(); | |
| console.log(ansis.bold(`Vulnerable Functions (${sanitizeText(advisory.advisoryId)})`)); | |
| if (advisory.publishedAt) { | |
| console.log(ansis.dim(`Published: ${formatDueDate(advisory.publishedAt)}`)); | |
| } | |
| if (advisory.vulnerableFunctions.length > 0) { | |
| console.log(); | |
| for (const fn of advisory.vulnerableFunctions) { | |
| console.log(` • ${sanitizeText(fn)}`); | |
| } | |
| } | |
| } |
| // Vulnerable functions (SCA issues with an OSV-linked advisory), compact form | ||
| if (issue.advisoryInformation) { | ||
| console.log(); | ||
| console.log(ansis.dim(`Vulnerable functions: ${summarizeFunctions(issue.advisoryInformation.vulnerableFunctions)}`)); | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Only render the vulnerable functions line if there are functions to display.
| // Vulnerable functions (SCA issues with an OSV-linked advisory), compact form | |
| if (issue.advisoryInformation) { | |
| console.log(); | |
| console.log(ansis.dim(`Vulnerable functions: ${summarizeFunctions(issue.advisoryInformation.vulnerableFunctions)}`)); | |
| } | |
| // Vulnerable functions (SCA issues with an OSV-linked advisory), compact form | |
| if (issue.advisoryInformation?.vulnerableFunctions?.length) { | |
| console.log(); | |
| console.log(ansis.dim(`Vulnerable functions: ${summarizeFunctions(issue.advisoryInformation.vulnerableFunctions)}`)); | |
| } |
External OSV advisory strings (function names, advisory ID) were printed raw, letting a crafted advisory smuggle terminal control sequences (CWE-150); route them through sanitizeText() like other repo-derived output. Also gate the vulnerable-functions line on a non-empty list and split printFindingCard into two helpers to stay under the line-count limit, and add the missing test for advisory-block suppression when a finding has a linked issue.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for displaying vulnerable and affected functions (SCA) in the issue, issues, pull-request, finding, and findings commands. It includes both compact card summaries and detailed advisory blocks, with JSON output support and a pinned API update to access these new fields.
While the implementation aligns with most acceptance criteria, including the logic to suppress duplicate advisory blocks when Codacy issue data is already present, there are security gaps regarding the sanitization of terminal output. Several fields in the findings list view (likelihood, effortToFix, cve, and cwe) are currently bypassing the sanitizeText() utility. Addressing these is necessary to ensure full protection against terminal control character injection (CWE-150).
About this PR
- The test suite for
advisoryInformationis highly repetitive acrossfindings.test.ts,finding.test.ts,issue.test.ts, andpull-request.test.ts. Consider creating a shared test helper to verify card output and JSON structures for advisories to improve maintainability and reduce future duplication.
Test suggestions
- Verify compact vulnerable functions line appears on issue cards when advisory data is present
- Verify compact vulnerable functions line appears in findings list view when advisory data is present
- Verify vulnerable functions list is truncated to 3 items with correct '+N more' suffix in compact views
- Verify full advisory block appears in 'issue' and 'pull-request --issue' detail views
- Verify advisory block appears in 'finding' detail view for non-Codacy (SCA/dependency) findings
- Verify advisory block is suppressed in 'finding' detail view if a linked Codacy issue is present (avoiding duplicates)
- Verify advisoryInformation is correctly projected in --output json for all commands
- Verify terminal control characters in function names or advisory IDs are neutralized via sanitization
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if (item.cve) parts.push(ansis.dim(item.cve)); | ||
| else if (item.cwe) parts.push(ansis.dim(`CWE-${item.cwe}`)); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Neutralize terminal control characters in item.cve and item.cwe using sanitizeText().
| if (item.cve) parts.push(ansis.dim(item.cve)); | |
| else if (item.cwe) parts.push(ansis.dim(`CWE-${item.cwe}`)); | |
| if (item.cve) parts.push(ansis.dim(sanitizeText(item.cve))); | |
| else if (item.cwe) parts.push(ansis.dim(`CWE-${sanitizeText(item.cwe)}`)); |
| const penTestParts = [item.likelihood, item.effortToFix].filter( | ||
| (v) => v && v !== "not_applicable", | ||
| ) as string[]; | ||
| if (penTestParts.length > 0) line1Parts.push(penTestParts.join(" ")); | ||
| if (penTestParts.length > 0) parts.push(penTestParts.join(" ")); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Neutralize terminal control characters in likelihood and effortToFix using sanitizeText() before they are pushed to the header parts. You can update buildFindingHeaderLine in src/commands/findings.ts to map these values through the sanitizer before joining them.
| function printFindingCard(item: SrmItem, showRepo: boolean): void { | ||
| const separator = ansis.dim("─".repeat(40)); | ||
| // Line 1: Priority | SecurityCategory ScanType | Likelihood EffortToFix | Repository | ||
| function buildFindingHeaderLine(item: SrmItem, showRepo: boolean): string { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The logic for constructing finding headers and status lines is shared between the finding (detail) and findings (list) commands. To avoid rendering inconsistencies, move buildFindingHeaderLine and buildFindingStatusLine to src/utils/formatting.ts and use them in both command files.
What
codacy-apischema (fetch-apiscript)56.2.9→57.3.9, picking upCommitIssue.advisoryInformation(OD-284) andSrmItem.advisoryInformation(OD-397).issue,issues, andpull-request --issueshow vulnerable/affected functions for SCA issues with a linked OSV advisory (CommitIssue.advisoryInformation): compact one-liner on card views (printIssueCard), full block with advisory ID + published date on detail views (printAdvisoryBlock, wired intoprintIssueCodeContext).findingandfindingsget the same treatment fromSrmItem.advisoryInformation, closing the gap noted when OD-296 first shipped for issues (SrmItemhad no equivalent field server-side until OD-397).findingskips its own block when a linked Codacy issue already renders the same data viaprintIssueCodeContext, so this is what actually makes vulnerable functions visible for SCA/dependency findings (and any other non-Codacy-source finding), which have no linked issue to borrow it from at all.summarizeFunctions/printAdvisoryBlockhelpers inutils/formatting.ts, shared across all five commands.--output jsonfor all five commands (pickDeepwhitelists extended).Why
OD-296: add affected/vulnerable functions to the
findingscommand. The issues-side plumbing (OD-284'sCommitIssue.advisoryInformation) was already usable; the findings-side was blocked onSrmItemnot carrying the field server-side, which OD-397 has now shipped.Testing
issue.test.ts, 3 inpull-request.test.ts(advisory info on--issuedetail), 3 infinding.test.ts, 3 infindings.test.ts— 12 new, 494 total, all passing.tsc --noEmitclean (aside from a pre-existing, unrelatedupdate-notifiermodule-resolution error insrc/utils/update-check.ts).minor).Related
SrmItem.advisoryInformationserver-side)qa-automation-tests(test/security-items-advisory-OD-397)