Add security assessment and remediation plan - #3
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new security assessment and remediation plan document intended to help instructors and student developers identify and address high-impact security gaps in the current API (authorization scoping, upload DoS risk, brute-force login protection, admin-only system settings, and SSE error leakage).
Changes:
- Introduces a severity-ranked set of security findings (P0–P2) with repro steps and remediation guidance.
- Provides ready-to-paste PR review summaries and inline comment templates to standardize security feedback.
- Includes an instructor-facing note framing P0 items as release blockers and emphasizing regression tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
| Upload handler reads entire file into memory (`await upload.read()`) before writing to disk. | ||
| No explicit server-side file size cap in FastAPI path. |
| ## P0-4: No brute-force protection on login | ||
|
|
||
| ### What is wrong | ||
| `POST /auth/login` has no rate limiting, lockout, or backoff. |
| ## 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. |
There was a problem hiding this comment.
Non-blocking feedback (2)
-
Clarify the trust boundary this assessment assumes — .../docs/security-assessment-and-remediation-plan.md#L17-L173
The repo currently contains both team-shared wording (Readme.md,server/db/init.sql) and per-user wording (docs/implementation.md). Since P0-1, P0-2, and P1-1 severity depends on that boundary, add a short assumptions/source-of-truth section so readers know whether these are policy violations or intentional team-level behavior. -
Add concrete code pointers for each P0/P1 finding — .../docs/security-assessment-and-remediation-plan.md#L22-L160
The findings are directionally strong, but remediation will be faster if each item references exact backend symbols/routes (for exampleserver/app/routers/conversations.py,server/app/routers/documents.py,server/app/routers/auth.py,server/app/routers/system.py,server/app/routers/chat.py). That also makes severity discussions more objective during review.
If you want Charlie to apply these doc edits directly, reply with please fix 1-2.
This pull request adds a comprehensive security assessment and remediation plan for the project. The document identifies several critical and high-severity vulnerabilities, provides practical exploit examples, and gives actionable remediation steps. It also includes ready-to-use PR review summaries and inline comment snippets for code review, making it easier to communicate security requirements to the team.
The most important changes are:
Critical Security Findings (P0):
High Severity Issues (P1):
Security Review Tools: