fix: API endpoint hardening and JSON serialization fixes#1004
Closed
ankushchk wants to merge 4 commits into
Closed
fix: API endpoint hardening and JSON serialization fixes#1004ankushchk wants to merge 4 commits into
ankushchk wants to merge 4 commits into
Conversation
Contributor
👀 Peer Review RequiredHi @ankushchk! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
WalkthroughCourse API responses now return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/views.py (1)
2328-2338:⚠️ Potential issue | 🟠 MajorValidate and normalize
price,max_students, andlevelbefore persisting.Currently these values are accepted as-is. Invalid numeric types can still throw server-side errors, and invalid
levelvalues can be stored if not explicitly checked.Suggested input normalization before
Course.objects.create+ try: + price = Decimal(str(data["price"])) + except Exception: + return JsonResponse({"error": "Invalid price"}, status=400) + + try: + max_students = int(data["max_students"]) + if max_students < 1: + raise ValueError + except (TypeError, ValueError): + return JsonResponse({"error": "Invalid max_students"}, status=400) + + allowed_levels = {choice[0] for choice in Course._meta.get_field("level").choices} + level = data.get("level", "beginner") + if level not in allowed_levels: + return JsonResponse({"error": "Invalid level"}, status=400) + course = Course.objects.create( teacher=request.user, title=data["title"], description=data["description"], learning_objectives=data["learning_objectives"], prerequisites=data.get("prerequisites", ""), - price=data["price"], - max_students=data["max_students"], + price=price, + max_students=max_students, subject=subject, - level=data.get("level", "beginner"), + level=level, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2328 - 2338, Before calling Course.objects.create, validate and normalize data["price"], data["max_students"], and data.get("level"): coerce price to a decimal/float (or Decimal) and max_students to an int with proper try/except to return a validation error on failure, enforce non-negative bounds, and ensure level is one of the allowed values (e.g., "beginner","intermediate","advanced") falling back to "beginner" if missing or invalid; replace the raw values passed into Course.objects.create with the normalized variables and return a clear HTTP 400/validation response when conversion or validation fails so invalid types are never persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/views.py`:
- Around line 2462-2466: The current required-field loop (using required_fields
and data) only checks key presence and allows empty strings or null relation IDs
to slip through; update the validation to ensure string fields ("title",
"content") are non-empty after trimming and that relation fields ("category" or
"topic") are present and not null/empty (and optionally castable to an int)
before proceeding, returning JsonResponse({"error": "..."} , status=400) on
failure; apply the same strengthened checks to the other forum create endpoint
that uses the same pattern (the block at the other location using
required_fields/data).
- Around line 2317-2321: The current required-fields loop only checks for key
presence (required_fields and data) and allows null/blank values; update the
validation to return 400 if a field is missing OR data[field] is None OR
(isinstance(data[field], str) and data[field].strip() == '') and for sequence
types (e.g., lists like learning_objectives) treat empty sequences as missing by
checking if not data[field] or len(data[field]) == 0; modify the loop that
iterates required_fields to perform these checks and return
JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400)
when any of those conditions are met so downstream object creation won't receive
null/blank inputs.
---
Outside diff comments:
In `@web/views.py`:
- Around line 2328-2338: Before calling Course.objects.create, validate and
normalize data["price"], data["max_students"], and data.get("level"): coerce
price to a decimal/float (or Decimal) and max_students to an int with proper
try/except to return a validation error on failure, enforce non-negative bounds,
and ensure level is one of the allowed values (e.g.,
"beginner","intermediate","advanced") falling back to "beginner" if missing or
invalid; replace the raw values passed into Course.objects.create with the
normalized variables and return a clear HTTP 400/validation response when
conversion or validation fails so invalid types are never persisted.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/views.py (2)
2462-2468:⚠️ Potential issue | 🔴 CriticalNon-numeric relation IDs can still bypass hardening and crash these endpoints.
category/topicare only checked for presence/blank. Non-numeric values can still reach ORM lookups and cause server errors instead of returning a 400.Suggested fix (validate relation IDs before ORM lookup)
# Required fields check (must be present and not empty) required_fields = ["title", "content", "category"] for field in required_fields: val = data.get(field) if val is None or (isinstance(val, str) and not val.strip()): return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400) + try: + category_id = int(data["category"]) + except (TypeError, ValueError): + return JsonResponse({"error": "Invalid category ID"}, status=400) - category = get_object_or_404(ForumCategory, id=data["category"]) + category = get_object_or_404(ForumCategory, id=category_id)# Required fields check (must be present and not empty) required_fields = ["topic", "content"] for field in required_fields: val = data.get(field) if val is None or (isinstance(val, str) and not val.strip()): return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400) + try: + topic_id = int(data["topic"]) + except (TypeError, ValueError): + return JsonResponse({"error": "Invalid topic ID"}, status=400) - topic = get_object_or_404(ForumTopic, id=data["topic"]) + topic = get_object_or_404(ForumTopic, id=topic_id)#!/bin/bash set -euo pipefail # Verify the current code path still uses raw payload values for relation IDs. rg -n -C4 'required_fields = \["title", "content", "category"\]|get_object_or_404\(ForumCategory, id=data\["category"\]\)|required_fields = \["topic", "content"\]|get_object_or_404\(ForumTopic, id=data\["topic"\]\)' web/views.py # Verify there is no local coercion/guard for these IDs in the touched blocks. rg -n -C2 'int\(data\["category"\]\)|int\(data\["topic"\]\)|Invalid category ID|Invalid topic ID' web/views.pyAlso applies to: 2496-2502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2462 - 2468, The current required_fields check only ensures presence/blank but allows non-numeric relation IDs to pass; before calling the ORM lookups (e.g. get_object_or_404(ForumCategory, id=data["category"]) and get_object_or_404(ForumTopic, id=data["topic"]")), validate and coerce the incoming relation IDs from data.get("category") / data.get("topic") to integers and return JsonResponse({"error": "Invalid category ID"} , status=400) or similar for topic on failure (non-numeric or out-of-range), so the required_fields loop stays but you explicitly parse/validate the relation IDs and short-circuit with 400 when int conversion fails.
2317-2323:⚠️ Potential issue | 🟡 MinorRequired-field check still allows empty non-string payloads.
Line [2321] rejects
Noneand blank strings, but required fields likelearning_objectivescan still pass as[]/{}.Suggested tightening for required-field emptiness checks
required_fields = ["title", "description", "learning_objectives", "price", "max_students", "subject"] for field in required_fields: val = data.get(field) - if val is None or (isinstance(val, str) and not val.strip()): + if val is None: + return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400) + if isinstance(val, str) and not val.strip(): + return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400) + if isinstance(val, (list, tuple, dict, set)) and len(val) == 0: return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2317 - 2323, The required-field validation currently only rejects None and blank strings; update the loop that iterates over required_fields (the block using required_fields and val = data.get(field)) to also treat empty containers as invalid by checking for empty lists/tuples/sets/dicts and zero-length iterables: after obtaining val, if val is None OR (isinstance(val, str) and not val.strip()) OR (isinstance(val, (list, tuple, set, dict)) and len(val) == 0) (or more generally: truthiness check for iterable containers), return the same JsonResponse error for that field; keep the same error message and status code so required fields like learning_objectives cannot be [] or {}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/views.py`:
- Around line 2337-2338: Normalize and validate the `level` value from `data`
before passing it to create: trim whitespace, treat null/empty/blank strings as
missing, default to "beginner", and ensure it is one of the allowed values
(e.g., "beginner", "intermediate", "advanced")—if invalid, set to the default or
raise a controlled validation error; replace the direct use of data.get("level",
"beginner") with a local `level` variable computed from `data` (trimmed,
lowercased, validated) and pass that `level` into the create call where
`subject=subject, level=...` is used.
---
Duplicate comments:
In `@web/views.py`:
- Around line 2462-2468: The current required_fields check only ensures
presence/blank but allows non-numeric relation IDs to pass; before calling the
ORM lookups (e.g. get_object_or_404(ForumCategory, id=data["category"]) and
get_object_or_404(ForumTopic, id=data["topic"]")), validate and coerce the
incoming relation IDs from data.get("category") / data.get("topic") to integers
and return JsonResponse({"error": "Invalid category ID"} , status=400) or
similar for topic on failure (non-numeric or out-of-range), so the
required_fields loop stays but you explicitly parse/validate the relation IDs
and short-circuit with 400 when int conversion fails.
- Around line 2317-2323: The required-field validation currently only rejects
None and blank strings; update the loop that iterates over required_fields (the
block using required_fields and val = data.get(field)) to also treat empty
containers as invalid by checking for empty lists/tuples/sets/dicts and
zero-length iterables: after obtaining val, if val is None OR (isinstance(val,
str) and not val.strip()) OR (isinstance(val, (list, tuple, set, dict)) and
len(val) == 0) (or more generally: truthiness check for iterable containers),
return the same JsonResponse error for that field; keep the same error message
and status code so required fields like learning_objectives cannot be [] or {}.
github-actions
Bot
dismissed
their stale review
All review conversations have been resolved.
Author
|
@A1L13N can you please review my changes? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/views.py (1)
2317-2322:⚠️ Potential issue | 🟠 MajorEmpty arrays/objects still bypass the required-field guard.
Line 2318 marks
learning_objectivesas required, but this loop only rejectsNoneand blank strings. Payloads like{"learning_objectives": []}still pass the “non-empty” check.Suggested tightening
required_fields = ["title", "description", "learning_objectives", "price", "max_students", "subject"] for field in required_fields: val = data.get(field) - if val is None or (isinstance(val, str) and not val.strip()): + if ( + val is None + or (isinstance(val, str) and not val.strip()) + or (isinstance(val, (list, dict)) and not val) + ): return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 2317 - 2322, The required-fields guard in the loop (the for field in required_fields block in web/views.py that validates "title", "description", "learning_objectives", "price", "max_students", "subject") only rejects None and empty strings, so empty containers like [] or {} still pass; update the check to also treat empty sequences/mappings as missing by rejecting values where isinstance(val, (list, tuple, dict, set)) and len(val) == 0 or more generally where not isinstance(val, str) and hasattr(val, "__len__") and len(val) == 0; apply this tightened validation for "learning_objectives" (and other non-string required fields) and return the same JsonResponse error when the container is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/views.py`:
- Around line 2312-2315: After parsing request.body with json.loads into data,
ensure data is a JSON object (dict) before using data.get(...) by rejecting
non-object payloads (arrays, null, strings, numbers); update the try/except
block around json.loads to, on success, check isinstance(data, dict) and return
JsonResponse({"error": "JSON must be an object"}, status=400) if not, so
subsequent uses of data.get(...) are safe (look for the json.loads(...) call and
the variable data and the places that call data.get(...) to apply this check).
- Around line 2468-2480: The handler currently assumes json.loads(request.body)
yields a dict and that get_object_or_404(ForumCategory, id=data["category"])
will always succeed; first validate that the parsed data is a dict (return
JsonResponse with a clear error if it's a list/null/primitive) before accessing
data.get(...) to avoid attribute errors, and when resolving relations
(ForumCategory) replace or wrap get_object_or_404 with an explicit lookup that
catches ValueError (bad id types) and ForumCategory.DoesNotExist (or return
JsonResponse on failure) so the endpoint always returns JSON validation errors
instead of HTML 404s or uncaught exceptions (update the code paths around
json.loads, the data variable usage, and the ForumCategory lookup).
---
Duplicate comments:
In `@web/views.py`:
- Around line 2317-2322: The required-fields guard in the loop (the for field in
required_fields block in web/views.py that validates "title", "description",
"learning_objectives", "price", "max_students", "subject") only rejects None and
empty strings, so empty containers like [] or {} still pass; update the check to
also treat empty sequences/mappings as missing by rejecting values where
isinstance(val, (list, tuple, dict, set)) and len(val) == 0 or more generally
where not isinstance(val, str) and hasattr(val, "__len__") and len(val) == 0;
apply this tightened validation for "learning_objectives" (and other non-string
required fields) and return the same JsonResponse error when the container is
empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6e1d9ea-c26f-4063-9a4a-50b0e2c82f63
📒 Files selected for processing (1)
web/views.py
Comment on lines
+2312
to
+2315
| try: | ||
| data = json.loads(request.body) | ||
| except json.JSONDecodeError: | ||
| return JsonResponse({"error": "Invalid JSON"}, status=400) |
Contributor
There was a problem hiding this comment.
Reject non-object JSON bodies before field validation.
json.loads() also accepts valid payloads like [], null, or "text". Those skip the JSONDecodeError branch, and data.get(...) on Line 2320 then raises AttributeError, so this endpoint still 500s on malformed-but-valid JSON.
Suggested hardening
try:
data = json.loads(request.body)
except json.JSONDecodeError:
return JsonResponse({"error": "Invalid JSON"}, status=400)
+ if not isinstance(data, dict):
+ return JsonResponse({"error": "JSON body must be an object"}, status=400)Also applies to: 2317-2322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/views.py` around lines 2312 - 2315, After parsing request.body with
json.loads into data, ensure data is a JSON object (dict) before using
data.get(...) by rejecting non-object payloads (arrays, null, strings, numbers);
update the try/except block around json.loads to, on success, check
isinstance(data, dict) and return JsonResponse({"error": "JSON must be an
object"}, status=400) if not, so subsequent uses of data.get(...) are safe (look
for the json.loads(...) call and the variable data and the places that call
data.get(...) to apply this check).
Comment on lines
+2468
to
2480
| try: | ||
| data = json.loads(request.body) | ||
| except json.JSONDecodeError: | ||
| return JsonResponse({"error": "Invalid JSON"}, status=400) | ||
|
|
||
| # Required fields check (must be present and not empty) | ||
| required_fields = ["title", "content", "category"] | ||
| for field in required_fields: | ||
| val = data.get(field) | ||
| if val is None or (isinstance(val, str) and not val.strip()): | ||
| return JsonResponse({"error": f"Missing or empty required field: {field}"}, status=400) | ||
|
|
||
| category = get_object_or_404(ForumCategory, id=data["category"]) |
Contributor
There was a problem hiding this comment.
Return JSON validation errors for malformed forum payloads and bad relation IDs.
These two endpoints still have two malformed-input paths that bypass the new hardening: json.loads("[]")/null makes data.get(...) fail, and the lookups on Lines 2480 and 2514 can still return an HTML 404 page or bubble a conversion error instead of a JSON error response.
Suggested fix pattern for both endpoints
try:
data = json.loads(request.body)
except json.JSONDecodeError:
return JsonResponse({"error": "Invalid JSON"}, status=400)
+ if not isinstance(data, dict):
+ return JsonResponse({"error": "JSON body must be an object"}, status=400)
@@
- category = get_object_or_404(ForumCategory, id=data["category"])
+ try:
+ category_id = int(data["category"])
+ if isinstance(data["category"], bool):
+ raise TypeError
+ category = ForumCategory.objects.get(id=category_id)
+ except (TypeError, ValueError, ForumCategory.DoesNotExist):
+ return JsonResponse({"error": "Invalid category ID"}, status=400) try:
data = json.loads(request.body)
except json.JSONDecodeError:
return JsonResponse({"error": "Invalid JSON"}, status=400)
+ if not isinstance(data, dict):
+ return JsonResponse({"error": "JSON body must be an object"}, status=400)
@@
- topic = get_object_or_404(ForumTopic, id=data["topic"])
+ try:
+ topic_id = int(data["topic"])
+ if isinstance(data["topic"], bool):
+ raise TypeError
+ topic = ForumTopic.objects.get(id=topic_id)
+ except (TypeError, ValueError, ForumTopic.DoesNotExist):
+ return JsonResponse({"error": "Invalid topic ID"}, status=400)Also applies to: 2502-2514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/views.py` around lines 2468 - 2480, The handler currently assumes
json.loads(request.body) yields a dict and that get_object_or_404(ForumCategory,
id=data["category"]) will always succeed; first validate that the parsed data is
a dict (return JsonResponse with a clear error if it's a list/null/primitive)
before accessing data.get(...) to avoid attribute errors, and when resolving
relations (ForumCategory) replace or wrap get_object_or_404 with an explicit
lookup that catches ValueError (bad id types) and ForumCategory.DoesNotExist (or
return JsonResponse on failure) so the endpoint always returns JSON validation
errors instead of HTML 404s or uncaught exceptions (update the code paths around
json.loads, the data variable usage, and the ForumCategory lookup).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues
Addressed self-identified priority items from the security and stability audit.
Description
This PR hardens the JSON API endpoints in
web/views.pyto prevent application crashes caused by malformed requests or uninitialized data types.Changes Made:
try-excepthandling forjson.loadsto return a400 Bad Requestinstead of a server crash (500) if a request contains invalid JSON.KeyErrorcrashes.JsonResponse. String serialization (.name) is now used correctly.@login_requireddecorators to prevent unauthorized access.Technical Code Changes
1. Serialization Fix
JsonResponsecaused aTypeError.course.subjecttocourse.subject.nameto ensure JSON compatibility.2. Input Hardening
KeyErrorandJSONDecodeError(500 Errors).3. Validation Loop
Checklist
Purpose
Key changes
Tests / checklist
Impact