docs: add security review of session changes (Phase 6)
Scoped security review of Wave 0 + H1-H4: confirms tenant isolation on new endpoints (query filters + tests), no injection/ReDoS, dev-only OpenAPI. Flags DataProtection key rotation as the operator action item. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
cesnimda
@@ -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 '<path>'` 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 '<target>'`. The target is
|
||||
`<Data:Root>/backups/jobtracker_backup_<UTC-timestamp>.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).
|
||||
Reference in New Issue
Block a user