fix(security): fail closed on telemetry ownership check and protect user activity log - #19
parthasdey2304 wants to merge 1 commit into
Conversation
… activity log, and prevent practice attempt spoofing
Strix Security Review1 open security finding on this PR:
Review summaryReviewed PR #19, a security remediation covering fail-closed ownership verification and activity-log authorization. The fail-closed changes to the AI conversation telemetry sync and practice attempt submission endpoints correctly propagate Fixed the findings? re-run the review, or tag Updated for Reviewed by Strix |
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
| # Prevent spoofing of another user's identity if authenticated | ||
| if user_uid: | ||
| if not is_admin or not entry.get("uid"): | ||
| entry["uid"] = user_uid | ||
| if user_name and (not is_admin or not entry.get("student_name")): | ||
| entry["student_name"] = user_name | ||
| if user_email and not entry.get("student_email"): | ||
| if user_email and (not is_admin or not entry.get("student_email")): | ||
| entry["student_email"] = user_email |
There was a problem hiding this comment.
🔵 Activity log identity spoofing still possible for unauthenticated requests in POST /api/v1/activity/log
Severity: MEDIUM · CWE-863
The anti-spoofing block added in ingest_log only executes when an authenticated user context is present. Because the endpoint resolves the caller via get_current_user_optional, a request with no Authorization header produces current_user == None, so user_uid, user_name, and user_email are all None and the new block is a no-op. Client-supplied uid, student_name, and student_email values are then persisted verbatim by db.save_activity_log. As a result, the PR's stated goal of preventing non-admin clients from impersonating other accounts is not achieved for unauthenticated callers: an anonymous caller who supplies valid HMAC headers can still inject a forged activity entry (e.g. a CHEAT_ATTEMPT) attributed to an arbitrary victim uid. The shared static HMAC secret is transport-level integrity, not per-user authentication, so it does not block this spoofing.
| # Prevent spoofing of another user's identity if authenticated | |
| if user_uid: | |
| if not is_admin or not entry.get("uid"): | |
| entry["uid"] = user_uid | |
| if user_name and (not is_admin or not entry.get("student_name")): | |
| entry["student_name"] = user_name | |
| if user_email and not entry.get("student_email"): | |
| if user_email and (not is_admin or not entry.get("student_email")): | |
| entry["student_email"] = user_email | |
| # Prevent spoofing of another user's identity | |
| if not user_uid: | |
| entry.pop("uid", None) | |
| entry.pop("student_name", None) | |
| entry.pop("student_email", None) | |
| if user_uid and (not is_admin or not entry.get("uid")): | |
| entry["uid"] = user_uid | |
| if user_name and (not is_admin or not entry.get("student_name")): | |
| entry["student_name"] = user_name | |
| if user_email and (not is_admin or not entry.get("student_email")): | |
| entry["student_email"] = user_email |
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: Activity log identity spoofing still possible for unauthenticated requests in POST /api/v1/activity/log
Severity: MEDIUM
CWE: CWE-863
The anti-spoofing block added in `ingest_log` only executes when an authenticated user context is present. Because the endpoint resolves the caller via `get_current_user_optional`, a request with no `Authorization` header produces `current_user == None`, so `user_uid`, `user_name`, and `user_email` are all `None` and the new block is a no-op. Client-supplied `uid`, `student_name`, and `student_email` values are then persisted verbatim by `db.save_activity_log`. As a result, the PR's stated goal of preventing non-admin clients from impersonating other accounts is not achieved for unauthenticated callers: an anonymous caller who supplies valid HMAC headers can still inject a forged activity entry (e.g. a `CHEAT_ATTEMPT`) attributed to an arbitrary victim `uid`. The shared static HMAC secret is transport-level integrity, not per-user authentication, so it does not block this spoofing.
Location: app/routers/activity.py:65-72
Context: Neutralize identity for anonymous callers
```
// Before:
# Prevent spoofing of another user's identity if authenticated
if user_uid:
if not is_admin or not entry.get("uid"):
entry["uid"] = user_uid
if user_name and (not is_admin or not entry.get("student_name")):
entry["student_name"] = user_name
if user_email and (not is_admin or not entry.get("student_email")):
entry["student_email"] = user_email
// After:
# Prevent spoofing of another user's identity
if not user_uid:
entry.pop("uid", None)
entry.pop("student_name", None)
entry.pop("student_email", None)
if user_uid and (not is_admin or not entry.get("uid")):
entry["uid"] = user_uid
if user_name and (not is_admin or not entry.get("student_name")):
entry["student_name"] = user_name
if user_email and (not is_admin or not entry.get("student_email")):
entry["student_email"] = user_email
```
How to fix:
Require authentication on this endpoint by switching from `get_current_user_optional` to `get_current_user` so anonymous requests are rejected. Alternatively, if anonymous telemetry must remain allowed, strip or reject client-supplied identity fields (`uid`, `student_name`, `student_email`) whenever no authenticated user is present, so spoofed attribution cannot be persisted. Treat the shared static HMAC secret as integrity protection only and never as a basis for attributing activity to a specific user account.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
1 participant
Security Fixes (Strix Review Remediation)