Charon/docs/plans/archive/codeql_hardening_spec.md

# Spec: Fix Remaining CodeQL Findings + Harden Local Security Scanning

**Branch:** `feature/beta-release`
**PR:** #800 (Email Notifications)
**Date:** 2026-03-06
**Status:** Planning

---

## 1. Introduction

Two CodeQL findings remain open in CI after the email-injection remediation in the prior commit (`ee224adc`), and local scanning does not block commits that would reproduce them. This spec covers the root-cause analysis, the precise code fixes, and the hardening of the local scanning pipeline so future regressions are caught before push.

### Objectives

1. Silence `go/email-injection` (CWE-640) in CI without weakening the existing multi-layer defence.
2. Silence `go/cookie-secure-not-set` (CWE-614) in CI for the intentional dev-mode loopback exception.
3. Ensure local scanning fails (exit-code 1) on Critical/High security findings of the same class before code reaches GitHub.

---

## 2. Research Findings

### 2.1 CWE-640 (`go/email-injection`) — Why It Is Still Flagged

CodeQL's `go/email-injection` rule tracks untrusted input from HTTP sources to `smtp.SendMail` / `(*smtp.Client).Rcpt` sinks. It treats a **validator** (a function that returns an error if bad data is present) differently from a **sanitizer** (a function that transforms and strips the bad data). Only sanitizers break the taint flow; validators do not.

The previous fix added `sanitizeForEmail()` in `notification_service.go` for `title` and `message`. It did **not** patch two other direct callers of `SendEmail`, both of which pass an HTTP-sourced `to` address without going through `notification_service.go` at all.

#### Confirmed taint sinks (from `codeql-results-go.sarif`)

| SARIF line | Function | Tainted argument |
|---|---|---|
| 365–367 | `(*MailService).SendEmail` | `[]string{toEnvelope}` in `smtp.SendMail` |
| ~530 | `(*MailService).sendSSL` | `toEnvelope` in `client.Rcpt(toEnvelope)` |
| ~583 | `(*MailService).sendSTARTTLS` | `toEnvelope` in `client.Rcpt(toEnvelope)` |

CodeQL reports 4 untrusted taint paths converging on each sink. These correspond to:

| Path # | Source file | Source expression | Route to sink |
|---|---|---|---|
| 1 | `backend/internal/api/handlers/settings_handler.go:637` | `req.To` (ShouldBindJSON, `binding:"required,email"`) | Direct `h.MailService.SendEmail(ctx, []string{req.To}, ...)` — bypasses `notification_service.go` entirely |
| 2 | `backend/internal/api/handlers/user_handler.go:597` | `userEmail` (HTTP request field) | `h.MailService.SendInvite(userEmail, ...)` → `mail_service.go:679` → `s.SendEmail(ctx, []string{email}, ...)` |
| 3 | `backend/internal/api/handlers/user_handler.go:1015` | `user.Email` (DB row, set from HTTP during registration) | Same `SendInvite` → `SendEmail` chain |
| 4 | `backend/internal/services/notification_service.go` | `rawRecipients` from `p.URL` (DB, admin-set) | `s.mailService.SendEmail(ctx, recipients, ...)` — CodeQL may trace DB values as tainted from prior HTTP writes |

#### Why CodeQL does not recognise the existing safeguards as sanitisers

```
validateEmailRecipients()  → ContainsAny check + mail.ParseAddress → error return (validator, not sanitizer)
parseEmailAddressForHeader → net/mail.ParseAddress                  → validator
rejectCRLF(toEnvelope)     → ContainsAny("\r\n") → error           → validator
```

CodeQL's taint model for Go requires the taint to be **transformed** (characters stripped or value replaced) before it considers the path neutralised. None of the helpers above strips CRLF — they gate on error returns. From CodeQL's perspective the original tainted bytes still flow into `smtp.SendMail`.

#### Why adding `sanitizeForEmail()` to `settings_handler.go` alone is insufficient

Even if added, `sanitizeForEmail()` calls `strings.ReplaceAll(s, "\r", "")` — stripping characters from an email address that contains `\r` would corrupt it. For recipient addresses, the correct model is to validate (which is already done) and suppress the residual finding with an annotated justification.

### 2.2 CWE-614 (`go/cookie-secure-not-set`) — Why It Appeared as "New"

**Only one `c.SetCookie` call exists** in production Go code:

```
backend/internal/api/handlers/auth_handler.go:152
```

The finding is "new" because it was introduced when `setSecureCookie()` was refactored to support the loopback dev-mode exception (`secure = false` for local HTTP requests). Prior to that refactor, `secure` was always `true`.

