Added Olympix static analysis with alerts review #1001
Conversation
WalkthroughThis pull request introduces two new GitHub Actions workflows that operate on events affecting Solidity files. The first workflow performs static analysis security checks using the Olympix integrated security tool, while the second workflow retrieves and reviews security alerts by extracting alert data and posting or updating comments on pull requests. Both workflows are triggered by specific pull request events and changes made to Solidity code. Changes
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/olympixStaticAnalysis.yml (1)
20-21: Update Checkout Action Version
The static analysis tool (actionlint) flagged that usingactions/checkout@v3may be outdated. Please consider updating to a newer version (e.g.,actions/[email protected]or the latest available) to ensure better performance and compatibility.- - name: Checkout Repository - uses: actions/checkout@v3 + - name: Checkout Repository + uses: actions/[email protected]🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/securityAlertsReview.yml (4)
27-38: Robustness of PR Number Fetching
The current approach fetches the first open PR number from the GitHub API. This may not reliably identify the PR related to the workflow event if multiple pull requests are open. Consider leveraging the GitHub event payload (such asgithub.event.pull_request.number) to reliably obtain the PR number.Example suggestion using the GitHub context:
- PR_NUMBER=$(curl -s -H "Authorization: token ${GITHUB_TOKEN}" \ - "https://api.github.com/repos/${{ github.repository }}/pulls?state=open" | jq -r '.[0].number') + PR_NUMBER=${{ github.event.pull_request.number }}
43-44: Remove Trailing Whitespace
There are trailing spaces detected at line 44. Please remove them to adhere to YAML lint standards.Example diff:
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: trailing spaces
(trailing-spaces)
80-86: Cleanup in Comment Search Step
In the "Find Existing PR Comment" step, trailing whitespace is also detected at line 86. Remove the extra spaces for cleaner formatting. Additionally, while the current jq filter works, ensure that the matching string uniquely identifies your workflow’s comment to avoid unintended matches.Example diff for whitespace removal:
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 86-86: trailing spaces
(trailing-spaces)
1-195: Overall Workflow Review and Testing Note
Both workflows (olympixStaticAnalysis.ymlandsecurityAlertsReview.yml) are well-integrated and enhance the security analysis process for Solidity files. However, note that tests for these workflows have not been added yet. Adding automated tests (or at least a dry-run validation mechanism) will help ensure that these workflows behave as expected across different scenarios.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/olympixStaticAnalysis.yml(1 hunks).github/workflows/securityAlertsReview.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/olympixStaticAnalysis.yml
21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
.github/workflows/securityAlertsReview.yml
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 86-86: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (5)
.github/workflows/olympixStaticAnalysis.yml (3)
1-19: Workflow Configuration Overview
The workflow is well-structured and clearly defines the trigger conditions, steps to obtain changed Solidity files, convert them into command-line arguments, run the Olympix Integrated Security tool, and finally upload the SARIF report. Ensure that the secretOLYMPIX_API_TOKENis properly configured in your repository settings.
22-37: Changed Files Argument Formatting
The conversion of changed files into command-line arguments is implemented neatly. It correctly usesxargsand appends the result to the$GITHUB_ENVfor later use.
39-52: Integrated Security Step and SARIF Upload
The integration of the Olympix security tool along with the subsequent upload of the SARIF report is clear and follows best practices with conditional execution based on file changes..github/workflows/securityAlertsReview.yml (2)
97-173: Post or Update PR Comment Logic
The logic for either updating an existing comment or posting a new one based on the presence of previous comments is implemented clearly. Ensure that the JSON payload is properly escaped if any of the alert data might include special characters. Overall, the use of curl with PATCH/POST requests appears correct.
174-195: Workflow Fail Conditions and Final Checks
The block that checks for unresolved alerts and alerts dismissed without comments is clear and functions as expected to fail the workflow when necessary. This ensures that security concerns are addressed before merging.
Test Coverage ReportLine Coverage: 79.05% (2249 / 2845 lines) |
…ckout version. Adde check if Static Analysis has run at least once
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/olympixStaticAnalysis.yml (1)
51-55: Uploading SARIF Report & Trailing Newline Concern
The final step uploads the SARIF report to GitHub’s code scanning. However, static analysis has flagged that there is no newline at the end of the file (line 55). Please add a newline at the end of the file to conform to YAML linting standards.Suggested diff:
- sarif_file: olympix.sarif \ No newline at end of file + sarif_file: olympix.sarif +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/securityAlertsReview.yml (3)
21-50: Validating Olympix Static Analysis Execution
The script in the “Check if Olympix Static Analysis has run at least once and was successful” step is well thought out. It handles a missing branch name by falling back to alternative GitHub context and uses a curl call with jq to extract the workflow status.Note: Although the logic is solid, you might consider adding error handling for a failed API call (for example, checking if
LATEST_RUNis empty) to make the step more robust.🧰 Tools
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 44-44: YAMLlint: trailing spaces detected.
70-115: Fetching and Parsing Security Alerts
The step that fetches security alerts using the GitHub API and parses them with jq is implemented in a clear, modular manner. It divides alerts into unresolved, dismissed without comments, and dismissed with comments.Suggestion: Consider adding basic error handling after the curl request (e.g., checking for a non-empty response or HTTP error code) to improve robustness.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 94-94: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: SC Core Dev Approval Check
[error] 86-86: YAMLlint: trailing spaces detected.
75-75: Removal of Trailing Spaces
Static analysis and pipeline checks have detected trailing whitespace issues at several lines (specifically around lines 75, 94, 122, and 180 as noted by YAMLlint and other tools). Please remove any trailing spaces to ensure compliance with YAML formatting standards.Suggested action: Review the affected lines in your editor (or use an automated formatter) and remove the extra spaces.
Also applies to: 94-94, 122-122, 180-180
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 75-75: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/olympixStaticAnalysis.yml(1 hunks).github/workflows/securityAlertsReview.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/securityAlertsReview.yml
[error] 75-75: trailing spaces
(trailing-spaces)
[error] 94-94: trailing spaces
(trailing-spaces)
[error] 122-122: trailing spaces
(trailing-spaces)
[error] 180-180: trailing spaces
(trailing-spaces)
.github/workflows/olympixStaticAnalysis.yml
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: SC Core Dev Approval Check
.github/workflows/securityAlertsReview.yml
[error] 44-44: YAMLlint: trailing spaces detected.
[error] 86-86: YAMLlint: trailing spaces detected.
.github/workflows/olympixStaticAnalysis.yml
[warning] 21-21: actionlint: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (14)
.github/workflows/olympixStaticAnalysis.yml (6)
1-7: Header Comment and Workflow Description
The introductory comments clearly summarize the purpose and behavior of this workflow. They describe that the action runs the Olympix static analyzer on newly added/modified Solidity files and uploads the SARIF report. This aligns well with the previous feedback requesting an initial summary.
8-17: Trigger Configuration Validation
Theonblock correctly triggers the workflow on pull request events (opened, synchronize, reopened, ready_for_review) and limits file scanning to Solidity files undersrc/**. This focused triggering is appropriate for reducing unnecessary scans.
24-26: Checkout Step is Up-to-date
The checkout step usesactions/checkout@v4, which addresses previous concerns about using outdated versions. This ensures that the repository is checked out reliably on the latest GitHub-hosted runners.
27-33: Changed Files Retrieval Step
Usingtj-actions/changed-files@v45to capture added, renamed, and modified Solidity files is well implemented. This will ensure that only the necessary files are passed to the analysis steps.
34-42: Formatting Changed Files into Arguments
The step to convert the list of changed files into command-line arguments (usingxargsandprintf) is concise and effective. It correctly exports the arguments via theGITHUB_ENVvariable.
43-50: Running the Olympix Integrated Security Tool
The integration of the Olympix security tool, including authentication via${{ secrets.OLYMPIX_API_TOKEN }}and proper argument passing, is correctly implemented. This step will generate the SARIF report for upload..github/workflows/securityAlertsReview.yml (8)
1-9: Workflow Header and Documentation
The header section provides a detailed description of the workflow’s purpose and behavior, including the security alert review process and PR state management. This introductory comment meets the earlier request for a summary at the beginning of the file.
10-15: Trigger and Manual Dispatch Configuration
The workflow triggers on theready_for_reviewevent and also supports manual triggering viaworkflow_dispatch. This dual trigger mechanism offers flexibility.
51-55: Repository Checkout and PR Identification
The action now checks out the repository (usingactions/checkout@v4) and usesjwalton/gh-find-current-pr@masterto identify the current PR. This approach is acceptable given the limitations of GitHub’s context variables in various event types.
56-69: PR Number Validation
The “Validate and set PR Number” step correctly handles the scenario where a pull request might not be found, exiting with an error if necessary. Storing the PR number in the environment for reuse is a good practice.
116-132: Existing PR Comment Lookup
The mechanism using curl and jq to determine if an existing comment (starting with the defined header) is present is effective. This avoids duplicating comments on subsequent workflow runs.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 122-122: trailing spaces
(trailing-spaces)
133-233: Constructing and Posting the PR Comment
The script to build the comment body is comprehensive. It iteratively constructs sections for unresolved alerts, dismissed alerts missing comments, invalid dismissals, and valid dismissals, then either updates an existing comment or creates a new one. The formatting using Markdown enhances readability in the PR.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 180-180: trailing spaces
(trailing-spaces)
234-268: Failing the Action & Reverting PR to Draft
The final step assesses if any blocking security issues exist and, if so, reverts the PR to draft using GitHub’s GraphQL API. This is a proactive measure to prevent merging when security alerts are present.Note: Ensure that the reliance on
${{ github.event.pull_request.node_id }}is valid for all event types that trigger this workflow.
269-269: Success Message
The final echo statement confirms that no blocking issues exist. This provides clear feedback in the workflow logs.
Which Jira task belongs to this PR?
LF-11765
Why did I implement it this way?
Explained in the notion page: https://www.notion.so/lifi/Olympix-Static-Analysis-tool-GitHub-integration-19df0ff14ac7801eaba6c7e757a61637
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)