Security: Fix XSS and JSON injection in SearchDemographicAutoComplete2Action#296
Security: Fix XSS and JSON injection in SearchDemographicAutoComplete2Action#296FinnBotOrca wants to merge 6 commits intocarlos-emr:developfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors SearchDemographicAutoComplete2Action to properly HTML-encode user-visible fields and to generate JSON responses via ObjectMapper instead of manual string concatenation, eliminating XSS and JSON injection risks. Sequence diagram for SearchDemographicAutoComplete2Action secure response generationsequenceDiagram
actor Client
participant SearchDemographicAutoComplete2Action
participant Demographic
participant Provider
participant Encode
participant ObjectMapper
Client->>SearchDemographicAutoComplete2Action: HTTP request (search term)
SearchDemographicAutoComplete2Action->>Demographic: query matching demographics
Demographic-->>SearchDemographicAutoComplete2Action: List<Demographic>
loop for each Demographic
SearchDemographicAutoComplete2Action->>Demographic: getFormattedName, getFormattedDob, getPatientStatus, getProviderNo
Demographic-->>SearchDemographicAutoComplete2Action: name, dob, status, providerNo
SearchDemographicAutoComplete2Action->>Provider: getProvider(providerNo)
Provider-->>SearchDemographicAutoComplete2Action: Provider
SearchDemographicAutoComplete2Action->>Provider: getSurname, getFirstName
Provider-->>SearchDemographicAutoComplete2Action: surname, firstName
SearchDemographicAutoComplete2Action->>Encode: forHtml(name, status, providerName)
Encode-->>SearchDemographicAutoComplete2Action: htmlEncoded values
SearchDemographicAutoComplete2Action->>SearchDemographicAutoComplete2Action: build HashMap record
end
SearchDemographicAutoComplete2Action->>ObjectMapper: writeValueAsString(results)
ObjectMapper-->>SearchDemographicAutoComplete2Action: JSON array string
SearchDemographicAutoComplete2Action-->>Client: HTTP response (JSON)
Class diagram for SearchDemographicAutoComplete2Action security refactorclassDiagram
class SearchDemographicAutoComplete2Action {
- ObjectMapper objectMapper
+ String execute() throws Exception
- String formatJSON(List<HashMap<String, String>> info) throws Exception
}
class Demographic {
+ String getFormattedDob()
+ String getFormattedName()
+ String getPatientStatus()
+ int getDemographicNo()
+ String getProviderNo()
}
class Provider {
+ String getSurname()
+ String getFirstName()
}
class ObjectMapper {
+ String writeValueAsString(Object value) throws Exception
}
class Encode {
<<static>>
+ String forHtml(String input)
}
SearchDemographicAutoComplete2Action --> Demographic : reads
SearchDemographicAutoComplete2Action --> Provider : reads
SearchDemographicAutoComplete2Action --> ObjectMapper : uses
SearchDemographicAutoComplete2Action --> Encode : uses
Flow diagram for secure JSON construction in formatJSONflowchart TD
A[Input info List<HashMap>] --> B[Initialize empty results List<HashMap>]
B --> C{For each record in info}
C -->|yes| D[Create new HashMap h]
D --> E[Set h label = formattedName + fomattedDob + status]
E --> F[Set h value = demographicNo]
F --> G[Set h providerNo, provider, nextAppt, formattedName]
G --> H[Add h to results]
H --> C
C -->|no| I["Call objectMapper.writeValueAsString(results)"]
I --> J[Return JSON string]
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 @FinnBotOrca, 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 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 and JSON injection vulnerabilities in SearchDemographicAutoComplete2Action
WalkthroughsDescription• Replace unsafe string escaping with OWASP Encode.forHtml() for XSS prevention • Refactor formatJSON() to use ObjectMapper instead of manual string concatenation • Apply HTML encoding to patient names, status, and provider names • Eliminate JSON injection vulnerability through proper serialization Diagramflowchart LR
A["Unsafe String Escaping"] -->|Replace with| B["OWASP Encode.forHtml()"]
C["Manual JSON String Building"] -->|Refactor to| D["ObjectMapper Serialization"]
B --> E["XSS Prevention"]
D --> F["JSON Injection Prevention"]
File Changes1. src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
|
PR Code Suggestions ✨
|
📝 WalkthroughWalkthroughAdds a runtime privilege check for demographic READ, derives providerNo from LoggedInInfo, applies OWASP HTML encoding to user-visible fields, replaces manual JSON/string assembly with Jackson ObjectMapper (formatJSON now throws JsonProcessingException), and switches several JSP autocomplete renderings to DOM-based jQuery construction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Action as SearchDemographicAutoComplete2Action
participant SecMgr as SecurityInfoManager
participant DB as Demographic / DemographicCust DAO
participant JSON as ObjectMapper
Client->>Action: autocomplete request (query)
Action->>SecMgr: check "demographic" READ privilege
SecMgr-->>Action: allow / deny
alt deny
Action-->>Client: "noPrivilege"
else allow
Action->>DB: fetch demographics + provider info
DB-->>Action: demographic list
Action->>Action: read LoggedInInfo (providerNo)
Action->>Action: apply Encode.forHtml to names/status/provider fields
Action->>JSON: build list-of-maps and serialize to JSON
JSON-->>Action: JSON payload
Action-->>Client: JSON response (application/json)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![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:
- Applying
Encode.forHtml()at this service/action layer will HTML-escape data for all consumers of this JSON endpoint; consider moving the HTML encoding to the view/template layer (or the specific HTML-rendering context) to avoid double-encoding or breaking non-HTML clients. - The
formatJSONmethod now declaresthrows Exception, which changes its contract; consider narrowing this to the specific checked exception(s) that can be thrown byobjectMapper.writeValueAsStringand confirming callers handle or propagate them appropriately.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Applying `Encode.forHtml()` at this service/action layer will HTML-escape data for all consumers of this JSON endpoint; consider moving the HTML encoding to the view/template layer (or the specific HTML-rendering context) to avoid double-encoding or breaking non-HTML clients.
- The `formatJSON` method now declares `throws Exception`, which changes its contract; consider narrowing this to the specific checked exception(s) that can be thrown by `objectMapper.writeValueAsString` and confirming callers handle or propagate them appropriately.
## Individual Comments
### Comment 1
<location> `src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java:185` </location>
<code_context>
- if (idx < size - 1) {
- json.append(",");
- }
+ private String formatJSON(List<HashMap<String, String>> info) throws Exception {
+ List<HashMap<String, String>> results = new ArrayList<>();
+ for (HashMap<String, String> record : info) {
</code_context>
<issue_to_address>
**suggestion:** The new `throws Exception` on `formatJSON` broadens the method’s contract more than necessary.
Given that the only new failure source is `objectMapper.writeValueAsString`, consider either catching `JsonProcessingException` inside `formatJSON` (and wrapping/handling it appropriately) or declaring that specific exception instead of `Exception`. A broad checked exception makes callers handle more than they need to and obscures the concrete failure modes.
```suggestion
private String formatJSON(List<HashMap<String, String>> info) throws com.fasterxml.jackson.core.JsonProcessingException {
```
</issue_to_address>
### Comment 2
<location> `src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java:187-194` </location>
<code_context>
- }
+ private String formatJSON(List<HashMap<String, String>> info) throws Exception {
+ List<HashMap<String, String>> results = new ArrayList<>();
+ for (HashMap<String, String> record : info) {
+ HashMap<String, String> h = new HashMap<>();
+ h.put("label", record.get("formattedName") + " " + record.get("fomattedDob") + " (" + record.get("status") + ")");
+ h.put("value", record.get("demographicNo"));
+ h.put("providerNo", record.get("providerNo"));
+ h.put("provider", record.get("providerName"));
+ h.put("nextAppt", record.get("nextAppointment"));
+ h.put("formattedName", record.get("formattedName"));
+ results.add(h);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Behavior change when some fields are missing: JSON `null` vs the string "null".
Previously, Java string concatenation would turn missing fields into the literal string "null". Now, `record.get(...)` returning `null` will become JSON `null` when serialized. If any downstream code or tests rely on always getting string values (including the literal "null"), this is a behavioral change and may be breaking. Consider normalizing missing values (e.g., to empty strings) before putting them into `h` if you need to preserve the old behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
![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 effectively addresses critical security vulnerabilities, namely Cross-Site Scripting (XSS) and JSON injection, by correctly employing Encode.forHtml() for output encoding and ObjectMapper for safe JSON serialization. The changes are well-aligned with security best practices. I have identified one high-severity functional bug where provider names can be rendered as "null, null" if a provider is not found. Additionally, I've suggested a medium-severity refactoring to enhance maintainability by using a POJO for JSON serialization, which will make the code more robust and readable.
| if (cust1 != null) { | ||
| h.put("cust1", cust1); | ||
| p = rx.getProvider(cust1); | ||
| h.put("cust1Name", p.getSurname() + ", " + p.getFirstName()); | ||
| h.put("cust1Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName())); | ||
| } | ||
| if (cust2 != null) { | ||
| h.put("cust2", cust2); | ||
| p = rx.getProvider(cust2); | ||
| h.put("cust2Name", p.getSurname() + ", " + p.getFirstName()); | ||
| h.put("cust2Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName())); | ||
| } | ||
| if (cust4 != null) { | ||
| h.put("cust4", cust4); | ||
| p = rx.getProvider(cust4); | ||
| h.put("cust4Name", p.getSurname() + ", " + p.getFirstName()); | ||
| h.put("cust4Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName())); | ||
| } |
There was a problem hiding this comment.
While adding HTML encoding is a good security improvement, there's a potential functional bug in this block. If rx.getProvider() is called with a provider number that doesn't exist, it returns a Provider object where getSurname() and getFirstName() return null. The current code would then concatenate these nulls, resulting in the string "null, null" being displayed in the UI. To prevent this, you should add null checks for the provider's first and last names, similar to the check performed for the primary provider on line 136.
if (cust1 != null) {
h.put("cust1", cust1);
p = rx.getProvider(cust1);
if (p.getSurname() != null && p.getFirstName() != null) {
h.put("cust1Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName()));
}
}
if (cust2 != null) {
h.put("cust2", cust2);
p = rx.getProvider(cust2);
if (p.getSurname() != null && p.getFirstName() != null) {
h.put("cust2Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName()));
}
}
if (cust4 != null) {
h.put("cust4", cust4);
p = rx.getProvider(cust4);
if (p.getSurname() != null && p.getFirstName() != null) {
h.put("cust4Name", Encode.forHtml(p.getSurname() + ", " + p.getFirstName()));
}
}| private String formatJSON(List<HashMap<String, String>> info) throws Exception { | ||
| List<HashMap<String, String>> results = new ArrayList<>(); | ||
| for (HashMap<String, String> record : info) { | ||
| HashMap<String, String> h = new HashMap<>(); | ||
| h.put("label", record.get("formattedName") + " " + record.get("fomattedDob") + " (" + record.get("status") + ")"); | ||
| h.put("value", record.get("demographicNo")); | ||
| h.put("providerNo", record.get("providerNo")); | ||
| h.put("provider", record.get("providerName")); | ||
| h.put("nextAppt", record.get("nextAppointment")); | ||
| h.put("formattedName", record.get("formattedName")); | ||
| results.add(h); | ||
| } | ||
| json.append("]"); | ||
|
|
||
| return json.toString(); | ||
| return objectMapper.writeValueAsString(results); | ||
| } |
There was a problem hiding this comment.
Replacing manual JSON string building with ObjectMapper is an excellent security enhancement. To further improve code quality, consider refactoring this method to use a dedicated Plain Old Java Object (POJO) instead of a HashMap for the data being serialized. This approach enhances type safety, readability, and maintainability by creating an explicit data contract.
You could define a static inner class for the autocomplete items and then use it in this method. This would make the code more robust and self-documenting.
private String formatJSON(List<HashMap<String, String>> info) throws Exception {
List<AutoCompleteItem> results = new ArrayList<>();
for (HashMap<String, String> record : info) {
AutoCompleteItem item = new AutoCompleteItem();
item.label = record.get("formattedName") + " " + record.get("fomattedDob") + " (" + record.get("status") + ")";
item.value = record.get("demographicNo");
item.providerNo = record.get("providerNo");
item.provider = record.get("providerName");
item.nextAppt = record.get("nextAppointment");
item.formattedName = record.get("formattedName");
results.add(item);
}
return objectMapper.writeValueAsString(results);
}
private static class AutoCompleteItem {
public String label;
public String value;
public String providerNo;
public String provider;
public String nextAppt;
public String formattedName;
}![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 (1)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (1)
72-73:⚠️ Potential issue | 🟠 MajorAdd mandatory
SecurityInfoManager.hasPrivilege()check toexecute()method.This action searches and returns patient demographic data without verifying user privileges. Every 2Action must perform security checks first, before any business logic. The check is currently missing.
Add at the start of
execute():if (!securityInfoManager.hasPrivilege(LoggedInInfo.getLoggedInInfoFromSession(request), "_demographic", "r", null)) { throw new SecurityException("Access denied: insufficient privileges for demographic search"); }Ensure
SecurityInfoManageris available viaSpringUtils.getBean(SecurityInfoManager.class)as a class field.
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java`:
- Line 44: Remove the unused import of StringEscapeUtils: delete the line
importing org.apache.commons.text.StringEscapeUtils from
SearchDemographicAutoComplete2Action and ensure there are no remaining
references to StringEscapeUtils in that class (all encoding should use
Encode.forHtml()). This cleans up the unused import and prevents unnecessary
dependency usage.
🧹 Nitpick comments (2)
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java (2)
185-198:formatJSONrefactored to use Jackson — good fix for JSON injection. A few things to tighten up.Using
ObjectMapper.writeValueAsString()instead of manual string concatenation is the correct approach to prevent JSON injection.Issues in the
labelconstruction on line 189:
record.get("status"),record.get("providerNo"),record.get("providerName"), andrecord.get("nextAppointment")can all benull(e.g.,providerNameis only set when both surname and firstName are non-null;nextAppointmentis only set whenworkflow_enhanceis active). String concatenation withnullproduces literal"null"in the output — e.g.,"Smith, John null (null)".The
throws Exceptionis overly broad —ObjectMapper.writeValueAsString()throwsJsonProcessingException, which is more precise.♻️ Suggested improvements
- private String formatJSON(List<HashMap<String, String>> info) throws Exception { + private String formatJSON(List<HashMap<String, String>> info) throws com.fasterxml.jackson.core.JsonProcessingException { List<HashMap<String, String>> results = new ArrayList<>(); for (HashMap<String, String> record : info) { HashMap<String, String> h = new HashMap<>(); - h.put("label", record.get("formattedName") + " " + record.get("fomattedDob") + " (" + record.get("status") + ")"); + String name = StringUtils.defaultString(record.get("formattedName")); + String dob = StringUtils.defaultString(record.get("fomattedDob")); + String status = StringUtils.defaultString(record.get("status")); + h.put("label", name + " " + dob + " (" + status + ")"); h.put("value", record.get("demographicNo")); h.put("providerNo", record.get("providerNo")); h.put("provider", record.get("providerName"));
66-67: Servlet API access via field initializers may fail at class-load time.
ServletActionContext.getRequest()andgetResponse()are initialized as instance fields. These rely on the Struts2 thread-localActionContextbeing set, which is only guaranteed during action invocation — not at construction time. If the container eagerly instantiates or pools this class, these will benullor throw. The conventional pattern is to callServletActionContext.getRequest()at the start ofexecute().This is a pre-existing issue and not introduced by this PR, so flagging for awareness only.
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
Code Review by Qodo
✅ 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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("formattedName", Encode.forHtml(demo.getFormattedName())); | ||
| h.put("demographicNo", String.valueOf(demo.getDemographicNo())); | ||
| h.put("status", demo.getPatientStatus()); | ||
| h.put("status", Encode.forHtml(demo.getPatientStatus())); |
There was a problem hiding this comment.
1. execute() lacks privilege check 📘 Rule violation ⛨ Security
• SearchDemographicAutoComplete2Action.execute() performs demographic search and returns patient/provider data without any SecurityInfoManager.hasPrivilege() authorization gate. • This allows the action to execute protected business logic without an explicit privilege check, which can enable unauthorized access if upstream protections are misconfigured or bypassed. • Add an early privilege check (before any DAO/search logic) and return an authorization error outcome when the privilege is missing.
Agent Prompt
## Issue description
`SearchDemographicAutoComplete2Action.execute()` performs demographic searches and returns patient/provider information without enforcing an explicit authorization check using `SecurityInfoManager.hasPrivilege()`.
## Issue Context
Compliance requires that every Struts2 `*2Action` entry point performs a privilege check *before* any protected business logic. This action currently proceeds directly into DAO search and response generation.
## Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[72-120]
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[171-181]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
|
||
| import org.owasp.encoder.Encode; | ||
| import org.apache.commons.text.StringEscapeUtils; |
There was a problem hiding this comment.
2. Unused import fails build 🐞 Bug ⛯ Reliability
• StringEscapeUtils is still imported but no longer referenced after the change away from escapeJava(...). • Checkstyle is configured to fail the build on UnusedImports, so this will block CI/merge.
Agent Prompt
### Issue description
`SearchDemographicAutoComplete2Action.java` still imports `org.apache.commons.text.StringEscapeUtils` but no longer uses it, causing Checkstyle `UnusedImports` to fail the build.
### Issue Context
The repository runs Checkstyle in `process-sources` with `failsOnError=true`, and `UnusedImports` is enabled.
### Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[41-46]
- utils/checkstyle.xml[93-97]
- pom.xml[1589-1645]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| 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.
3. Html encoding breaks autocomplete values 🐞 Bug ✓ Correctness
• The action now applies Encode.forHtml(...) to formattedName, status, and provider names before returning them in JSON. • Multiple UIs set these JSON fields directly into form inputs via .val(...); with the server now returning HTML entities (e.g., ', &), the input value becomes the encoded text, causing visible regressions and potential mismatches where raw values are expected. • If the goal is XSS prevention, the safer fix is contextual escaping at the HTML rendering sink (e.g., render with .text()/text nodes) or providing separate *Html fields, rather than mutating canonical data fields used as form values.
Agent Prompt
### Issue description
`SearchDemographicAutoComplete2Action` HTML-encodes strings before returning JSON. These strings are then reused as raw values in many autocomplete consumers via `.val(...)`, causing encoded entities to appear in inputs and changing semantics of the returned data.
### Issue Context
Some consumers render suggestions by concatenating strings into HTML (XSS sink), while also using the same fields as raw input values. Encoding should be applied at the HTML sink, not in the canonical JSON data fields.
### Fix Focus Areas
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[123-138]
- src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java[185-197]
- src/main/webapp/appointment/addappointment.jsp[570-589]
- src/main/webapp/documentManager/showDocument.js[23-44]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java`:
- Around line 75-76: In SearchDemographicAutoComplete2Action (method handling
the request) replace the early return of "noPrivilege" in the privilege check
(the block using securityInfoManager.hasPrivilege and loggedInInfo) with
throwing a SecurityException("missing required sec object") so the request is
definitively rejected and the exception propagates; ensure you do not catch and
swallow SecurityException elsewhere in this action per guidelines and keep the
change inside the privilege-check branch of
SearchDemographicAutoComplete2Action.
- Line 201: The label construction in SearchDemographicAutoComplete2Action uses
String concatenation with values from the map `record` which can be null and
result in the literal "null" in the UI; update the code that puts into `h` (the
line using h.put("label", Encode.forHtml(...))) to build the label null-safely
by calling a null-coalescing helper (e.g.,
Objects.toString(record.get("formattedName"), ""), or an inline ternary) for
`formattedName`, `fomattedDob`, and `status`, trim/omit empty parts and only add
surrounding punctuation (space, parentheses) when those parts are non-empty,
then pass the sanitized concatenated string to Encode.forHtml.
- Around line 72-79: In execute() of SearchDemographicAutoComplete2Action adjust
the privilege check: call SecurityInfoManager.hasPrivilege(loggedInInfo,
"_demographic", SecurityInfoManager.READ, null) (use the underscore-prefixed
object name and null for non-patient-specific demographic checks) and, instead
of returning the "noPrivilege" string, throw a SecurityException with a clear
message like "missing required sec object (_demographic)" when the check fails
so the error flow follows the security guidelines.
- Around line 197-210: In formatJSON, replace the unencoded fields so the JSON
emits the pre-encoded HTML variants: update the h.put("provider", ...) call to
use record.get("providerNameHtml") and update the h.put("formattedName", ...)
call to use record.get("formattedNameHtml"); these changes in the formatJSON
method ensure the JSON returns the existing OWASP-encoded values
(providerNameHtml and formattedNameHtml) instead of raw providerName and
formattedName.
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Outdated
Show resolved
Hide resolved
…fe jQuery rendering
|
Thanks for the contribution! There is a lot of legacy code that has areas to improve on, so the bots tend to be busy. Core contribution looks good, I am just reviewing now. |
|
@claude review this PR and all bot and reviewer comments, identify all valid identified issues and any unidentified issues and provide a review with recommendations. |
|
Claude Code is working… I'll analyze this and get back to you. |
2 participants
User description
This PR fixes XSS vulnerabilities in the SearchDemographicAutoComplete2Action endpoint by using Encode.forHtml() for escaping patient names, status, and provider names. It also refactors formatJSON() to use ObjectMapper to prevent JSON injection. Fixes #250
Description
Encode.forHtml()to escape user inputs, mitigating XSS vulnerabilities.formatJSON()method to utilizeObjectMapper, improving JSON handling and security.Changes walkthrough 📝
SearchDemographicAutoComplete2Action.java
Improve XSS and JSON injection prevention inSearchDemographicAutoComplete2Actionsrc/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java
Encode.forHtml()to escape patient names, statuses, and providernames to prevent XSS.
formatJSON()to useObjectMapperfor JSON serialization,enhancing security against JSON injection.
Summary by CodeRabbit