fix: XSS vulnerability in autocomplete endpoints#280
fix: XSS vulnerability in autocomplete endpoints#280mohamadSaleh82 wants to merge 5 commits intocarlos-emr:developfrom
Conversation
- Replace StringEscapeUtils.escapeJava() with OWASP Encode.forHtml() - Add HTML encoding for patient names, status, and provider names - Refactor formatJSON() to use Jackson ObjectMapper instead of manual string building - Add null checks for provider name fields - Fix typo: fomattedDob -> formattedDob Fixes carlos-emr#253
Reviewer's GuideFixes XSS vulnerabilities in demographic and provider autocomplete endpoints by switching to OWASP HTML encoding for user-controlled fields and refactoring manual JSON string construction to use Jackson ObjectMapper, with minor robustness and naming fixes. Sequence diagram for secured demographic autocomplete endpointsequenceDiagram
actor User
participant Browser
participant WebApp
participant SearchDemographicAutoComplete2Action
participant DemographicDao
participant RxProviderData
participant DemographicCustDao
participant ObjectMapper
User->>Browser: Type in demographic search box
Browser->>WebApp: HTTP GET /searchDemographicAutoComplete2?term=...
WebApp->>SearchDemographicAutoComplete2Action: Dispatch request
SearchDemographicAutoComplete2Action->>DemographicDao: searchDemographicByDOB or searchDemographicByName
DemographicDao-->>SearchDemographicAutoComplete2Action: List<Demographic>
loop For each Demographic
SearchDemographicAutoComplete2Action->>RxProviderData: getProvider(providerNo)
RxProviderData-->>SearchDemographicAutoComplete2Action: Provider
alt workflow_enhance enabled
SearchDemographicAutoComplete2Action->>DemographicCustDao: find(demographicNo)
DemographicCustDao-->>SearchDemographicAutoComplete2Action: DemographicCust
end
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: Encode.forHtml(formattedDob)
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: Encode.forHtml(formattedName)
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: Encode.forHtml(status)
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: Encode.forHtml(providerName)
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: Encode.forHtml(cust1Name,cust2Name,cust4Name)
end
SearchDemographicAutoComplete2Action->>ObjectMapper: writeValueAsString(resultsList)
ObjectMapper-->>SearchDemographicAutoComplete2Action: JSON array string
SearchDemographicAutoComplete2Action-->>WebApp: HTTP response body (JSON)
WebApp-->>Browser: Return JSON autocomplete results
Browser-->>User: Render safe autocomplete suggestions
Sequence diagram for secured provider autocomplete endpointsequenceDiagram
actor User
participant Browser
participant WebApp
participant SearchProviderAutoComplete2Action
participant ProviderDataDao
participant ObjectMapper
User->>Browser: Type in provider search box
Browser->>WebApp: HTTP GET /searchProviderAutoComplete2?term=...
WebApp->>SearchProviderAutoComplete2Action: Dispatch request
SearchProviderAutoComplete2Action->>SearchProviderAutoComplete2Action: Parse term into firstName,lastName
SearchProviderAutoComplete2Action->>ProviderDataDao: findByName(firstName,lastName,true)
ProviderDataDao-->>SearchProviderAutoComplete2Action: List<ProviderData>
loop For each ProviderData
SearchProviderAutoComplete2Action->>SearchProviderAutoComplete2Action: Build label string
SearchProviderAutoComplete2Action->>SearchProviderAutoComplete2Action: Encode.forHtml(label)
end
SearchProviderAutoComplete2Action->>ObjectMapper: writeValueAsString(resultsList)
ObjectMapper-->>SearchProviderAutoComplete2Action: JSON array string
SearchProviderAutoComplete2Action-->>WebApp: HTTP response body (JSON)
WebApp-->>Browser: Return JSON autocomplete results
Browser-->>User: Render safe provider autocomplete suggestions
Updated class diagram for autocomplete action classesclassDiagram
class ActionSupport
class SearchDemographicAutoComplete2Action {
-HttpServletRequest request
-HttpServletResponse response
-static ObjectMapper objectMapper
+String execute()
-String formatJSON(List_HashMap_String_String info)
}
class SearchProviderAutoComplete2Action {
-HttpServletRequest request
-HttpServletResponse response
-static ObjectMapper objectMapper
+String execute()
+String labSearch()
}
class DemographicDao {
+List_Demographic searchDemographicByDOB(String dob,int limit,int offset,String providerNo,boolean outOfDomain)
+List_Demographic searchDemographicByNameAndNotStatus(String searchStr,List_String stati,int limit,int offset,String providerNo,boolean outOfDomain)
}
class Demographic {
+String getFormattedDob()
+String getFormattedName()
+String getPatientStatus()
+String getProviderNo()
+int getDemographicNo()
}
class RxProviderData {
+Provider getProvider(String providerNo)
}
class Provider {
+String getSurname()
+String getFirstName()
}
class DemographicCustDao {
+DemographicCust find(int demographicNo)
}
class DemographicCust {
+String getCust1()
+String getCust2()
+String getCust4()
}
class ProviderDataDao {
+List_ProviderData findByName(String firstName,String lastName,boolean activeOnly)
}
class ProviderData {
+String getFirstName()
+String getLastName()
+String getId()
}
class ObjectMapper {
+String writeValueAsString(Object value)
}
class Encode {
+static String forHtml(String input)
}
SearchDemographicAutoComplete2Action --|> ActionSupport
SearchProviderAutoComplete2Action --|> ActionSupport
SearchDemographicAutoComplete2Action --> DemographicDao
SearchDemographicAutoComplete2Action --> RxProviderData
SearchDemographicAutoComplete2Action --> DemographicCustDao
SearchDemographicAutoComplete2Action --> ObjectMapper
SearchDemographicAutoComplete2Action ..> Encode
SearchProviderAutoComplete2Action --> ProviderDataDao
SearchProviderAutoComplete2Action --> ObjectMapper
SearchProviderAutoComplete2Action ..> Encode
DemographicDao --> Demographic
DemographicCustDao --> DemographicCust
RxProviderData --> Provider
ProviderDataDao --> ProviderData
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @mohamadSaleh82, 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 and robustness of the autocomplete endpoints by addressing XSS vulnerabilities and improving JSON serialization practices. It ensures that user-controlled data is properly escaped before being rendered in HTML or serialized into JSON, thereby preventing potential injection attacks and improving data integrity. 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
Activity
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
|
PR Review 🔍
|
Review Summary by QodoFix XSS vulnerability in autocomplete endpoints
WalkthroughsDescription• Replace unsafe StringEscapeUtils.escapeJava() with OWASP Encode.forHtml() • Add HTML encoding to all user-controlled output fields • Refactor JSON building from manual string concatenation to Jackson ObjectMapper • Add null checks for provider name fields to prevent NPE • Fix typo: fomattedDob → formattedDob Diagramflowchart LR
A["User Input<br/>Patient/Provider Names"] -->|"Unsafe escapeJava()"| B["XSS Vulnerability"]
A -->|"OWASP Encode.forHtml()"| C["HTML Encoded Output"]
D["Manual JSON<br/>String Building"] -->|"JSON Injection Risk"| B
D -->|"Jackson ObjectMapper"| C
C --> E["Secure Response"]
File Changes1. src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
|
📝 WalkthroughWalkthroughAdds runtime privilege checks, replaces unsafe escaping with OWASP Encode.forHtml(), switches manual JSON assembly to Jackson serialization, and adds null-safety and early-return handling across two autocomplete action endpoints to prevent XSS and stabilize responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Action as AutocompleteAction
participant SecMgr as SecurityInfoManager
participant DAO
participant JSON as ObjectMapper
Client->>Action: HTTP autocomplete request (q)
Action->>SecMgr: verify READ privilege for resource
alt privilege denied
SecMgr-->>Action: deny
Action-->>Client: error response (no results)
else privilege granted
Action->>DAO: query demographics/providers (parsed params)
DAO-->>Action: results list
Action->>JSON: build LinkedHashMap nodes and serialize
JSON-->>Action: JSON string
Action-->>Client: JSON results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 Code Suggestions ✨
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
SearchDemographicAutoComplete2Action,Provider p = rx.getProvider(demo.getProviderNo());is used without a null check beforep.getSurname()/p.getFirstName(), which can still cause an NPE; consider guarding the main provider block the same way you did forcust1/2/4. - Both autocomplete actions now HTML-encode values (via
Encode.forHtml) before returning JSON; if the client uses these fields in non-HTML contexts or applies its own encoding, this may cause double-encoding or incorrect escaping, so consider keeping JSON responses raw and performing context-appropriate encoding at render time instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SearchDemographicAutoComplete2Action`, `Provider p = rx.getProvider(demo.getProviderNo());` is used without a null check before `p.getSurname()` / `p.getFirstName()`, which can still cause an NPE; consider guarding the main provider block the same way you did for `cust1/2/4`.
- Both autocomplete actions now HTML-encode values (via `Encode.forHtml`) before returning JSON; if the client uses these fields in non-HTML contexts or applies its own encoding, this may cause double-encoding or incorrect escaping, so consider keeping JSON responses raw and performing context-appropriate encoding at render time instead.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a critical XSS vulnerability in the autocomplete endpoints by replacing manual JSON string building with Jackson's ObjectMapper and using the OWASP Java Encoder for proper HTML encoding. The refactoring significantly improves security and code maintainability. I've added a couple of comments: one to fix a potential NullPointerException and another to suggest a minor code simplification. Overall, this is a solid and important fix.
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
| String formattedName = record.get("formattedName") != null ? record.get("formattedName") : ""; | ||
| String formattedDob = record.get("formattedDob") != null ? record.get("formattedDob") : ""; | ||
| String status = record.get("status") != null ? record.get("status") : ""; |
There was a problem hiding this comment.
These null checks can be simplified by using Map.getOrDefault(), which is available since Java 8. This would make the code more concise and readable.
| String formattedName = record.get("formattedName") != null ? record.get("formattedName") : ""; | |
| String formattedDob = record.get("formattedDob") != null ? record.get("formattedDob") : ""; | |
| String status = record.get("status") != null ? record.get("status") : ""; | |
| String formattedName = record.getOrDefault("formattedName", ""); | |
| String formattedDob = record.getOrDefault("formattedDob", ""); | |
| String status = record.getOrDefault("status", ""); |
There was a problem hiding this comment.
Actionable comments posted: 2
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/commn/web/SearchProviderAutoComplete2Action.java (3)
82-98:⚠️ Potential issue | 🔴 CriticalPotential NPE if
termparameter is null.
request.getParameter("term")can returnnull, and line 87 callssearchStr.indexOf(",")without a null guard. This will throw aNullPointerExceptionat runtime if thetermquery parameter is absent.🛡️ Proposed fix
public String labSearch() throws Exception { String searchStr = request.getParameter("term"); + if (searchStr == null || searchStr.isBlank()) { + response.setContentType("text/x-json"); + response.getWriter().write("[]"); + return null; + } String firstName, lastName;
53-66:⚠️ Potential issue | 🟠 MajorAdd
SecurityInfoManagerprivilege checks to bothexecute()andlabSearch()methods.All 2Action classes must verify user authorization before processing healthcare data. This action currently lacks the required
SecurityInfoManager.hasPrivilege()checks present in all peer 2Action implementations.Add the field:
private SecurityInfoManager securityInfoManager = SpringUtils.getBean(SecurityInfoManager.class);Then call
securityInfoManager.hasPrivilege()at the start of bothexecute()andlabSearch()methods per the 2Action pattern: security checks first, then input validation, then business logic.
59-80:⚠️ Potential issue | 🟠 MajorXSS fix is incomplete —
execute()still serializes provider data without HTML encoding.The
labSearch()path correctly appliesEncode.forHtml()to provider names, butexecute()serializes the rawprovListfromProviderData.searchProvider()directly into JSON with no encoding. If provider names contain malicious content, this endpoint path remains vulnerable.Apply
Encode.forHtml()to user-controlled fields in theexecute()path as well, or route the results through a similar encoded-map construction as done inlabSearch().#!/bin/bash # Check what fields ProviderData.searchProvider returns and whether they contain user-controlled strings ast-grep --pattern 'public $_ searchProvider($$$) { $$$ }'src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (1)
68-69:⚠️ Potential issue | 🟠 MajorMissing
SecurityInfoManager.hasPrivilege()authorization check.Same issue as the other autocomplete action: this action retrieves patient demographic data (PHI) without verifying user privileges. Per coding guidelines, authorization checks should be the first operation. Since this file handles patient data, the risk is higher here.
As per coding guidelines: "ALL actions MUST include
SecurityInfoManager.hasPrivilege()checks to verify user authorization before processing healthcare data."
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java`:
- Around line 100-103: The code calls searchStr.length() without ensuring
searchStr is non-null, which can throw an NPE when no request parameters are
present; update the conditional around the DOB branch to first verify searchStr
is not null (and optionally not empty/trimmed) before checking length and regex,
e.g. guard the block with searchStr != null && searchStr.length() == 8 &&
searchStr.matches(...); this ensures the call to
demographicDao.searchDemographicByDOB (and the substring operations) only run
when searchStr is valid.
- Around line 122-136: The current check if (p.getSurname() != null &&
p.getFirstName() != null) in SearchDemographicAutoComplete2Action causes
providerName to be omitted when either name part is null; change the code to
always populate "providerName" when p != null (after Provider p =
rx.getProvider(...)) by coalescing parts (e.g., String surname = p.getSurname()
!= null ? p.getSurname() : ""; String first = p.getFirstName() != null ?
p.getFirstName() : ""; if both empty use "Unknown" or "?" otherwise build
"surname, first" or just one part) and put that into the HashMap with
h.put("providerName", Encode.forHtml(...)); also guard against p being null
before accessing its getters.
🧹 Nitpick comments (2)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java (1)
103-111: Use imports instead of fully qualified class names.
java.util.LinkedHashMapandjava.util.ArrayListare used with their fully qualified names in multiple places (lines 103, 106). Consider adding proper imports for readability.src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (1)
138-170:DemographicCustDaois instantiated inside the loop on every iteration.
SpringUtils.getBean(DemographicCustDao.class)at lines 140–141 is called for each demographic in the results list. Spring bean lookups are not free. Move it outside the loop.♻️ Proposed fix
+ DemographicCustDao demographicCustDao = (DemographicCustDao) SpringUtils + .getBean(DemographicCustDao.class); + for (Demographic demo : list) { HashMap<String, String> h = new HashMap<String, String>(); ... if (OscarProperties.getInstance().isPropertyActive("workflow_enhance")) { h.put("nextAppointment", AppointmentUtil.getNextAppointment(demo.getDemographicNo() + "")); - DemographicCustDao demographicCustDao = (DemographicCustDao) SpringUtils - .getBean(DemographicCustDao.class); DemographicCust demographicCust = demographicCustDao.find(demo.getDemographicNo());
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Show resolved
Hide resolved
| List<HashMap<String, String>> secondList = new ArrayList<HashMap<String, String>>(); | ||
| for (Demographic demo : list) { | ||
| HashMap<String, String> h = new HashMap<String, String>(); | ||
| h.put("fomattedDob", demo.getFormattedDob()); | ||
| h.put("formattedName", StringEscapeUtils.escapeJava(demo.getFormattedName().replaceAll("\"", "\\\""))); | ||
| h.put("formattedDob", Encode.forHtml(demo.getFormattedDob())); | ||
| h.put("formattedName", Encode.forHtml(demo.getFormattedName())); | ||
| h.put("demographicNo", String.valueOf(demo.getDemographicNo())); | ||
| h.put("status", demo.getPatientStatus()); | ||
|
|
||
| h.put("status", Encode.forHtml(demo.getPatientStatus())); | ||
|
|
||
| Provider p = rx.getProvider(demo.getProviderNo()); | ||
| if (demo.getProviderNo() != null) { | ||
| h.put("providerNo", demo.getProviderNo()); | ||
| } | ||
| if (p.getSurname() != null && p.getFirstName() != null) { | ||
| h.put("providerName", p.getSurname() + ", " + p.getFirstName()); | ||
| h.put("providerName", Encode.forHtml(p.getSurname() + ", " + p.getFirstName())); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for providerName usage in frontend files
echo "=== Searching for providerName in JavaScript/TypeScript files ==="
rg -l "providerName" --type=js --type=ts 2>/dev/null | head -20
echo -e "\n=== Searching for providerName in JSP files ==="
rg -l "providerName" --glob "*.jsp" 2>/dev/null | head -20
echo -e "\n=== Sample usage context in JS/TS ==="
rg "providerName" --type=js --type=ts -B2 -A2 2>/dev/null | head -50
echo -e "\n=== Sample usage context in JSP ==="
rg "providerName" --glob "*.jsp" -B2 -A2 2>/dev/null | head -50Repository: carlos-emr/carlos
Length of output: 50375
🏁 Script executed:
#!/bin/bash
# Search for SearchDemographicAutoComplete2Action references
echo "=== Searching for SearchDemographicAutoComplete2Action usage ==="
rg -n "SearchDemographicAutoComplete2Action" --type=java -A3 -B3 2>/dev/null | head -50
# Search for the action URL pattern to find JSP consumers
echo -e "\n=== Searching for demographic autocomplete calls in JSPs ==="
rg -n "demographicAutoComplete\|SearchDemographic" --glob="*.jsp" -B2 -A2 2>/dev/null | head -50
# Search for providerName property access in JSPs
echo -e "\n=== Searching for how providerName is accessed in JSPs ==="
rg -n "\.providerName|providerName\]" --glob="*.jsp" -B2 -A2 2>/dev/null | head -80Repository: carlos-emr/carlos
Length of output: 8839
🏁 Script executed:
#!/bin/bash
# Check how providerNames is used after concatenation
echo "=== Context of providerNames usage in formonarenhancedpg2.jsp ==="
rg -n "providerNames" src/main/webapp/form/formonarenhancedpg2.jsp -B3 -A3 | head -60
# Check formonarenhancedpg1.jsp as well
echo -e "\n=== Context of providerNames usage in formonarenhancedpg1.jsp ==="
rg -n "providerNames" src/main/webapp/form/formonarenhancedpg1.jsp -B3 -A3 | head -60Repository: carlos-emr/carlos
Length of output: 2589
🏁 Script executed:
#!/bin/bash
# Search for ||, ?., or other null-handling patterns around providerName
echo "=== Checking for null-safety patterns in ONAR pages ==="
rg -n "providerName\s*(\|\||&&|\?)" src/main/webapp/form/ -B1 -A1 2>/dev/null
# Also check if there's any defensive coding pattern
rg -n "typeof.*providerName|providerName.*undefined" src/main/webapp/form/ -B1 -A1 2>/dev/nullRepository: carlos-emr/carlos
Length of output: 116
Add null coalescing fallback for missing provider names in frontend or populate even when first name is null.
The condition on line 134 (if (p.getSurname() != null && p.getFirstName() != null)) causes the providerName entry to be omitted from the HashMap if either surname or first name is null. The frontend code in formonarenhancedpg1.jsp (line 1029) and formonarenhancedpg2.jsp (line 760) concatenates val.providerName without null checks, resulting in the literal string "undefined" appearing in the tooltip and breaking the provider list display.
Either:
- Populate
providerNameeven when first name is missing (e.g., "Smith, ?" or surname only), or - Add a null-coalescing fallback in the frontend JavaScript (e.g.,
val.providerName || "Unknown")
🤖 Prompt for AI Agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java`
around lines 122 - 136, The current check if (p.getSurname() != null &&
p.getFirstName() != null) in SearchDemographicAutoComplete2Action causes
providerName to be omitted when either name part is null; change the code to
always populate "providerName" when p != null (after Provider p =
rx.getProvider(...)) by coalescing parts (e.g., String surname = p.getSurname()
!= null ? p.getSurname() : ""; String first = p.getFirstName() != null ?
p.getFirstName() : ""; if both empty use "Unknown" or "?" otherwise build
"surname, first" or just one part) and put that into the HashMap with
h.put("providerName", Encode.forHtml(...)); also guard against p being null
before accessing its getters.
Code Review by Qodo
✅
📘 Rule violation ⛨ Security |
| public String execute() throws Exception { | ||
| String providerNo = LoggedInInfo.getLoggedInInfoFromSession(request).getLoggedInProviderNo(); | ||
|
|
There was a problem hiding this comment.
1. Missing hasprivilege() checks 📘 Rule violation ⛨ Security
• The Struts actions execute request-driven searches and return results without any SecurityInfoManager.hasPrivilege(...) authorization gate. • This violates the requirement to enforce authorization in action handlers before accessing protected resources. • An attacker with an authenticated session but insufficient privileges could potentially call these endpoints directly.
Agent Prompt
## Issue description
The modified Struts2 actions perform request-driven searches and return protected data without an explicit `SecurityInfoManager.hasPrivilege(...)` authorization check.
## Issue Context
PR Compliance requires action handlers to enforce privilege checks before business logic. Relying on implicit assumptions (e.g., session existence) is not sufficient for this checklist item.
## Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[63-120]
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java[53-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (searchStr.length() == 8 && searchStr.matches("([0-9]*)")) { | ||
| list = demographicDao.searchDemographicByDOB(searchStr.substring(0, 4) + "-" + searchStr.substring(4, 6) + "-" + searchStr.substring(6, 8), 100, 0, providerNo, outOfDomain); | ||
| list = demographicDao.searchDemographicByDOB( | ||
| searchStr.substring(0, 4) + "-" + searchStr.substring(4, 6) + "-" + searchStr.substring(6, 8), 100, | ||
| 0, providerNo, outOfDomain); |
There was a problem hiding this comment.
2. searchstr null dereference 📘 Rule violation ⛯ Reliability
• searchStr is read from multiple request parameters but can still remain null if none are present. • The code then calls searchStr.length() without a null guard, which can throw a NullPointerException and return a 500 error. • This violates the edge-case handling requirement for null/empty inputs.
Agent Prompt
## Issue description
`searchStr` can be `null` if none of the expected request parameters are provided, but the code calls `searchStr.length()`.
## Issue Context
This is an edge-case bug that can cause NPE/500 responses and violates robust error handling requirements.
## Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[76-116]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Provider p = rx.getProvider(demo.getProviderNo()); | ||
| if (demo.getProviderNo() != null) { | ||
| h.put("providerNo", demo.getProviderNo()); | ||
| } | ||
| if (p.getSurname() != null && p.getFirstName() != null) { | ||
| h.put("providerName", p.getSurname() + ", " + p.getFirstName()); | ||
| h.put("providerName", Encode.forHtml(p.getSurname() + ", " + p.getFirstName())); | ||
| } |
There was a problem hiding this comment.
3. p null dereference 📘 Rule violation ⛯ Reliability
• The code calls p.getSurname() / p.getFirstName() after `p = rx.getProvider(demo.getProviderNo()) without checking whether p` is null. • If rx.getProvider(...) returns null (e.g., missing/invalid provider), this can throw a NullPointerException and break the endpoint. • This is inconsistent with the later cust* provider blocks where null checks were added.
Agent Prompt
## Issue description
`Provider p` is dereferenced without checking if `rx.getProvider(...)` returned null.
## Issue Context
The code already added null checks for cust* provider lookups, but the primary provider block still lacks that guard.
## Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[129-136]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| h.put("formattedDob", Encode.forHtml(demo.getFormattedDob())); | ||
| h.put("formattedName", Encode.forHtml(demo.getFormattedName())); |
There was a problem hiding this comment.
4. Dob key breaks clients 🐞 Bug ✓ Correctness
• The demographic search JSON response renamed the DOB field from legacy "fomattedDob" to "formattedDob". • Multiple existing consumers explicitly expect "fomattedDob" (YUI DataSource schemas and jQuery mapping), so DOB will become undefined and UI labels/selection formatting will break. • This is a backward-incompatible change on a shared endpoint (/demographic/SearchDemographic.do) with broad usage.
Agent Prompt
## Issue description
The endpoint `/demographic/SearchDemographic.do` returns JSON records that historically contained a misspelled DOB field key `fomattedDob`. This PR changed the key to `formattedDob`, which breaks multiple existing consumers that still expect `fomattedDob`.
## Issue Context
Several UI pages configure JSON schemas / mapping logic explicitly referencing `fomattedDob`. When the key is missing, DOB becomes `undefined`, breaking display/selection formatting and potentially patient identification workflows.
## Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[122-129]
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[175-184]
## Suggested change
When building each `h` map, add both keys:
- `formattedDob`
- `fomattedDob` (legacy)
Populate both with the same (encoded or raw—consistent with your chosen strategy) DOB string so existing clients keep working while newer clients can adopt the corrected key.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
I think I remember this. typo! This has come up before, it could just be the same person making the same mistake. I think we need to leave original miss spell on. @claude review this and make issue ticket to involve fixing the spelling mistakes without breaking clients, if possible.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Ensure DOB is always present under both keys ("formattedDob" and legacy "fomattedDob") with null-safe encoding so existing clients do not break when DOB is missing.
| h.put("formattedDob", Encode.forHtml(demo.getFormattedDob())); | |
| h.put("formattedName", Encode.forHtml(demo.getFormattedName())); | |
| for (Demographic demo : list) { | |
| HashMap<String, String> h = new HashMap<>(); | |
| String encodedDob = Encode.forHtml(StringUtils.defaultString(demo.getFormattedDob())); | |
| h.put("formattedDob", encodedDob); | |
| h.put("fomattedDob", encodedDob); // Legacy key for backward compatibility | |
| h.put("formattedName", Encode.forHtml(StringUtils.defaultString(demo.getFormattedName()))); | |
| h.put("demographicNo", String.valueOf(demo.getDemographicNo())); | |
| h.put("status", Encode.forHtml(StringUtils.defaultString(demo.getPatientStatus()))); |
See review comment here
…tion checks in both actions - Add null guards for searchStr to prevent NPE - Add null check for Provider p before accessing its methods - Add HTML encoding for nextAppointment field
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 (1)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java (1)
87-93:⚠️ Potential issue | 🟠 MajorInconsistent output encoding between execute() and labSearch() paths creates a maintainability and security concern.
The
labSearch()method correctly appliesEncode.forHtml()to provider names before JSON serialization (line 133), but theexecute()path serializes fullProviderDataobjects directly without any HTML encoding. This exposes all provider fields (firstName, lastName, address, phone, email, etc.) without sanitization.Since both paths return JSON responses with the same type of content, and
labSearch()demonstrates that HTML encoding is expected, theexecute()path should follow the same pattern. Manually construct a sanitized result object (similar tolabSearch()) and applyEncode.forHtml()to text fields before serialization, or ensure the response is only consumed programmatically without DOM insertion.
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java`:
- Around line 62-70: The labSearch method is public and can be invoked directly,
bypassing the hasPrivilege check in execute; change the visibility of labSearch
to private inside the SearchProviderAutoComplete2Action class so it can only be
called from execute(), and verify there are no external callers to labSearch
(update or refactor any callers to use execute() instead if found).
🧹 Nitpick comments (4)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java (1)
125-130: Consider using an import with alias or extracting a type alias to reduce inline FQCNs.The fully qualified
io.github.carlos_emr.carlos.commn.model.ProviderDatais used because line 46 already importsio.github.carlos_emr.carlos.providers.data.ProviderData. This is understandable but clutters the code. Consider renaming the import of the legacy class or restructuring to reduce inline FQCNs.Similarly,
java.util.LinkedHashMapandjava.util.ArrayListon lines 127 and 130 can simply be imported at the top of the file since they don't conflict with anything.♻️ Suggested imports to reduce FQCNs
import java.util.Hashtable; import java.util.List; +import java.util.ArrayList; +import java.util.LinkedHashMap;Then simplify lines 127 and 130:
- List<java.util.LinkedHashMap<String, String>> searchResults = new java.util.ArrayList<>(); + List<LinkedHashMap<String, String>> searchResults = new ArrayList<>(); for (io.github.carlos_emr.carlos.commn.model.ProviderData provData : provList) { - java.util.LinkedHashMap<String, String> node = new java.util.LinkedHashMap<>(); + LinkedHashMap<String, String> node = new LinkedHashMap<>();src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (3)
153-158: MoveDemographicCustDaobean retrieval outside the loop.
SpringUtils.getBean(DemographicCustDao.class)is called on every iteration inside theforloop (lines 156–157). While Spring returns the same singleton, the repeated lookup is unnecessary overhead. Hoist it alongside the other DAO at line 84.♻️ Proposed fix
Move the DAO retrieval next to the existing
DemographicDaoinstantiation:DemographicDao demographicDao = (DemographicDao) SpringUtils.getBean(DemographicDao.class); + DemographicCustDao demographicCustDao = (DemographicCustDao) SpringUtils.getBean(DemographicCustDao.class);Then inside the loop, remove the local instantiation:
- DemographicCustDao demographicCustDao = (DemographicCustDao) SpringUtils - .getBean(DemographicCustDao.class);
205-222: Good refactor: Jackson-based JSON serialization eliminates JSON injection risk.Replacing manual string concatenation with
ObjectMapper.writeValueAsString()is the correct fix. The use ofLinkedHashMappreserves key order for consistent output.Minor style nit:
java.util.LinkedHashMap(line 207, 209) andcom.fasterxml.jackson.core.JsonProcessingException(line 206) could use imports instead of FQCNs for readability.
70-203: Consider extracting helper methods to reduceexecute()complexity.Static analysis flags
execute()(lines 70–203) as a complex method, and it handles parameter resolution, search dispatch, result mapping, provider lookups, and response formatting. Consider extracting:
- Search parameter resolution (lines 85–97) → e.g.,
resolveSearchString(request)- Result-to-HashMap mapping (lines 138–189) → e.g.,
mapDemographicsToResults(list, rx)This would improve readability and testability without changing behavior.
| public String execute() throws Exception { | ||
| LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); | ||
|
|
||
| if (!securityInfoManager.hasPrivilege(loggedInInfo, "_provider", SecurityInfoManager.READ, null)) { | ||
| throw new RuntimeException("missing required security object (_provider)"); | ||
| } | ||
|
|
||
| if ("labSearch".equals(request.getParameter("method"))) { | ||
| return labSearch(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Struts2 DMI configuration
echo "=== Checking for Struts2 DMI configuration ==="
rg -n 'struts.enable.DynamicMethodInvocation' --type=properties --type=xml --type=conf -C 2
echo -e "\n=== Checking for struts2 XML config files ==="
fd -e "xml" | grep -i struts | head -20
echo -e "\n=== Looking for SearchProviderAutoComplete configuration ==="
rg -n 'SearchProviderAutoComplete' --type=xml -C 5Repository: carlos-emr/carlos
Length of output: 6541
🏁 Script executed:
#!/bin/bash
# Read the full SearchProviderAutoComplete2Action.java file
echo "=== Full SearchProviderAutoComplete2Action.java content ==="
fd -name "SearchProviderAutoComplete2Action.java" -type fRepository: carlos-emr/carlos
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Find the file and read it
find . -name "SearchProviderAutoComplete2Action.java" -type f 2>/dev/null | head -1 | xargs wc -lRepository: carlos-emr/carlos
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Read the full SearchProviderAutoComplete2Action.java file
cat -n ./src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.javaRepository: carlos-emr/carlos
Length of output: 6796
🏁 Script executed:
#!/bin/bash
# Check for DMI configuration in properties files and constants
echo "=== Checking for DMI in struts configuration ==="
rg -n 'DynamicMethodInvocation' --type=java --type=xml
echo ""
echo "=== Checking struts.properties or application.properties ==="
fd "\.properties$" | xargs rg -n 'struts.enable' 2>/dev/null || echo "No struts.enable properties found"
echo ""
echo "=== Checking for default struts configuration ==="
rg -n 'struts\.' src/main/resources/ 2>/dev/null | head -20 || echo "No src/main/resources found"Repository: carlos-emr/carlos
Length of output: 344
🏁 Script executed:
#!/bin/bash
# Check if there's a struts properties file or Spring XML configuration
find . -name "*.properties" -o -name "*.xml" | grep -E "(struts|spring)" | grep -v "test" | grep -v "TrackingRegexp"Repository: carlos-emr/carlos
Length of output: 303
Make labSearch() private to prevent direct invocation bypassing the security check.
The labSearch() method is public but lacks the required SecurityInfoManager.hasPrivilege() check. Since it's only called from within execute() (line 70), it should be private to ensure all requests go through the security gate.
- public String labSearch() throws Exception {
+ private String labSearch() throws Exception {🤖 Prompt for AI Agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java`
around lines 62 - 70, The labSearch method is public and can be invoked
directly, bypassing the hasPrivilege check in execute; change the visibility of
labSearch to private inside the SearchProviderAutoComplete2Action class so it
can only be called from execute(), and verify there are no external callers to
labSearch (update or refactor any callers to use execute() instead if found).
There was a problem hiding this comment.
@claude, this still requires authentication checks and the nature of this platform does largely treat authentication as circle of care, but this is something to look at further in general as the code base matures. Does this allow for bypass of authentication check? I don't think so? Please make a issue ticket for this for follow up.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Ask❓
@claude, this still requires authentication checks and the nature of this platform does largely treat authentication as circle of care, but this is something to look at further in general as the code base matures. Does this allow for bypass of authentication check? I don't think so? Please make a issue ticket for this for follow up.
Answer:
Does this allow a bypass of the authentication/authorization check?
Potentially yes, depending on Struts2 configuration—specifically whether Dynamic Method Invocation (DMI) is enabled.
Why it could be bypassable
In SearchProviderAutoComplete2Action, the privilege check was added in execute():
if (!securityInfoManager.hasPrivilege(loggedInInfo, "_provider", SecurityInfoManager.READ, null)) {
throw new RuntimeException("missing required security object (_provider)");
}However, labSearch() remains public and contains no privilege check. In Struts2, if DMI is enabled, a request can invoke non-execute() public methods directly via the actionName!methodName pattern (e.g., SearchProviderAutoComplete2Action!labSearch). In that scenario, the request would reach labSearch() without running execute(), and thus without running the _provider privilege gate.
If DMI is disabled (common in hardened setups), then this direct invocation path generally won’t work, and you’re right that it likely does not bypass authz in practice. But because this is security-sensitive and configuration can drift over time (or differ across deployments), it’s still worth addressing as defense-in-depth.
Recommended follow-up (low-risk hardening)
Any one of these would close the potential gap:
- Make
labSearch()private(or at leastprotected) if it’s not intended as a Struts action method. - Add the same
hasPrivilegecheck insidelabSearch()(belt-and-suspenders). - Confirm/ensure DMI is disabled globally in Struts config, and document it as a security requirement.
Option (1) is simplest and aligns with CodeRabbit’s suggestion, provided there are no external callers (and since Struts uses reflection, “external caller” here includes the framework via DMI).
Follow-up issue ticket to create
Title: security: prevent potential Struts2 DMI bypass for public action methods (labSearch)
Description:
SearchProviderAutoComplete2Actionenforces_providerREAD privilege inexecute(), butlabSearch()ispublicand has no privilege check.- If Struts2 Dynamic Method Invocation (DMI) is enabled, attackers could invoke
!labSearchdirectly and bypass theexecute()authorization gate.
Acceptance criteria: - Make
labSearch()non-public or add an explicit privilege check withinlabSearch(). - Verify Struts2 DMI setting across deployments (ensure disabled) and document expected config.
- Add a regression/security test or documentation note to prevent reintroducing public methods without authz checks.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java`:
- Around line 90-96: The code in SearchProviderAutoComplete2Action uses
searchStr.split(",") which drops trailing empty tokens and causes
ArrayIndexOutOfBoundsException when searchStr ends with a comma; change the
split call to searchStr.split(",", -1) (matching labSearch()) and then safely
assign lastName = array[0].trim(); firstName = array.length > 1 ?
array[1].trim() : "" to handle an empty second element, ensuring the logic
around variables lastName and firstName remains correct.
- Around line 154-161: In labSearch() update the construction of the label to
HTML-encode provider name parts: call Encode.forHtml() on provLastName and
provFirstName (from the for-loop that builds node and sets "label") before
concatenating them, and ensure the class imports org.owasp.encoder.Encode; leave
node.put("value", provData.getId()) unchanged so only the display label is
sanitized; this mirrors the encoding used in execute().
- Around line 103-111: In SearchProviderAutoComplete2Action update the loop that
builds provList to HTML-encode all user-controlled fields (firstName, lastName,
ohipNo) using Encode.forHtml(), mirroring the approach in
SearchDemographicAutoComplete2Action; add the import org.owasp.encoder.Encode
and replace the raw p.getFirstName(), p.getLastName(), and p.getOhipNo() values
with Encode.forHtml(...) when putting them into the result map (keep providerNo
as-is if it is not user-controlled).
🧹 Nitpick comments (4)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java (2)
113-118: Minor: preferwriteValueAsString()overvalueToTree().toString().
objectMapper.valueToTree(d)creates an intermediateObjectNodejust to call.toString()on it. UsingwriteValueAsString(d)directly (as done inlabSearch()on line 164 and in the companion file) is simpler and avoids the unnecessary intermediate tree allocation.♻️ Proposed simplification
- ObjectNode jsonArray = objectMapper.valueToTree(d); - response.getWriter().write(jsonArray.toString()); + response.getWriter().write(objectMapper.writeValueAsString(d));This also allows removing the unused
import com.fasterxml.jackson.databind.node.ObjectNode;at line 38.
149-151: Redundant localProviderDataDaoshadows the instance field.Line 59 already initializes
providerDataDaoas an instance field. Line 149 creates a second local instance of the same bean, shadowing it. Use the existing field instead.♻️ Remove the redundant local variable
- ProviderDataDao providerDataDao = SpringUtils.getBean(ProviderDataDao.class); - List<io.github.carlos_emr.carlos.commn.model.ProviderData> provList = providerDataDao.findByName(firstName, + List<io.github.carlos_emr.carlos.commn.model.ProviderData> provList = this.providerDataDao.findByName(firstName, lastName, true);src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (2)
71-78: Good addition of the privilege check — consider adding audit logging.The
securityInfoManager.hasPrivilege()check with_demographicREAD is correctly placed as the first operation, andproviderNois now cleanly derived fromloggedInInfo. This aligns well with theSearchProviderAutoComplete2Actionpattern and the coding guidelines.One gap: the coding guidelines require audit logging for patient data access via
LogAction.addLogSynchronous(). This is a pre-existing omission, but worth addressing to maintain PHI access audit trails.,
156-161: HoistDemographicCustDaobean lookup andworkflow_enhancecheck out of the loop.Both
OscarProperties.getInstance().isPropertyActive("workflow_enhance")andSpringUtils.getBean(DemographicCustDao.class)return the same value on every iteration. With up to 100 demographics, this is 100 redundant bean lookups and property checks.♻️ Proposed fix — move before the loop
Add these before line 138 (before the
forloop):boolean workflowEnhance = OscarProperties.getInstance().isPropertyActive("workflow_enhance"); DemographicCustDao demographicCustDao = workflowEnhance ? (DemographicCustDao) SpringUtils.getBean(DemographicCustDao.class) : null;Then update the loop body:
- if (OscarProperties.getInstance().isPropertyActive("workflow_enhance")) { + if (workflowEnhance) { String nextAppt = AppointmentUtil.getNextAppointment(demo.getDemographicNo() + ""); h.put("nextAppointment", nextAppt != null ? Encode.forHtml(nextAppt) : ""); - DemographicCustDao demographicCustDao = (DemographicCustDao) SpringUtils - .getBean(DemographicCustDao.class); DemographicCust demographicCust = demographicCustDao.find(demo.getDemographicNo());
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java
Show resolved
Hide resolved
|
Hello @mohamedSaleh82! Thank you again for your quick work on this and your engagement with feedback! There is a lot of it with the bots :-) but I find it helpful particular as we are looking to run a lot of AI as well as enable collaborators with lots of background. Sometimes they are right, sometimes they are wrong, but usually it is more an issue of right and "right, but that is already existing / correct but technical debt for another time" that we can at worst make an issue ticket for. I am keeping the claude connector somewhat locked down, but (at)coderabbitai I think will respond to you if you want to use it to flag other issues tickets that come out. |
|
Just reviewing your work on this now, leaning towards LGTM but just want to review the other automated notes and if anything should be followed up or flagged for follow up before I do the code by code review manually in detail. Thanks again! |
|
(also, don't mind the x - I'll pull that integration off, I am still tweaking some of the workflows) |
|
@claude review |
|
I'll analyze this and get back to you. |

User description
Summary
Fix XSS vulnerability in SearchDemographicAutoComplete2Action and SearchProviderAutoComplete2Action endpoints.
Fixes #250
Changes Made
Security Fixes
Replaced
StringEscapeUtils.escapeJava()withEncode.forHtml()(OWASP Java Encoder)escapeJava()only escapes Java string literals, NOT HTML contextEncode.forHtml()properly escapes<,>,&which are XSS attack vectorsAdded HTML encoding to all user-controlled fields:
formattedName(patient names)status(patient status)providerName(provider names)cust1Name,cust2Name,cust4Name(custom provider fields)Refactored formatJSON() to use Jackson ObjectMapper
objectMapper.writeValueAsString()for proper JSON encodingAdditional Improvements
fomattedDob→formattedDobTesting
Test with malicious patient name:
Description
StringEscapeUtils.escapeJava()withEncode.forHtml()in autocomplete endpoints.Jackson ObjectMapper, enhancing security and reliability.Changes walkthrough 📝
SearchDemographicAutoComplete2Action.java
Security enhancements and JSON refactor inSearchDemographicAutoComplete2Actionsrc/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
StringEscapeUtils.escapeJava()withEncode.forHtml()for XSSprotection.
formattedName,status,providerName, and custom fields.formatJSON()to useJackson ObjectMapperfor JSONserialization.
SearchProviderAutoComplete2Action.java
Security improvements in SearchProviderAutoComplete2Actionsrc/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.java
Encode.forHtml()for provider name encoding to prevent XSS.Jackson ObjectMapper.Summary by CodeRabbit
Bug Fixes
Improvements
Security