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:

      - 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+):

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:

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:

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:

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:

// 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:

// 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:

// 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:

// 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

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.