Code Quality & Security Hardening

On this page

Status

COMPLETE — archived 2026-04-17 (plan-hygiene sweep). All epic &5 child issues are closed; residual work (functions >40 lines, structured errors, silent-discard cleanup) is tracked under the active Code Quality Remediation plan.

  • Create .adoc plan file and link in nav.adoc

  • Create GitLab epic (&5) and issues (#38–#45) — epic closed with child-MR traceability 2026-04-17

  • #38 — Replace Client::new() per-request with shared reqwest::Client (MR !21)

  • #39 — Replace unwrap_or_default() with proper error handling — closed; residual structured-error work migrated to Code Quality Remediation

  • #40 — Restrict CORS allow_methods to explicit list (MR !41)

  • #41 — Replace hand-rolled HTML sanitizer with ammonia (MR !41)

  • #42 — Convert string-based state machines to enums (MR !62)

  • #43 — Update implementation-guide.adoc for 5-stage CI and E2E counts (closed via Documentation Overhaul epic &18)

  • #44 — Clean up dead_code markers + per_page minimum enforcement (MR !41; per_page .clamp(1, MAX_PER_PAGE) applied across 31 endpoints)

  • #45 — Gate Swagger UI behind auth or feature flag (MR !21; closed as won’t-do — source is AGPL-licensed and public-visibility enforced per security-baseline)

Background

On 2026-03-15, three automated reviews were run against the CRAIG codebase:

  1. Security review — examined auth, CORS, input validation, SSRF, path traversal, unsafe code, dependencies, error leakage

  2. Code antipattern review — examined unwrap/expect usage, Client::new() pattern, dead code, blocking in async, pagination, state machines

  3. Documentation staleness review — compared docs against actual code state for accuracy

Overall assessment: LOW risk. No critical vulnerabilities found. Strong fundamentals: zero unsafe blocks, all queries parameterized, proper auth on all endpoints, PKCE OAuth, CAPTCHA + rate limiting on public endpoints.

Implementation Order

Recommended priority order, grouped by risk and effort:

Tier 1 — Security (do first)

#40: Restrict CORS allow_methods

File: crates/craig-api/src/lib.rs (line ~125), services/craig-intake/src/main.rs (line ~155)

Replace .allow_methods(Any) with explicit method list:

use axum::http::Method;

.allow_methods([Method::GET, Method::POST, Method::PUT, Method::DELETE])

Also consider changing default cors_origins from "*" to empty string in ServerOptions::default().

Effort: Small (2 lines changed)

#41: Replace HTML sanitizer with ammonia

File: services/craig-intake/src/api/validation.rs (lines 70–82)

Current hand-rolled tag stripper is bypassable. Replace with:

// Cargo.toml: ammonia = "4"
pub fn sanitize_text(input: &str) -> String {
    ammonia::clean(input)
}

Update existing unit tests to verify XSS payloads are stripped.

Effort: Small (add dependency, replace function body, update tests)

Tier 2 — Code Quality (high impact)

#38: Shared reqwest::Client

7 locations create Client::new() per request.

craig-web: Add reqwest::Client to AppState, pass to route handlers. The ApiClient struct already exists — ensure its constructor accepts a shared client instead of building one.

craig-intake: Add shared client to AppState for captcha verification and case referral.

craig-cli: ApiClient::new() creates a client — acceptable for CLI (one client per command invocation, not per-request). Lower priority.

Key files: - services/craig-web/src/main.rs — inject shared client into AppState - services/craig-web/src/api_client.rs — accept client in constructor - services/craig-web/src/routes/report.rs — use shared client from state - services/craig-intake/src/api/captcha.rs — use shared client from state - services/craig-intake/src/api/internal.rs — use shared client from state

Effort: Medium (refactor constructors and state injection across 2 services)

#39: Replace unwrap_or_default() with proper error handling

9 locations silently swallow HTTP response parsing errors. Also 1 .expect() that panics.

Pattern to follow:

// Before (bad):
let data: Value = resp.json().await.unwrap_or_default();

// After (good):
let data: Value = resp.json().await.map_err(|e| {
    tracing::error!(error = %e, "failed to parse response");
    // return appropriate error to caller
})?;

For api_client.rs:23, change .expect() to return Result:

pub fn new(base_url: &str, token: &str) -> Result<Self> {
    let http = reqwest::Client::builder()
        .timeout(Duration::from_secs(10))
        .build()
        .context("failed to build HTTP client")?;
    Ok(Self { http, base_url, token })
}

Key files: - services/craig-web/src/routes/report.rs — 3 locations - services/craig-web/src/api_client.rs — 3 locations - services/craig-cli/src/client.rs — 4 locations

Effort: Medium (error propagation changes may ripple to callers)

Tier 3 — Refactoring (compile-time safety)

#42: Enum state machines

Replace string-based transition validators with enum types.

Files: - services/craig-financial/src/transitions.rs - services/craig-placement/src/transitions.rs - services/craig-exchange/src/transitions.rs

Pattern:

use strum::{Display, EnumString};

#[derive(Debug, Clone, Copy, PartialEq, Eq, Display, EnumString, Serialize, Deserialize)]
#[strum(serialize_all = "snake_case")]
#[serde(rename_all = "snake_case")]
pub enum PaymentStatus {
    Pending,
    Approved,
    Issued,
    Cleared,
    Voided,
}

impl PaymentStatus {
    pub fn can_transition_to(&self, target: Self) -> bool {
        matches!((*self, target),
            (Self::Pending, Self::Approved)
            | (Self::Approved, Self::Issued)
            | (Self::Issued, Self::Cleared)
            | (Self::Pending, Self::Voided)
            | (Self::Approved, Self::Voided)
        )
    }
}

Database columns remain TEXT — strum handles serialization. API contracts unchanged (same string values).

Effort: Large (3 services, database queries need type conversion, integration tests need verification)

#44: Pagination bounds and dead code cleanup

Pagination: Add MIN_PER_PAGE constant, use .clamp(MIN_PER_PAGE, MAX_PER_PAGE) across all list endpoints (~19 endpoints across 8 services).

Dead code: Audit all #[allow(dead_code)] annotations (~60). For each: - If the function is planned for future use (e.g., publish_payment_issued), add a // TODO: wire up in Phase 11 (Notifications) comment - If truly unused with no future plan, remove it

Key files: - services//src/api/.rs — pagination in list handlers - services/craig-financial/src/events.rs — unused publisher - services/craig-placement/src/events.rs — unused publisher

Effort: Medium (repetitive but straightforward)

Tier 4 — Low priority

#45: Gate Swagger UI

Options: 1. Feature flag (recommended): #[cfg(feature = "swagger")] around utoipa/swagger routes, disable in release Dockerfile 2. Env toggle: CRAIG_SWAGGER_ENABLED=true checked at startup 3. Auth gate: require JWT on /swagger-ui and /api-doc/*

Effort: Small-Medium depending on approach

#43: Documentation updates

Update docs/modules/ROOT/pages/implementation-guide.adoc: 1. Rewrite CI pipeline section (lines 130–165): 6-stage → 5-stage, mention auto-trigger on MR 2. Update E2E test count: 80/15 → 103/17 3. Update runner tier: medium → large

Effort: Small (docs only)

Positive Findings (no action needed)

These were verified as correct during the security review:

  • Zero unsafe blocks in entire codebase

  • All SQL queries use parameterized bindings ($1, $2)

  • All protected endpoints require Extension<Claims> extraction

  • File paths constructed with UUIDs + sanitized filenames (no traversal risk)

  • OAuth state parameter properly validated (CSRF protection)

  • Session cookies use http_only + SameSite::Lax + configurable secure

  • Rate limiting + CAPTCHA on public intake endpoints

  • Honeypot field on public report form

  • Recent dependency versions (Axum 0.8, sqlx 0.8, jsonwebtoken 10)

Files Modified

File Action

docs/modules/ROOT/pages/plans/code-quality-security-hardening.adoc

Created (this file)

docs/modules/ROOT/nav.adoc

Link plan file

GitLab

  • Epic: &5 (Code Quality & Security Hardening)

  • Issues: #38, #39, #40, #41, #42, #43, #44, #45

Edit this page · latest