Conversation
…and allowing unauthorized modifications
Security vulnerabilities fixed:
1. OscarJobService (/jobs/*) - All 12 endpoints lacked authorization checks.
Any authenticated user could view, create, modify, enable/disable, and
cancel system jobs, including saving job types with arbitrary class names.
Added _admin read/write privilege checks to all endpoints.
2. ProviderService (/providerService/settings/{providerNo}/save) - IDOR
vulnerability allowed any authenticated user to modify any other
provider's settings by providing their providerNo in the URL path.
Fixed by enforcing that users can only modify their own settings
unless they have _admin write privileges.
3. ProviderService (/providerService/settings/get) - eRxPassword
(e-prescribing credential) was returned in plaintext in the API
response. Now redacted to "********" in responses, with logic to
preserve existing passwords when the redacted placeholder is sent back.
4. DemographicService - 7 data endpoints lacked _demographic privilege
checks (getAllDemographics, getDemographicData, getBasicDemographicData,
getDemographicSummary, createDemographicData, updateDemographicData,
deleteDemographicData). The search endpoints already had checks but
direct ID-based access did not. Added appropriate read/write privilege
checks with demographic-specific scoping.
5. ProviderService (/providerService/suggestProviderNo) - Admin-only
functionality (suggesting next available provider number) was accessible
to any authenticated user. Added _admin read privilege check.
Generated with Claude Code
…er endpoints
SOAP web service security hardening:
1. ScheduleWs - Added _appointment write privilege checks to
addAppointment() and updateAppointment(). Previously any
authenticated SOAP user could create or modify appointments
for any patient without authorization.
2. MeasurementWs - Added _measurement write privilege check to
addMeasurement(). Previously any authenticated SOAP user could
create clinical measurements (vital signs) for any patient.
3. DemographicWs - Added _demographic read privilege checks to
getDemographic(), getDemographic2(), and searchDemographicByName().
Previously any authenticated SOAP user could access any patient's
full demographic record including HIN, address, and contacts.
4. PrescriptionWs - Added _rx read privilege check to
getPrescription(). Previously any authenticated SOAP user could
read any patient's prescription/medication data.
5. SystemInfoWs - Removed helloWorld(), getMaxListReturnSize(),
getServerTime(), and getServerTimeGmtOffset() methods from this
unauthenticated endpoint. These exposed system internals (server
timestamps, timezone, DAO configuration) that aid reconnaissance.
Retained only isAlive() as a basic health check.
REST API hardening:
6. ProviderService /provider/me - Changed from serializing raw
Provider entity (which includes all JPA fields) to using
ProviderTransfer DTO, which provides a controlled field set.
7. ProviderService /providerjson/{id} - Same fix as above, using
ProviderTransfer instead of raw Provider entity serialization.
Generated with Claude Code
Reviewer's GuideThe PR tightens security across SOAP and REST APIs by stripping sensitive data from public endpoints, enforcing privilege checks via SecurityInfoManager on admin, provider, demographic, appointment, measurement, and prescription operations, and improving data exposure patterns (e.g., transfer objects, redacted credentials, IDOR protection). Sequence diagram for secure provider settings update via REST APIsequenceDiagram
actor Provider
participant ProviderService
participant SecurityInfoManager
participant ProviderManager
Provider->>ProviderService: PUT /providers/{providerNo}/settings ProviderSettings json
ProviderService->>ProviderService: getLoggedInInfo()
ProviderService->>ProviderService: extract currentProviderNo
alt providerNo != currentProviderNo
ProviderService->>SecurityInfoManager: hasPrivilege(info,_admin,w,null)
alt missing_admin_privilege
ProviderService-->>Provider: SecurityException
else has_admin_privilege
opt eRxPassword_placeholder
ProviderService->>ProviderManager: getProviderSettings(info,providerNo)
ProviderManager-->>ProviderService: existingSettings
ProviderService->>ProviderService: copy existing eRxPassword to json
end
ProviderService->>ProviderManager: updateProviderSettings(info,providerNo,json)
ProviderManager-->>ProviderService: updated
ProviderService-->>Provider: 200 OK GenericRESTResponse
end
else providerNo == currentProviderNo
opt eRxPassword_placeholder
ProviderService->>ProviderManager: getProviderSettings(info,providerNo)
ProviderManager-->>ProviderService: existingSettings
ProviderService->>ProviderService: copy existing eRxPassword to json
end
ProviderService->>ProviderManager: updateProviderSettings(info,providerNo,json)
ProviderManager-->>ProviderService: updated
ProviderService-->>Provider: 200 OK GenericRESTResponse
end
Class diagram for updated API service security integrationclassDiagram
class AbstractWs
class AbstractServiceImpl
class SecurityInfoManager {
+boolean hasPrivilege(LoggedInInfo info, String objectName, String permission, String resourceId)
}
class SystemInfoWs {
+String isAlive()
}
class OscarJobService {
-SecurityInfoManager securityInfoManager
+OscarJobTypeResponse getCurrentlyAvailableJobTypes()
+OscarJobTypeResponse getAllJobTypes()
+OscarJobResponse getAllJobs()
+OscarJobResponse getJob(Integer jobId)
+OscarJobResponse saveJob(MultivaluedMap params)
+OscarJobResponse cancelJob(Integer jobId)
+OscarJobResponse saveCrontabExpression(MultivaluedMap params)
+OscarJobTypeResponse getJobType(Integer jobTypeId)
+OscarJobTypeResponse saveJobType(MultivaluedMap params)
+OscarJobResponse enableJob(Integer jobId)
+OscarJobResponse disableJob(Integer jobId)
-void requireAdminRead()
-void requireAdminWrite()
}
class ProviderService {
-SecurityInfoManager securityInfoManager
+Response getLoggedInProvider()
+String getProviderAsJSON(String id)
+AbstractSearchResponse getProviderSettings()
+GenericRESTResponse saveProviderSettings(ProviderSettings json, String providerNo)
+GenericRESTResponse suggestProviderNo()
}
class DemographicService {
-SecurityInfoManager securityInfoManager
+OscarSearchResponse getAllDemographics(Integer offset, Integer limit)
+DemographicTo1 getDemographicData(Integer id, List includes)
+DemographicTo1 getBasicDemographicData(Integer id, List includes)
+DemographicTo1 getDemographicSummary(Integer demographicNo)
+DemographicTo1 createDemographicData(DemographicTo1 data)
+DemographicTo1 updateDemographicData(DemographicTo1 data)
+DemographicTo1 deleteDemographicData(Integer id)
}
class DemographicWs {
-SecurityInfoManager securityInfoManager
+DemographicTransfer getDemographic(Integer demographicId)
+DemographicTransfer2 getDemographic2(Integer demographicId)
+DemographicTransfer[] searchDemographicByName(String searchString, int startIndex, int itemsToReturn)
}
class ScheduleWs {
-SecurityInfoManager securityInfoManager
+ScheduleTemplateCodeTransfer[] getScheduleTemplateCodes()
+AppointmentTypeTransfer[] getAppointmentTypes()
+Integer addAppointment(AppointmentTransfer appointmentTransfer)
+void updateAppointment(AppointmentTransfer appointmentTransfer)
}
class MeasurementWs {
-SecurityInfoManager securityInfoManager
+MeasurementTransfer getMeasurement(Integer measurementId)
+MeasurementTypeTransfer[] getMeasurementTypes()
+MeasurementMapTransfer[] getMeasurementMaps()
+Integer addMeasurement(MeasurementTransfer measurementTransfer)
}
class PrescriptionWs {
-SecurityInfoManager securityInfoManager
+PrescriptionTransfer getPrescription(Integer prescriptionId)
}
class ProviderTransfer {
+static ProviderTransfer toTransfer(Provider provider)
}
class ProviderSettings {
+String geteRxPassword()
+void seteRxPassword(String eRxPassword)
}
class ProviderManager {
+ProviderSettings getProviderSettings(LoggedInInfo info, String providerNo)
+void updateProviderSettings(LoggedInInfo info, String providerNo, ProviderSettings settings)
}
SystemInfoWs --|> AbstractWs
OscarJobService --|> AbstractServiceImpl
ProviderService --|> AbstractServiceImpl
DemographicService --|> AbstractServiceImpl
DemographicWs --|> AbstractWs
ScheduleWs --|> AbstractWs
MeasurementWs --|> AbstractWs
PrescriptionWs --|> AbstractWs
OscarJobService --> SecurityInfoManager
ProviderService --> SecurityInfoManager
DemographicService --> SecurityInfoManager
DemographicWs --> SecurityInfoManager
ScheduleWs --> SecurityInfoManager
MeasurementWs --> SecurityInfoManager
PrescriptionWs --> SecurityInfoManager
ProviderService --> ProviderManager
ProviderService --> ProviderTransfer
ProviderService --> ProviderSettings
DemographicService --> DemographicManager
DemographicWs --> DemographicManager
ScheduleWs --> ScheduleManager
MeasurementWs --> MeasurementManager
PrescriptionWs --> PrescriptionManager
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Summary of ChangesHello @yingbull, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the application's API endpoints. It introduces granular privilege checks for both SOAP and REST services, ensuring that users can only access or modify data for which they have explicit permissions. Additionally, it hardens a public-facing system information endpoint by removing potentially sensitive data exposure and improves data handling in provider-related services to prevent information leakage and unauthorized modifications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughSecurity authorization checks are integrated across multiple web service endpoints via SecurityInfoManager dependency injection. Privilege-based access control enforces read/write restrictions on demographic, measurement, prescription, appointment, and other resources. ProviderService adds IDOR protection and password redaction. SystemInfoWs removes four public health-check endpoints, retaining only isAlive(). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
PR Review 🔍
|
PR Code Suggestions ✨
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In several SOAP services the new privilege checks are only applied to write operations (e.g., MeasurementWs.addMeasurement) while corresponding read operations (e.g., MeasurementWs.getMeasurement) remain unrestricted; consider consistently enforcing appropriate read privileges for these entry points as well.
- The new security checks in REST resources (e.g., OscarJobService, ProviderService, DemographicService) throw plain SecurityException; if your JAX-RS stack does not already map this to a consistent HTTP error, it may be clearer and safer to throw a specific WebApplicationException/mapper-backed exception that returns an appropriate status code and message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several SOAP services the new privilege checks are only applied to write operations (e.g., MeasurementWs.addMeasurement) while corresponding read operations (e.g., MeasurementWs.getMeasurement) remain unrestricted; consider consistently enforcing appropriate read privileges for these entry points as well.
- The new security checks in REST resources (e.g., OscarJobService, ProviderService, DemographicService) throw plain SecurityException; if your JAX-RS stack does not already map this to a consistent HTTP error, it may be clearer and safer to throw a specific WebApplicationException/mapper-backed exception that returns an appropriate status code and message.
## Individual Comments
### Comment 1
<location> `src/main/java/io/github/carlos_emr/carlos/webserv/rest/ProviderService.java:403-405` </location>
<code_context>
ProviderSettings settings = providerManager.getProviderSettings(getLoggedInInfo(), getLoggedInInfo().getLoggedInProviderNo());
+
+ // Redact sensitive credentials - never expose passwords via API
+ if (settings.geteRxPassword() != null && !settings.geteRxPassword().isEmpty()) {
+ settings.seteRxPassword("********");
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Redacting by mutating the ProviderSettings entity may have side effects on shared instances
Mutating the `ProviderSettings` returned from `providerManager` to mask the password can leak the masked value into persistence or caches if this object is shared. Instead, derive a separate DTO or copy for the API response and apply redaction there so the domain object remains unchanged.
Suggested implementation:
```java
ProviderSettings settings = providerManager.getProviderSettings(getLoggedInInfo(), getLoggedInInfo().getLoggedInProviderNo());
// Create a detached copy for API exposure so we don't mutate the managed entity
ProviderSettings redactedSettings = new ProviderSettings();
org.springframework.beans.BeanUtils.copyProperties(settings, redactedSettings);
// Redact sensitive credentials - never expose passwords via API
if (redactedSettings.geteRxPassword() != null && !redactedSettings.geteRxPassword().isEmpty()) {
redactedSettings.seteRxPassword("********");
}
```
1. Anywhere in this method (or downstream) where the original `settings` instance is used for building the HTTP response / JSON payload, switch to using `redactedSettings` instead. For example, if you currently do something like `response.setResult(settings)` or serialize `settings` with `objectMapper`, change those usages to `redactedSettings`.
2. If `ProviderSettings` does not have a no-arg constructor or is not suitable as a DTO, you may want to introduce a dedicated DTO class (e.g., `ProviderSettingsDto`) and copy properties into that instead of using `ProviderSettings` directly.
3. If you prefer to avoid the fully-qualified `org.springframework.beans.BeanUtils` reference, add `import org.springframework.beans.BeanUtils;` at the top of this file and then call `BeanUtils.copyProperties(settings, redactedSettings);`.
</issue_to_address>
### Comment 2
<location> `src/main/java/io/github/carlos_emr/carlos/webserv/MeasurementWs.java:53-57` </location>
<code_context>
@Autowired
private PatientConsentManager patientConsentManager;
+ @Autowired
+ private SecurityInfoManager securityInfoManager;
+
public DemographicTransfer getDemographic(Integer demographicId) {
</code_context>
<issue_to_address>
**🚨 question (security):** SecurityInfoManager is injected but only used for writes; reads remain unguarded
You’ve added a `_measurement` write-privilege check for `addMeasurement`, but `getMeasurement` and `getMeasurementMaps` are still callable without any authorization and don’t use the injected `SecurityInfoManager`. If reads are meant to be restricted, add corresponding read checks there; if not, the new injection is unused on read paths and may indicate a missing auth requirement or a design inconsistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Redact sensitive credentials - never expose passwords via API | ||
| if (settings.geteRxPassword() != null && !settings.geteRxPassword().isEmpty()) { | ||
| settings.seteRxPassword("********"); |
There was a problem hiding this comment.
suggestion (bug_risk): Redacting by mutating the ProviderSettings entity may have side effects on shared instances
Mutating the ProviderSettings returned from providerManager to mask the password can leak the masked value into persistence or caches if this object is shared. Instead, derive a separate DTO or copy for the API response and apply redaction there so the domain object remains unchanged.
Suggested implementation:
ProviderSettings settings = providerManager.getProviderSettings(getLoggedInInfo(), getLoggedInInfo().getLoggedInProviderNo());
// Create a detached copy for API exposure so we don't mutate the managed entity
ProviderSettings redactedSettings = new ProviderSettings();
org.springframework.beans.BeanUtils.copyProperties(settings, redactedSettings);
// Redact sensitive credentials - never expose passwords via API
if (redactedSettings.geteRxPassword() != null && !redactedSettings.geteRxPassword().isEmpty()) {
redactedSettings.seteRxPassword("********");
}- Anywhere in this method (or downstream) where the original
settingsinstance is used for building the HTTP response / JSON payload, switch to usingredactedSettingsinstead. For example, if you currently do something likeresponse.setResult(settings)or serializesettingswithobjectMapper, change those usages toredactedSettings. - If
ProviderSettingsdoes not have a no-arg constructor or is not suitable as a DTO, you may want to introduce a dedicated DTO class (e.g.,ProviderSettingsDto) and copy properties into that instead of usingProviderSettingsdirectly. - If you prefer to avoid the fully-qualified
org.springframework.beans.BeanUtilsreference, addimport org.springframework.beans.BeanUtils;at the top of this file and then callBeanUtils.copyProperties(settings, redactedSettings);.
| @Autowired | ||
| private MeasurementManager measurementManager; | ||
|
|
||
| @Autowired | ||
| private SecurityInfoManager securityInfoManager; |
There was a problem hiding this comment.
🚨 question (security): SecurityInfoManager is injected but only used for writes; reads remain unguarded
You’ve added a _measurement write-privilege check for addMeasurement, but getMeasurement and getMeasurementMaps are still callable without any authorization and don’t use the injected SecurityInfoManager. If reads are meant to be restricted, add corresponding read checks there; if not, the new injection is unused on read paths and may indicate a missing auth requirement or a design inconsistency.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security enhancements across both SOAP and REST APIs. It correctly adds privilege checks to various endpoints, protecting sensitive patient and system data. The changes also include redacting sensitive information like passwords from API responses and preventing insecure direct object references. The cleanup of the unauthenticated SystemInfoWs endpoint is a great hardening step. I've identified a couple of areas for improvement: one related to using the correct privilege type for a delete operation, and another for improving consistency in the DTOs used in the REST services. Overall, this is a solid contribution to improving the application's security posture.
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "w", id.toString())) { | ||
| throw new SecurityException("missing required security object (_demographic)"); | ||
| } |
There was a problem hiding this comment.
The privilege check for deleting a demographic record uses the write privilege ("w"). According to the SecurityInfoManager implementation, the delete privilege ("d") is distinct from the write privilege. To ensure proper access control, this check should specifically verify the delete privilege. Using the SecurityInfoManager.DELETE constant is recommended for clarity and maintainability.
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "w", id.toString())) { | |
| throw new SecurityException("missing required security object (_demographic)"); | |
| } | |
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "d", id.toString())) { | |
| throw new SecurityException("missing required security object (_demographic)"); | |
| } |
| // Use ProviderTransfer to avoid exposing raw entity fields | ||
| ProviderTransfer transfer = ProviderTransfer.toTransfer(provider); | ||
| String body = objectMapper.valueToTree(transfer).toString(); |
There was a problem hiding this comment.
This REST endpoint uses ProviderTransfer, which appears to be intended for SOAP services. For consistency with other REST endpoints and to leverage the conversion logic (e.g., for status fields), it would be better to use ProviderTo1 and the autowired providerConverter.
| // Use ProviderTransfer to avoid exposing raw entity fields | |
| ProviderTransfer transfer = ProviderTransfer.toTransfer(provider); | |
| String body = objectMapper.valueToTree(transfer).toString(); | |
| // Use ProviderTo1 to avoid exposing raw entity fields and for consistency | |
| ProviderTo1 transfer = providerConverter.getAsTransferObject(getLoggedInInfo(), provider); | |
| String body = objectMapper.valueToTree(transfer).toString(); |
| // Use ProviderTransfer to avoid exposing raw entity fields | ||
| ProviderTransfer transfer = ProviderTransfer.toTransfer(provider); | ||
| logger.info("Successfully retrieved provider {} as JSON", id); | ||
| return objectMapper.valueToTree(provider).toString(); | ||
| return objectMapper.valueToTree(transfer).toString(); |
There was a problem hiding this comment.
This REST endpoint uses ProviderTransfer, which appears to be intended for SOAP services. For consistency with other REST endpoints and to leverage the conversion logic, it would be better to use ProviderTo1 and the autowired providerConverter.
| // Use ProviderTransfer to avoid exposing raw entity fields | |
| ProviderTransfer transfer = ProviderTransfer.toTransfer(provider); | |
| logger.info("Successfully retrieved provider {} as JSON", id); | |
| return objectMapper.valueToTree(provider).toString(); | |
| return objectMapper.valueToTree(transfer).toString(); | |
| // Use ProviderTo1 to avoid exposing raw entity fields and for consistency | |
| ProviderTo1 transfer = providerConverter.getAsTransferObject(getLoggedInInfo(), provider); | |
| logger.info("Successfully retrieved provider {} as JSON", id); | |
| return objectMapper.valueToTree(transfer).toString(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/io/github/carlos_emr/carlos/webserv/MeasurementWs.java (1)
59-95:⚠️ Potential issue | 🟠 MajorRead endpoints lack authorization checks — PHI exposure risk.
addMeasurementis now gated, but the read methods (getMeasurement,getMeasurementsCreatedAfterDate,getMeasurementsByProgramProviderDemographicDate,getMeasurementsByDemographicIdAfter) have no privilege checks. Measurements are PHI and should require at least"_measurement"read ("r") privilege. This is especially important given HIPAA/PIPEDA compliance requirements.Example fix for getMeasurement
public MeasurementTransfer getMeasurement(Integer measurementId) { + if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_measurement", "r", null)) { + throw new SecurityException("missing required security object (_measurement)"); + } Measurement measurement = measurementManager.getMeasurement(getLoggedInInfo(), measurementId); return (MeasurementTransfer.toTransfer(measurement)); }Based on learnings: "PHI (Patient Health Information) protection is legally mandated under HIPAA/PIPEDA compliance. Security is non-negotiable in all code changes and is the top priority for this healthcare system."
src/main/java/io/github/carlos_emr/carlos/webserv/PrescriptionWs.java (1)
74-90:⚠️ Potential issue | 🟠 MajorRemaining prescription read endpoints lack authorization checks.
getPrescriptionis now gated with_rxread privilege, butgetPrescriptionUpdatedAfterDate,getPrescriptionsByProgramProviderDemographicDate, andgetPrescriptionsByDemographicIdAfterare unprotected. Prescription data is highly sensitive PHI — all read endpoints should enforce the same_rxread check.Example fix for getPrescriptionUpdatedAfterDate
public PrescriptionTransfer[] getPrescriptionUpdatedAfterDate(Date updatedAfterThisDateExclusive, int itemsToReturn) { + if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_rx", "r", null)) { + throw new SecurityException("missing required security object (_rx)"); + } LoggedInInfo loggedInInfo = getLoggedInInfo();Based on learnings: "PHI (Patient Health Information) protection is legally mandated under HIPAA/PIPEDA compliance."
src/main/java/io/github/carlos_emr/carlos/webserv/ScheduleWs.java (1)
80-116:⚠️ Potential issue | 🟠 MajorRead endpoints for appointments lack authorization checks.
Multiple read endpoints (
getAppointment,getAppointment2,getAppointmentsForProvider,getAppointmentsForProvider2,getAppointmentsForPatient,getAppointmentsForPatient2, etc.) do not enforce any privilege check. Appointment data may include patient-identifying information. Consider adding_appointmentread checks for consistency with the write-side enforcement.src/main/java/io/github/carlos_emr/carlos/webserv/DemographicWs.java (1)
102-105:⚠️ Potential issue | 🟠 Major
searchDemographicsByAttributeslacks an authorization check.This method accepts HIN (Health Insurance Number) as a search parameter and returns demographic data — both are PHI. It should have the same
_demographicread privilege check assearchDemographicByName.Proposed fix
public DemographicTransfer[] searchDemographicsByAttributes(String hin, String firstName, String lastName, Gender gender, Calendar dateOfBirth, String city, String province, String phone, String email, String alias, int startIndex, int itemsToReturn) { + if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", null)) { + throw new SecurityException("missing required security object (_demographic)"); + } List<Demographic> demographics = demographicManager.searchDemographicsByAttributes(getLoggedInInfo(), hin, firstName, lastName, gender, dateOfBirth, city, province, phone, email, alias, startIndex, itemsToReturn);Based on learnings: "PHI (Patient Health Information) protection is legally mandated under HIPAA/PIPEDA compliance."
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/webserv/rest/DemographicService.java`:
- Around line 242-244: Guard against id being null before calling id.toString()
in DemographicService: change the hasPrivilege call(s) to pass a null-safe
string (e.g., (id == null ? null : id.toString()) or
Optional.ofNullable(id).map(Object::toString).orElse(null)) instead of
id.toString(); apply this fix in the three affected methods (the method around
the shown diff, getBasicDemographicData, and getDemographicSummary) so the
security check doesn’t throw a NullPointerException when dataId is null.
🧹 Nitpick comments (2)
src/main/java/io/github/carlos_emr/carlos/webserv/MeasurementWs.java (1)
56-57:@Autowiredused instead ofSpringUtils.getBean()pattern.The coding guidelines specify using
SpringUtils.getBean(SecurityInfoManager.class)for dependency injection. However, the existing fieldmeasurementManageron line 53–54 already uses@Autowired, so this is at least consistent within the file. Consider aligning with the guideline in a follow-up.As per coding guidelines: "Use Spring dependency injection via SpringUtils.getBean(ClassName.class) pattern for retrieving managed beans from the Spring container. Do not use
@Autowiredor@Injectannotations in action classes."src/main/java/io/github/carlos_emr/carlos/webserv/rest/ProviderService.java (1)
422-434: Good IDOR protection and password preservation logic.The check at line 424 correctly prevents non-admin users from modifying other providers' settings. The password placeholder handling at lines 429–434 properly preserves the existing password when the client sends back the redacted value.
One minor note: the magic string
"********"is duplicated (lines 405 and 429). Consider extracting it to a constant.Extract redaction constant
+ private static final String REDACTED_PASSWORD = "********"; + // In getProviderSettings: - settings.seteRxPassword("********"); + settings.seteRxPassword(REDACTED_PASSWORD); // In saveProviderSettings: - if ("********".equals(json.geteRxPassword())) { + if (REDACTED_PASSWORD.equals(json.geteRxPassword())) {
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", id.toString())) { | ||
| throw new SecurityException("missing required security object (_demographic)"); | ||
| } |
There was a problem hiding this comment.
NullPointerException if id is null — id.toString() called without null guard.
If a client sends a request with a null dataId, id.toString() on line 242 will throw an NPE before the security check can reject the request. The same issue exists in getBasicDemographicData (line 440) and getDemographicSummary (line 547). DemographicWs.getDemographic (line 73) handles this correctly with a ternary null check.
Proposed fix
- if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", id.toString())) {
+ if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", (id != null) ? id.toString() : null)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", id.toString())) { | |
| throw new SecurityException("missing required security object (_demographic)"); | |
| } | |
| if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "r", (id != null) ? id.toString() : null)) { | |
| throw new SecurityException("missing required security object (_demographic)"); | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/io/github/carlos_emr/carlos/webserv/rest/DemographicService.java`
around lines 242 - 244, Guard against id being null before calling id.toString()
in DemographicService: change the hasPrivilege call(s) to pass a null-safe
string (e.g., (id == null ? null : id.toString()) or
Optional.ofNullable(id).map(Object::toString).orElse(null)) instead of
id.toString(); apply this fix in the three affected methods (the method around
the shown diff, getBasicDemographicData, and getDemographicSummary) so the
security check doesn’t throw a NullPointerException when dataId is null.
There was a problem hiding this comment.
Pull request overview
This pull request implements comprehensive security hardening across REST and SOAP API endpoints in the CARLOS EMR system. The changes add authorization checks, prevent information disclosure, implement IDOR protection, and establish secure credential handling patterns to protect Protected Health Information (PHI) and system internals.
Changes:
- Added SecurityInfoManager privilege checks to 15+ API endpoints across multiple services
- Implemented password redaction and preservation pattern for provider settings
- Added IDOR prevention for cross-user resource access requiring admin privileges
- Removed information disclosure from unauthenticated SystemInfoWs endpoint
- Introduced ProviderTransfer objects to control exposed fields in provider APIs
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ProviderService.java | Added security checks to suggestProviderNo, implemented password redaction in getProviderSettings, added IDOR protection in saveProviderSettings, and introduced ProviderTransfer usage to avoid exposing raw entity fields |
| OscarJobService.java | Added comprehensive admin privilege checks (read/write) to all job management endpoints with well-documented helper methods |
| DemographicService.java | Added _demographic privilege checks to all CRUD operations (getAllDemographics, getDemographicData, getBasicDemographicData, getDemographicSummary, createDemographicData, updateDemographicData, deleteDemographicData) |
| SystemInfoWs.java | Removed information disclosure methods (helloWorld, getServerTime, getServerTimeGmtOffset, getMaxListReturnSize) from unauthenticated endpoint, leaving only isAlive health check |
| ScheduleWs.java | Added _appointment write privilege checks to addAppointment and updateAppointment methods |
| PrescriptionWs.java | Added _rx read privilege check to getPrescription method |
| MeasurementWs.java | Added _measurement write privilege check to addMeasurement method |
| DemographicWs.java | Added _demographic read privilege checks to getDemographic, getDemographic2, and searchDemographicByName methods |
Comments suppressed due to low confidence (2)
src/main/java/io/github/carlos_emr/carlos/webserv/rest/ProviderService.java:413
- Inconsistent IDOR protection between GET and POST endpoints. The getProviderSettings() method only allows users to retrieve their own settings (hardcoded to getLoggedInProviderNo()), but saveProviderSettings() accepts a providerNo path parameter and allows admins to modify other providers' settings. For consistency and flexibility, consider either: (1) allowing getProviderSettings() to also accept an optional providerNo parameter with similar admin privilege checks, or (2) documenting that the current design intentionally restricts GET to self-access only while allowing POST for admin delegation. The asymmetry could confuse API consumers.
@GET
@Path("/settings/get")
@Produces("application/json")
public AbstractSearchResponse<ProviderSettings> getProviderSettings() {
AbstractSearchResponse<ProviderSettings> response = new AbstractSearchResponse<ProviderSettings>();
ProviderSettings settings = providerManager.getProviderSettings(getLoggedInInfo(), getLoggedInInfo().getLoggedInProviderNo());
// Redact sensitive credentials - never expose passwords via API
if (settings.geteRxPassword() != null && !settings.geteRxPassword().isEmpty()) {
settings.seteRxPassword("********");
}
List<ProviderSettings> content = new ArrayList<ProviderSettings>();
content.add(settings);
response.setContent(content);
response.setTotal(1);
return response;
}
src/main/java/io/github/carlos_emr/carlos/webserv/ScheduleWs.java:106
- Missing security checks on read operations. While write operations (addAppointment, updateAppointment) now correctly check _appointment privileges, the read operations (getAppointment, getAppointmentsForProvider, getAppointmentsForPatient, getScheduleTemplateCodes, etc.) lack any privilege checks. Appointment data contains PHI and should require "_appointment" read privileges. Consider adding securityInfoManager.hasPrivilege(getLoggedInInfo(), "_appointment", "r", null) checks to all read methods for consistency with the security pattern established in DemographicWs and other services.
public ScheduleTemplateCodeTransfer[] getScheduleTemplateCodes() {
List<ScheduleTemplateCode> scheduleTemplateCodes = scheduleManager.getScheduleTemplateCodes();
return (ScheduleTemplateCodeTransfer.toTransfer(scheduleTemplateCodes));
}
/**
* @deprecated you should use the method with the useGMTTime option
*/
@Deprecated
public AppointmentTransfer getAppointment(Integer appointmentId) {
Appointment appointment = scheduleManager.getAppointment(getLoggedInInfo(), appointmentId);
return (AppointmentTransfer.toTransfer(appointment, false));
}
/**
* @deprecated you should use the method with the useGMTTime option
*/
@Deprecated
public AppointmentTransfer[] getAppointmentsForProvider(String providerNo, Calendar date) {
List<Appointment> appointments = scheduleManager.getDayAppointments(getLoggedInInfo(), providerNo, date);
return (AppointmentTransfer.toTransfers(appointments, false));
}
/**
* @deprecated you should use the method with the useGMTTime option
*/
@Deprecated
public AppointmentTransfer[] getAppointmentsForPatient(Integer demographicId, int startIndex, int itemsToReturn) {
List<Appointment> appointments = scheduleManager.getAppointmentsForPatient(getLoggedInInfo(), demographicId, startIndex, itemsToReturn);
return (AppointmentTransfer.toTransfers(appointments, false));
}
public AppointmentTransfer getAppointment2(Integer appointmentId, boolean useGMTTime) {
Appointment appointment = scheduleManager.getAppointment(getLoggedInInfo(), appointmentId);
return (AppointmentTransfer.toTransfer(appointment, useGMTTime));
}
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. Review Summary: This PR successfully adds comprehensive authorization checks across 8 files in the web service layer. The implementation follows established security patterns and CLAUDE.md requirements: ✅ Security Checks: All endpoints now include proper ✅ IDOR Prevention: Object-level authorization properly implemented in ✅ PHI Protection: eRx passwords redacted in API responses, use of ✅ Attack Surface Reduction: Removed information disclosure methods from unauthenticated ✅ Documentation: JavaDoc is comprehensive and meets all CLAUDE.md standards The authorization checks use correct access modes ("r" for read, "w" for write) and pass record IDs where applicable for per-record authorization. 🤖 Generated with Claude Code |
|
@claude review |
|
Claude finished @yingbull's task in 2m 6s —— View job Code Review CompleteTasks:
Review SummaryThis PR successfully adds comprehensive authorization checks across 8 files in the web service layer. The implementation follows established security patterns and CLAUDE.md requirements with one critical issue and several important gaps to address. ✅ StrengthsSecurity Implementation:
Code Quality:
🚨 Critical Issue1. Delete operation uses wrong privilege type ( // INCORRECT - uses "w" (write) for delete operation
if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "w", id.toString())) {Issue: According to CARLOS security architecture, delete operations require the "d" privilege, not "w". Using "w" allows users with write-only permissions to delete records, which violates the principle of least privilege. Fix: Change to use "d" privilege: if (!securityInfoManager.hasPrivilege(getLoggedInInfo(), "_demographic", "d", id.toString())) {
|
Description
Changes walkthrough 📝
DemographicWs.java
Enhance Security in Demographic Web Servicesrc/main/java/io/github/carlos_emr/carlos/webserv/DemographicWs.java
MeasurementWs.java
Secure Measurement Web Service Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/MeasurementWs.java
PrescriptionWs.java
Secure Prescription Web Service Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/PrescriptionWs.java
ScheduleWs.java
Secure Appointment Management Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/ScheduleWs.java
SystemInfoWs.java
Harden System Info Web Servicesrc/main/java/io/github/carlos_emr/carlos/webserv/SystemInfoWs.java
DemographicService.java
Secure Demographic REST Service Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/rest/DemographicService.java
OscarJobService.java
Secure Oscar Job Management Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/rest/OscarJobService.java
ProviderService.java
Secure Provider Service Endpointssrc/main/java/io/github/carlos_emr/carlos/webserv/rest/ProviderService.java
Summary by CodeRabbit
New Features
Bug Fixes
Chores