The `// codeql[go/cookie-secure-not-set]` suppression comment **is** present on `auth_handler.go:152`. However, the SARIF stored in the repository (`codeql-results-go.sarif`) shows the finding at **line 151** — a 1-line discrepancy caused by a subsequent commit that inserted the `// secure is intentionally false...` explanation comment, shifting the `c.SetCookie(` line from 151 → 152.

The inline suppression **should** work now that it is on the correct line (152). However, inline suppressions are fragile under line-number churn. The robust fix is a `query-filter` in `.github/codeql/codeql-config.yml`, which targets the rule ID independent of line number.

The `query-filters` section does not yet exist in the CodeQL config — only `paths-ignore` is used. This must be added for the first time.

### 2.3 Local Scanning Gap

The table below maps which security tools catch which findings locally.

| Tool | Stage | Fails on High/Critical? | Catches CWE-640? | Catches CWE-614? |
|---|---|---|---|---|
| `golangci-lint-fast` (gosec G101,G110,G305,G401,G501-503) | commit (blocking) | ✅ | ❌ no rule | ❌ gosec has no Gin cookie rule |
| `go-vet` | commit (blocking) | ✅ | ❌ | ❌ |
| `security-scan.sh` (govulncheck) | manual | Warn only | ❌ (CVEs only) | ❌ |
| `semgrep-scan.sh` (auto config, `--error`) | **manual only** | ✅ if run | ✅ `p/golang` | ✅ `p/golang` |
| `codeql-go-scan` + `codeql-check-findings` | **manual only** | ✅ if run | ✅ | ✅ |
| `golangci-lint-full` | manual | ✅ if run | ❌ | ❌ |

**The gap:** `semgrep-scan` already has `--error` (blocking) and covers both issue classes via `p/golang` and `p/owasp-top-ten`, but it runs as `stages: [manual]` only. Moving it to `pre-push` is the highest-leverage single change.

gosec rule audit for missing coverage:

| CWE | gosec rule | Covered by fast config? |
|---|---|---|
| CWE-614 cookie security | No standard gosec rule for Gin's `c.SetCookie` | ❌ |
| CWE-640 email injection | No gosec rule exists for SMTP injection | ❌ |
| CWE-89 SQL injection | G201, G202 — NOT in fast config | ❌ |
| CWE-22 path traversal | G305 — in fast config | ✅ |

`semgrep` fills the gap for CWE-614 and CWE-640 where gosec has no rules.

---

## 3. Technical Specifications

### 3.1 Phase 1 — Harden Local Scanning

#### 3.1.1 Move `semgrep-scan` to `pre-push` stage

**File:** `/projects/Charon/.pre-commit-config.yaml`

Locate the `semgrep-scan` hook entry (currently has `stages: [manual]`). Change the stage and name:

```yaml
      - id: semgrep-scan
        name: Semgrep Security Scan (Blocking - pre-push)
        entry: scripts/pre-commit-hooks/semgrep-scan.sh
        language: script
        pass_filenames: false
        verbose: true
        stages: [pre-push]
```

Rationale: `p/golang` includes:
- `go.cookie.security.insecure-cookie.insecure-cookie` → CWE-614 equivalent
- `go.lang.security.injection.tainted-smtp-email.tainted-smtp-email` → CWE-640 equivalent

#### 3.1.2 Restrict semgrep to WARNING+ and use targeted config

**File:** `/projects/Charon/scripts/pre-commit-hooks/semgrep-scan.sh`

The current invocation uses `--config auto --error`. Two changes:
1. Change default config from `auto` → `p/golang`. `auto` fetches 1,000-3,000+ rules and takes 60-180s — too slow for a blocking pre-push hook. `p/golang` covers all Go-specific OWASP/CWE rules and completes in ~10-30s.
2. Add `--severity ERROR --severity WARNING` to filter out INFO-level noise (these are OR logic, both required for WARNING+):

```bash
semgrep scan \
  --config "${SEMGREP_CONFIG_VALUE:-p/golang}" \
  --severity ERROR \
  --severity WARNING \
  --error \
  --exclude "frontend/node_modules" \
  --exclude "frontend/coverage" \
  --exclude "frontend/dist" \
  backend frontend/src .github/workflows
```

The `SEMGREP_CONFIG` env var can be overridden to `auto` or `p/golang p/owasp-top-ten` for a broader audit: `SEMGREP_CONFIG=auto git push`.

#### 3.1.3 Add `make security-local` target

**File:** `/projects/Charon/Makefile`

Add after the `security-scan-deps` target:

```make
security-local: ## Run local security scan (govulncheck + semgrep)
@echo "Running govulncheck..."
@./scripts/security-scan.sh
@echo "Running semgrep..."
@SEMGREP_CONFIG=p/golang ./scripts/pre-commit-hooks/semgrep-scan.sh
```

#### 3.1.4 Expand golangci-lint-fast gosec ruleset (Deferred)

**Status: DEFERRED** — G201/G202 (SQL injection via format string / string concat) are candidates for the fast config but must be pre-validated against the existing codebase first. GORM's raw query DSL can produce false positives. Run the following before adding:

```bash
cd backend && golangci-lint run --enable=gosec --disable-all --config .golangci-fast.yml ./...
```

…with G201/G202 temporarily uncommented. If zero false positives, add in a separate hardening commit. This is out of scope for this PR to avoid blocking PR #800.

Note: CWE-614 and CWE-640 remain CodeQL/Semgrep territory — gosec has no rules for these.

### 3.2 Phase 2 — Fix CWE-640 (`go/email-injection`)

#### Strategy

Add `// codeql[go/email-injection]` inline suppression annotations at all three sink sites in `mail_service.go`, with a structured justification comment immediately above each. This is the correct approach because:

1. The actual runtime defence is already correct and comprehensive (4-layer defence).
2. The taint is a CodeQL false-positive caused by the tool not modelling validators as sanitisers.
3. Restructuring to call `strings.ReplaceAll` on email addresses would corrupt valid addresses.

**The 4-layer defence that justifies these suppressions:**

```
Layer 1: HTTP boundary       — gin binding:"required,email" validates RFC 5321 format; CRLF fails well-formedness
Layer 2: Service boundary    — validateEmailRecipients() → ContainsAny("\r\n") error + net/mail.ParseAddress
Layer 3: Mail layer parse    — parseEmailAddressForHeader() → net/mail.ParseAddress returns only .Address field
Layer 4: Pre-sink validation — rejectCRLF(toEnvelope) immediately before smtp.SendMail / client.Rcpt calls
```

#### 3.2.1 Suppress at `smtp.SendMail` sink

**File:** `backend/internal/services/mail_service.go` (around line 367)

Locate the default-encryption branch in `SendEmail`. Replace the `smtp.SendMail` call line:

```go
default:
// toEnvelope passes through 4-layer CRLF defence:
//   1. gin binding:"required,email" at HTTP entry (CRLF invalid per RFC 5321)
//   2. validateEmailRecipients → ContainsAny("\r\n") + net/mail.ParseAddress
//   3. parseEmailAddressForHeader → net/mail.ParseAddress (returns .Address only)
//   4. rejectCRLF(toEnvelope) guard earlier in this function
// CodeQL does not model validators as sanitisers; suppression is correct here.
if err := smtp.SendMail(addr, auth, fromEnvelope, []string{toEnvelope}, msg); err != nil { // codeql[go/email-injection]
```

#### 3.2.2 Suppress at `client.Rcpt` in `sendSSL`

**File:** `backend/internal/services/mail_service.go` (around line 530)

Locate in `sendSSL`. Replace the `client.Rcpt` call line:

```go
// toEnvelope validated by rejectCRLF + net/mail.ParseAddress before this call (see SendEmail).
if rcptErr := client.Rcpt(toEnvelope); rcptErr != nil { // codeql[go/email-injection]
return fmt.Errorf("RCPT TO failed: %w", rcptErr)
}
```

#### 3.2.3 Suppress at `client.Rcpt` in `sendSTARTTLS`

**File:** `backend/internal/services/mail_service.go` (around line 583)

Same pattern as sendSSL:

```go
// toEnvelope validated by rejectCRLF + net/mail.ParseAddress before this call (see SendEmail).
if rcptErr := client.Rcpt(toEnvelope); rcptErr != nil { // codeql[go/email-injection]
return fmt.Errorf("RCPT TO failed: %w", rcptErr)
}
```

#### 3.2.4 Document safe call in `settings_handler.go`

**File:** `backend/internal/api/handlers/settings_handler.go` (around line 637)

Add a comment immediately above the `SendEmail` call — the sinks in mail_service.go are already annotated, so this is documentation only:

```go
// req.To is validated as RFC 5321 email via gin binding:"required,email".
// SendEmail applies validateEmailRecipients + net/mail.ParseAddress + rejectCRLF as defence-in-depth.
// Suppression annotations are on the sinks in mail_service.go.
if err := h.MailService.SendEmail(c.Request.Context(), []string{req.To}, "Charon - Test Email", htmlBody); err != nil {
```

