fix: API endpoint hardening and JSON serialization fixes#1004
Open
ankushchk wants to merge 3 commits intoalphaonelabs:mainfrom
Open
fix: API endpoint hardening and JSON serialization fixes#1004ankushchk wants to merge 3 commits intoalphaonelabs:mainfrom
ankushchk wants to merge 3 commits intoalphaonelabs:mainfrom
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
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAPI responses now use 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.
web/views.py
Outdated
Comment on lines
+2317
to
+2321
| # Required fields check | ||
| required_fields = ["title", "description", "learning_objectives", "price", "max_students", "subject"] | ||
| for field in required_fields: | ||
| if field not in data: | ||
| return JsonResponse({"error": f"Missing required field: {field}"}, status=400) |
Contributor
There was a problem hiding this comment.
Required-field check still accepts null/blank values.
This guard only checks key presence. Payloads with blank strings or null can still pass and fail later during object creation.
Suggested tightening for required field validation
required_fields = ["title", "description", "learning_objectives", "price", "max_students", "subject"]
for field in required_fields:
if field not in data:
return JsonResponse({"error": f"Missing required field: {field}"}, status=400)
+ if data[field] is None:
+ return JsonResponse({"error": f"Field cannot be null: {field}"}, status=400)
+
+ for field in ["title", "description", "learning_objectives"]:
+ if not str(data[field]).strip():
+ return JsonResponse({"error": f"Field cannot be blank: {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 - 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.
![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.
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
Summary by CodeRabbit