From 084f90a851b1311d29d13ef7e0f98af9a6785af0 Mon Sep 17 00:00:00 2001 From: JacksCodeVault Date: Fri, 26 Jun 2026 12:50:21 +0300 Subject: [PATCH] Add security assessment and remediation plan --- ...ecurity-assessment-and-remediation-plan.md | 240 ++++++++++++++++++ 1 file changed, 240 insertions(+) create mode 100644 docs/security-assessment-and-remediation-plan.md diff --git a/docs/security-assessment-and-remediation-plan.md b/docs/security-assessment-and-remediation-plan.md new file mode 100644 index 0000000..94169a3 --- /dev/null +++ b/docs/security-assessment-and-remediation-plan.md @@ -0,0 +1,240 @@ +# Security Assessment and Remediation Plan + +Audience: instructors and student developers. + +This document is intentionally direct. These are not theoretical concerns; several are immediately exploitable by any authenticated user. + +--- + +## Severity legend + +- **P0 / Critical**: must fix before real use +- **P1 / High**: fix next +- **P2 / Medium**: important hardening + +--- + +## P0-1: Broken Access Control (IDOR) across conversations + +### What is wrong +Authentication exists (cookie session), but authorization scoping is missing in conversation queries. + +Observed behavior from code paths: +- `GET /conversations` returns all conversations +- `GET /conversations/{id}` fetches by id without user/team ownership check +- `DELETE /conversations/{id}` deletes by id without ownership check +- `POST /chat` accepts `conversation_id` and validates existence, not ownership + +### Why this is severe +Any logged-in user can read, hijack, and delete another user’s chat history. + +### Practical exploit path +1. User A creates conversations. +2. User B logs in normally. +3. User B requests conversation list and gets IDs/titles not belonging to them. +4. User B reads and deletes those conversations. + +### Repro examples + +```bash +# 1) Login as user B (captures cookie) +curl -i -c cookie.txt -X POST http://localhost:8000/auth/login \ + -H 'Content-Type: application/json' \ + -d '{"username":"user_b","password":"password"}' + +# 2) List all conversations (should be scoped, but currently global) +curl -b cookie.txt http://localhost:8000/conversations + +# 3) Read a specific conversation (including another user's) +curl -b cookie.txt http://localhost:8000/conversations/ + +# 4) Delete victim conversation +curl -i -b cookie.txt -X DELETE http://localhost:8000/conversations/ +``` + +### Fix +- Add ownership/team columns and policy. +- Pass current user scope into DB methods. +- Enforce WHERE clauses by owner/team on list/get/delete and chat conversation resolution. +- Add tests that user B gets 404/403 on user A resources. + +--- + +## P0-2: Broken Access Control likely affecting document operations + +### What is wrong +Document endpoints are authenticated but not scoped by owner/team in query methods. + +### Why this is severe +Any authenticated user can enumerate and delete documents outside their authorization boundary. + +### Repro examples + +```bash +# List documents (expected: own/team docs only) +curl -b cookie.txt http://localhost:8000/documents + +# Soft delete someone else's doc +curl -i -b cookie.txt -X DELETE \ + http://localhost:8000/documents/ + +# Hard delete someone else's doc +curl -i -b cookie.txt -X DELETE \ + 'http://localhost:8000/documents/?hard=true' +``` + +### Fix +Same as conversations: strict authorization scoping at DB layer + route layer. + +--- + +## P0-3: Upload memory DoS and parser abuse + +### What is wrong +Upload handler reads entire file into memory (`await upload.read()`) before writing to disk. +No explicit server-side file size cap in FastAPI path. + +### Why this is severe +Single oversized request can consume memory and degrade or crash service. +Crafted PDFs can force expensive parser work. + +### Practical exploit path +- Send very large multipart uploads repeatedly. +- Worker/API memory and CPU spike; requests queue/fail. + +### Repro idea + +```bash +# Example: upload a very large file repeatedly +curl -b cookie.txt -F 'files=@large.pdf' http://localhost:8000/documents/upload +``` + +### Fix +- Stream uploads in chunks to disk. +- Enforce max size in app code (not only nginx). +- Cap PDF page count and parsing time. +- Add request rate limits for upload endpoint. + +--- + +## P0-4: No brute-force protection on login + +### What is wrong +`POST /auth/login` has no rate limiting, lockout, or backoff. + +### Why this is severe +Enables rapid online password guessing and credential stuffing. + +### Repro idea +Automate repeated credential attempts from one host. + +### Fix +- Per-IP + per-username throttling. +- Temporary lockout or exponential delay after failures. +- Security logging/alerting for failed auth spikes. + +--- + +## P1-1: Any authenticated user can switch global LLM mode + +### What is wrong +`PUT /system/llm` changes deployment-wide behavior and is available to any signed-in user. + +### Why this matters +Operational sabotage / accidental outages / unexpected cloud usage and cost. + +### Fix +Require admin role for global system setting changes. + +--- + +## P1-2: Internal error leakage in chat SSE + +### What is wrong +SSE error events include raw exception text. + +### Why this matters +May leak internals (stack detail, infra hints, model/backend failures). + +### Fix +Return generic client error messages; log detailed exceptions server-side. + +--- + +## P1-3: Authorization model mismatch with project brief + +### What is wrong +Brief mentions per-user history and internal tool controls; implementation currently behaves as globally shared in core queries. + +### Why this matters +Policy expectations and implementation differ — dangerous in org context. + +### Fix +Define and implement explicit model: per-user, per-team, or role-based shared spaces. + +--- + +## P2: Additional hardening and resilience + +- Add pagination and bounds on list endpoints. +- Add optimistic locking or transactional strategy for filename version race handling. +- Add CSRF protections for cookie-authenticated state-changing endpoints. +- Add audit trail (who deleted what, who changed LLM mode, when). +- Add content-security headers and stricter production defaults. + +--- + +## Ready-to-paste PR review summary + +```markdown +Requesting changes before merge due to critical security blockers: + +P0: +1) Broken access control (IDOR) in conversations and likely documents +2) Upload endpoint allows memory/CPU DoS conditions +3) Login endpoint has no brute-force protections + +P1: +1) Global LLM mode switch should be admin-only +2) Chat SSE leaks internal errors + +Please fix P0 items with regression tests, then request re-review. +``` + +--- + +## Ready-to-paste inline comment snippets + +### On conversation list/get/delete routes +```markdown +P0: AuthN exists, but authZ scoping is missing. This query must be constrained by current user/team; otherwise any authenticated user can read/delete others' data. +``` + +### On chat conversation resolution +```markdown +P0: `conversation_id` existence check is not enough. Validate ownership/team membership before loading or appending messages. +``` + +### On upload read path +```markdown +P0: Avoid `await upload.read()` for full-file memory load. Stream to disk in chunks and reject files above a strict server-side max size. +``` + +### On login handler +```markdown +P0: Missing brute-force defenses (rate limits/lockout/backoff). Please add per-IP + per-username controls and tests. +``` + +### On system LLM mode route +```markdown +P1: This mutates deployment-wide state and should require admin role. +``` + +--- + +## Instructor note + +Treat the P0 findings as release blockers. Students should demonstrate: +1) threat model, +2) secure-by-default authorization boundaries, +3) regression tests proving isolation between users.