Git workflow, environments & CI/CD pipeline (#1) #6
+242
@@ -0,0 +1,242 @@
|
|||||||
|
# InboxIntel — Full Application Audit Report
|
||||||
|
|
||||||
|
**Date:** 2026-07-02 · **Scope:** entire repository (backend `src/`, `frontend/`, migrations,
|
||||||
|
CI/CD `.gitea/workflows/`, `docker-compose*`, `deploy/`, git history) · **Phase:** 1 (read-only)
|
||||||
|
|
||||||
|
**Stack:** .NET 8 / ASP.NET Core (Clean Architecture) · EF Core + PostgreSQL (pgvector image)
|
||||||
|
· React 18 + Vite SPA · Google OAuth2 (cookie session) · Gitea Actions CI/CD · self-hosted
|
||||||
|
Docker Compose (single-operator deployment).
|
||||||
|
|
||||||
|
## Executive summary
|
||||||
|
This codebase is **markedly more hardened than typical** — previous hardening phases (V-01…V-12,
|
||||||
|
visible in code comments) already addressed the classic killers: IDOR (EF global query filters),
|
||||||
|
SSRF (a genuinely thorough `SafeHttpGuard` with DNS-rebinding defence), token encryption at rest,
|
||||||
|
non-root containers, stack-trace suppression, forwarded-header trust, secret hygiene (nothing in
|
||||||
|
git history), and confirmed-destructive-actions. **No Critical findings.** The most significant
|
||||||
|
issues are: **registered input validators that never execute (H-1)**, **no rate limiting (H-2)**,
|
||||||
|
and a **data-at-rest posture** for email bodies that should be a deliberate, documented decision
|
||||||
|
(H-3). CI has real security gates (gitleaks + dependency audit) already.
|
||||||
|
|
||||||
|
Severity counts: **Critical 0 · High 3 · Medium 6 · Low 7**
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Security
|
||||||
|
|
||||||
|
### H-1 · FluentValidation validators are registered but NEVER executed
|
||||||
|
- **File:** `src/InboxIntel.Application/DependencyInjection.cs:11` (registration) vs
|
||||||
|
`src/InboxIntel.Api/Program.cs` (no `AddFluentValidationAutoValidation()` anywhere; no
|
||||||
|
controller injects `IValidator<>`).
|
||||||
|
- **Why it's a problem:** `Validators.cs` defines real safety rules — `CleanupRequestValidator`
|
||||||
|
requires `Confirmed=true` for Trash/HardDelete, `SearchRequestValidator` bounds paging, etc. —
|
||||||
|
but FluentValidation 11.x requires explicit auto-validation enablement, which is absent. The
|
||||||
|
rules are dead code; requests reach services unvalidated. *Mitigating factor:* the services
|
||||||
|
re-check the most dangerous rules (`CleanupService.cs:48` re-enforces `Confirmed`), and the
|
||||||
|
controller clamps paging — so this is defence-in-depth loss, not an open hole. But rules that
|
||||||
|
exist **only** in the validators (e.g. `From <= To`, LabelId-required) are silently unenforced.
|
||||||
|
- **Fix:** add `services.AddFluentValidationAutoValidation()` (package already referenced) in
|
||||||
|
`Program.cs`; add a regression test posting an invalid DTO and asserting 400.
|
||||||
|
- **Blast radius:** isolated (1 line + tests). Verify no existing client sends payloads that
|
||||||
|
would now 400.
|
||||||
|
|
||||||
|
### H-2 · No rate limiting on any endpoint
|
||||||
|
- **File:** `src/InboxIntel.Api/Program.cs` (no `AddRateLimiter`/middleware anywhere).
|
||||||
|
- **Why:** login (`/auth/login` → OAuth), search (now with FTS + trigram + future AI), export
|
||||||
|
(PDF generation), unsubscribe (server-side outbound HTTP), and AI endpoints are all
|
||||||
|
unthrottled. A single authenticated user (or an unauthenticated client hammering
|
||||||
|
`/auth/login`/`/appinfo`) can exhaust CPU/DB/outbound quota. For the future multi-user platform
|
||||||
|
(see `docs/discovery/multi-provider/`), this is a prerequisite.
|
||||||
|
- **Fix:** ASP.NET Core's built-in `RateLimiter` — a global fixed-window per-IP policy + stricter
|
||||||
|
policies on `auth`, `export`, `unsubscribe`, `ai`. Return 429 with Retry-After.
|
||||||
|
- **Blast radius:** isolated (Program.cs + policy constants + tests).
|
||||||
|
|
||||||
|
### Verified-good (no finding)
|
||||||
|
- **Authorization / IDOR:** `[Authorize]` on `ApiControllerBase`; only `AppInfo` + `Auth.login`
|
||||||
|
are `[AllowAnonymous]` (correct). Global query filters on every tenant-scoped entity enforce
|
||||||
|
per-user isolation even if a query forgets its `Where` — covered by `TenantIsolationTests`.
|
||||||
|
- **Injection:** all data access via EF parameterisation; no raw SQL string concatenation in app
|
||||||
|
code; FTS uses `websearch_to_tsquery` parameters. Frontend has **zero**
|
||||||
|
`dangerouslySetInnerHTML`/`innerHTML`/`eval`; the search-highlight feature deliberately uses
|
||||||
|
non-HTML sentinels rendered as escaped React elements (XSS-safe by construction).
|
||||||
|
- **SSRF:** `SafeHttpGuard` validates scheme, resolves DNS and checks **every** address against
|
||||||
|
loopback/private/link-local/CGNAT/metadata/multicast (v4+v6, v4-mapped), fails closed on
|
||||||
|
unknown families, and is paired with `AllowAutoRedirect=false` (`DependencyInjection.cs:65`).
|
||||||
|
This is better than most production code.
|
||||||
|
- **Secrets:** none hardcoded (empty placeholders in `appsettings.json`); no `.env` ever
|
||||||
|
committed (git history checked); real secrets live in git-ignored `deploy/.env*` and Gitea
|
||||||
|
Actions secrets; pre-commit hook + CI gitleaks scan both guard regressions.
|
||||||
|
- **Session cookies:** HttpOnly, SameSite=Lax, Secure-always outside dev, 401-not-redirect for
|
||||||
|
XHR. Forwarded headers only trusted from configured proxy CIDRs (V-08).
|
||||||
|
- **Headers:** nosniff, X-Frame-Options DENY, Referrer-Policy, COOP on API; HSTS in prod.
|
||||||
|
|
||||||
|
### M-1 · No absolute session lifetime (sliding-only)
|
||||||
|
- **File:** `Program.cs:58-59` — `ExpireTimeSpan = 7d` with `SlidingExpiration = true`.
|
||||||
|
- **Why:** a session that's touched at least weekly renews forever; a stolen cookie never
|
||||||
|
expires as long as the attacker uses it. No server-side revocation list exists either
|
||||||
|
(cookie is self-contained).
|
||||||
|
- **Fix:** add an absolute cap via an `issued-at` claim checked in `OnValidatePrincipal`
|
||||||
|
(e.g. re-auth after 30 days), and/or a session-stamp validated against the DB to enable
|
||||||
|
revocation. (The multi-provider design doc 02/06 already specs DB-backed sessions — this
|
||||||
|
aligns.)
|
||||||
|
- **Blast radius:** isolated.
|
||||||
|
|
||||||
|
### M-2 · No Content-Security-Policy on the SPA or API
|
||||||
|
- **File:** `Program.cs:156-167` (comment says CSP "report-only for now" but none is actually
|
||||||
|
set); `frontend/nginx.conf` serves the SPA without security headers.
|
||||||
|
- **Why:** CSP is the main mitigation layer against any future XSS slip; currently absent.
|
||||||
|
- **Fix:** add CSP (default-src 'self'; connect-src API origin; etc.) + nosniff/XFO to the SPA
|
||||||
|
nginx config; optionally a report-only CSP on the API.
|
||||||
|
- **Blast radius:** isolated (nginx conf + one middleware line), needs SPA smoke-test (Chart.js
|
||||||
|
inline styles etc.).
|
||||||
|
|
||||||
|
### L-1 · CSRF: no antiforgery tokens (accepted-risk, documented here)
|
||||||
|
- SameSite=Lax cookies + strict CORS allowlist + JSON-only POST bodies make classic CSRF
|
||||||
|
impractical in modern browsers. Acceptable for now; revisit if cookie SameSite is ever
|
||||||
|
relaxed or non-JSON form endpoints are added.
|
||||||
|
|
||||||
|
### L-2 · `AllowedHosts: "*"` (`appsettings.json`)
|
||||||
|
- Host-header filtering disabled; low risk behind the proxy but set it to the real hostnames
|
||||||
|
at production deployment.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Data protection (top priority)
|
||||||
|
|
||||||
|
**Inventory of sensitive data:**
|
||||||
|
| Data | Where | At-rest protection | Verdict |
|
||||||
|
|---|---|---|---|
|
||||||
|
| Google OAuth **refresh/access tokens** | `users.EncryptedRefreshToken` | **Encrypted** (Data Protection API, keys on `/keys` volume) | ✅ good |
|
||||||
|
| **Passwords** | — | **None stored** (OAuth-only login; no password column exists) | ✅ best-possible |
|
||||||
|
| **Email bodies / subjects / snippets** | `emails.BodyText` etc. | **Plaintext** in Postgres | ⚠️ **H-3** below |
|
||||||
|
| Sender names/addresses (PII) | `senders`, `emails` | Plaintext | part of H-3 |
|
||||||
|
| User email + display name | `users` | Plaintext | part of H-3 |
|
||||||
|
| DB password | `deploy/.env` (git-ignored) + Actions secret | not in repo | ✅ |
|
||||||
|
| Data Protection keys | `/keys` Docker volume | filesystem | M-3 below |
|
||||||
|
| IPs / payment / health data | — | not collected | ✅ n/a |
|
||||||
|
|
||||||
|
### H-3 · Email content stored in plaintext at rest, with unbounded retention
|
||||||
|
- **File:** `src/InboxIntel.Domain/Entities/Email.cs` (`BodyText`, `Snippet`, `Subject`);
|
||||||
|
Postgres `pgdata` volume; also flows into PDF/CSV/JSON **exports** and (when enabled) to the
|
||||||
|
local Ollama process.
|
||||||
|
- **Why:** the entire product is a copy of the user's mailbox. On this self-hosted,
|
||||||
|
single-operator deployment the DB lives on the operator's own disk — a defensible posture —
|
||||||
|
but: (a) anyone with disk/volume/backup access reads all mail; (b) there is **no retention or
|
||||||
|
purge policy** (mail persists even after unsubscribe/cleanup in Gmail; "remove account"
|
||||||
|
flows don't exist yet); (c) the planned **multi-user platform** (docs/discovery/multi-provider)
|
||||||
|
makes plaintext-bodies-readable-by-host-admin a real privacy issue (its own security doc
|
||||||
|
promises "admins never read users' mail" — the DB must back that up).
|
||||||
|
- **Fix (phased):** 1) *Document* the current posture in README/threat model (deliberate,
|
||||||
|
local-first). 2) Enable **pgcrypto/field-level encryption or full-disk/volume encryption**
|
||||||
|
before any multi-user deployment. 3) Add a **data-retention setting** + purge job and an
|
||||||
|
account-deletion path (GDPR-style erasure). 4) Ensure DB **backups** inherit the same
|
||||||
|
protection.
|
||||||
|
- **Blast radius:** documentation = trivial; field-level encryption = **large** (touches search
|
||||||
|
— FTS can't index encrypted columns; would need architectural decision). Recommend
|
||||||
|
volume-level encryption + retention/deletion first.
|
||||||
|
|
||||||
|
### M-3 · Data Protection keys stored unencrypted on the `/keys` volume
|
||||||
|
- **File:** `Program.cs:26-28` — `PersistKeysToFileSystem` without `ProtectKeysWith*`.
|
||||||
|
- **Why:** whoever reads the volume can decrypt all stored refresh tokens. Same-disk-as-DB
|
||||||
|
caveat applies, but keys and ciphertext living side-by-side weakens the encryption's value.
|
||||||
|
- **Fix:** `ProtectKeysWithCertificate(...)` (cert from env/secret), or OS-level DPAPI on
|
||||||
|
Windows hosts; at minimum document the volume-permissions requirement.
|
||||||
|
- **Blast radius:** isolated, but requires a key-migration step for existing tokens.
|
||||||
|
|
||||||
|
### M-4 · Default DB credentials in `appsettings.json`
|
||||||
|
- **File:** `appsettings.json:3` — `Password=inboxintel` as the fallback connection string.
|
||||||
|
- **Why:** if a deployment forgets the env override, the app happily connects with a guessable
|
||||||
|
password (compose enforces `POSTGRES_PASSWORD` but a non-compose deployment might not).
|
||||||
|
- **Fix:** empty the default and fail fast at startup with a clear message when unset.
|
||||||
|
- **Blast radius:** isolated (plus updating dev docs to use user-secrets).
|
||||||
|
|
||||||
|
### Logging & transit — verified
|
||||||
|
- **Logs:** no token/password logging found; the one PII-ish log is `SmtpEmailSender.cs:28`
|
||||||
|
(recipient address + subject at Info when SMTP is unconfigured) — acceptable, downgrade to
|
||||||
|
Debug if desired (L-3). Serilog request logging does not include query strings or bodies.
|
||||||
|
- **Transit:** TLS terminates at nginx (HSTS enabled in prod); API+DB bound to loopback/compose
|
||||||
|
network only. **DB connection itself is non-TLS** — fine while Postgres is co-located on the
|
||||||
|
compose network; **must add `SSL Mode=Require` if the DB ever moves to another host** (L-4).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Dependencies & supply chain
|
||||||
|
- **`dotnet list package --vulnerable --include-transitive`:** **clean** (all projects) — the
|
||||||
|
earlier round of CVE pins (System.Text.Json 8.0.6 etc.) is holding, and CI re-checks on every PR.
|
||||||
|
- **`npm audit` (production deps):** **0 vulnerabilities**.
|
||||||
|
- **M-5 · `npm audit` (dev deps):** vite 5.x depends on a vulnerable **esbuild** (1 moderate,
|
||||||
|
1 high, dev-server-only vectors). Not shipped to users and CI already scopes to prod deps —
|
||||||
|
but the fix is a routine **vite 5→6/7 upgrade**. Effort S.
|
||||||
|
- **Duplication (L-5):** both `MailKit` (Infrastructure) and the frontend carry sizeable deps;
|
||||||
|
`react-grid-layout` exists solely for the draggable dashboard the redesign plans to retire —
|
||||||
|
candidate for removal with the Analytics migration. No abandoned packages spotted.
|
||||||
|
- CI supply-chain posture: gitleaks (full history) + NuGet/npm audits are **required checks** —
|
||||||
|
good. No SAST/CodeQL (L-6, nice-to-have).
|
||||||
|
|
||||||
|
## 4. Error handling & reliability
|
||||||
|
- **Good:** global `UseExceptionHandler` + RFC7807 ProblemDetails (no stack traces to clients);
|
||||||
|
Polly retry/backoff on Gmail sync; idempotent sync upserts; AI calls wrapped in try/catch with
|
||||||
|
non-AI fallbacks ("sync must never fail because the AI provider is down").
|
||||||
|
- **M-6 · Known EF model warning:** `Email` has a global query filter but is the required end
|
||||||
|
of the `Email↔EmailLabel` relationship — logged on every boot; can yield surprising results
|
||||||
|
when filtered parents are excluded. Fix: matching filter on `EmailLabel` (one line) + test.
|
||||||
|
(Long-noted in logs; this is the nudge to actually do it.)
|
||||||
|
- **L-7 · Multi-step writes without explicit transactions:** e.g. `GoogleAuthEvents` upsert and
|
||||||
|
sync batches rely on EF's single-SaveChanges transactionality — mostly fine; the sync
|
||||||
|
cursor-advance + batch-upsert pairing is the one place an explicit transaction would guard a
|
||||||
|
mid-batch crash (currently self-heals via idempotent re-sync — acceptable).
|
||||||
|
|
||||||
|
## 5. Performance
|
||||||
|
- **Fixed this cycle (verified live):** relevance-ranked FTS with weighted tsvector + GIN;
|
||||||
|
trigram GIN indexes for the previously non-sargable sender/domain `.Contains()`; HNSW vector
|
||||||
|
index ready for semantic search.
|
||||||
|
- **Remaining (all Low, roadmapped):** offset pagination degrades on deep pages (keyset planned
|
||||||
|
in `docs/discovery/05`); frontend ships one **678 KB JS bundle** (no code-splitting — vite
|
||||||
|
`manualChunks`/dynamic import, effort S); no HTTP caching/ETags on read-heavy endpoints
|
||||||
|
(sidebar counts are fetched per page-load); `docker-compose` frontend nginx lacks gzip/brotli
|
||||||
|
confirmation for the bundle.
|
||||||
|
- No N+1 patterns found (queries project with joins; aggregates precomputed in
|
||||||
|
`AnalyticsAggregate`).
|
||||||
|
|
||||||
|
## 6. Code quality & architecture
|
||||||
|
- Clean Architecture discipline is genuinely observed (dependency rule intact; thin controllers;
|
||||||
|
DTO mapping at boundaries). Config is env-driven throughout.
|
||||||
|
- **L-8:** `AiService.GenerateQueryAsync` returns raw LLM output as a search query (advisory-only,
|
||||||
|
becomes a search string — harmless today; keep it that way when NL search lands: model output
|
||||||
|
must stay data, never an executable/action).
|
||||||
|
- Dead-ish code: none significant; `docs/specs/*` and discovery docs are current.
|
||||||
|
|
||||||
|
## 7. Testing & CI
|
||||||
|
- **41 tests** (unit: parser/guard/unsubscribe/embeddings-null; integration: authz challenge,
|
||||||
|
tenant isolation, pagination clamp, search fallback ordering). Critical security invariants
|
||||||
|
(IDOR filter, SSRF guard, confirmed-destructive) **are tested** — better than most.
|
||||||
|
- **Gap (M-7):** no live-Postgres test harness — FTS/ranking/fuzzy/pgvector paths are verified
|
||||||
|
manually against staging (documented in commit messages) but not repeatably in CI. A
|
||||||
|
Testcontainers-Postgres (pgvector image) job would convert those throwaway verifications into
|
||||||
|
permanent regression tests. Effort M.
|
||||||
|
- Gap (L-9): no coverage for `CleanupService`/`SyncService` beyond compilation; no E2E of the
|
||||||
|
OAuth flow (hard without creds — acceptable).
|
||||||
|
- CI: build+test+secrets+deps as **required** PR checks; auto-deploy to staging with a
|
||||||
|
post-deploy health gate; prod is tag-gated. Solid. Missing: lint/format check in CI (the
|
||||||
|
pre-commit hook enforces locally; add `dotnet format --verify-no-changes` job, effort S) (L-10).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Prioritized remediation plan
|
||||||
|
|
||||||
|
| # | Finding | Sev | Fix effort | Blast radius |
|
||||||
|
|---|---------|-----|-----------|--------------|
|
||||||
|
| 1 | **H-1** Enable FluentValidation auto-validation + 400 tests | High | S | Isolated (1 line + tests; verify clients) |
|
||||||
|
| 2 | **H-2** Rate limiting (global + auth/export/unsub/AI policies) | High | S–M | Isolated (Program.cs + tests) |
|
||||||
|
| 3 | **H-3** Data-at-rest posture: document now; retention setting + purge job + account-deletion; volume-encryption guidance; (defer field-level encryption decision) | High | S (doc) → M (retention) → L (encryption) | Doc: none · Retention: moderate · Encryption: large/architectural |
|
||||||
|
| 4 | **M-3** Protect Data Protection keys at rest | Med | S–M | Isolated + key migration |
|
||||||
|
| 5 | **M-1** Absolute session lifetime (+ revocation groundwork) | Med | S | Isolated |
|
||||||
|
| 6 | **M-4** Remove default DB password; fail fast | Med | S | Isolated |
|
||||||
|
| 7 | **M-2** CSP + security headers on the SPA nginx | Med | S | Isolated (needs SPA smoke test) |
|
||||||
|
| 8 | **M-6** Fix EF query-filter warning (EmailLabel filter) | Med | S | Isolated + test |
|
||||||
|
| 9 | **M-5** Vite upgrade (dev-dep CVEs) | Med | S | Frontend build only |
|
||||||
|
| 10 | **M-7** Testcontainers live-Postgres CI job | Med | M | CI + new test project wiring |
|
||||||
|
| 11 | L-2/L-3/L-4/L-10 (AllowedHosts, SMTP log level, DB TLS note, CI format check) | Low | S each | Isolated |
|
||||||
|
| 12 | L-5 bundle split / grid-layout removal (with redesign) | Low | S–M | Frontend |
|
||||||
|
|
||||||
|
**Stopping here per Phase 1 instructions — no changes made. Awaiting approval of this plan
|
||||||
|
(or an edited subset) before implementing anything.**
|
||||||
@@ -7,6 +7,7 @@ namespace InboxIntel.Api.Controllers;
|
|||||||
/// AI endpoints are read-only / advisory. They never trigger destructive
|
/// AI endpoints are read-only / advisory. They never trigger destructive
|
||||||
/// actions - suggestions are returned for the user to act on via /cleanup.
|
/// actions - suggestions are returned for the user to act on via /cleanup.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
|
[Microsoft.AspNetCore.RateLimiting.EnableRateLimiting("expensive")] // AUDIT H-2: LLM calls are the most expensive path
|
||||||
public class AiController : ApiControllerBase
|
public class AiController : ApiControllerBase
|
||||||
{
|
{
|
||||||
private readonly IAiService _ai;
|
private readonly IAiService _ai;
|
||||||
|
|||||||
@@ -5,10 +5,12 @@ using Microsoft.AspNetCore.Authentication.Cookies;
|
|||||||
using Microsoft.AspNetCore.Authentication.Google;
|
using Microsoft.AspNetCore.Authentication.Google;
|
||||||
using Microsoft.AspNetCore.Authorization;
|
using Microsoft.AspNetCore.Authorization;
|
||||||
using Microsoft.AspNetCore.Mvc;
|
using Microsoft.AspNetCore.Mvc;
|
||||||
|
using Microsoft.AspNetCore.RateLimiting;
|
||||||
|
|
||||||
namespace InboxIntel.Api.Controllers;
|
namespace InboxIntel.Api.Controllers;
|
||||||
|
|
||||||
[ApiController]
|
[ApiController]
|
||||||
|
[EnableRateLimiting("auth")] // AUDIT H-2: throttle login/challenge attempts per IP
|
||||||
[ApiVersion("1.0")]
|
[ApiVersion("1.0")]
|
||||||
[Route("api/v{version:apiVersion}/[controller]")]
|
[Route("api/v{version:apiVersion}/[controller]")]
|
||||||
public class AuthController : ControllerBase
|
public class AuthController : ControllerBase
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Mvc;
|
|||||||
|
|
||||||
namespace InboxIntel.Api.Controllers;
|
namespace InboxIntel.Api.Controllers;
|
||||||
|
|
||||||
|
[Microsoft.AspNetCore.RateLimiting.EnableRateLimiting("expensive")] // AUDIT H-2: PDF/CSV generation is costly
|
||||||
public class ExportController : ApiControllerBase
|
public class ExportController : ApiControllerBase
|
||||||
{
|
{
|
||||||
private readonly IExportService _export;
|
private readonly IExportService _export;
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ using Microsoft.AspNetCore.Mvc;
|
|||||||
|
|
||||||
namespace InboxIntel.Api.Controllers;
|
namespace InboxIntel.Api.Controllers;
|
||||||
|
|
||||||
|
[Microsoft.AspNetCore.RateLimiting.EnableRateLimiting("expensive")] // AUDIT H-2: triggers server-side outbound HTTP
|
||||||
public class UnsubscribeController : ApiControllerBase
|
public class UnsubscribeController : ApiControllerBase
|
||||||
{
|
{
|
||||||
private readonly IUnsubscribeService _unsub;
|
private readonly IUnsubscribeService _unsub;
|
||||||
|
|||||||
@@ -1,5 +1,7 @@
|
|||||||
using System.Security.Claims;
|
using System.Security.Claims;
|
||||||
|
using System.Threading.RateLimiting;
|
||||||
using Asp.Versioning;
|
using Asp.Versioning;
|
||||||
|
using FluentValidation.AspNetCore;
|
||||||
using InboxIntel.Api.Auth;
|
using InboxIntel.Api.Auth;
|
||||||
using InboxIntel.Application;
|
using InboxIntel.Application;
|
||||||
using InboxIntel.Application.Abstractions;
|
using InboxIntel.Application.Abstractions;
|
||||||
@@ -8,8 +10,10 @@ using InboxIntel.Infrastructure.Configuration;
|
|||||||
using InboxIntel.Infrastructure.Persistence;
|
using InboxIntel.Infrastructure.Persistence;
|
||||||
using Microsoft.AspNetCore.Authentication.Cookies;
|
using Microsoft.AspNetCore.Authentication.Cookies;
|
||||||
using Microsoft.AspNetCore.Authentication.Google;
|
using Microsoft.AspNetCore.Authentication.Google;
|
||||||
|
using Microsoft.AspNetCore.Authentication;
|
||||||
using Microsoft.AspNetCore.DataProtection;
|
using Microsoft.AspNetCore.DataProtection;
|
||||||
using Microsoft.AspNetCore.HttpOverrides;
|
using Microsoft.AspNetCore.HttpOverrides;
|
||||||
|
using Microsoft.AspNetCore.RateLimiting;
|
||||||
using Microsoft.EntityFrameworkCore;
|
using Microsoft.EntityFrameworkCore;
|
||||||
using Microsoft.Extensions.Options;
|
using Microsoft.Extensions.Options;
|
||||||
using Serilog;
|
using Serilog;
|
||||||
@@ -57,6 +61,24 @@ builder.Services.AddAuthentication(options =>
|
|||||||
options.Cookie.Name = "inboxintel.session";
|
options.Cookie.Name = "inboxintel.session";
|
||||||
options.ExpireTimeSpan = TimeSpan.FromDays(7);
|
options.ExpireTimeSpan = TimeSpan.FromDays(7);
|
||||||
options.SlidingExpiration = true;
|
options.SlidingExpiration = true;
|
||||||
|
// AUDIT M-1: sliding expiration alone lets a stolen cookie renew forever. Stamp an
|
||||||
|
// absolute start at sign-in and reject principals older than the configured cap,
|
||||||
|
// forcing a full re-login. (Pre-existing sessions without the stamp are rejected
|
||||||
|
// once — a single re-login, then they carry the stamp.)
|
||||||
|
var absoluteDays = builder.Configuration.GetValue("Auth:AbsoluteSessionDays", 30);
|
||||||
|
options.Events.OnSigningIn = ctx =>
|
||||||
|
{
|
||||||
|
ctx.Properties.SetString("abs-start", DateTimeOffset.UtcNow.ToString("O"));
|
||||||
|
return Task.CompletedTask;
|
||||||
|
};
|
||||||
|
options.Events.OnValidatePrincipal = async ctx =>
|
||||||
|
{
|
||||||
|
if (SessionLifetime.IsExpired(ctx.Properties.GetString("abs-start"), DateTimeOffset.UtcNow, TimeSpan.FromDays(absoluteDays)))
|
||||||
|
{
|
||||||
|
ctx.RejectPrincipal();
|
||||||
|
await ctx.HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
|
||||||
|
}
|
||||||
|
};
|
||||||
// API-style behaviour: return status codes rather than redirecting to a login page.
|
// API-style behaviour: return status codes rather than redirecting to a login page.
|
||||||
options.Events.OnRedirectToLogin = ctx =>
|
options.Events.OnRedirectToLogin = ctx =>
|
||||||
{
|
{
|
||||||
@@ -98,6 +120,35 @@ builder.Services.AddApiVersioning(o =>
|
|||||||
}).AddApiExplorer(o => { o.GroupNameFormat = "'v'VVV"; o.SubstituteApiVersionInUrl = true; });
|
}).AddApiExplorer(o => { o.GroupNameFormat = "'v'VVV"; o.SubstituteApiVersionInUrl = true; });
|
||||||
|
|
||||||
builder.Services.AddControllers();
|
builder.Services.AddControllers();
|
||||||
|
// AUDIT H-1: the validators in InboxIntel.Application/Validation were registered but never
|
||||||
|
// executed (FluentValidation 11.x needs explicit auto-validation). This wires them into
|
||||||
|
// model binding so invalid DTOs 400 at the boundary instead of reaching services.
|
||||||
|
builder.Services.AddFluentValidationAutoValidation();
|
||||||
|
|
||||||
|
// AUDIT H-2: rate limiting. Global per-user (or per-IP when anonymous) window, plus stricter
|
||||||
|
// named policies for auth and expensive endpoints (export/unsubscribe/AI). Limits are
|
||||||
|
// config-driven so tests and deployments can tune them.
|
||||||
|
var rl = builder.Configuration.GetSection("RateLimiting");
|
||||||
|
int Limit(string key, int def) => rl.GetValue(key, def);
|
||||||
|
var rlWindow = TimeSpan.FromSeconds(Limit("WindowSeconds", 60));
|
||||||
|
static string Partition(HttpContext ctx) =>
|
||||||
|
ctx.User.Identity?.IsAuthenticated == true
|
||||||
|
? ctx.User.FindFirstValue("inboxintel:uid") ?? "auth-unknown"
|
||||||
|
: ctx.Connection.RemoteIpAddress?.ToString() ?? "anon";
|
||||||
|
builder.Services.AddRateLimiter(o =>
|
||||||
|
{
|
||||||
|
o.RejectionStatusCode = StatusCodes.Status429TooManyRequests;
|
||||||
|
o.GlobalLimiter = PartitionedRateLimiter.Create<HttpContext, string>(ctx =>
|
||||||
|
RateLimitPartition.GetFixedWindowLimiter(Partition(ctx), _ =>
|
||||||
|
new FixedWindowRateLimiterOptions { PermitLimit = Limit("GlobalPermitLimit", 300), Window = rlWindow, QueueLimit = 0 }));
|
||||||
|
o.AddPolicy("auth", ctx =>
|
||||||
|
RateLimitPartition.GetFixedWindowLimiter(Partition(ctx), _ =>
|
||||||
|
new FixedWindowRateLimiterOptions { PermitLimit = Limit("AuthPermitLimit", 10), Window = rlWindow, QueueLimit = 0 }));
|
||||||
|
o.AddPolicy("expensive", ctx =>
|
||||||
|
RateLimitPartition.GetFixedWindowLimiter(Partition(ctx), _ =>
|
||||||
|
new FixedWindowRateLimiterOptions { PermitLimit = Limit("ExpensivePermitLimit", 20), Window = rlWindow, QueueLimit = 0 }));
|
||||||
|
});
|
||||||
|
|
||||||
builder.Services.AddEndpointsApiExplorer();
|
builder.Services.AddEndpointsApiExplorer();
|
||||||
builder.Services.AddSwaggerGen();
|
builder.Services.AddSwaggerGen();
|
||||||
// V-06: RFC7807 ProblemDetails so the global exception handler returns a safe,
|
// V-06: RFC7807 ProblemDetails so the global exception handler returns a safe,
|
||||||
@@ -115,7 +166,18 @@ using (var scope = app.Services.CreateScope())
|
|||||||
{
|
{
|
||||||
var db = scope.ServiceProvider.GetRequiredService<AppDbContext>();
|
var db = scope.ServiceProvider.GetRequiredService<AppDbContext>();
|
||||||
if (db.Database.IsRelational() && app.Configuration.GetValue("Database:AutoMigrate", true))
|
if (db.Database.IsRelational() && app.Configuration.GetValue("Database:AutoMigrate", true))
|
||||||
|
{
|
||||||
|
// AUDIT M-4: the shipped appsettings no longer carries a guessable default DB
|
||||||
|
// password. Fail fast with a clear message rather than connecting with weak or
|
||||||
|
// missing credentials (compose/staging/prod inject the full connection string).
|
||||||
|
var connStr = app.Configuration.GetConnectionString("Postgres") ?? string.Empty;
|
||||||
|
var csb = new Npgsql.NpgsqlConnectionStringBuilder(connStr);
|
||||||
|
if (string.IsNullOrWhiteSpace(csb.Password))
|
||||||
|
throw new InvalidOperationException(
|
||||||
|
"ConnectionStrings:Postgres has no password. Set the full connection string via " +
|
||||||
|
"environment/user-secrets (see README) — a default password is deliberately not shipped.");
|
||||||
await db.Database.MigrateAsync();
|
await db.Database.MigrateAsync();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Honor X-Forwarded-* from the nginx reverse proxy so OAuth redirect URIs and
|
// Honor X-Forwarded-* from the nginx reverse proxy so OAuth redirect URIs and
|
||||||
@@ -169,9 +231,22 @@ app.Use(async (ctx, next) =>
|
|||||||
app.UseSerilogRequestLogging();
|
app.UseSerilogRequestLogging();
|
||||||
app.UseCors("frontend");
|
app.UseCors("frontend");
|
||||||
app.UseAuthentication();
|
app.UseAuthentication();
|
||||||
|
// AUDIT H-2: after authentication so authenticated traffic partitions per-user; anonymous
|
||||||
|
// traffic partitions per-IP. Endpoint policies ("auth", "expensive") apply via attributes.
|
||||||
|
app.UseRateLimiter();
|
||||||
app.UseAuthorization();
|
app.UseAuthorization();
|
||||||
app.MapControllers();
|
app.MapControllers();
|
||||||
|
|
||||||
app.Run();
|
app.Run();
|
||||||
|
|
||||||
public partial class Program { }
|
public partial class Program { }
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// AUDIT M-1: absolute session lifetime check, extracted for unit testing. A session with no
|
||||||
|
/// issued stamp (pre-dating this feature) is treated as expired — one forced re-login.
|
||||||
|
/// </summary>
|
||||||
|
public static class SessionLifetime
|
||||||
|
{
|
||||||
|
public static bool IsExpired(string? issuedAtIso, DateTimeOffset now, TimeSpan maxAge)
|
||||||
|
=> !DateTimeOffset.TryParse(issuedAtIso, out var issued) || now - issued > maxAge;
|
||||||
|
}
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"ConnectionStrings": {
|
"ConnectionStrings": {
|
||||||
"Postgres": "Host=localhost;Port=5432;Database=inboxintel;Username=inboxintel;Password=inboxintel"
|
"Postgres": ""
|
||||||
},
|
},
|
||||||
"Database": {
|
"Database": {
|
||||||
"AutoMigrate": true
|
"AutoMigrate": true
|
||||||
|
|||||||
@@ -25,7 +25,7 @@ public class SmtpEmailSender : IEmailSender
|
|||||||
{
|
{
|
||||||
if (!IsEnabled)
|
if (!IsEnabled)
|
||||||
{
|
{
|
||||||
_logger.LogInformation("SMTP not configured; skipping email \"{Subject}\" to {To}", subject, toAddress);
|
_logger.LogDebug("SMTP not configured; skipping email \"{Subject}\" to {To}", subject, toAddress);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -53,6 +53,10 @@ public class AppDbContext : DbContext, IAppDbContext
|
|||||||
modelBuilder.Entity<MailDomain>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<MailDomain>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
modelBuilder.Entity<Attachment>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<Attachment>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
modelBuilder.Entity<Label>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<Label>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
|
// AUDIT M-6: EmailLabel is the required end of a relationship with the filtered Email
|
||||||
|
// entity; without a matching filter EF warns on boot and joins could surface rows whose
|
||||||
|
// parent is filtered out. Filter via the Email navigation so the pair is consistent.
|
||||||
|
modelBuilder.Entity<EmailLabel>().HasQueryFilter(el => CurrentUserId == Guid.Empty || el.Email!.UserId == CurrentUserId);
|
||||||
modelBuilder.Entity<SyncState>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<SyncState>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
modelBuilder.Entity<AnalyticsAggregate>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<AnalyticsAggregate>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
modelBuilder.Entity<WidgetLayout>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
modelBuilder.Entity<WidgetLayout>().HasQueryFilter(e => CurrentUserId == Guid.Empty || e.UserId == CurrentUserId);
|
||||||
|
|||||||
@@ -0,0 +1,172 @@
|
|||||||
|
using System.Net;
|
||||||
|
using System.Net.Http.Json;
|
||||||
|
using System.Security.Claims;
|
||||||
|
using System.Text.Encodings.Web;
|
||||||
|
using FluentAssertions;
|
||||||
|
using InboxIntel.Application.Abstractions;
|
||||||
|
using InboxIntel.Domain.Entities;
|
||||||
|
using InboxIntel.Infrastructure.Persistence;
|
||||||
|
using Microsoft.AspNetCore.Authentication;
|
||||||
|
using Microsoft.AspNetCore.Hosting;
|
||||||
|
using Microsoft.AspNetCore.Mvc.Testing;
|
||||||
|
using Microsoft.EntityFrameworkCore;
|
||||||
|
using Microsoft.Extensions.Configuration;
|
||||||
|
using Microsoft.Extensions.DependencyInjection;
|
||||||
|
using Microsoft.Extensions.Hosting;
|
||||||
|
using Microsoft.Extensions.Logging;
|
||||||
|
using Microsoft.Extensions.Options;
|
||||||
|
using Xunit;
|
||||||
|
|
||||||
|
namespace InboxIntel.IntegrationTests;
|
||||||
|
|
||||||
|
// ── Tests proving the Phase-2 audit fixes (see AUDIT_REPORT.md) ────────────────────────────
|
||||||
|
|
||||||
|
/// <summary>Pass-through auth scheme so integration tests can exercise authenticated
|
||||||
|
/// endpoints (model validation, per-user rate limits) without a real Google login.</summary>
|
||||||
|
public class TestAuthHandler : AuthenticationHandler<AuthenticationSchemeOptions>
|
||||||
|
{
|
||||||
|
public const string Scheme = "Test";
|
||||||
|
// Stable across requests so per-user rate-limit partitions accumulate correctly.
|
||||||
|
public static readonly string Uid = Guid.NewGuid().ToString();
|
||||||
|
|
||||||
|
public TestAuthHandler(IOptionsMonitor<AuthenticationSchemeOptions> o, ILoggerFactory l, UrlEncoder e)
|
||||||
|
: base(o, l, e) { }
|
||||||
|
|
||||||
|
protected override Task<AuthenticateResult> HandleAuthenticateAsync()
|
||||||
|
{
|
||||||
|
var identity = new ClaimsIdentity(new[]
|
||||||
|
{
|
||||||
|
new Claim(ClaimTypes.NameIdentifier, "test-sub"),
|
||||||
|
new Claim("inboxintel:uid", Uid),
|
||||||
|
}, Scheme);
|
||||||
|
return Task.FromResult(AuthenticateResult.Success(
|
||||||
|
new AuthenticationTicket(new ClaimsPrincipal(identity), Scheme)));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Factory with the test auth scheme + tiny rate-limit windows so limits trip fast.</summary>
|
||||||
|
public class AuditTestAppFactory : WebApplicationFactory<Program>
|
||||||
|
{
|
||||||
|
protected override IHost CreateHost(IHostBuilder builder)
|
||||||
|
{
|
||||||
|
builder.ConfigureHostConfiguration(cfg => cfg.AddInMemoryCollection(new Dictionary<string, string?>
|
||||||
|
{
|
||||||
|
["Database:AutoMigrate"] = "false",
|
||||||
|
["GoogleOAuth:ClientId"] = "test-client-id",
|
||||||
|
["GoogleOAuth:ClientSecret"] = "test-client-secret",
|
||||||
|
// H-2: make the auth policy trip on the 3rd request within the window.
|
||||||
|
["RateLimiting:AuthPermitLimit"] = "2",
|
||||||
|
["RateLimiting:WindowSeconds"] = "60",
|
||||||
|
}));
|
||||||
|
return base.CreateHost(builder);
|
||||||
|
}
|
||||||
|
|
||||||
|
protected override void ConfigureWebHost(IWebHostBuilder builder)
|
||||||
|
{
|
||||||
|
builder.ConfigureServices(services =>
|
||||||
|
{
|
||||||
|
services.AddAuthentication(TestAuthHandler.Scheme)
|
||||||
|
.AddScheme<AuthenticationSchemeOptions, TestAuthHandler>(TestAuthHandler.Scheme, _ => { });
|
||||||
|
services.PostConfigure<AuthenticationOptions>(o =>
|
||||||
|
{
|
||||||
|
o.DefaultAuthenticateScheme = TestAuthHandler.Scheme;
|
||||||
|
o.DefaultChallengeScheme = TestAuthHandler.Scheme;
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
public class AuditFixesTests : IClassFixture<AuditTestAppFactory>
|
||||||
|
{
|
||||||
|
private readonly AuditTestAppFactory _factory;
|
||||||
|
public AuditFixesTests(AuditTestAppFactory factory) => _factory = factory;
|
||||||
|
|
||||||
|
// H-1: FluentValidation auto-validation now rejects invalid DTOs at the boundary with 400
|
||||||
|
// (previously the registered validators never executed).
|
||||||
|
[Fact]
|
||||||
|
public async Task Invalid_search_request_is_rejected_with_400_by_the_validator()
|
||||||
|
{
|
||||||
|
var client = _factory.CreateClient();
|
||||||
|
var resp = await client.PostAsJsonAsync("/api/v1/search", new { page = 1, pageSize = 0 });
|
||||||
|
resp.StatusCode.Should().Be(HttpStatusCode.BadRequest);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Search_with_from_after_to_is_rejected_with_400()
|
||||||
|
{
|
||||||
|
var client = _factory.CreateClient();
|
||||||
|
var resp = await client.PostAsJsonAsync("/api/v1/search",
|
||||||
|
new { page = 1, pageSize = 10, from = "2026-02-01", to = "2026-01-01" });
|
||||||
|
resp.StatusCode.Should().Be(HttpStatusCode.BadRequest);
|
||||||
|
}
|
||||||
|
|
||||||
|
// H-2: the "auth" rate-limit policy returns 429 once the per-window permit is exhausted.
|
||||||
|
[Fact]
|
||||||
|
public async Task Auth_endpoint_rate_limits_with_429_after_the_permit_is_exhausted()
|
||||||
|
{
|
||||||
|
var client = _factory.CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false });
|
||||||
|
var s1 = (await client.GetAsync("/api/v1/auth/login")).StatusCode;
|
||||||
|
var s2 = (await client.GetAsync("/api/v1/auth/login")).StatusCode;
|
||||||
|
var s3 = (await client.GetAsync("/api/v1/auth/login")).StatusCode;
|
||||||
|
|
||||||
|
s1.Should().NotBe(HttpStatusCode.TooManyRequests);
|
||||||
|
s2.Should().NotBe(HttpStatusCode.TooManyRequests);
|
||||||
|
s3.Should().Be(HttpStatusCode.TooManyRequests);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// M-1: absolute session lifetime — a session older than the cap (or missing its issued
|
||||||
|
// stamp) is expired regardless of sliding renewal.
|
||||||
|
public class SessionLifetimeTests
|
||||||
|
{
|
||||||
|
private static readonly DateTimeOffset Now = new(2026, 07, 02, 12, 0, 0, TimeSpan.Zero);
|
||||||
|
private static readonly TimeSpan Max = TimeSpan.FromDays(30);
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Fresh_session_is_not_expired()
|
||||||
|
=> SessionLifetime.IsExpired(Now.AddDays(-1).ToString("O"), Now, Max).Should().BeFalse();
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Session_older_than_the_cap_is_expired()
|
||||||
|
=> SessionLifetime.IsExpired(Now.AddDays(-31).ToString("O"), Now, Max).Should().BeTrue();
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void Session_without_a_stamp_is_expired()
|
||||||
|
=> SessionLifetime.IsExpired(null, Now, Max).Should().BeTrue();
|
||||||
|
}
|
||||||
|
|
||||||
|
// M-6: EmailLabel now carries a matching tenant query filter (via its Email navigation), so
|
||||||
|
// join rows can never leak across users even without a manual Where.
|
||||||
|
public class EmailLabelFilterTests
|
||||||
|
{
|
||||||
|
private sealed class FakeCurrentUser : ICurrentUser
|
||||||
|
{
|
||||||
|
public Guid UserId { get; set; }
|
||||||
|
public bool IsAuthenticated => UserId != Guid.Empty;
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task EmailLabels_are_invisible_across_users()
|
||||||
|
{
|
||||||
|
var userA = Guid.NewGuid();
|
||||||
|
var userB = Guid.NewGuid();
|
||||||
|
var opts = new DbContextOptionsBuilder<AppDbContext>()
|
||||||
|
.UseInMemoryDatabase(nameof(EmailLabels_are_invisible_across_users)).Options;
|
||||||
|
|
||||||
|
using (var seed = new AppDbContext(opts, new FakeCurrentUser()))
|
||||||
|
{
|
||||||
|
var emailA = new Email { UserId = userA, GmailMessageId = "a1" };
|
||||||
|
var emailB = new Email { UserId = userB, GmailMessageId = "b1" };
|
||||||
|
var labelA = new Label { UserId = userA, GmailLabelId = "LA", Name = "A" };
|
||||||
|
var labelB = new Label { UserId = userB, GmailLabelId = "LB", Name = "B" };
|
||||||
|
seed.AddRange(emailA, emailB, labelA, labelB,
|
||||||
|
new EmailLabel { EmailId = emailA.Id, LabelId = labelA.Id },
|
||||||
|
new EmailLabel { EmailId = emailB.Id, LabelId = labelB.Id });
|
||||||
|
await seed.SaveChangesAsync();
|
||||||
|
}
|
||||||
|
|
||||||
|
using var ctx = new AppDbContext(opts, new FakeCurrentUser { UserId = userA });
|
||||||
|
var visible = await ctx.Set<EmailLabel>().ToListAsync(); // no manual Where — filter must enforce
|
||||||
|
visible.Should().HaveCount(1);
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user