|
| 1 | +# Security Findings — Gatekeeper |
| 2 | + |
| 3 | +**Sweep date:** 2026-05-28 |
| 4 | +**Tools:** semgrep (auto) · bandit · gitleaks · pip-audit |
| 5 | +**Triage model:** claude-opus-4-7 (manual) |
| 6 | +**Posture before sweep:** **MEDIUM** (existing CI: bandit + semgrep + Trivy fs/IaC + secret-scan; gaps in shell-injection coverage and a CDN-style false positive) |
| 7 | +**Posture after sweep:** **LOW** (3 real findings closed, 2 documented FPs, 5 deferred k8s hardening notes) |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Summary |
| 12 | + |
| 13 | +| Severity | Open | Fixed this sweep | False positives | |
| 14 | +|---|---|---|---| |
| 15 | +| HIGH | 0 | 2 (shell-injection) | 1 (insecure-websocket on dev scheme transform) | |
| 16 | +| MEDIUM | 0 | 0 | 1 (B608 SQL — placeholders are `?` params) | |
| 17 | +| LOW / WARNING | 5 (k8s securityContext) | 0 | 0 | |
| 18 | +| INFO | 9 (mostly JS format strings) | 0 | 0 | |
| 19 | + |
| 20 | +Compared to the prior fleet (BG/Herald/QM), Gatekeeper's CI is already mature (Bandit + Semgrep + Trivy fs + Trivy IaC + secret-scan all wired). The local sweep surfaced findings the existing CI either missed or hadn't yet caught at HIGH level. |
| 21 | + |
| 22 | +--- |
| 23 | + |
| 24 | +## Fixed in this sweep |
| 25 | + |
| 26 | +### 1. Workflow shell-injection (2 instances, semgrep ERROR) |
| 27 | +Same `${{ github.* }}` interpolation pattern that BG had — patched via env vars + `jq -nc` JSON construction. |
| 28 | + |
| 29 | +| File | Line | Vector | |
| 30 | +|---|---|---| |
| 31 | +| `.github/workflows/ci.yml` | 141 | Discord notify with `${{ github.repository }}`, `${{ github.ref_name }}`, `${{ job.status }}` | |
| 32 | +| `.github/workflows/deploy.yml` | 76 | Same pattern | |
| 33 | + |
| 34 | +### 2. Bandit B608 suppression (`pathfinding.py:369`) |
| 35 | +The line already had `# nosemgrep` for the same finding from semgrep's sqlalchemy rule. Bandit uses its own suppression syntax — added `# nosec B608` inline. Underlying construction is safe: `placeholders = ",".join("?" * len(avoid_regions))` builds a count-derived literal; the actual `avoid_regions` values are passed as bound `?` parameters. |
| 36 | + |
| 37 | +--- |
| 38 | + |
| 39 | +## Open — needs your call |
| 40 | + |
| 41 | +### M1. k8s SecurityContext missing (4 findings: 2× postgres, 2× redis) |
| 42 | +`k8s/postgres.yaml` and `k8s/redis.yaml` lack `securityContext` on the container spec: |
| 43 | +- `runAsNonRoot: true` |
| 44 | +- `allowPrivilegeEscalation: false` |
| 45 | + |
| 46 | +**Not auto-patched because:** Per `CLAUDE.md`, Gatekeeper deploys on Fly.io — these k8s manifests appear to be reference / aspirational. Patching them safely requires understanding `postgres:15-alpine` init order (initdb runs as root before dropping to postgres user) and `redis:7-alpine` user expectations. If you do plan to deploy on k8s, the minimal-impact addition is: |
| 47 | + |
| 48 | +```yaml |
| 49 | +spec: |
| 50 | + containers: |
| 51 | + - name: postgres |
| 52 | + image: postgres:15-alpine |
| 53 | + securityContext: |
| 54 | + runAsNonRoot: false # postgres image requires root for initdb |
| 55 | + allowPrivilegeEscalation: false |
| 56 | + capabilities: |
| 57 | + drop: [ALL] |
| 58 | + add: [SETUID, SETGID] # required for initdb's user-drop |
| 59 | +``` |
| 60 | +
|
| 61 | +For redis (no init-as-root requirement): |
| 62 | +```yaml |
| 63 | + - name: redis |
| 64 | + securityContext: |
| 65 | + runAsNonRoot: true |
| 66 | + runAsUser: 999 # redis-alpine user |
| 67 | + allowPrivilegeEscalation: false |
| 68 | + capabilities: |
| 69 | + drop: [ALL] |
| 70 | +``` |
| 71 | +
|
| 72 | +### L1. apps/mobile unsafe-formatstring (7 INFO) |
| 73 | +JS template-literal patterns in `CacheService.ts`, `GatekeeperAPI.ts`, `ZKillboardService.ts`. INFO-level — review when next touching those files. Likely safe (template literals concatenating known-typed values), but worth confirming if any of them flow user input into a `RegExp` constructor or eval-shaped callsite. |
| 74 | + |
| 75 | +### L2. cookies.ts non-literal-regexp (1 WARNING) |
| 76 | +`apps/web/src/lib/cookies.ts:17` — `new RegExp(\`(?:^|; )${name}=([^;]*)\`)`. The `name` parameter is currently only called with the constant `CONSENT_KEY = 'gk_consent'`, so no current injection vector. If `parseCookie()` is ever called with user input, this becomes ReDoS-vulnerable. Defensive fix: escape `name` via a regex-escape helper. |
| 77 | + |
| 78 | +--- |
| 79 | + |
| 80 | +## False positives (validated, not patched) |
| 81 | + |
| 82 | +| Finding | Location | Why | |
| 83 | +|---|---|---| |
| 84 | +| `insecure-websocket` (semgrep ERROR) | `apps/web/src/components/map/useKillStream.ts:251` | Code at lines 249-251 does the correct scheme transform: `https://`→`wss://` then `http://`→`ws://`. The semgrep rule fires on the second `replace` (the dev-time `http://`→`ws://`) — but in production, `baseUrl` is always `https://` so the first replace runs and the second is a no-op. Safe by construction. | |
| 85 | +| `B608 hardcoded_sql_expressions` (bandit) | `pathfinding.py:369` | Suppressed with `# nosec B608` this sweep. Placeholders are count-derived, values bound as params. | |
| 86 | + |
| 87 | +--- |
| 88 | + |
| 89 | +## Verification |
| 90 | + |
| 91 | +```bash |
| 92 | +# Re-run sweep |
| 93 | +semgrep --config=auto --json --quiet \ |
| 94 | + --exclude=node_modules --exclude=.venv --exclude=.next --exclude=htmlcov \ |
| 95 | + --exclude=.pytest_cache --exclude=.ruff_cache --exclude=helm \ |
| 96 | + -o /tmp/gk-semgrep.json . |
| 97 | +
|
| 98 | +bandit -r backend streamlit -f json -q -x backend/.venv,.venv |
| 99 | +gitleaks detect --no-banner |
| 100 | +pip-audit -r backend/requirements.txt |
| 101 | +``` |
| 102 | + |
| 103 | +Existing CI (`.github/workflows/sast.yml`) covers: Bandit (HIGH+), Semgrep (auto + p/security-audit + p/python), Trivy fs (CRITICAL), Trivy IaC. Adequate ongoing gate — no new workflow needed. |
0 commit comments