Fix: prevent 500 error when request body is not JSON object#3562
Open
RCcoders wants to merge 2 commits intoohcnetwork:developfrom
Open
Fix: prevent 500 error when request body is not JSON object#3562RCcoders wants to merge 2 commits intoohcnetwork:developfrom
RCcoders wants to merge 2 commits intoohcnetwork:developfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request adds input body type validation to facility and patient API viewsets, introduces facility-context authorization checks for identifier updates, replaces unsafe dictionary indexing with safe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/api/viewsets/patient.py`:
- Line 414: The code in update_identifier uses facility =
self.get_serializer_list_context().get("facility"), which incorrectly enforces
can_list_facility_tag_config and couples identifier updates to tag-list
permissions; instead, fetch the facility directly without calling
get_serializer_list_context to avoid that auth gate. Replace that lookup with a
direct facility resolver (e.g., use an existing helper like self.get_facility(),
derive from self.request/kwargs, or query Facility by the provided facility_id)
inside update_identifier so it no longer depends on
get_serializer_list_context() or can_list_facility_tag_config.
- Around line 416-420: The cross-facility guard currently only runs when
facility is provided, allowing callers to omit facility and bypass the check;
make the facility context mandatory or derive it server-side and enforce the
check unconditionally: ensure the code handling request_config.facility always
has a resolved facility (e.g., derive from the patient record or current
request/user context) and if no facility can be resolved raise PermissionDenied,
then compare request_config.facility.id with the resolved facility.id (same
symbols: request_config.facility, facility, PermissionDenied) so the mismatch
cannot be skipped by omitting facility.
In `@care/emr/resources/facility/spec.py`:
- Line 175: Do not invent a new serialized value "Unknown"; instead stop using
the "Unknown" default and preserve the canonical mapping or null/absence when a
mapping is missing. Replace mapping["facility_type"] =
REVERSE_FACILITY_TYPES.get(obj.facility_type, "Unknown") with either
mapping["facility_type"] = REVERSE_FACILITY_TYPES.get(obj.facility_type) (so it
becomes None/null when missing) or only set mapping["facility_type"] if
obj.facility_type in REVERSE_FACILITY_TYPES; apply the same change for the other
occurrence at the noted line.
- Line 157: The assignment silently maps unknown facility_type to None; change
it to fail fast by checking REVERSE_REVERSE_FACILITY_TYPES for
self.facility_type before assignment: if a mapping exists assign
obj.facility_type, otherwise raise a validation error (e.g., ValueError or the
module's ValidationError) with a clear message referencing the invalid
self.facility_type; use the symbols REVERSE_REVERSE_FACILITY_TYPES,
self.facility_type, and obj.facility_type to locate and update the logic in the
relevant method.
In `@care/emr/tests/test_patient_security_api.py`:
- Around line 19-27: The test fails because the user lacks the facility-scoped
identifier-config write permission and the request payload doesn't include the
facility, so the endpoint rejects the call before the BOLA cross-facility check
runs; update the test to grant the appropriate facility-scoped permission via
create_role_with_permissions (add the identifier-config write permission enum
from your permissions set) and attach it to the user with
attach_role_organization_user for the current facility context, and modify the
test request payload to include the facility field (use the same facility
instance used when attaching the role) so the BOLA guard is actually exercised;
apply the same changes to the related tests referenced in lines 31-53.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 100928ff-d408-4eeb-9c0a-5fc0b33c455b
📒 Files selected for processing (4)
care/emr/api/viewsets/facility.pycare/emr/api/viewsets/patient.pycare/emr/resources/facility/spec.pycare/emr/tests/test_patient_security_api.py
| PatientIdentifierConfig, external_id=request_data.config | ||
| ) | ||
|
|
||
| facility = self.get_serializer_list_context().get("facility") |
Contributor
There was a problem hiding this comment.
Don’t resolve identifier context through the tag helper.
get_serializer_list_context() also enforces can_list_facility_tag_config, so update_identifier now depends on tag-view permission just to look up a facility. That’s an unrelated authorization gate and will block legitimate identifier updates in a fairly confusing way.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/api/viewsets/patient.py` at line 414, The code in update_identifier
uses facility = self.get_serializer_list_context().get("facility"), which
incorrectly enforces can_list_facility_tag_config and couples identifier updates
to tag-list permissions; instead, fetch the facility directly without calling
get_serializer_list_context to avoid that auth gate. Replace that lookup with a
direct facility resolver (e.g., use an existing helper like self.get_facility(),
derive from self.request/kwargs, or query Facility by the provided facility_id)
inside update_identifier so it no longer depends on
get_serializer_list_context() or can_list_facility_tag_config.
Comment on lines
+416
to
+420
| # Ensure identifier config belongs to the patient's facility context | ||
| if request_config.facility and facility and request_config.facility.id != facility.id: | ||
| raise PermissionDenied( | ||
| "Identifier configuration does not belong to the patient's facility" | ||
| ) |
Contributor
There was a problem hiding this comment.
The cross-facility guard is still optional.
This branch only runs when facility is present, so a caller can omit it and skip the mismatch check entirely. If facility-scoped configs must stay inside the current facility context, that context needs to be mandatory here or derived server-side.
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 418-420: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/api/viewsets/patient.py` around lines 416 - 420, The cross-facility
guard currently only runs when facility is provided, allowing callers to omit
facility and bypass the check; make the facility context mandatory or derive it
server-side and enforce the check unconditionally: ensure the code handling
request_config.facility always has a resolved facility (e.g., derive from the
patient record or current request/user context) and if no facility can be
resolved raise PermissionDenied, then compare request_config.facility.id with
the resolved facility.id (same symbols: request_config.facility, facility,
PermissionDenied) so the mismatch cannot be skipped by omitting facility.
| external_id=self.geo_organization, org_type="govt" | ||
| ).first() | ||
| obj.facility_type = REVERSE_REVERSE_FACILITY_TYPES[self.facility_type] | ||
| obj.facility_type = REVERSE_REVERSE_FACILITY_TYPES.get(self.facility_type) |
Contributor
There was a problem hiding this comment.
Don’t silently coerce an invalid facility_type to None.
REVERSE_REVERSE_FACILITY_TYPES.get(self.facility_type) makes unknown input look valid until much later, which is a neat way to trade a clean 400 for harder-to-debug state. Fail fast here instead of storing a missing mapping.
Suggested fix
- obj.facility_type = REVERSE_REVERSE_FACILITY_TYPES.get(self.facility_type)
+ try:
+ obj.facility_type = REVERSE_REVERSE_FACILITY_TYPES[self.facility_type]
+ except KeyError as exc:
+ raise ValueError("Invalid facility_type") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/resources/facility/spec.py` at line 157, The assignment silently
maps unknown facility_type to None; change it to fail fast by checking
REVERSE_REVERSE_FACILITY_TYPES for self.facility_type before assignment: if a
mapping exists assign obj.facility_type, otherwise raise a validation error
(e.g., ValueError or the module's ValidationError) with a clear message
referencing the invalid self.facility_type; use the symbols
REVERSE_REVERSE_FACILITY_TYPES, self.facility_type, and obj.facility_type to
locate and update the logic in the relevant method.
| if obj.created_by: | ||
| mapping["created_by"] = model_from_cache(UserSpec, id=obj.created_by_id) | ||
| mapping["facility_type"] = REVERSE_FACILITY_TYPES[obj.facility_type] | ||
| mapping["facility_type"] = REVERSE_FACILITY_TYPES.get(obj.facility_type, "Unknown") |
Contributor
There was a problem hiding this comment.
Avoid inventing "Unknown" as a new serialized facility type.
These endpoints previously returned one of the canonical labels from REVERSE_FACILITY_TYPES. Falling back to "Unknown" hides bad data and can break consumers that compare against the defined facility-type values.
Also applies to: 292-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/resources/facility/spec.py` at line 175, Do not invent a new
serialized value "Unknown"; instead stop using the "Unknown" default and
preserve the canonical mapping or null/absence when a mapping is missing.
Replace mapping["facility_type"] = REVERSE_FACILITY_TYPES.get(obj.facility_type,
"Unknown") with either mapping["facility_type"] =
REVERSE_FACILITY_TYPES.get(obj.facility_type) (so it becomes None/null when
missing) or only set mapping["facility_type"] if obj.facility_type in
REVERSE_FACILITY_TYPES; apply the same change for the other occurrence at the
noted line.
Author
|
Thanks for the review! I’ve addressed the CodeRabbit comments in the latest commit. Changes made:
All tests pass locally after the changes. Please let me know if any further adjustments are needed. |
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
2 participants
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.
Summary
Fixes a Broken Object Level Authorization (BOLA) vulnerability in
PatientViewSet.update_identifier.Previously, a user could update a patient identifier using a
PatientIdentifierConfigbelonging to another facility if they knewthe configuration UUID.
Fix
Added facility-level validation before updating identifiers:
can_write_facility_patient_identifier_config.Tests
Added a security test (
test_patient_security_api.py) that verifies:All existing tests pass locally.
Summary by CodeRabbit
Bug Fixes
Tests