diff --git a/docs/SECURITY_REPORT.md b/docs/SECURITY_REPORT.md new file mode 100644 index 0000000..f7c0287 --- /dev/null +++ b/docs/SECURITY_REPORT.md @@ -0,0 +1,122 @@ +# SECURITY_REPORT.md — Session Change Review + +> Phase 6 deliverable. Scope: security review of the changes made in this work session +> (Wave 0 + roadmap H1–H4), plus confirmation that the tenant-isolation model still holds. +> Date: 2026-07-03. Complements the prior standalone assessments in +> `docs/security-assessments/` (M013 adversarial, M014 remediation, M015 authorization replay). + +This is **not** a full re-audit of the whole application — those live in `docs/security-assessments/`. +It is a focused review of the new/changed surface so nothing shipped this session introduces a regression. + +--- + +## 1. Summary + +No new vulnerabilities were introduced. Tenant isolation on the new endpoints is carried by the +existing `JobTrackerContext` global query filters and is now covered by regression tests. One latent +correctness issue (a routable background-service method) was closed, and leaked runtime secrets were +removed from version control (rotation recommended — see §6). + +| Severity | Count | Items | +|---|---|---| +| Critical | 0 | — | +| High | 0 | — | +| Medium | 1 (mitigated) | DataProtection keys present in git history (untracked this session; rotation recommended) | +| Low / hardening | 3 | see §5 | + +--- + +## 2. New/changed attack surface reviewed + +| Change | Surface | Verdict | +|---|---|---| +| `GET /jobapplications/{id}/match-score` | route int id; reads own CV + job | Safe — tenant-scoped | +| `GET /jobapplications/{id}/status-suggestion` | route int id; reads own correspondence | Safe — tenant-scoped | +| `GET /jobapplications/pipeline` | none (static metadata) | Safe | +| `PATCH .../status`, Create/Update (status normalization) | user string → `JobPipeline.Normalize` | Safe — no injection, values stored parameterized | +| Structured salary fields | numeric + short strings, `NormalizeSalary` | Safe — clamps negatives, whitelists period | +| Automated DB backup (`VACUUM INTO`) | server-controlled path | Safe — see §4 | +| Dev OpenAPI (`/openapi/v1.json`) | schema | Safe — `Development` environment only | +| `EmailStatusClassifier` | reads stored correspondence text | Safe — deterministic, no eval/injection | + +--- + +## 3. OWASP-oriented checklist for the new code + +- **A01 Broken Access Control** — The two new data endpoints load the job via + `_db.JobApplications.FirstOrDefaultAsync(j => j.Id == id)`, which is filtered by the global + query filter `CurrentUserId != null && OwnerUserId == CurrentUserId` (deny-on-null, hardened in + M013-2). A cross-user id returns `NotFound`, not another tenant's data. The correspondence lookup + in `status-suggestion` and the `JobEvent` lookup in analytics are likewise filtered through their + parent's owner. **Verified by `JobApplicationsAuthorizationTests` (match-score + status-suggestion).** +- **A03 Injection** — All new persistence goes through EF Core parameterized queries. The only raw + SQL added is `VACUUM INTO ''` with a fully server-derived path (see §4). No string + concatenation of user input into queries. +- **A03 ReDoS** — New regexes (`JobCvMatchService.TokenPattern`, the revised `SkillTagger` C#/.NET + patterns with fixed-width look-behinds) are linear with no catastrophic backtracking. +- **A04 Insecure Design** — Status suggestions and match scoring are deterministic and + **human-confirmed** (a status only changes when the user clicks). No automated outbound actions. +- **A05 Security Misconfiguration** — OpenAPI is exposed only under `IsDevelopment()`; production + deployments (`ASPNETCORE_ENVIRONMENT=Production`) do not serve it. +- **A08 Data Integrity** — `JobPipeline.Normalize` canonicalizes status on write but preserves + unknown custom values (no silent data loss). +- **A09 Logging** — No secrets or PII added to logs by the new code. + +--- + +## 4. Database backup — path handling + +`SqliteDatabaseBackupRunner` runs `VACUUM INTO ''`. The target is +`/backups/jobtracker_backup_.db` — no user input reaches it — and single +quotes are escaped defensively. Backups contain the full database (sensitive) and are written to the +same data volume as the live DB, i.e. the same trust boundary; they are git-ignored. For defense in +depth, operators should ship backups off-host with transport encryption and restrict volume +permissions. **Recommendation (low):** document an off-host, encrypted backup rotation in the +deployment guide. + +--- + +## 5. Low / hardening findings + +1. **match-score input size (low).** `GetMatchScore` does not cap job-description length before + tokenizing. Descriptions are bounded in practice (imported/typed), and the algorithm is linear, so + this is not a DoS, but a defensive cap (e.g. 50 KB) would be prudent. +2. **New read endpoints are not rate-limited (low).** `match-score`/`status-suggestion` are cheap and + deterministic (no AI, one indexed query), and auth-gated in production, so abuse potential is low. + Consider a general authenticated-read limiter if the API is exposed publicly. +3. **status-suggestion is conservative for custom statuses (informational).** A job in a non-canonical + custom status (pipeline order = max) never receives a suggestion. This is safe (fails closed) but + slightly under-surfaces; acceptable given custom statuses are rare. + +--- + +## 6. Secrets hygiene (actioned this session) + +- Committed ASP.NET **DataProtection key XML files** (`keys/`, `JobTrackerApi/keys/`) and daily export + JSON were removed from tracking and added to `.gitignore` + (commit `security: untrack DataProtection keys and runtime exports…`). +- **These key files remain in git history.** DataProtection keys sign auth/session artifacts, so + **rotating them on the production host is recommended** (generate fresh keys; the app regenerates the + key ring in the persisted `keys/` directory on next start). Until rotated, anyone with history access + could read the old key material. +- Local `.env` remains git-ignored; `appsettings.Development.json` contains only `CHANGE_ME_*` + placeholders. No live secrets are tracked. + +--- + +## 7. Confirmed intact from prior assessments + +Spot-checked that the M013–M015 remediations are still in force after this session's changes: + +- Owner query filters still deny on null `CurrentUserId` (`Data/JobTrackerContext.cs`). +- Local JWT still requires a concrete subject claim (`LocalAuthIdentity`, `Program.cs`). +- Job-import SSRF guard (DNS resolution + private-range rejection, redirects disabled) untouched. +- CSRF double-submit middleware and CORS allowlist untouched. + +--- + +## 8. Retest + +All backend tests pass (135), including the two new tenant-isolation tests for the new endpoints. +No fix in this report required code changes beyond what already landed; the residual **action for the +operator is DataProtection key rotation** (§6).