fix: reject scalar JSON bodies on v1 write endpoints - #9912
Conversation
- Views call request.data.get()/.pop(), so a scalar JSON body such as "hello", 42 or true raised AttributeError and returned HTTP 500. - Add an initial() hook to BaseAPIView and BaseViewSet that raises ParseError for POST/PUT/PATCH when the parsed body is not a dict or list, so every subclass gets a 400 instead of a crash. - Add contract tests covering scalar bodies (400) and a normal object body (still 201).
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughBase API views now validate parsed request bodies for POST, PUT, and PATCH. Scalar JSON bodies produce HTTP 400 responses. Unit and contract tests also cover object and array bodies. ChangesWrite Request Body Validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Scalar JSON bodies on POST, PUT, and PATCH are rejected, while supported form and multipart bodies pass the shared checks. No actionable merge-blocking risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Scalar JSON bodies are rejected earlier on affected write endpoints. The reviewed paths show no new access to protected operations, but the checks do not establish behavior for every endpoint using these base views. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The error text excludes valid arrays, and coverage omits the duplicated viewset branch and supported method/body combinations.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Rejects scalar JSON payloads on v1 write endpoints to prevent AttributeError responses.
Changes:
- Adds scalar-body validation to both v1 base view classes.
- Adds contract tests for scalar rejection and object acceptance.
| File | Description |
|---|---|
apps/api/plane/api/views/base.py |
Validates POST, PUT, and PATCH body shapes. |
apps/api/plane/tests/contract/api/test_non_object_json_body.py |
Tests scalar rejection and object acceptance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| super().initial(request, *args, **kwargs) | ||
| # Views call request.data.get()/.pop(); a scalar JSON body would 500. | ||
| if request.method in ("POST", "PUT", "PATCH") and not isinstance(request.data, (dict, list)): | ||
| raise ParseError("Request body must be a JSON object.") |
| def initial(self, request, *args, **kwargs): | ||
| super().initial(request, *args, **kwargs) | ||
| # Views call request.data.get()/.pop(); a scalar JSON body would 500. | ||
| if request.method in ("POST", "PUT", "PATCH") and not isinstance(request.data, (dict, list)): | ||
| raise ParseError("Request body must be a JSON object.") |
Copilot flagged the ParseError text, which told clients only an object was valid while the guard also accepts arrays. Cover POST, PUT, and PATCH on both BaseAPIView and BaseViewSet, including an array that is allowed through.
Description
v1 write endpoints returned HTTP 500 when the JSON body was a scalar (
"hello",42,true). The parser accepts those values, then views callrequest.data.get()or.pop()and raiseAttributeError.BaseAPIViewandBaseViewSetnow reject that body ininitial()forPOST,PUT, andPATCH. If the parsed body is not a dict or a list, the request raisesParseErrorand returns 400. Object and array bodies are unchanged, andGETdoes not read the body.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/comments/with a JSON body of"hello",42, ortrueand confirm the response is 400.{"comment_html": "<p>hi</p>"}and confirm the response is 201.docker compose -f docker-compose-test.yml run --rm api-tests pytest plane/tests/contract/api/test_non_object_json_body.pyReferences
🤖 Generated with Claude Code
Summary by CodeRabbit