fix(API): reject NUL bytes in query parameters instead of crashing with 500 - #8204
fix(API): reject NUL bytes in query parameters instead of crashing with 500#8204bardock-2393 wants to merge 2 commits into
Conversation
…th 500 Any view that passes a query param straight into a Postgres string query (e.g. environments/identities/views.py's identifier lookup) raised an unhandled ValueError when the value contained a NUL byte, since psycopg rejects NUL characters in string literals. Reject such requests centrally in middleware instead of patching every call site individually.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8dd6727b-1443-45a5-9d6e-8cae670e2b63
📒 Files selected for processing (3)
api/app/settings/common.pyapi/core/middleware/query_params.pyapi/tests/unit/core/middleware/test_unit_core_middleware_query_params.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8204 +/- ##
==========================================
- Coverage 98.71% 98.57% -0.15%
==========================================
Files 1531 1533 +2
Lines 61263 61292 +29
==========================================
- Hits 60475 60417 -58
- Misses 788 875 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Run the middleware after CorsMiddleware so a short-circuited 400 response still gets CORS headers, instead of the request bypassing CorsMiddleware entirely. - Use QueryDict.lists() instead of .values(), which only yields the last value per key and let a NUL byte in an earlier value of a repeated query key slip through undetected.
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #2901
Several views (e.g.
environments/identities/views.py's identifier lookup) pass a raw query parameter into a Postgres string query. When the value contains a NUL (0x00) byte, psycopg raisesValueError: A string literal cannot contain NUL (0x00) characters, which isn't caught anywhere and surfaces as an unhandled 500. The original report found 8 occurrences of this pattern across the codebase.Rather than adding per-field serializer validation at each of the 8 call sites (and any future ones), this adds a small
RejectNulByteQueryParamsMiddlewarethat checksrequest.GETfor NUL bytes centrally and returns a 400 with a clear message before any view or the ORM ever sees the value — fixing all current and future call sites in one place.How did you test this code?
Added unit tests for the middleware (
test_unit_core_middleware_query_params.py), following the existing pattern used forNeverCacheMiddleware. Also manually verified end-to-end against a realAPIClientrequest to the SDK identities endpoint withidentifier=foo\x00bar: before the fix this would raise the reportedValueError; after the fix it returns400 Bad Requestwith the message body instead. Rantests/unit/core/and the identities integration suite (67 tests) — all green, no regressions.make lintandmake typecheckboth clean.