(old) feat: output security reports as JSON when requested #136
Conversation
jajanet
requested review from
QuanZhang-William,
QuinnDACollins,
capachino,
evanotero,
heltonduarte and
shrishabh
as code owners
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
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.
mcp-server/src/parser.ts
Outdated
| 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
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
shrishabh
left a comment
shrishabh
left a comment
There was a problem hiding this comment.
Thank you for the PR!
I question that I was wondering - here we are deterministically parsing the markdown into json using regexes. Another option could have been using LLMs to adjust their outputs so that they conform to this particular format. I think deterministically parsing is a great first step, but we might want to keep an eye out for failure cases and potentially move to the alternative if we see lots of errors.
mcp-server/src/parser.ts
Outdated
| 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'); |
| vulnerabilityType: extract("Vulnerability Type"), | ||
| severity: extract("Severity"), | ||
| dataType: extract("Data Type"), | ||
| sourceLocation: parseLocation(rawSource), |
There was a problem hiding this comment.
What happens if there is an error in any of these extraction methods? It might be worth considering failing gracefully here as the rest of the extraction might still produce a reasonable report?
There was a problem hiding this comment.
It returns null! Examples:
[
{
"vulnerability": "Cross-Site Scripting (XSS)",
"vulnerabilityType": "Security",
"severity": "High",
"dataType": null, // <-- only relevant to privacy issues
"sourceLocation": {
"file": "js/src/util.js",
"startLine": 120,
"endLine": 120
},
"sinkLocation": {
"file": null,
"startLine": null,
"endLine": null
},
"lineContent": "`const $selector = $(selector)`",
"description": "The `getSelectorFromElement` function is vulnerable to DOM-based XSS. It constructs a jQuery selector from the `data-target` or `href` attributes of an element without proper sanitization. An attacker can inject malicious JavaScript code into these attributes, which will then be executed when the selector is processed by jQuery.",
"recommendation": "Sanitize the selector before passing it to jQuery. For example, ensure that the selector only contains valid CSS selector characters. A simple fix is to escape the selector using a library like `CSS.escape()` if available, or to validate the selector against a regex for valid characters. Given the context of this file, a more robust solution would be to ensure that any user-provided input is properly encoded before being used in a selector."
}
]and
[
{
"vulnerability": "Cross-Site Scripting (XSS)",
"vulnerabilityType": "Security",
"severity": "High",
"dataType": null,
"sourceLocation": {
"file": "bin/mk-server",
"startLine": null, // sometimes missing
"endLine": null
},
"sinkLocation": {
"file": null,
"startLine": null,
"endLine": null
},
"lineContent": "`res.end \"<h1>User No: <span style='color: red;'>\" + req.params.id + \"</span></h1>\"`",
"description": "The application is vulnerable to reflected XSS. The `id` parameter from the URL is directly embedded into the HTML response without any sanitization. This allows an attacker to inject malicious scripts into the response, which will be executed in the context of the user's browser.",
"recommendation": "Sanitize the `req.params.id` variable before embedding it in the HTML response. Use a library like `he` to encode HTML entities."
}
]There was a problem hiding this comment.
Also not sure what you mean by "can we fix this?" and can't reply directly to that comment -- do you mean pull it out of the function? I did add caching the regex pattern (for performance) and small readability changes like pulling it out of the function and using a list for field names, but that might not be what you're referring to? :
const FIELD_NAMES = [
'Vulnerability',
'Severity',
'Source',
'Sink',
'Data',
'Line',
'Description',
'Recommendation',
].join('|'); // add more labels here
const patternCache = new Map<string, RegExp>();
const escapeRegExp = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
...
/**
* Parses a markdown string containing security findings into a structured format.
* The markdown should follow a specific format where each finding starts with "Vulnerability:" and includes fields like "Severity:", "Source Location:", etc.
* The function uses regular expressions to extract the relevant information and returns an array of findings.
*
* @param content - The markdown string to parse.
* @returns An array of structured findings extracted from the markdown.
*/
function extractFromSection(section: string, label: string): string | null {
const pattern = buildPattern(label);
const match = section.match(pattern);
return match ? match[1].trim() : null;
};
...
const rawSource = extractFromSection(section, "Source Location");
...
QuanZhang-William
left a comment
QuanZhang-William
left a comment
There was a problem hiding this comment.
/lgtm to me.
Please address other feedbacks before merge :)
|
@QuanZhang-William I really can't resolve the failing CLA check -- I tried doing a lot of reauthoring / fixing of commits but doesn't work! I'm gonna open up another PR with these changes |
jajanet
changed the title
|
closing and replacing with #138 due to CLA issues |
3 participants
(closed in favor of #138 due to CLA check / merging issues that arose from applying a suggestion from GitHub actions bot)
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