Ross
232 lines
10 KiB
Markdown
232 lines
10 KiB
Markdown
# Security Audit Report
|
|
|
|
**Date:** 2026-04-30
|
|
**Scope:** All `views.py` files and `generic/decorators.py` across the entire project.
|
|
|
|
---
|
|
|
|
## Issues Fixed
|
|
|
|
### 1. Missing `@login_required` on `exam_collection_exams_partial` (generic/views.py)
|
|
|
|
**Severity:** High
|
|
**File:** `generic/views.py`
|
|
The `exam_collection_exams_partial` view was publicly accessible with no authentication. It exposed exam names, candidate counts, and author information from any collection to anonymous users.
|
|
**Fix:** Added `@login_required` decorator.
|
|
|
|
---
|
|
|
|
### 2. Unauthenticated `get_*_id` popup-helper views (multiple files)
|
|
|
|
**Severity:** High
|
|
**Files:** `generic/views.py`, `anatomy/views.py`, `rapids/views.py`, `longs/views.py`, `atlas/views.py`
|
|
|
|
All of the following views were decorated with `@csrf_exempt` but had **no authentication**. They allowed unauthenticated users to enumerate internal database records (Examinations, BodyParts, Structures, Abnormalities, Regions) by name via GET requests.
|
|
|
|
Additionally, all of these views contained a bug: `request.accepts("application/json")()` calls the boolean result of `accepts()` as a function, which raises `TypeError: 'bool' object is not callable`. This made them effectively non-functional (always returning `HttpResponse("/")`).
|
|
|
|
| File | View |
|
|
|---|---|
|
|
| `generic/views.py` | `get_examination_id` |
|
|
| `anatomy/views.py` | `get_body_part_id`, `get_examination_id`, `get_structure_id` |
|
|
| `rapids/views.py` | `get_abnormality_id`, `get_examination_id`, `get_region_id` |
|
|
| `longs/views.py` | `get_examination_id` |
|
|
| `atlas/views.py` | `get_examination_id` |
|
|
|
|
**Fix:**
|
|
- Removed `@csrf_exempt` (GET-only views do not need CSRF exemption).
|
|
- Added `@login_required`.
|
|
- Fixed `request.accepts("application/json")()` → `request.accepts("application/json")`.
|
|
- Replaced `request.GET["key"]` dict access (raises `KeyError` on missing key) with `request.GET.get("key", "")`.
|
|
- Added docstrings documenting scope and functionality.
|
|
|
|
---
|
|
|
|
### 3. Unauthenticated `answer_suggestion_submit` with CSRF exemption (rad/views.py)
|
|
|
|
**Severity:** High
|
|
**File:** `rad/views.py` (line ~781)
|
|
|
|
This view accepted anonymous POST requests with no CSRF protection to create new `Answer` records (marked `proposed=True`) on any existing question. This allowed anyone on the internet to submit arbitrary proposed answers to anatomy or rapids questions, polluting the moderation queue.
|
|
|
|
**Fix:**
|
|
- Removed `@csrf_exempt`.
|
|
- Added `@login_required`.
|
|
- Added `if request.method != "POST": return ...` guard.
|
|
- Fixed a minor typo in the response message (`"submited"` → `"submitted"`).
|
|
- Added docstring documenting scope and functionality.
|
|
|
|
---
|
|
|
|
### 4. Debug `print()` in `check_user_in_group` decorator (generic/decorators.py)
|
|
|
|
**Severity:** Medium
|
|
**File:** `generic/decorators.py`
|
|
|
|
The `check_user_in_group` decorator printed `request.user` to stdout on every invocation, leaking usernames/email addresses to application logs.
|
|
**Fix:** Removed the `print(request.user)` statement.
|
|
|
|
---
|
|
|
|
### 5. Debug `print()` statements leaking sensitive data (multiple files)
|
|
|
|
**Severity:** Medium
|
|
**Files:** `generic/views.py`, `atlas/views.py`, `longs/views.py`, `sbas/views.py`, `physics/views.py`, `shorts/views.py`, `rcr/views.py`, `oef/views.py`
|
|
|
|
Numerous active (non-commented) `print()` calls throughout view code leaked sensitive data to stdout/production logs, including:
|
|
- User objects and usernames
|
|
- CID numbers and passcodes
|
|
- Email address lists being processed
|
|
- Exam and queryset data
|
|
|
|
**Fix:** All active `print()` statements replaced with `logger.debug()` (using the project-standard `loguru` logger) or removed where they provided no diagnostic value. Commented-out print statements were left in place.
|
|
|
|
---
|
|
|
|
### 6. `@csrf_exempt` on `collection_case_displaysetup` (atlas/views.py)
|
|
|
|
**Severity:** Low
|
|
**File:** `atlas/views.py`
|
|
|
|
The view had `@csrf_exempt` applied alongside a comment saying "Only if you have CSRF issues; otherwise, keep CSRF protection". The view already has proper authentication via `@user_is_collection_author_or_atlas_editor`, so the CSRF exemption was unnecessary and weakened security.
|
|
**Fix:** Removed `@csrf_exempt`.
|
|
|
|
---
|
|
|
|
### 7. Missing `@login_required` on `toggle_share_with_supervisor` (generic/views.py)
|
|
|
|
**Severity:** Medium
|
|
**File:** `generic/views.py`
|
|
|
|
This HTMX view had no authentication check. An anonymous user with a known `CidUserExam` PK could toggle the `share_with_supervisor` flag (although the `request.user != cid_user_exam.user_user` check would prevent the toggle from succeeding, it still leaks whether the record exists).
|
|
**Fix:** Added `@login_required` and updated docstring.
|
|
|
|
---
|
|
|
|
### 8. Missing `@login_required` on `active_exams` in sbas/views.py
|
|
|
|
**Severity:** Medium
|
|
**File:** `sbas/views.py`
|
|
|
|
The `active_exams` view had no authentication. It returned a list of all active SBA exams to anonymous users.
|
|
**Fix:** Added `@login_required`. Updated docstring.
|
|
|
|
---
|
|
|
|
### 9. Unreachable dead code in `rcr/views.py`
|
|
|
|
**Severity:** Low
|
|
**File:** `rcr/views.py`
|
|
|
|
In the `get_object` method of an `UpdateView` subclass, two lines appeared after a `return obj` statement:
|
|
```python
|
|
return obj
|
|
print("test") # unreachable
|
|
return super().get_object() # unreachable
|
|
```
|
|
**Fix:** Removed the unreachable `print("test")` and `return super().get_object()` lines.
|
|
|
|
---
|
|
|
|
## Issues Requiring Clarification (Not Fixed)
|
|
|
|
### A. `@csrf_exempt` on `exam_submit` (rad/views.py)
|
|
|
|
**File:** `rad/views.py`
|
|
This view accepts POST submissions of exam answers without CSRF protection and without `@login_required`. It appears to be a legacy mobile/external client endpoint that dispatches to `post_exam_answers()` on individual exam view classes, which may perform their own CID/passcode authentication.
|
|
|
|
**Risk:** Without authentication at this layer, the endpoint could be abused to submit bogus exam answers for real users if a valid CID/passcode combination is known.
|
|
|
|
**Recommendation:** Clarify whether this endpoint is still in active use by a mobile client. If so, implement token-based authentication (e.g. Django REST Framework token auth) or require CID/passcode in the request body and validate before dispatching. If not in use, remove it.
|
|
|
|
---
|
|
|
|
### B. Unauthenticated views in `rcr/views.py`
|
|
|
|
**File:** `rcr/views.py`
|
|
The following views have no authentication decorators:
|
|
- `radiology_gap` — renders a full RCR gap analysis page with all curriculum items
|
|
- `radiology_results` — renders all RCR assessment results
|
|
|
|
**Risk:** If these pages are intended to be internal-only (for assessors), they should require authentication. If they are intentionally public (e.g., public-facing curriculum overview), this is acceptable.
|
|
|
|
**Recommendation:** Confirm intended visibility. If internal, add `@login_required` (or a more specific decorator).
|
|
|
|
---
|
|
|
|
### C. Unauthenticated class-based views in `oef/views.py`
|
|
|
|
**File:** `oef/views.py`
|
|
The following CBVs lack `LoginRequiredMixin`:
|
|
- `EntryUpdateView` — allows updating OEF entry fields
|
|
- `EntryTableView` — lists all entries
|
|
- `FormatsCreateView` — creates new Formats objects
|
|
- `FormatsUpdateView` — updates Formats objects
|
|
|
|
The update views in particular are a concern if this is not an intentionally public tool.
|
|
|
|
**Recommendation:** Add `LoginRequiredMixin` (and appropriate permission checks) if this app is not intended to be publicly editable.
|
|
|
|
---
|
|
|
|
### D. Commented-out `@login_required` on `exam_toggle_flag` in physics/views.py
|
|
|
|
**File:** `physics/views.py`
|
|
The `exam_toggle_flag` view has `#@login_required` commented out. The view falls back to `exam.check_user_can_take(cid, passcode, request.user)` for access control, suggesting this is intentionally accessible to CID users (non-Django-auth users). However, the intent should be confirmed.
|
|
|
|
**Recommendation:** If the CID/passcode check is the intended authentication mechanism, document this clearly. If the view should also support Django-authenticated users, restore the decorator appropriately.
|
|
|
|
---
|
|
|
|
### E. `exam_submit` potential `AttributeError` (rad/views.py)
|
|
|
|
**File:** `rad/views.py`
|
|
```python
|
|
exam_type, exam_id = request.POST.get("eid").split("/")
|
|
```
|
|
If `eid` is missing from the POST body, `request.POST.get("eid")` returns `None`, and `.split("/")` raises `AttributeError`. This would return a 500 response rather than a clean error.
|
|
|
|
**Recommendation:** Guard with:
|
|
```python
|
|
eid = request.POST.get("eid", "")
|
|
if "/" not in eid:
|
|
return JsonResponse({"success": False, "error": "Invalid eid"})
|
|
exam_type, exam_id = eid.split("/", 1)
|
|
```
|
|
|
|
---
|
|
|
|
### F. Open redirect risk in `RedirectMixin` and `UpdateUserProfileView` (generic/views.py, rad/views.py)
|
|
|
|
**File:** `generic/views.py`, `rad/views.py`
|
|
Both `RedirectMixin.get_success_url` and `UpdateUserProfileView.get_success_url` use `self.request.GET["redirect"]` / `self.request.GET.get("redirect")` as the redirect target without validation. A malicious actor could craft a URL like `?redirect=https://evil.com` to redirect authenticated users to a phishing site after a successful form submission.
|
|
|
|
**Recommendation:** Validate the redirect URL with Django's `url_has_allowed_host_and_scheme`:
|
|
```python
|
|
from django.utils.http import url_has_allowed_host_and_scheme
|
|
|
|
redirect_url = self.request.GET.get("redirect", "")
|
|
if url_has_allowed_host_and_scheme(redirect_url, allowed_hosts={self.request.get_host()}):
|
|
return redirect_url
|
|
return super().get_success_url()
|
|
```
|
|
|
|
---
|
|
|
|
### G. Missing login check on `sbas/views.py` `#@login_required` commented-out views
|
|
|
|
**File:** `sbas/views.py`
|
|
Two views have `@login_required` commented out:
|
|
- `question_view` (around line 222) — currently commented out entirely
|
|
- `question_detail` (around line 226) — currently commented out entirely
|
|
|
|
These are fully commented-out functions; no action needed unless they are re-enabled.
|
|
|
|
---
|
|
|
|
## Notes on Existing `@csrf_exempt` Usage
|
|
|
|
The following `@csrf_exempt` usages remain after fixes. All are on GET-only admin popup helpers that are now also `@login_required`. CSRF protection is not relevant for idempotent GET requests, but the decorators are harmless:
|
|
|
|
> All `@csrf_exempt` instances have been removed from the codebase as part of this audit. The pattern `@csrf_exempt` on GET-only views is redundant since CSRF protection only applies to state-changing requests (POST/PUT/PATCH/DELETE).
|