#### 3.2.5 Document safe calls in `user_handler.go`

**File:** `backend/internal/api/handlers/user_handler.go` (lines ~597 and ~1015)

Add the same explanatory comment above both `SendInvite` call sites:

```go
// userEmail validated as RFC 5321 email format; suppression on mail_service.go sinks covers this path.
if err := h.MailService.SendInvite(userEmail, userToken, appName, baseURL); err != nil {
```

### 3.3 Phase 3 — Fix CWE-614 (`go/cookie-secure-not-set`)

#### Strategy

Two complementary changes: (1) add a `query-filters` exclusion in `.github/codeql/codeql-config.yml` which is robust to line-number churn, and (2) verify the inline `// codeql[go/cookie-secure-not-set]` annotation is correctly positioned.

**Justification for exclusion:**

The `secure` parameter in `setSecureCookie()` is `true` for **all** external production requests. It is `false` only when `isLocalRequest()` returns `true` — i.e., when the request comes from `127.x.x.x`, `::1`, or `localhost` over HTTP. In that scenario, browsers reject `Secure` cookies over non-TLS connections anyway, so setting `Secure: true` would silently break auth for local development. The conditional is tested and documented.

#### 3.3.1 Skip `query-filters` approach — inline annotation is sufficient

**Status: NOT IMPLEMENTING** — `query-filters` is a CodeQL query suite (`.qls`) concept, NOT a valid top-level key in GitHub's `codeql-config.yml`. Adding it risks silent failure or breaking the entire CodeQL analysis. The inline annotation at `auth_handler.go:152` is the documented mechanism and is already correct. No changes to `.github/codeql/codeql-config.yml` are needed for CWE-614.

**Why inline annotation is sufficient and preferred:** It is scoped to the single intentional instance. Any future `c.SetCookie(...)` call without `Secure:true` anywhere else in the codebase will correctly flag. Global exclusion via config would silently hide future regressions.

#### 3.3.1 (REFERENCE ONLY) Current valid `codeql-config.yml` structure

```yaml
name: "Charon CodeQL Config"

# Paths to ignore from all analysis (use sparingly - prefer query-filters for rule-level exclusions)
paths-ignore:
  - "frontend/coverage/**"
  - "frontend/dist/**"
  - "playwright-report/**"
  - "test-results/**"
  - "coverage/**"
```

DO NOT add `query-filters:` — it is not supported.
  - exclude:
      id: go/cookie-secure-not-set
      # Justified: setSecureCookie() in auth_handler.go intentionally sets Secure=false
      # ONLY for local loopback (127.x.x.x / ::1 / localhost) HTTP requests.
      # Browsers reject Secure cookies over HTTP regardless, so Secure=true would silently
      # break local development auth. All external HTTPS flows always set Secure=true.
      # Code: backend/internal/api/handlers/auth_handler.go → setSecureCookie()
      # Tests: TestSetSecureCookie_HTTPS_Strict, TestSetSecureCookie_HTTP_Loopback_Insecure
```

#### 3.3.2 Verify inline suppression placement in `auth_handler.go`

**File:** `backend/internal/api/handlers/auth_handler.go` (around line 152)

Confirm the `// codeql[go/cookie-secure-not-set]` annotation is on the same line as `c.SetCookie(`. The code should read:

```go
c.SetSameSite(sameSite)
// secure is intentionally false for local non-HTTPS loopback (development only).
c.SetCookie( // codeql[go/cookie-secure-not-set]
name,
value,
maxAge,
"/",
domain,
secure,
true,
)
```

The `query-filters` in §3.3.1 provides the primary fix. The inline annotation provides belt-and-suspenders coverage that survives if the config is ever reset.

---

## 4. Implementation Plan

### Phase 1 — Local Scanning (implement first to gate subsequent work)

| Task | File | Change | Effort |
|---|---|---|---|
| P1-1 | `.pre-commit-config.yaml` | Change semgrep-scan stage from `[manual]` → `[pre-push]`, update name | XS |
| P1-2 | `scripts/pre-commit-hooks/semgrep-scan.sh` | Add `--severity ERROR --severity WARNING` flags, exclude generated dirs | XS |
| P1-3 | `Makefile` | Add `security-local` target | XS |
| P1-4 | `backend/.golangci-fast.yml` | Add G201, G202 to gosec includes | XS |

### Phase 2 — CWE-640 Fix

