feat: add missing security workflows (dependency-review, trivy, zizmor)#56
feat: add missing security workflows (dependency-review, trivy, zizmor)#56
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Set Complaice |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 862d41e3e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds the missing GitHub Actions security workflows referenced in the repo’s compliance/security documentation, closing the gap between documented controls and implemented controls.
Changes:
- Adds a Dependency Review workflow to block PRs that introduce HIGH/CRITICAL vulnerable dependencies.
- Adds a Trivy filesystem scan workflow that uploads SARIF results to GitHub Code Scanning (with fork-PR upload safeguards).
- Adds a zizmor workflow to audit workflow-file changes in advisory mode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/dependency-review.yml |
Adds dependency diff vulnerability gating for PRs into master/work. |
.github/workflows/trivy.yml |
Adds Trivy FS scanning and SARIF upload to Code Scanning with fork-PR upload skip logic. |
.github/workflows/zizmor.yml |
Adds advisory zizmor auditing for changes under .github/workflows/**. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: | | ||
| zizmor --format plain .github/workflows/ | ||
| EXIT_CODE=$? | ||
| if [ $EXIT_CODE -ne 0 ]; then | ||
| echo "::warning::zizmor found workflow security findings (advisory). Review the output above before merging." | ||
| fi | ||
| exit 0 |
| permissions: | ||
| contents: read | ||
| security-events: write | ||
|
|
||
| jobs: | ||
| trivy: | ||
| name: Trivy filesystem scan | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - name: Run Trivy filesystem scan | ||
| uses: aquasecurity/trivy-action@6e7b7d1fd3e4fef0c5fa8cce1229c54b2c9bd0d8 # v0.30.0 |
|
This is a strong narrow candidate for near-term merge pending focused human review of workflow permissions and SARIF behavior. |
|
This is a strong, narrow candidate for near-term merge pending focused human review of workflow permissions and SARIF behavior. |
chrismaz11
added
the
merge-soon
chrismaz11
force-pushed
the
copilot/setup-trustsignal-dev-org
branch
from
e9c90fb to
774b905
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 774b905bf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| branches: | ||
| - master | ||
| - work |
There was a problem hiding this comment.
This trigger only matches PRs into master/work, so PRs targeting main will never execute the blocking dependency diff check. Given the repository’s branch-protection guidance and required-check setup are defined for main, this leaves the intended high/critical dependency gate inactive on the branch where merges are expected; include main (or remove the branch filter) so the control actually enforces there.
Useful? React with 👍 / 👎.
| - master | ||
| - work | ||
| pull_request: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
The Trivy workflow is scoped to master/work, so it will not run for activity on main; that means no filesystem scan and no SARIF refresh for the primary protected branch described in the repo docs. Add main to both push and pull_request branch filters (or broaden the trigger) to avoid silently losing security coverage on the branch used for compliance checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6441e5c26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!parsed.password) { | ||
| parsed.password = encodeURIComponent(supabasePassword); |
There was a problem hiding this comment.
In the SUPABASE_DB_PASSWORD fallback path, this check only injects the provided password when parsed.password is empty, but Supabase pooler URLs can contain placeholder credentials like [YOUR-PASSWORD] that parse as a non-empty password. In that case, DATABASE_URL is set to a URL with invalid credentials and Prisma startup/auth fails even though the real password is present in SUPABASE_DB_PASSWORD. Treat placeholder/stale passwords as missing (or overwrite unconditionally in this fallback branch).
Useful? React with 👍 / 👎.
| path.resolve(process.cwd(), '../../.env.local'), | ||
| path.resolve(process.cwd(), '../../.env') |
There was a problem hiding this comment.
These fallback candidates walk two directories up from the current working directory; when the API is started from the repository root, they resolve to /.env.local and /.env. That allows unrelated host-level env files to be loaded and silently influence runtime configuration (including DB credentials), making environment resolution nondeterministic across runners and developer machines. Limit fallback paths to known project locations instead of traversing above repo root.
Useful? React with 👍 / 👎.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| function stripHtml(text: string): string { | ||
| return text | ||
| .replace(/<script[\s\S]*?<\/script>/gi, ' ') |
Check failure
Code scanning / CodeQL
Bad HTML filtering regexp High
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general terms, the fix is to improve the script-stripping regular expression so that it correctly matches script end tags even when they contain whitespace or attributes, aligning with how browsers actually recognize script end tags. We should still avoid trying to be a full HTML parser, but we can handle the specific weakness CodeQL flags by widening the closing-tag pattern without changing the rest of the function.
Concretely, in stripHtml in apps/api/src/services/registryAdapters.ts, update the first .replace call from:
.replace(/<script[\s\S]*?<\/script>/gi, ' ')to a pattern that accepts optional whitespace and attributes after </script and before >; for example:
.replace(/<script\b[\s\S]*?<\/script[^>]*>/gi, ' ')Changes:
- Add
\bafterscriptin the opening tag so<scripting>is not matched. - Change the closing tag from
<\/script>to<\/script[^>]*>so constructs like</script foo="bar">or</script >are matched.
This keeps the overall behavior (removing script blocks) while covering browser-tolerated closing tag variations. No new imports or helpers are needed, and all other.replacecalls instripHtmlremain unchanged.
| @@ -809,7 +809,7 @@ | ||
|
|
||
| function stripHtml(text: string): string { | ||
| return text | ||
| .replace(/<script[\s\S]*?<\/script>/gi, ' ') | ||
| .replace(/<script\b[\s\S]*?<\/script[^>]*>/gi, ' ') | ||
| .replace(/<style[\s\S]*?<\/style>/gi, ' ') | ||
| .replace(/<\/?(br|p|tr|td|th|li|div|section|article|h[1-6])[^>]*>/gi, ' ') | ||
| .replace(/<[^>]+>/g, ' ') |
chrismaz11
committed this autofix suggestion 5 days ago.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc4b32342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # ============================================================= | ||
|
|
||
| API_URL="https://api.trustsignal.dev" | ||
| API_KEY="d4a2bd92be56c54905a99f2b5709e14064e9eaeb99c44aa74898125aedf5028a" |
There was a problem hiding this comment.
This script hardcodes a bearer API key and targets the production api.trustsignal.dev endpoint by default, which exposes a reusable credential to anyone with repository access and allows accidental production writes when developers run local tests. Because this is a direct secret-management and compliance breach, the script should read both values from environment variables and fail fast when they are not provided.
Useful? React with 👍 / 👎.
Applied both review thread changes in commit
|
|
@copilot apply changes based on the comments in this thread |
Both review thread changes were already applied in commit
|
Co-authored-by: chrismaz11 <24700273+chrismaz11@users.noreply.github.com>
chrismaz11
force-pushed
the
copilot/setup-trustsignal-dev-org
branch
from
7d1d0a7 to
5274888
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52748881d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reasons: JSON.parse(record.reasons) as string[], | ||
| riskScore: record.riskScore, | ||
| verifierId: 'deed-shield', | ||
| verifierId: 'trustsignal', |
There was a problem hiding this comment.
Keep backward compatibility for previously issued receipts: receiptFromDb now hardcodes verifierId to trustsignal, but historical rows were hashed/signed with deed-shield and verifyStoredReceipt recomputes integrity from this reconstructed payload. Because verifierId is part of the unsigned payload hash/signature, older receipts will start failing integrity/signature verification after deploy even though the stored receipt data is unchanged. Persisting/verifying the original verifier ID (or handling legacy values during hydration) is needed to avoid breaking existing receipts.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| apiKey: string, | ||
| secret = process.env.API_KEY_FINGERPRINT_SECRET || DEV_API_KEY_FINGERPRINT_SECRET | ||
| ): string { | ||
| return createHmac('sha256', secret).update(apiKey).digest('hex').slice(0, 16); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
General fix: Replace the use of a single-iteration fast hash (HMAC‑SHA256) over the API key with a password hashing / key-derivation function designed to be computationally expensive, such as PBKDF2, bcrypt, scrypt, or Argon2. Since we must avoid new external dependencies and stay within the shown file, PBKDF2 from Node’s crypto module is the most practical: it is standard, well-supported, and configurable (iterations, key length, digest).
Concrete approach here:
- Switch from
createHmac('sha256', secret).update(apiKey).digest('hex')topbkdf2Sync(apiKey, secret, iterations, keylen, 'sha256'). - Keep the existing API of
fingerprintApiKey(same parameters and return typestring, still returning a short hex string) to avoid altering surrounding functionality. - Use a reasonable default iteration count and key length (for example, 100,000 iterations and 32-byte output) and then truncate to 16 hex characters as before so callers remain unaffected.
- Import
pbkdf2Syncfromnode:cryptoalongside existing imports.
All changes are localized to apps/api/src/security.ts: one import line change and one function body change (in fingerprintApiKey). No other functions or call sites need modification.
| @@ -1,4 +1,4 @@ | ||
| import { createHmac, generateKeyPairSync } from 'node:crypto'; | ||
| import { createHmac, generateKeyPairSync, pbkdf2Sync } from 'node:crypto'; | ||
|
|
||
| import { getAddress, verifyMessage } from 'ethers'; | ||
| import { FastifyReply, FastifyRequest } from 'fastify'; | ||
| @@ -273,7 +273,12 @@ | ||
| apiKey: string, | ||
| secret = process.env.API_KEY_FINGERPRINT_SECRET || DEV_API_KEY_FINGERPRINT_SECRET | ||
| ): string { | ||
| return createHmac('sha256', secret).update(apiKey).digest('hex').slice(0, 16); | ||
| // Derive a computationally expensive fingerprint from the API key using PBKDF2. | ||
| // Keep the output format and truncation length the same to avoid changing callers. | ||
| const iterations = 100_000; | ||
| const keyLength = 32; // 32 bytes -> 64 hex chars before truncation | ||
| const derived = pbkdf2Sync(apiKey, secret, iterations, keyLength, 'sha256'); | ||
| return derived.toString('hex').slice(0, 16); | ||
| } | ||
|
|
||
| export function requireApiKeyScope(config: SecurityConfig, requiredScope: AuthScope) { |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a037894368
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| "dependencies": { | ||
| "@deed-shield/core": "file:../../packages/core", | ||
| "@trustsignal/core": "file:../../packages/core", |
There was a problem hiding this comment.
This change switches apps/api to depend on @trustsignal/core, but the committed apps/api/package-lock.json still tracks @deed-shield/core, so clean installs in that workspace now fail (npm -C apps/api ci --dry-run reports the package/lock mismatch and missing @trustsignal/core). That breaks deterministic installs for anyone running CI or local setup from apps/api directly; update and commit the workspace lockfile alongside this dependency rename.
Useful? React with 👍 / 👎.
| permissions: | ||
| contents: read | ||
| security-events: write |
There was a problem hiding this comment.
security-events: write is declared at workflow scope, so the third-party aquasecurity/trivy-action step runs with that write-capable token too. This expands blast radius unnecessarily and contradicts the stated least-privilege intent in the workflow comments; scope security-events: write only to the SARIF upload job/step so scanner execution keeps read-only permissions.
Useful? React with 👍 / 👎.
|
Superseded by #92 which has the merge conflicts resolved. |
Three GitHub Actions security workflows referenced throughout compliance docs (
security-workflows.md,github-settings-checklist.md,security-posture.md) did not exist in the repo, creating a gap between documented controls and implemented controls. This resolves that gap.Summary
.github/workflows/dependency-review.yml— blocks PRs onmaster/workthat introduce HIGH/CRITICAL dependency vulnerabilities viaactions/dependency-review-action@v4.7.1.github/workflows/trivy.yml— filesystem scan on every push/PR; split into two jobs to enforce least-privilege: thetrivyjob runs withcontents: readonly, uploads SARIF as a workflow artifact; theupload-sarifjob holdssecurity-events: writeexclusively and pushes SARIF to code scanning (skipped on forked PRs); the third-partyaquasecurity/trivy-actionstep never runs with the elevated token.github/workflows/zizmor.yml— audits.github/workflows/**changes for workflow security issues viapip install zizmor; advisory mode usescontinue-on-error: trueso genuine runtime failures (tool crash, bad args) remain visible in the job summary while audit findings do not block the PRAll three workflows:
persist-credentials: falseon checkoutpermissions: contents: readat workflow level;security-events: writescoped only to the SARIF upload job intrivy.ymlWhat still requires manual GitHub UI action
Branch protection rulesets, Dependabot alerts, secret scanning, and CodeQL default setup — see
docs/github-settings-checklist.md.AI Disclosure (optional)
Review Checklist
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.