feat: output security reports as JSON when requested#136
feat: output security reports as JSON when requested#136
Conversation
Summary of ChangesHello @jajanet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the security analysis tool by introducing the ability to output findings in a structured JSON format. This change is crucial for enabling automated processing, integration with external systems like CI/CD pipelines, and improving the overall utility of the security reports. The implementation involves adding a new parsing utility, integrating it as a server tool, and updating the workflow configuration to support conditional JSON generation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🤖 Hi @jajanet, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to output security reports in JSON format, which will greatly improve programmatic consumption of the results. The implementation includes a new markdown parser, tests, and a server tool to handle the conversion.
My review focuses on improving the robustness and correctness of the new parser and its integration. I've identified a critical bug in the parsing logic that could lead to incorrect JSON output, along with a few medium-severity issues related to error handling, a typo in a prompt, and code consistency. The provided code suggestions aim to resolve these issues.
There was a problem hiding this comment.
📋 Review Summary
This pull request introduces a valuable feature to output security reports in JSON format. The implementation is well-executed, with a robust markdown parser and comprehensive test coverage. The code is clean, and the new functionality is a great addition for enabling programmatic use of the security reports.
🔍 General Feedback
- I've left a few minor suggestions for improving type safety and code readability, but overall this is a solid contribution.
| } | ||
| }) as any | ||
| ); | ||
|
|
There was a problem hiding this comment.
|
|
||
| findings.push({ | ||
| vulnerability: extract("Vulnerability"), | ||
| vulnerabilityType: extract("Vulnerability Type"), |
There was a problem hiding this comment.
| const rawSink = extract("Sink Location"); | ||
|
|
||
| let lineContent = extract("Line Content"); | ||
| if (lineContent) { |
There was a problem hiding this comment.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
This pull request introduces a new feature to output security reports in JSON format. The implementation adds a new tool to the MCP server and a parser to convert the markdown report to JSON. The code is well-structured and includes tests for the new parser.
🔍 General Feedback
- The new feature is a valuable addition that will improve the usability of the security extension.
- The code is clean and easy to understand.
- One potential security vulnerability was identified in the new parser.
| const extract = (label: string): string | null => { | ||
| const fieldNames = 'Vulnerability Type|Severity|Source Location|Sink Location|Data Type|Line Content|Description|Recommendation'; | ||
| const patternStr = `(?:-?\\s*\\**)?${label}\\**:\\s*([\\s\\S]*?)(?=\\n(?:-?\\s*\\**)?(?:${fieldNames})|$)`; | ||
| const pattern = new RegExp(patternStr, 'i'); |
There was a problem hiding this comment.
The regular expression used to parse the markdown report is vulnerable to Regular Expression Denial of Service (ReDoS). An attacker can craft a malicious markdown file that will cause the regex engine to backtrack excessively, leading to a denial of service. The vulnerable part is ([\\s\\S]*?) which can cause catastrophic backtracking.
| const pattern = new RegExp(patternStr, 'i'); | |
| const fieldNames = 'Vulnerability Type|Severity|Source Location|Sink Location|Data Type|Line Content|Description|Recommendation'; | |
| const patternStr = `(?:-?\\\\s*\\\\**)?${label}\\\\**:\\\\s*([\\\\s\\\\S]*?)(?=\\\\n(?:-?\\\\s*\\\\**)?(?:${fieldNames})|$)`; | |
| const pattern = new RegExp(patternStr, 'i'); |
QuanZhang-William
left a comment
There was a problem hiding this comment.
Thank you for the PR!
Left some questions and I think we should also update the README.md
| vulnerability: string | null; | ||
| vulnerabilityType: string | null; | ||
| severity: string | null; | ||
| extension: { |
There was a problem hiding this comment.
Why is this called extension?
| * **Action:** Output the final, reviewed report as your response to the user. | ||
| * **Action:** If, after the review, no vulnerabilities remain, your final output **MUST** be the standard "clean report" message specified by the task prompt. | ||
| * **Action:** Remove the temporary files (`SECURITY_ANALYSIS_TODO.md` and `DRAFT_SECURITY_REPORT.md`) from the `.gemini_security/` directory. Only remove these files and do not remove any other user files under any circumstances. | ||
| * **Action:** ONLY IF the user requested JSON output (e.g., via `--json` in context or natural language), call the `convert_report_to_json` tool. Inform the user that the JSON version of the report is available at .gemini_security/security_report.json. |
There was a problem hiding this comment.
Could you give an example of how to use the --json in the prompt, together with the custom audit scope that we implemented in #131?
There was a problem hiding this comment.
Certainly! To use the --json flag with a custom audit scope, you would combine them in your command line prompt. The --json flag signals that you want the final security report to be output in JSON format, and the --scope flag allows you to specify which files or directories should be included in the security analysis.
Here's an example of how you might use them together:
gemini security analyze --json --scope "mcp-server/src/filesystem.ts mcp-server/src/security.ts"In this example:
gemini security analyzeis the command to initiate a security analysis.--jsonis the new flag from this pull request, instructing the tool to generate asecurity_report.jsonfile in the.gemini_security/directory.--scope "mcp-server/src/filesystem.ts mcp-server/src/security.ts"specifies that the security analysis should only be performed on thefilesystem.tsandsecurity.tsfiles within themcp-server/src/directory. You can list multiple files or directories, separated by spaces, within the quotes.
After the analysis is complete, you would find the JSON version of the report at .gemini_security/security_report.json.
| * **Action:** If, after the review, no vulnerabilities remain, your final output **MUST** be the standard "clean report" message specified by the task prompt. | ||
| * **Action:** Remove the temporary files (`SECURITY_ANALYSIS_TODO.md` and `DRAFT_SECURITY_REPORT.md`) from the `.gemini_security/` directory. Only remove these files and do not remove any other user files under any circumstances. | ||
| * **Action:** ONLY IF the user requested JSON output (e.g., via `--json` in context or natural language), call the `convert_report_to_json` tool. Inform the user that the JSON version of the report is available at .gemini_security/security_report.json. | ||
| * **Action:** After the final report is delivered and any requested JSON report is complete, remove the temporary files (`SECURITY_ANALYSIS_TODO.md` and `DRAFT_SECURITY_REPORT.md`) from the `.gemini_security/` directory. Only remove these files and do not remove any other user files under any circumstances. |
There was a problem hiding this comment.
We can also call out not remove .gemini_security/security_report.json if generated
This PR adds the ability to output security report results in JSON. This enables programmatic parsing for accuracy checks, standardization, and integration with SCM tools and CI/CD pipelines (e.g., GitHub Actions, Jenkins)
Example of original markdown report and corresponding JSON:
turns into
[ { "vulnerability": "Path Traversal and Command Injection", "vulnerabilityType": "Security", "severity": "Critical", "extension": { "sourceLocation": { "File": "lib/router.js", "startLine": null, "endLine": null }, "sinkLocation": { "File": null, "startLine": null, "endLine": null }, "dataType": null }, "lineContent": "`full_path = \"\" + dispatch.static_route + (unescape(pathname));`", "description": "The `pathname` variable, derived from the URL, is not sanitized before being used to construct a file path. An attacker can use URLencoded characters like `../` to traverse the file system and access arbitrary files. This vulnerability is further escalated to command injection because the `full_path` is used in a `spawn` call, allowing an attacker to execute arbitrary commands on the system.", "recommendation": "Sanitize the `pathname` variable by removing any directory traversal characters before using it to construct a file path. Use `path.normalize()` or a similar function to resolve the path and ensure it stays within the intended directory." }, ... ]Fields are optional and written as null if not present, as the tool assumes that the
DRAFT_SECURITY_REPORT.mdfile is well-formed and has the expected fieldsThis is an initial implementation to help improve processes, and we may iterate using Vertex calls in the future. There is an upcoming PR on adding a subfield for code changes sometimes present under
recommendationas well