| Task | File | Change | Effort |
|---|---|---|---|
| P2-1 | `backend/internal/services/mail_service.go` | Add `// codeql[go/email-injection]` on smtp.SendMail line + 4-layer defence comment | XS |
| P2-2 | `backend/internal/services/mail_service.go` | Add `// codeql[go/email-injection]` on sendSSL client.Rcpt line | XS |
| P2-3 | `backend/internal/services/mail_service.go` | Add `// codeql[go/email-injection]` on sendSTARTTLS client.Rcpt line | XS |
| P2-4 | `backend/internal/api/handlers/settings_handler.go` | Add explanatory comment above SendEmail call | XS |
| P2-5 | `backend/internal/api/handlers/user_handler.go` | Add explanatory comment above both SendInvite calls (~line 597, ~line 1015) | XS |

### Phase 3 — CWE-614 Fix

| Task | File | Change | Effort |
|---|---|---|---|
| P3-1 | `backend/internal/api/handlers/auth_handler.go` | Verify `// codeql[go/cookie-secure-not-set]` is on `c.SetCookie(` line (no codeql-config.yml changes needed) | XS |

**Total estimated file changes: 6 files, all comment/config additions — no logic changes.**

---

## 5. Acceptance Criteria

### CI (CodeQL must pass with zero error-level findings)

- [ ] `codeql.yml` CodeQL analysis (Go) passes with **0 blocking findings**
- [ ] `go/email-injection` is absent from the Go SARIF output
- [ ] `go/cookie-secure-not-set` is absent from the Go SARIF output

### Local scanning

- [ ] A `git push` with any `.go` file touched **blocks** if semgrep finds WARNING+ severity issues
- [ ] `pre-commit run semgrep-scan` on the current codebase exits 0 (no new findings)
- [ ] `make security-local` runs and exits 0

### Regression safety

- [ ] `go test ./...` in `backend/` passes (all changes are comments/config — no test updates required)
- [ ] `golangci-lint run --config .golangci-fast.yml ./...` passes in `backend/`
- [ ] The existing runtime defence (rejectCRLF, validateEmailRecipients) is **unchanged** — confirmed by diff

---

## 6. Commit Slicing Strategy

### Decision: Single commit on `feature/beta-release`

**Rationale:** All three phases are tightly related (one CI failure, two root findings, one local gap). All changes are additive (comments, config, no logic mutations). Splitting into multiple PRs would create an intermediate state where CI still fails and the local gap remains open. A single well-scoped commit keeps PR #800 atomic and reviewable.

**Suggested commit message:**
```
fix(security): suppress CodeQL false-positives for email-injection and cookie-secure

CWE-640 (go/email-injection): Add // codeql[go/email-injection] annotations at all 3
smtp sink sites in mail_service.go (smtp.SendMail, sendSSL client.Rcpt, sendSTARTTLS
client.Rcpt). The 4-layer defence (gin binding:"required,email", validateEmailRecipients,
net/mail.ParseAddress, rejectCRLF) is comprehensive; CodeQL's taint model does not
model validators as sanitisers, producing false-positive paths from
settings_handler.go:637 and user_handler.go invite flows that bypass
notification_service.go.

CWE-614 (go/cookie-secure-not-set): Add query-filter to codeql-config.yml excluding
this rule with documented justification. setSecureCookie() correctly sets Secure=false
only for local loopback HTTP requests where the Secure attribute is browser-rejected.
All external HTTPS flows set Secure=true.

Local scanning: Promote semgrep-scan from manual to pre-push stage so WARNING+
severity findings block push. Addresses gap where CWE-614 and CWE-640 equivalents
are not covered by any blocking local scan tool.
```

**PR:** All changes target PR #800 directly.

---

## 7. Risk and Rollback

| Risk | Likelihood | Mitigation |
|---|---|---|
| Inline suppression ends up on wrong line after future rebase | Medium | `query-filters` in codeql-config.yml provides independent suppression independent of line numbers |
| `semgrep-scan` at `pre-push` produces false-positive blocking | Low | `--severity WARNING --error` limits to genuine findings; use `SEMGREP_CONFIG=p/golang` for targeted override |
| G201/G202 gosec rules trigger on existing legitimate code | Low | Run `golangci-lint run --config .golangci-fast.yml` locally before committing; suppress specific instances if needed |
| CodeQL `query-filters` YAML syntax changes in future GitHub CodeQL versions | Low | Inline `// codeql[...]` annotations serve as independent fallback |

**Rollback:** All changes are additive config and comments. Reverting the commit restores the prior state exactly. No schema, API, or behaviour changes are made.