-
-
Notifications
You must be signed in to change notification settings - Fork 656
feat: implement enforced Semgrep scanning with clean baseline (#2984) #3406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
8d4456c
8cc47c8
7dbe9cf
0a2d82b
7a4b09a
a3f9287
180901e
c22ec83
9ba1a8c
65ab648
4109040
56b3c69
ea7a930
a2bb8e3
7b49f9a
ba59528
441b7f9
491dfe3
5da4042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| name: Semgrep Scan | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - "feature/**" | ||
| push: | ||
| branches: | ||
| - main | ||
| - "feature/**" | ||
|
|
||
| permissions: | ||
| contents: read | ||
| security-events: write | ||
|
|
||
| jobs: | ||
| semgrep: | ||
| name: Run Semgrep Security Scan | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 | ||
|
|
||
| - name: Run Semgrep | ||
| uses: semgrep/semgrep-action@2f647c0b06b515d0da467389a425a898668700a0 | ||
| with: | ||
| config: >- | ||
| p/owasp-top-ten | ||
| p/python | ||
| p/javascript | ||
| continue-on-error: false |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,3 +87,9 @@ repos: | |
| rev: v2.11.1 | ||
| hooks: | ||
| - id: pyproject-fmt | ||
|
|
||
| - repo: https://github.com/semgrep/semgrep | ||
| rev: v1.148.0 | ||
| hooks: | ||
| - id: semgrep | ||
| args: [--config=p/owasp-top-ten, --config=p/python, --config=p/javascript, --error] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use consistent syntax
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also use centralized configuration file for both pre-commit and ci/cd workflow.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, we can create a centralized .semgrep.yml file in the root directory to act as the single source of truth. I’ll then refactor the .pre-commit-config.yaml and the CI/CD workflow to point to it so the rules stay consistent everywhere. I'll get started on this refactor and push the changes for your review! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| .venv/ | ||
| node_modules/ | ||
| backend/static/ | ||
| frontend/.next/ | ||
| .git/ | ||
| .github/ | ||
| backend/data/ | ||
| backend/poetry.lock | ||
| frontend/src/app/organizations/[organizationKey]/layout.tsx | ||
|
SpruhaCK marked this conversation as resolved.
Outdated
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be fixed -- not ignored. What's the exact issue with this file?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue Semgrep flagged in layout.tsx was a potential Client-Side Injection vulnerability. Because the organizationKey is a dynamic route parameter, Semgrep identified it as untrusted user input that was being used in a context (like a metadata field or a direct UI element) where it could lead to XSS if not handled carefully. I used .semgrepignore to keep the initial CI setup clean, but I'm now looking at implementing a proper fix either by ensuring the input is strictly validated or by using a safe React rendering pattern to neutralize the risk.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking of using a strict regex validation for organizationKey prop or is there a centralized utility in the Nest codebase you'd prefer I use for sanitizing these route parameters? |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need it to be part of the main ci/cd pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I'll integrate the Semgrep job directly into the main CI/CD workflow i.e. run-ci-cd.yaml as a required dependency.