fix(security): enforce schema + role gate on PATCH /api/profiles/{user_id} (closes #2894)#2901
Conversation
…h-1918#2894) The PATCH /api/profiles/{user_id} endpoint accepted a raw dict and had no authorization check, allowing any authenticated user to escalate to admin by sending {"role": "admin"}, take over accounts via {"email": ...}, or escape tenants via {"company_id": ...}. Changes: - New ProfileUpdate Pydantic model with extra='forbid' and a 6-field allowlist (full_name, avatar_url, bio, phone, timezone, locale). All other fields — including role, status, company_id, email — are now rejected with 422. - admin.py enforces that callers can only PATCH their own profile, or hold role in (admin, master_admin). Other roles get 403. - Handler uses model_dump(exclude_unset=True) so partial PATCHes don't accidentally null out fields the client didn't send. - New regression test suite (12 cases) pins schema rejection, role gating, allowlisted-field round-trip, and the no-op empty body path.
|
Someone is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Hello! The Vercel deployment check is currently waiting for authorization from a team member. Could someone click through the authorization link so the build can run and deploy the preview? The changes are fully tested and linted, and ready for review. Thanks! |
1 participant
Closes #2894
Summary
Fixes a HIGH mass-assignment privilege-escalation vulnerability in
PATCH /api/profiles/{user_id}(CVSS 8.0). The endpoint accepted a rawdictbody and had no authorization check, so any authenticated user could:role: "admin"orrole: "master_admin"on their own profile (privilege escalation)emailof any user (account takeover)statusto bypass email verification or suspend other userscompany_id(tenant escape)Changes
backend/schemas.pyProfileUpdatePydantic model withextra="forbid"and a 6-field allowlist:full_name,avatar_url,bio,phone,timezone,localeOptionalso PATCH can update a subsetfield_validatoronavatar_urlenforces http(s) scheme (rejectsjavascript:,data:, etc.)backend/routers/admin.pyProfileUpdateinstead ofdict("admin", "master_admin"), else 403updates.model_dump(exclude_unset=True)so partial PATCHes don't null out fields the client didn't send{}without touching the DB)backend/tests/test_admin_mass_assignment_2894.py(new)12 regression tests covering:
role,status,company_id,email(all → 422, no DB call)avatar_urlrejects non-http(s) schemesexclude_unsetensures only sent fields hit the DBWhy this is the right fix
updates: dict→ Pydantic couldn't enforce shapeProfileUpdateis a deliberate, reviewable change — not a silent privilege expansioncurrent_user.get("id") or current_user.get("user_id")to be defensive against either user-dict shape (auth_cookie returns one or the other depending on the user object)Testing
ProfileUpdate(role="admin")→ ValidationError ✓ProfileUpdate(avatar_url="javascript:alert(1)")→ ValidationError ✓ProfileUpdate(full_name="X").model_dump(exclude_unset=True)→{"full_name": "X"}✓ProfileUpdate().model_dump(exclude_unset=True)→{}✓test_admin_mass_assignment_2894.py(12 cases) runs in CIRisk
Low. The new
ProfileUpdateis a strict superset of the intended use — any well-formed PATCH the project was already doing will keep working. The auth check is the only behavior change for the happy path: a user trying to PATCH another user's profile will now correctly get 403.