security-review

Context

Every PR that touches authentication, credentials, environment variables, file system operations, child process execution, user input handling, GitHub API calls, or workflow files must be reviewed for security. This skill provides a systematic checklist for catching security issues before they reach production.

Use this skill when a PR includes any of:

• Changes to .github/workflows/ files
• New or modified environment variable usage
• File system read/write operations driven by user input
• Child process execution (child_process.exec, child_process.spawn)
• GitHub API calls (Octokit, gh CLI wrappers)
• New npm dependencies
• Credential or token handling
• Template rendering with dynamic values

Patterns

1. Secrets and Credentials

No secrets in source code — ever.

Review checklist:

• ❌ No API keys, tokens, passwords, or connection strings in committed files
• ❌ No hardcoded GITHUB_TOKEN, NPM_TOKEN, or similar values
• ❌ No .env files committed (verify .gitignore covers them)
• ✅ Secrets must be referenced via process.env.X or ${{ secrets.X }} in workflows
• ✅ If the PR adds a new secret reference, verify it's documented in setup instructions

Watch for disguised secrets:

• Base64-encoded strings that decode to tokens
• URLs with embedded credentials (https://user:token@host)
• Test fixtures that contain real credentials from copy-paste

2. Personal Data in Committed Files

Personal data (especially email addresses) must never be written to committed files.

Squad-specific rule: git config user.email is explicitly banned from being written to any file that gets committed. This prevents PII leakage into the repository.

Review checklist:

• ❌ No email addresses written to committed files (even in config generation)
• ❌ No git config user.email output captured and stored in source
• ✅ If user identity is needed at runtime, read it dynamically and never persist to a committed file

3. Environment Variable Safety

Review checklist:

• Are environment variables validated before use? (e.g., checking for undefined)
• Are sensitive environment variables excluded from logs and error messages?
• Does the PR avoid writing environment variable values to committed files?
• If a new process.env.X reference is added, is the variable documented?

4. File System Safety

Path traversal prevention:

• ❌ No direct concatenation of user input into file paths (path.join(baseDir, userInput) without validation)
• ✅ Validate that resolved paths stay within expected boundaries (use path.resolve and verify the result starts with the expected base directory)
• ✅ Reject paths containing .., absolute paths when relative are expected, or null bytes

Arbitrary file write prevention:

• If user input determines a file path, verify the code constrains writes to a known safe directory
• Template rendering that writes files should validate output paths against a whitelist or known directory

5. Child Process Safety

Shell injection prevention:

• ❌ No unsanitized user input passed to child_process.exec() (exec uses a shell and is vulnerable to injection)
• ✅ Prefer child_process.execFile() or child_process.spawn() with argument arrays (no shell interpretation)
• ✅ If exec must be used, validate/sanitize all interpolated values
• ✅ Check for template literals in shell commands — exec(\git commit -m "${userMessage}"`)` is injectable

Review checklist for gh CLI wrappers:

• The gh-cli.ts module wraps GitHub CLI calls. Verify that arguments are passed as array elements, not interpolated into a shell string
• If the PR modifies gh-cli.ts or adds new gh commands, check for injection vectors

6. GitHub Actions Workflow Security

pull_request_target safety:

• pull_request_target + checking out PR code = critical security risk (the workflow runs with write token but executes untrusted code from the fork)
• pull_request_target + NO code checkout = safe (write token used only with trusted workflow code)
• If a PR adds pull_request_target, verify it does NOT checkout ${{ github.event.pull_request.head.ref }} or ${{ github.event.pull_request.head.sha }}

Token scope minimization:

• Workflows should declare explicit permissions: blocks
• Follow principle of least privilege — no unnecessary write permissions
• If the PR adds permissions: write-all or omits permissions (defaults to broad), flag it

Recursion safety:

• GITHUB_TOKEN-generated events do NOT trigger new workflow runs (GitHub prevents infinite loops)
• If a workflow uses a PAT (Personal Access Token) instead of GITHUB_TOKEN, it CAN trigger downstream workflows — verify this is intentional

Secrets in workflows:

• ❌ No ${{ secrets.X }} in workflow run: steps where the value could leak to logs
• ✅ Use ::add-mask:: to mask secret values in workflow output
• ✅ Minimize the number of steps that have access to secrets

7. Git Operation Safety

These rules are from the project's mandatory Git Safety guidelines:

Staging safety:

• ❌ No git add . or git add -A — these stage unintended deletions from incomplete working trees
• ❌ No git commit -a — same risk
• ✅ Only git add path/to/specific/file.ts with explicit file paths

Push safety:

• ❌ No git push --force or --force-with-lease to shared branches (dev, main)
• ❌ No direct push to dev or main — must use a PR
• ✅ Feature branches only: squad/{issue-number}-{slug}

If a PR adds git operations (e.g., in a script, workflow, or agent action), verify every git add, git commit, and git push command follows these rules.

8. Export/Import Integrity

Typos in export names cause silent runtime failures — the export resolves to undefined instead of throwing.

Review checklist:

• Check for typos in export names (e.g., FSStorageProvidr vs. FSStorageProvider)
• Verify new re-exports actually reference existing modules
• If the PR renames an export, confirm all import sites are updated
• Watch for export { X as Y } where X doesn't exist — TypeScript may not catch this in all configurations

9. Dependency Supply Chain

New npm dependencies should be audited:

• Is the package well-maintained? (check last publish date, download count, open issues)
• Does the package have known vulnerabilities? (npm audit)
• Is there a built-in Node.js alternative? (prefer node:fs, node:path, node:crypto over third-party equivalents)
• Does the package pull in a large transitive dependency tree?
• Is the package from a trusted publisher?

Squad-specific: Protected bootstrap files must use ONLY node:* built-in modules. New dependencies in these files are a critical finding.

10. Multi-Account Auth Isolation

When the gh CLI is used in contexts where multiple GitHub accounts might be active:

• Verify that auth tokens are scoped to the correct account
• Check that gh auth status is validated before operations
• If the PR adds GitHub API calls, verify it handles auth failures gracefully

Examples

Example 1: Critical — Shell injection in git command

PR adds:
  exec(`git commit -m "${commitMessage}"`)

Finding: [critical] Shell injection — commitMessage is user-controlled.
A message containing "; rm -rf /" would execute arbitrary commands.
Fix: Use execFile('git', ['commit', '-m', commitMessage]) instead.

Example 2: Critical — pull_request_target with checkout

PR adds workflow:
  on: pull_request_target
  steps:
    - uses: actions/checkout@v4
      with:
        ref: ${{ github.event.pull_request.head.ref }}
    - run: npm test

Finding: [critical] pull_request_target + PR head checkout = untrusted
code execution with write token. An attacker can modify package.json
scripts in their fork to exfiltrate GITHUB_TOKEN.
Fix: Use 'pull_request' trigger instead, or do not checkout PR code.

Example 3: High — Email written to committed file

PR adds to init.ts:
  const email = execSync('git config user.email').toString().trim();
  fs.writeFileSync('.squad/config.json', JSON.stringify({ author: email }));

Finding: [high] Personal data (email) written to committed file.
This violates the PII-in-source rule. Squad config files are committed
to the repo and shared across contributors.
Fix: Read email at runtime only, never persist to committed files.

Example 4: Medium — Overly broad workflow permissions

PR adds workflow with no permissions block (inherits default write-all):
  on: push
  jobs:
    deploy:
      runs-on: ubuntu-latest

Finding: [medium] No explicit permissions block. Workflow inherits
default permissions which may be overly broad.
Fix: Add 'permissions:' block with minimum required scopes.

Example 5: Medium — New dependency when built-in exists

PR adds to package.json:
  "dependencies": { "mkdirp": "^3.0.0" }

Finding: [medium] mkdirp is unnecessary — Node.js built-in
fs.mkdirSync(path, { recursive: true }) provides the same functionality.
Fix: Use node:fs built-in instead of adding a new dependency.

Example 6: Low — Missing environment variable validation

PR uses process.env.SQUAD_API_KEY without checking if it's defined:
  const apiKey = process.env.SQUAD_API_KEY;
  fetch(url, { headers: { Authorization: `Bearer ${apiKey}` } });

Finding: [low] No validation that SQUAD_API_KEY is defined.
If undefined, the Bearer token will be "Bearer undefined" which
may produce confusing auth errors.
Fix: Check for undefined and throw a descriptive error early.

Review Output Format

Structure your security review with severity levels:

## Security Review

**Verdict:** APPROVE | APPROVE WITH NOTES | REQUEST CHANGES | REJECT

### Findings

1. [severity: critical] — description
   - File(s): path/to/file.ts:L42
   - Risk: what could go wrong
   - Fix: specific remediation

2. [severity: high] — ...

3. [severity: medium] — ...

4. [severity: low] — ...

### Severity Guide
- **Critical:** Exploitable vulnerability, credential exposure, or code execution risk. Must fix before merge.
- **High:** Security design flaw that could lead to exploitation. Must fix before merge.
- **Medium:** Security best practice violation. Should fix before merge.
- **Low:** Minor hardening opportunity. Can fix in a follow-up PR.

### Summary
Brief overview of security posture and required actions.

Anti-Patterns

• ❌ Approving PRs with exec() and user-controlled input ("it's just a commit message")
• ❌ Ignoring pull_request_target trigger changes ("the workflow looks fine otherwise")
• ❌ Skipping dependency audit for "small" packages ("it's only 10 lines of code")
• ❌ Treating git add . in scripts as acceptable ("it works on my machine")
• ❌ Allowing secrets in test fixtures ("it's just a test, not production")
• ❌ Approving workflows without explicit permissions: blocks ("defaults are probably fine")
• ❌ Dismissing PII in committed files ("it's just an email address")
• ❌ Ignoring export typos ("TypeScript would catch that") — not always true with re-exports
• ❌ Allowing force pushes to shared branches in scripts ("we know what we're doing")
• ❌ Skipping review of new node_modules additions in bootstrap files ("it's a small package")