Conversation
|
Claude finished @satvik007's task in 10m 37s —— View job Review: Add Allure upload support
CI status: Passing This PR adds well-structured Allure upload support that follows existing codebase patterns. The Zod schema validation, attachment handling, TMS marker extraction, test fixture organization, and integration test parameterization are all solid work. I've posted 4 inline review comments on the following issues:
Two additional issues were already flagged by another reviewer and not duplicated: path traversal via attachment sources (line 116) and TMS link marker regex uppercase-only restriction (line 217). |
Summary of ChangesHello @satvik007, 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 significantly enhances the Highlights
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
|
![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 support for uploading Allure test results. However, a high-severity path traversal vulnerability was identified in the Allure result parser. This vulnerability allows an attacker to read and exfiltrate arbitrary files from the system, particularly in CI/CD environments, due to the parser failing to validate attachment source paths. Additionally, the new Allure parser (allureParser.ts) has areas for improvement, including refactoring the main parsing function to use asynchronous I/O for better robustness and clarity, and a correction to a regular expression for more flexible test case marker detection.
| attachments: [], | ||
| }) - 1 | ||
|
|
||
| const attachmentSources = (parsed.attachments ?? []).map((attachment) => attachment.source) |
There was a problem hiding this comment.
A high-severity path traversal vulnerability exists in the parseAllureResults function. The attachment.source paths are extracted directly from Allure result JSON files without validation. These paths are then used with path.join to combine with the base directory and read files from the filesystem. This allows an attacker to provide malicious paths (e.g., ../../../../etc/passwd) in the source field, potentially leading to the reading and exfiltration of arbitrary files from the system, especially in CI/CD environments. The provided code suggestion adds validation to prevent this.
const attachmentSources = (parsed.attachments ?? []).map((attachment) => {
const source = attachment.source
const resolvedPath = path.join(attachmentBaseDirectory, source)
if (!resolvedPath.startsWith(path.resolve(attachmentBaseDirectory))) {
throw new Error(`Invalid attachment source: ${source}`)
}
return source
})| } | ||
|
|
||
| if (link.name) { | ||
| const match = link.name.match(/\b([A-Z]+)-(\d+)\b/) |
There was a problem hiding this comment.
The regex for extracting a test case marker from a link name is too restrictive. It only allows uppercase letters for the project code and doesn't enforce a minimum number of digits for the sequence number. This is inconsistent with how markers are detected elsewhere in the codebase (e.g., in ResultUploadCommandHandler.ts), which could lead to valid markers not being recognized.
This updated regex allows for alphanumeric project codes (case-insensitive) and requires at least three digits for the sequence number, aligning it with other parts of the application.
const match = link.name.match(/\b([A-Z0-9]{1,5})-(\d{3,})\b/i)![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds well-structured Allure upload support that follows existing codebase patterns. The Zod schema validation, attachment handling, TMS marker extraction, test fixtures, and integration test parameterization are all solid. CI passes.
Issues flagged (4 inline comments):
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | Medium | allureParser.ts:134-148 |
unknown status mapped to passed — could mask real failures; dead default branch |
| 2 | Medium | allureParser.ts:75 |
No directory validation before readdirSync — poor error UX for invalid paths |
| 3 | Low | allureParser.ts:170-185 |
skipStdout/skipStderr mapped to Allure's message/trace — semantic mismatch worth documenting |
| 4 | Low | allure-parsing.spec.ts:8-13 |
Missing test coverage for skipStdout/skipStderr on-success behavior |
Already flagged by another reviewer (not duplicated):
- Path traversal via attachment sources (line 116) — pre-existing pattern across all parsers
- TMS link marker regex uppercase-only restriction (line 217)
| const mapAllureStatus = (status: z.infer<typeof allureStatusSchema>): ResultStatus => { | ||
| switch (status) { | ||
| case 'passed': | ||
| return 'passed' | ||
| case 'failed': | ||
| return 'failed' | ||
| case 'broken': | ||
| return 'blocked' | ||
| case 'skipped': | ||
| return 'skipped' | ||
| case 'unknown': | ||
| return 'passed' | ||
| default: | ||
| return 'passed' | ||
| } |
There was a problem hiding this comment.
Questionable status mapping: Allure's unknown status means the framework could not determine the test outcome. Mapping it to passed could silently mask real test failures. Consider mapping unknown to blocked or skipped instead.
Additionally, the default branch on line 147 is unreachable dead code — the Zod enum already constrains input to the five explicitly handled cases, so this branch can never execute. Removing it would make the exhaustiveness clear.
| const mapAllureStatus = (status: z.infer<typeof allureStatusSchema>): ResultStatus => { | |
| switch (status) { | |
| case 'passed': | |
| return 'passed' | |
| case 'failed': | |
| return 'failed' | |
| case 'broken': | |
| return 'blocked' | |
| case 'skipped': | |
| return 'skipped' | |
| case 'unknown': | |
| return 'passed' | |
| default: | |
| return 'passed' | |
| } | |
| const mapAllureStatus = (status: z.infer<typeof allureStatusSchema>): ResultStatus => { | |
| switch (status) { | |
| case 'passed': | |
| return 'passed' | |
| case 'failed': | |
| return 'failed' | |
| case 'broken': | |
| return 'blocked' | |
| case 'skipped': | |
| return 'skipped' | |
| case 'unknown': | |
| return 'blocked' | |
| } | |
| } |
| attachmentBaseDirectory: string, | ||
| options: ParserOptions | ||
| ): Promise<TestCaseResult[]> => { | ||
| const entries = readdirSync(resultsDirectory, { withFileTypes: true }) |
There was a problem hiding this comment.
No directory validation before readdirSync: Unlike the JUnit/Playwright parsers which receive pre-read file content, the Allure parser handles its own I/O. If a user passes a file path or non-existent path, the error will be a raw Node.js ENOENT/ENOTDIR with no user-friendly context.
Consider adding a check before this call:
const stat = statSync(resultsDirectory, { throwIfNoEntry: false })
if (!stat?.isDirectory()) {
throw new Error(`Allure results path is not a directory: "${resultsDirectory}"`)
}| const buildMessage = ( | ||
| statusDetails: z.infer<typeof allureStatusDetailsSchema>, | ||
| status: ResultStatus, | ||
| options: ParserOptions | ||
| ) => { | ||
| let message = '' | ||
|
|
||
| const includeStdout = !(status === 'passed' && options.skipStdout === 'on-success') | ||
| const includeStderr = !(status === 'passed' && options.skipStderr === 'on-success') | ||
|
|
||
| if (includeStdout && statusDetails?.message) { | ||
| message += `<pre><code>${escapeHtml(statusDetails.message)}</code></pre>` | ||
| } | ||
| if (includeStderr && statusDetails?.trace) { | ||
| message += `<pre><code>${escapeHtml(statusDetails.trace)}</code></pre>` | ||
| } |
There was a problem hiding this comment.
Semantic mismatch with skip options: skipStdout controls whether statusDetails.message is included, and skipStderr controls statusDetails.trace. However, Allure's message is an assertion/error message (not stdout) and trace is a stack trace (not stderr). A user setting --skip-report-stdout on-success would expect actual stdout to be suppressed, not the error summary.
This is a pragmatic mapping (the existing parsers have a similar loose correspondence), but it's worth at minimum a code comment documenting this semantic difference. Consider adding:
// Allure doesn't have stdout/stderr; we map message → stdout and trace → stderr
// for consistency with the skip options used by JUnit and Playwright parsers.| describe('Allure results parsing', () => { | ||
| beforeAll(async () => { | ||
| testcases = await parseAllureResults(allureParserBasePath, allureParserBasePath, { | ||
| skipStdout: 'never', | ||
| skipStderr: 'never', | ||
| }) |
There was a problem hiding this comment.
Missing test coverage for skipStdout/skipStderr on-success behavior: Every test here uses skipStdout: 'never' and skipStderr: 'never'. The buildMessage function has branching logic for on-success that is never exercised. The JUnit and Playwright parser tests each have dedicated tests for this behavior.
Suggested additions:
- Test that
messageis omitted for passed tests whenskipStdoutison-success - Test that
traceis omitted for passed tests whenskipStderrison-success - Test that both are still included for failed tests with
on-successoptions
1 participant
No description provided.