fix(#25): fix push-triggered security jobs failing on main

[OmarEhab007]

Summary

• Semgrep SAST: restricted to pull_request events only — returntocorp/semgrep-action@v1 is unreliable on push events without SEMGREP_APP_TOKEN; CodeQL already covers push-to-main deep SAST analysis
• Secrets Detection (TruffleHog): fixed base/head refs for push events — use github.event.before as base and github.sha as head so diff range is meaningful after a squash-merge to main; added guard to skip initial-commit pushes (all-zeros before SHA)
• Added docs/agdr/AgDR-0002-security-push-trigger-fix.md recording the decision

Root Causes

1. TruffleHog: base: ${{ github.event.repository.default_branch }} resolved to the string "main" on push events. After a squash-merge, base: main (branch tip) and head: HEAD are the same commit → error: unable to resolve ref: no base refs succeeded for base: 'main'
2. Semgrep: returntocorp/semgrep-action@v1 fails on push events without SEMGREP_APP_TOKEN (rate-limited / auth); scan is redundant post-merge since the PR scan ran first

Test plan

• Merge this PR and verify Security Scan / Secrets Detection (push) passes on the resulting push to main
• Verify Security Scan / Semgrep SAST is skipped (condition github.event_name == 'pull_request' false on push)
• Open a follow-up test PR and verify both Semgrep SAST and Secrets Detection run normally on the PR event

Closes #25

---

Glossary

Term	Definition
github.event.before	SHA of the commit on the ref immediately before the push
github.sha	The commit SHA that triggered the workflow run
Squash merge	GitHub strategy that collapses all PR commits into one on main
returntocorp/semgrep-action@v1	Semgrep's GitHub Actions integration — diff-based SAST, best suited to PR events

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@OmarEhab007 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 33 minutes and 49 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 49 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 92eefaed-9c3a-4e9a-94ba-caa6be9f10f6

📥 Commits

Reviewing files that changed from the base of the PR and between 33d302a and 549669d.

📒 Files selected for processing (4)

• .github/workflows/security.yml
• .trivyignore
• docs/agdr/AgDR-0002-security-push-trigger-fix.md
• frontend/Dockerfile

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/#25-security-push-trigger-failures

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[OmarEhab007]

Code Review: PR #26

Commit: `a6ea52205c93f6c9f6614d8753427b8f8baaa470`

Summary

This PR fixes two broken push-triggered jobs in `.github/workflows/security.yml` by restricting Semgrep to PR events and correcting TruffleHog's base/head refs for push events. It also bumps the frontend base image from `node:20-alpine` to `node:22-alpine` and trims the `.trivyignore` list to match. An AgDR documents the decisions. The fixes are broadly correct and well-reasoned, but there are two issues — one blocking, one non-blocking — that need to be addressed before merge.

---

Checklist Results

• Architecture & Design: N/A (CI config only)
• Code Quality: PASS
• Testing: PASS (test plan in description, manual verification required by nature)
• Security: FAIL (one blocking issue — see below)
• Performance: N/A
• PR Description & Glossary: PASS
• Technical Decisions (AgDR): PASS — AgDR-0002 is linked and covers both decisions

---

Issues Found

BLOCKING — Force-push silently widens TruffleHog scan to full history

File: .github/workflows/security.yml, secrets job
Line: base: ${{ github.event_name == 'pull_request' && github.event.repository.default_branch || github.event.before }}

On a force-push to main, github.event.before is the SHA that was at the tip of main before the force-push, which may be an ancestor of the new HEAD, an unrelated commit, or — in the case of a non-fast-forward force-push — a SHA that is no longer reachable in the repository. When TruffleHog receives an unreachable base SHA it falls back to a full-history scan. This means:

1. Force-pushes silently trigger a significantly broader (and slower) scan with no indication to the operator that the diff range changed.
2. If the SHA is completely unreachable, the job may still fail with a ref-resolution error — the same class of failure this PR is trying to fix.

The PR description mentions the all-zeros guard for initial pushes but does not address force-pushes. Force-pushes to main should be either blocked by branch protection (preferred) or handled explicitly.

Recommended fix (choose one):

Option A — Rely on branch protection. If main has "Require linear history" or "Do not allow force pushes" enabled (which it should for a production branch), this is a non-issue in practice. Add a comment to the YAML noting the assumption and add it to the AgDR's Consequences section.

Option B — Add an explicit guard. Detect force-push via github.event.forced and either skip the job or default to a shallow scan:

if: |
  github.event_name != 'push' ||
  (github.event.before != '0000000000000000000000000000000000000000' &&
   github.event.forced != true)

This keeps behavior predictable: TruffleHog only runs on normal pushes where the before-SHA is guaranteed to be a reachable ancestor.

---

NON-BLOCKING — TruffleHog ternary uses the non-null-safe short-circuit form

File: .github/workflows/security.yml, secrets job
Lines:

base: ${{ github.event_name == 'pull_request' && github.event.repository.default_branch || github.event.before }}
head: ${{ github.event_name == 'pull_request' && 'HEAD' || github.sha }}

GitHub Actions expressions do not have a native ternary operator. The A && B || C pattern acts as a ternary only when B is always truthy. For the base line, github.event.repository.default_branch is a string that evaluates to truthy when non-empty — so it is safe in practice. For the head line, the string literal 'HEAD' is always truthy. This is fine as written, but the pattern is fragile: if default_branch were ever empty (e.g. on a misconfigured repo), the expression silently falls through to github.event.before on a PR event. A brief inline comment would help the next reader:

# Ternary: (event == 'pull_request') ? default_branch : event.before
base: ${{ github.event_name == 'pull_request' && github.event.repository.default_branch || github.event.before }}

---

NON-BLOCKING — node:22-alpine bump is undocumented in the PR summary

File: frontend/Dockerfile

All three stages are bumped from node:20-alpine to node:22-alpine. This is the right call (it cleans up 11 CVEs from .trivyignore). However, the PR title and summary only mention "fix push-triggered security jobs" — a reader doing a post-merge audit would not expect a Node.js version bump. Add one bullet to "## Summary" so the change is visible without reading the full diff.

---

Positive Notes

• Root-cause diagnosis for both failures is correct and clearly documented.
• The all-zeros guard for initial pushes matches TruffleHog's own documentation and is the right approach.
• Restricting Semgrep to PR events is the correct trade-off: CodeQL already covers push-to-main SAST; Semgrep's value is in fast PR-time feedback.
• AgDR-0002 follows the template, covers both decisions, documents options and consequences, and is linked from the PR description.
• Glossary is present and accurate.
• fetch-depth: 0 is correctly in place on the secrets job checkout — TruffleHog needs full history to resolve refs.
• Removing 11 stale CVE ignores from .trivyignore (resolved by the node:22 upgrade) is clean hygiene.

---

Verdict

CHANGES REQUESTED (cannot use the GitHub UI to request-changes on own PR — treated as blocking)

The force-push edge case on the TruffleHog base ref is a security-coverage gap: a force-push to main could cause TruffleHog to silently do a full-history scan or fail with an unreachable-ref error, neither of which is the intended behavior. Address via branch protection confirmation + AgDR update (Option A) or the explicit github.event.forced guard (Option B) before merge. The two non-blocking notes are at your discretion.

---

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: `a6ea52205c93f6c9f6614d8753427b8f8baaa470`