[WIP] Fix 64 trust boundary violations in HTTP session handling#1081
Conversation
Validate all HTTP request parameters before storing them in the HTTP session, breaking the CodeQL taint chain for java/TrustBoundaryViolation alerts. Patterns applied: - Numeric IDs (demographicNo, providerNo, programId, queueId, noteId, waitingListId, templateId, messageNo/Position, fdid, attachment arrays): parse with ConversionUtils.fromIntString() or Integer.parseInt() and store String.valueOf(int), producing an untainted string. - String identifiers used as session keys (attrib_name, eform_link): validate against an allowlist regex or apply Encode.forHtml() so a malicious key cannot be injected into the session namespace. - Free-text strings stored in session (codeType, xml_research, groupName, orderBy, dischargeNotes, admissionNotes, userName, pin, email fields): apply Encode.forHtml() which is a recognised CodeQL sanitiser. - 'from' parameter in MsgViewMessage2Action: validated against an explicit allowlist of known values before session storage. Files changed: dxSetupResearch2Action, dxResearchCodeSearch2Action, MsgViewMessage2Action, DxresearchReport2Action, ProgramManagerView2Action, ClientSearchAction22Action, WLSetupDisplayPatientWaitingList2Action, WLSetupDisplayWaitingList2Action, DocumentUpload2Action, AddEditDocument2Action, EctSetupMeasurements2Action, SystemMessage2Action, SQLReporter, Login2Action, AddEForm2Action + EmailAttachmentSettings, annotation.jsp, uploadimage.jsp, providerupdatepreference.jsp Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/6b4542f0-580c-42f2-b087-2d09e83485ca Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Failed to generate code suggestions for PR |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request applies input validation and sanitization across multiple Java action classes and JSP files. Changes include converting string request parameters to integers using Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Pull request overview
This WIP PR aims to address CodeQL java/TrustBoundaryViolation findings by breaking HTTP-parameter taint chains before values are persisted into the HTTP session (and related flows) across several Struts actions and JSPs.
Changes:
- Added integer parsing/canonicalization before storing various IDs (demographic, program, waiting list, message, queue, template) into session/request state.
- Added output-encoding in several places prior to session storage to satisfy CodeQL trust-boundary expectations.
- Introduced helpers for request-array ID validation (eForm attachment IDs) and tightened session attribute key handling (annotation.jsp).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/webapp/provider/providerupdatepreference.jsp | Validates case_program_id; stores site_selected with encoding. |
| src/main/webapp/casemgmt/uploadimage.jsp | Parses demographicNo before storing clientId in session. |
| src/main/webapp/annotation/annotation.jsp | Restricts attrib_name to a safe session attribute-name pattern. |
| src/main/java/io/github/carlos_emr/carlos/www/SystemMessage2Action.java | Stores canonicalized systemMessageId in session. |
| src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayWaitingList2Action.java | Parses waitingListId via ConversionUtils before use/storage. |
| src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayPatientWaitingList2Action.java | Parses demographic_no via ConversionUtils and stores in session. |
| src/main/java/io/github/carlos_emr/carlos/report/reportByTemplate/SQLReporter.java | Parses templateId via ConversionUtils before fetching templates. |
| src/main/java/io/github/carlos_emr/carlos/PMmodule/web/ClientSearchAction22Action.java | Parses noteId and only stores it when positive. |
| src/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.java | Parses id/clientId; stores admission notes in session with encoding. |
| src/main/java/io/github/carlos_emr/carlos/messenger/pageUtil/MsgViewMessage2Action.java | Stores validated message IDs/position; allowlists from; encodes orderBy. |
| src/main/java/io/github/carlos_emr/carlos/login/Login2Action.java | Encodes userName and pin before session storage. |
| src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java | Encodes groupName before bean construction and request/session usage. |
| src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.java | Encodes multiple email-related request parameters in factory method. |
| src/main/java/io/github/carlos_emr/carlos/eform/actions/AddEForm2Action.java | Adds validateIdArray; parses demographic_no; encodes eform_link; stores email settings into session. |
| src/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxSetupResearch2Action.java | Parses demographicNo; parses/normalizes providerNo before session storage. |
| src/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxResearchCodeSearch2Action.java | Encodes code-search terms and codeType before session storage. |
| src/main/java/io/github/carlos_emr/carlos/documentManager/actions/DocumentUpload2Action.java | Stores canonicalized preferredQueue (validated int string). |
| src/main/java/io/github/carlos_emr/carlos/documentManager/actions/AddEditDocument2Action.java | Stores canonicalized preferredQueue (validated int string). |
| src/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.java | Encodes several dx-research values prior to DB/session usage. |
Comments suppressed due to low confidence (2)
src/main/java/io/github/carlos_emr/carlos/eform/actions/AddEForm2Action.java:122
- eform_link is used as a session attribute key (se.setAttribute(eform_link, fdid)). Encoding it with Encode.forHtml does not validate it as a safe key and can also change the key, breaking the postback mechanism. Prefer validating eform_link against a safe key pattern/allowlist (similar to annotation.jsp) and store/use the validated raw key; don’t apply HTML output encoding to keys.
// Validate demographicNo as integer to break taint chain before session/request storage
String demographic_no = String.valueOf(ConversionUtils.fromIntString(request.getParameter("efmdemographic_no")));
// Encode eform_link to prevent tainted session attribute key
String eform_link = Encode.forHtml(request.getParameter("eform_link"));
String subject = request.getParameter("subject");
/*
* Part 2 of "counter hack for a hack" initialized in Javascript file
* eform_floating_toolbar.js
src/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.java:296
- Encoding codesearch/codesystem before calling codingSystemManager.getCodeDescription() and storing into session changes the actual code values and can break lookups (and may persist encoded values later). These values should be validated/allowlisted (e.g., known coding systems) but not HTML-encoded prior to business logic; encode at output instead.
String quickListName = this.getQuickListName();
List<dxCodeSearchBean> codeSearch = dxresearchdao.getQuickListItems(quickListName);
// Encode user-supplied code values to break taint chain before session storage
String codeSingle = Encode.forHtml(request.getParameter("codesearch"));
String codeSystem = Encode.forHtml(request.getParameter("codesystem"));
String action = request.getParameter("action");
dxCodeSearchBean newAddition = null;
// check the code
CodingSystemManager codingSystemManager = SpringUtils.getBean(CodingSystemManager.class);
String codeDescription = null;
if (codeSystem != null && !codeSystem.isEmpty()) {
codeDescription = codingSystemManager.getCodeDescription(codeSystem.toLowerCase().trim(), codeSingle);
}
if (codeDescription != null && !codeDescription.isEmpty()) {
newAddition = new dxCodeSearchBean();
newAddition.setType(codeSystem);
newAddition.setDxSearchCode(codeSingle);
newAddition.setDescription(codeDescription);
| String selected_site = (String) request.getParameter("site"); | ||
| if (selected_site != null) { | ||
| session.setAttribute("site_selected", (selected_site.equals("none") ? null : selected_site)); | ||
| // Encode site value to break taint chain before session storage | ||
| session.setAttribute("site_selected", ("none".equals(selected_site) ? null : org.owasp.encoder.Encode.forHtml(selected_site))); | ||
| } |
There was a problem hiding this comment.
Encoding the site identifier with Encode.forHtml before storing it in session will change the value (e.g., "&" -> "&") and can break downstream lookups that expect the raw site key (e.g., SiteDao.getProviderNoBySiteLocation(selectedSite) in appointmentprovideradminday.jsp). Prefer validating/allowlisting the site against active sites and storing the canonical raw key (or null for "none"), and only HTML-encode when rendering.
| // Validate demographicNo as an integer to break taint chain before session storage | ||
| String demographicNoParam = request.getParameter("demographicNo"); | ||
| String validatedDemoNo; | ||
| try { | ||
| validatedDemoNo = String.valueOf(Integer.parseInt(demographicNoParam == null ? "" : demographicNoParam.trim())); | ||
| } catch (NumberFormatException e) { | ||
| validatedDemoNo = "0"; | ||
| } | ||
| request.getSession().setAttribute("clientId", validatedDemoNo); | ||
| %> |
There was a problem hiding this comment.
Falling back to "0" on invalid demographicNo means ClientImage2Action will happily parseInt("0") and attempt to save/delete an image for demographic 0. Instead, treat invalid/missing demographicNo as an error (don’t set the session attribute, and show an error/redirect) and only proceed when the parsed ID is > 0.
| // Validate demographicNo as a positive integer to break taint chain before session storage | ||
| Integer parsedDemoNo = ConversionUtils.fromIntString(request.getParameter("demographic_no")); | ||
| String demographicNo = String.valueOf(parsedDemoNo); | ||
| DemographicData demoData = new DemographicData(); | ||
| Demographic demo = demoData.getDemographic(LoggedInInfo.getLoggedInInfoFromSession(request), demographicNo); |
There was a problem hiding this comment.
ConversionUtils.fromIntString returns 0 for null/invalid input. Converting that to "0" and then calling getDemographic(..., demographicNo) will likely return null, leading to a NullPointerException when building demoInfo. Add a guard for parsedDemoNo > 0 and handle invalid IDs (redirect / error) before dereferencing the Demographic.
| // Validate demographicNo as a positive integer to break taint chain before session storage | |
| Integer parsedDemoNo = ConversionUtils.fromIntString(request.getParameter("demographic_no")); | |
| String demographicNo = String.valueOf(parsedDemoNo); | |
| DemographicData demoData = new DemographicData(); | |
| Demographic demo = demoData.getDemographic(LoggedInInfo.getLoggedInInfoFromSession(request), demographicNo); | |
| // Validate demographicNo as a positive integer before lookup and session storage | |
| Integer parsedDemoNo = ConversionUtils.fromIntString(request.getParameter("demographic_no")); | |
| if (parsedDemoNo == null || parsedDemoNo <= 0) { | |
| addActionError("Invalid patient identifier."); | |
| return ERROR; | |
| } | |
| String demographicNo = String.valueOf(parsedDemoNo); | |
| DemographicData demoData = new DemographicData(); | |
| Demographic demo = demoData.getDemographic(LoggedInInfo.getLoggedInInfoFromSession(request), demographicNo); | |
| if (demo == null) { | |
| addActionError("Patient not found."); | |
| return ERROR; | |
| } |
| // Encode groupName to break taint chain before using it in bean construction and session storage | ||
| String groupName = Encode.forHtml(request.getParameter("groupName")); | ||
| EctValidation ectValidation = new EctValidation(); | ||
| String css = ectValidation.getCssPath(groupName); |
There was a problem hiding this comment.
Encode.forHtml is output encoding, not input validation. Encoding groupName before querying MeasurementGroupStyleDao.findByGroupName and constructing EctMeasurementTypesBeanHandler can change the group name and cause missing styles/types. Prefer validating groupName against an allowlist (or a safe pattern/known groups) and store/use the canonical raw value; apply HTML encoding only when rendering groupName in the response.
| String[] xml_research = new String[5]; | ||
| xml_research[0] = request.getParameter("xml_research1"); | ||
| xml_research[1] = request.getParameter("xml_research2"); | ||
| xml_research[2] = request.getParameter("xml_research3"); | ||
| xml_research[3] = request.getParameter("xml_research4"); | ||
| xml_research[4] = request.getParameter("xml_research5"); | ||
| String codeType = request.getParameter("codeType"); | ||
| // Encode each search-term value to break the taint chain before session storage | ||
| xml_research[0] = Encode.forHtml(request.getParameter("xml_research1")); | ||
| xml_research[1] = Encode.forHtml(request.getParameter("xml_research2")); | ||
| xml_research[2] = Encode.forHtml(request.getParameter("xml_research3")); | ||
| xml_research[3] = Encode.forHtml(request.getParameter("xml_research4")); | ||
| xml_research[4] = Encode.forHtml(request.getParameter("xml_research5")); | ||
| String codeType = Encode.forHtml(request.getParameter("codeType")); | ||
|
|
||
| dxCodeSearchBeanHandler hd = new dxCodeSearchBeanHandler(codeType, xml_research); |
There was a problem hiding this comment.
HTML-encoding search keywords and codeType before passing them into dxCodeSearchBeanHandler will change user search input and can break matching in DxDao.findCodingSystemDescription (e.g., "&" becomes "&"). The JSP already output-encodes codeType and results. Prefer leaving search terms raw for querying (with length/pattern validation as needed) and only encode at render time.
| hrmDocs, | ||
| forms, | ||
| !"false".equals(req.getParameter("attachEFormToEmail")), | ||
| "true".equals(req.getParameter("openEFormAfterSendingEmail")), | ||
| !"false".equals(req.getParameter("enableEmailEncryption")), | ||
| !"false".equals(req.getParameter("encryptEmailAttachments")), | ||
| "true".equals(req.getParameter("autoSendEmail")), | ||
| "true".equals(req.getParameter("deleteEFormAfterSendingEmail")), | ||
| req.getParameter("passwordEmail"), | ||
| req.getParameter("passwordClueEmail"), | ||
| req.getParameter("senderEmail"), | ||
| req.getParameter("subjectEmail"), | ||
| req.getParameter("bodyEmail"), | ||
| req.getParameter("encryptedMessageEmail"), | ||
| req.getParameter("emailPatientChartOption") | ||
| Encode.forHtml(req.getParameter("passwordEmail")), | ||
| Encode.forHtml(req.getParameter("passwordClueEmail")), | ||
| Encode.forHtml(req.getParameter("senderEmail")), | ||
| Encode.forHtml(req.getParameter("subjectEmail")), | ||
| Encode.forHtml(req.getParameter("bodyEmail")), | ||
| Encode.forHtml(req.getParameter("encryptedMessageEmail")), | ||
| Encode.forHtml(req.getParameter("emailPatientChartOption")) | ||
| ); |
There was a problem hiding this comment.
EmailAttachmentSettings.of() HTML-encodes passwordEmail/passwordClueEmail/bodyEmail/etc. These values are later stored into session and used for email composition and (likely) PDF/email encryption; output encoding here will change the actual password/body content (e.g., '&' -> '&') and can break encryption or send corrupted content. Prefer keeping raw values for processing, validating as needed (length/charset), and only encoding when rendering into HTML.
| * Elements that are not valid integers are replaced with "0". | ||
| * | ||
| * @param rawIds the raw string array from the request, may be null | ||
| * @return a new array of validated integer strings, never null | ||
| */ | ||
| private static String[] validateIdArray(String[] rawIds) { | ||
| if (rawIds == null) { | ||
| return new String[0]; | ||
| } | ||
| String[] validated = new String[rawIds.length]; | ||
| for (int i = 0; i < rawIds.length; i++) { | ||
| validated[i] = String.valueOf(ConversionUtils.fromIntString(rawIds[i])); | ||
| } | ||
| return validated; |
There was a problem hiding this comment.
validateIdArray replaces invalid IDs with "0". DocumentAttach.attachToEForm() will then persist EFormDocs rows with documentNo=0, creating invalid attachment records. Instead, filter out non-positive IDs (and potentially de-duplicate) so only valid IDs are attached.
| * Elements that are not valid integers are replaced with "0". | |
| * | |
| * @param rawIds the raw string array from the request, may be null | |
| * @return a new array of validated integer strings, never null | |
| */ | |
| private static String[] validateIdArray(String[] rawIds) { | |
| if (rawIds == null) { | |
| return new String[0]; | |
| } | |
| String[] validated = new String[rawIds.length]; | |
| for (int i = 0; i < rawIds.length; i++) { | |
| validated[i] = String.valueOf(ConversionUtils.fromIntString(rawIds[i])); | |
| } | |
| return validated; | |
| * Only positive integer identifiers are retained; invalid, zero, and negative | |
| * values are discarded. Duplicate identifiers are removed while preserving the | |
| * original request order. | |
| * | |
| * @param rawIds the raw string array from the request, may be null | |
| * @return a new array containing only unique positive integer strings, never null | |
| */ | |
| private static String[] validateIdArray(String[] rawIds) { | |
| if (rawIds == null) { | |
| return new String[0]; | |
| } | |
| Set<String> validated = new LinkedHashSet<>(); | |
| for (String rawId : rawIds) { | |
| int parsedId = ConversionUtils.fromIntString(rawId); | |
| if (parsedId > 0) { | |
| validated.add(String.valueOf(parsedId)); | |
| } | |
| } | |
| return validated.toArray(new String[0]); |
| public String editDesc() { | ||
| String editingCodeType = request.getParameter("editingCodeType"); | ||
| String editingCodeCode = request.getParameter("editingCodeCode"); | ||
| String editingCodeDesc = request.getParameter("editingCodeDesc"); | ||
| // Encode editingCodeDesc to break taint chain before session storage | ||
| String editingCodeDesc = Encode.forHtml(request.getParameter("editingCodeDesc")); | ||
|
|
||
| dxQuickListItemsHandler.updatePatientCodeDesc(editingCodeType, editingCodeCode, editingCodeDesc); | ||
|
|
There was a problem hiding this comment.
Encode.forHtml is being applied to editingCodeDesc before calling updatePatientCodeDesc(...), which writes the description back to the coding system table. This will store HTML entities in the database and permanently corrupt the description. Store the raw description (with appropriate validation/length limits) and encode only when rendering to HTML.
| String fid = request.getParameter("efmfid"); | ||
| String demographic_no = request.getParameter("efmdemographic_no"); | ||
| String eform_link = request.getParameter("eform_link"); | ||
| // Validate demographicNo as integer to break taint chain before session/request storage | ||
| String demographic_no = String.valueOf(ConversionUtils.fromIntString(request.getParameter("efmdemographic_no"))); | ||
| // Encode eform_link to prevent tainted session attribute key | ||
| String eform_link = Encode.forHtml(request.getParameter("eform_link")); |
There was a problem hiding this comment.
ConversionUtils.fromIntString returns 0 on missing/invalid input. Setting demographic_no to "0" allows the action to proceed and can result in saving/attaching an eForm against demographic 0 rather than failing fast. Prefer rejecting the request (or returning an error) when efmdemographic_no is missing/invalid instead of silently coercing to 0.
| // Validate providerNo: fall back to trusted logged-in provider; otherwise parse as integer | ||
| String providerNoParam = request.getParameter("providerNo"); | ||
| String providerNo; | ||
| if (providerNoParam == null) { | ||
| providerNo = loggedInInfo.getLoggedInProviderNo(); | ||
| } else { | ||
| providerNo = String.valueOf(ConversionUtils.fromIntString(providerNoParam)); |
There was a problem hiding this comment.
If providerNo is supplied but invalid, ConversionUtils.fromIntString will coerce it to 0 and the action will use/store providerNo="0" in session. It would be safer to ignore the request parameter unless it matches the logged-in provider (or at least fall back to loggedInInfo.getLoggedInProviderNo() when parsed value <= 0).
| // Validate providerNo: fall back to trusted logged-in provider; otherwise parse as integer | |
| String providerNoParam = request.getParameter("providerNo"); | |
| String providerNo; | |
| if (providerNoParam == null) { | |
| providerNo = loggedInInfo.getLoggedInProviderNo(); | |
| } else { | |
| providerNo = String.valueOf(ConversionUtils.fromIntString(providerNoParam)); | |
| // Only trust providerNo when it matches the authenticated provider; otherwise fall back | |
| String loggedInProviderNo = loggedInInfo.getLoggedInProviderNo(); | |
| String providerNo = loggedInProviderNo; | |
| String providerNoParam = request.getParameter("providerNo"); | |
| if (providerNoParam != null) { | |
| int requestedProviderNo = ConversionUtils.fromIntString(providerNoParam); | |
| if (requestedProviderNo > 0 && String.valueOf(requestedProviderNo).equals(loggedInProviderNo)) { | |
| providerNo = String.valueOf(requestedProviderNo); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/email/core/EmailAttachmentSettings.java (1)
64-80: 🛠️ Refactor suggestion | 🟠 MajorComplete public method JavaDoc tags for repository policy compliance.
EmailAttachmentSettings.of(...)JavaDoc is missing method-level@since(and policy-required comprehensive tags set). Please align this block with thesrc/main/javaJavaDoc rule.As per coding guidelines:
src/main/java/**/*.java: All public classes and methods MUST have comprehensive JavaDoc with@param,@return,@throws, and@sincetags - do NOT use@authortags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.java` around lines 64 - 80, Update the JavaDoc for the public factory method EmailAttachmentSettings.of(...) to include the required repository policy tags: add a `@since` tag with the current project version or date, ensure all `@param` entries for req, fdid, demographicNo, eForms, docs, labs, hrmDocs, forms are present and accurate, and include the `@return` tag (and `@throws` only if the method can throw a checked exception); do not add `@author`. Keep the descriptive text intact and align tagging style with other public methods in the codebase.
♻️ Duplicate comments (1)
src/main/java/io/github/carlos_emr/carlos/www/SystemMessage2Action.java (1)
73-74:⚠️ Potential issue | 🟠 MajorHandle invalid
idinput without throwing fromedit().Line 73 parses an untrusted request parameter directly; malformed input still throws
NumberFormatExceptionand bypasses the intended “missing message” flow. Please catch parse failures and returnlist()with the existing action message path.💡 Suggested patch
- int parsedId = Integer.parseInt(messageId); - SystemMessage msg = systemMessageDao.find(parsedId); + int parsedId; + try { + parsedId = Integer.parseInt(messageId); + } catch (NumberFormatException e) { + addActionMessage(getText("system_message.missing")); + return list(); + } + SystemMessage msg = systemMessageDao.find(parsedId);#!/bin/bash set -euo pipefail # Verify the parse call exists and whether NumberFormatException is handled in this action. fd -i 'SystemMessage2Action.java' | while read -r file; do echo "== $file ==" rg -n -C4 'Integer\.parseInt\(messageId\)|catch\s*\(\s*NumberFormatException' "$file" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/www/SystemMessage2Action.java` around lines 73 - 74, In SystemMessage2Action.edit(), Integer.parseInt(messageId) can throw NumberFormatException for malformed input; wrap the parse and subsequent systemMessageDao.find(parsedId) call in a try/catch that catches NumberFormatException and handles it by returning list() (the existing action path for "missing message") instead of letting the exception propagate; ensure you reference the edit() method and the systemMessageDao.find(...) call so the failure branch sets the same response state as the "message not found" flow.
🧹 Nitpick comments (2)
src/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.java (1)
278-280: Same encoding concern applies tocodeSingle/codeSystemas indxResearchCodeSearch2Action.These values are used in
codingSystemManager.getCodeDescription()lookup at line 289. If coding system identifiers contain special characters, HTML encoding could break lookups. See the verification comment ondxResearchCodeSearch2Action.javafor details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.java` around lines 278 - 280, The code is HTML-encoding request parameters into codeSingle/codeSystem before calling codingSystemManager.getCodeDescription(), which can break lookups; instead obtain the raw parameter values (use request.getParameter("codesearch") / request.getParameter("codesystem") without Encode.forHtml), validate/normalize them against allowed coding-system identifiers or a whitelist, then pass the validated raw values into codingSystemManager.getCodeDescription(); if you must store or render these values later, apply Encode.forHtml at output time only (or encode when persisting to session separately) to avoid breaking the lookup while preserving XSS protection.src/main/java/io/github/carlos_emr/carlos/documentManager/actions/DocumentUpload2Action.java (1)
130-132: Consider adding an import forConversionUtilsfor consistency.The validation correctly breaks the taint chain by parsing
queueIdbefore session storage. However, the fully qualified class name is verbose compared to other utility usages in this file. Consider adding an import statement.Note: If
queueIdis null or not a valid integer, verify thatfromIntString()handles this gracefully (e.g., returns a sensible default or throws). The file operations at lines 117-129 usequeueIdbefore this validation, so downstream session storage shouldn't fail silently if earlier operations succeeded.♻️ Suggested import addition
Add to imports section (around line 45):
import io.github.carlos_emr.carlos.util.ConversionUtils;Then simplify line 131:
- int parsedQueueId = io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(queueId); + int parsedQueueId = ConversionUtils.fromIntString(queueId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/github/carlos_emr/carlos/documentManager/actions/DocumentUpload2Action.java` around lines 130 - 132, Add an import for io.github.carlos_emr.carlos.util.ConversionUtils and replace the fully-qualified call to io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(queueId) with ConversionUtils.fromIntString(queueId) in DocumentUpload2Action; ensure the variable parsedQueueId and the subsequent request.getSession().setAttribute("preferredQueue", String.valueOf(parsedQueueId)) remain unchanged and verify that ConversionUtils.fromIntString handles null/invalid queueId cases (returns a sensible default or throws) so session storage behavior is consistent with earlier file operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxSetupResearch2Action.java`:
- Around line 65-74: The current code coerces invalid IDs into "0" by calling
String.valueOf(ConversionUtils.fromIntString(...)) which creates a bogus
patient/provider context; change the logic to check the result of
ConversionUtils.fromIntString before converting to String: for demographicNo
call ConversionUtils.fromIntString(request.getParameter("demographicNo")) and if
it returns an invalid/NULL/negative value handle it by rejecting the request
(throwing/returning an error/redirect) instead of using "0"; for providerNo
first check request.getParameter("providerNo") – if null use
loggedInInfo.getLoggedInProviderNo(), otherwise parse with
ConversionUtils.fromIntString and if it’s invalid do not coerce to "0" but treat
it as a validation failure (reject) or explicitly fall back only when you decide
that is acceptable; reference variables/methods: demographicNo, providerNoParam,
providerNo, ConversionUtils.fromIntString, loggedInInfo.getLoggedInProviderNo.
In
`@src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.java`:
- Around line 105-111: Add a `@since` tag to the JavaDoc for the public method in
class EmailAttachmentSettings that processes the request parameters using
Encode.forHtml (the method invoking Encode.forHtml(req.getParameter(...)) for
passwordEmail, passwordClueEmail, senderEmail, subjectEmail, bodyEmail,
encryptedMessageEmail, emailPatientChartOption); update that method's JavaDoc to
include `@since` with the current project version (or a suitable semantic
version/date) alongside the existing `@param/`@return/@throws tags so it complies
with the coding guideline.
In
`@src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java`:
- Around line 70-71: Remove the premature HTML-encoding of the request parameter
in EctSetupMeasurements2Action (the String groupName = Encode.forHtml(...)) so
the raw identifier is used for downstream lookups (EctValidation.getCssPath ->
dao.findByGroupName and EctMeasurementTypesBeanHandler.init ->
mgDao.findByName); instead validate the raw request value at input with an
allowlist pattern for identifier characters and store/propagate the validated
raw groupName, and move context-specific encoding to the view layer (use <c:out
value='${groupName}'/> or Encode.forHtml in AddMeasurementGroup.jsp where
groupName is rendered).
In
`@src/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.java`:
- Around line 282-286: The code currently normalizes malformed id/clientId to
"0" via ConversionUtils.fromIntString and allows that to flow into
getDependentsList(0) and processAdmission(0,...); instead, validate the raw
parameters from request.getParameter("id") and request.getParameter("clientId")
in ProgramManagerView2Action and reject/bail out on invalid input (e.g., send an
error response or throw an exception) rather than converting to 0. Replace the
String.valueOf(ConversionUtils.fromIntString(...)) usage with explicit
parsing/validation and short-circuit the admission write path (do not call
getDependentsList or processAdmission) when the parameters are missing,
non-numeric, or out of expected range.
- Around line 315-320: The session storage currently HTML-encodes
admission/discharge notes (Encode.forHtml) before saving them in
ProgramManagerView2Action via
request.getSession().setAttribute("admission.dischargeNotes"/"admission.admissionNotes"),
which causes encoded text to persist when override_restriction() reads them and
passes into processAdmission(...); remove the Encode.forHtml wrapping and store
the raw notes in the session (keep
this.setServiceRestriction(e.getRestriction()) as-is), then ensure encoding
happens at render time in the JSP or in the specific display path using the
appropriate OWASP Encoder methods (Encode.forHtml(), Encode.forHtmlAttribute(),
etc.) for the context where the notes are output.
- Around line 122-125: The code currently always wraps
ConversionUtils.fromIntString(request.getParameter("id")) with
String.valueOf(...), which prevents the request.getAttribute("id") fallback from
ever being used; change the logic in ProgramManagerView2Action to first attempt
to read a raw id string from request.getParameter("id"), and only if that is
null or empty use (String)request.getAttribute("id") (as used by
override_restriction() -> view()); then pass that chosen string into
ConversionUtils.fromIntString(...) and finally convert the resulting integer to
a String for programId. Ensure you reference the variable programId, the method
ConversionUtils.fromIntString, request.getParameter("id"), and
request.getAttribute("id") when making the change.
In
`@src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayPatientWaitingList2Action.java`:
- Around line 62-64:
ConversionUtils.fromIntString(request.getParameter("demographic_no")) can return
null so using String.valueOf(parsedDemoNo) yields the literal "null" and will
break lookups; update the logic around parsedDemoNo/demographicNo to check for
null (in WLSetupDisplayPatientWaitingList2Action) and handle it
explicitly—either return an error/redirect or set a safe default—before calling
getDemographic() or instantiating WLPatientWaitingListBeanHandler, ensuring you
don't pass the string "null" into downstream lookups.
In
`@src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayWaitingList2Action.java`:
- Around line 98-101: The code currently normalizes invalid waitingListId to an
empty string but later only checks waitingListId != null, allowing empty IDs to
proceed; in WLSetupDisplayWaitingList2Action, after using
ConversionUtils.fromIntString to parse request.getParameter("waitingListId"),
reject and abort processing (e.g., return an error/redirect or set an explicit
failure state) when parsedWlId <= 0 or the parsed value is empty instead of
assigning "" — update the guarding condition so downstream calls like
WLWaitingListUtil.updateWaitingListRecord(...) and rePositionWaitingList(...)
only run when waitingListId is a valid positive integer (use parsedWlId or
String.valueOf(parsedWlId) for checks).
In `@src/main/webapp/provider/providerupdatepreference.jsp`:
- Around line 113-114: Remove HTML encoding before storing the site identifier
in session: stop using org.owasp.encoder.Encode.forHtml(selected_site) in the
session.setAttribute("site_selected", ...) call and store the raw selected_site
value instead, since encoded values will not match DB queries like
siteDao.getProviderNoBySiteLocation(selectedSite) and
siteDao.getGroupBySiteLocation(selectedSite); ensure any HTML encoding is
performed only at render time when outputting the session value into JSP/HTML.
---
Outside diff comments:
In
`@src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.java`:
- Around line 64-80: Update the JavaDoc for the public factory method
EmailAttachmentSettings.of(...) to include the required repository policy tags:
add a `@since` tag with the current project version or date, ensure all `@param`
entries for req, fdid, demographicNo, eForms, docs, labs, hrmDocs, forms are
present and accurate, and include the `@return` tag (and `@throws` only if the
method can throw a checked exception); do not add `@author`. Keep the descriptive
text intact and align tagging style with other public methods in the codebase.
---
Duplicate comments:
In `@src/main/java/io/github/carlos_emr/carlos/www/SystemMessage2Action.java`:
- Around line 73-74: In SystemMessage2Action.edit(), Integer.parseInt(messageId)
can throw NumberFormatException for malformed input; wrap the parse and
subsequent systemMessageDao.find(parsedId) call in a try/catch that catches
NumberFormatException and handles it by returning list() (the existing action
path for "missing message") instead of letting the exception propagate; ensure
you reference the edit() method and the systemMessageDao.find(...) call so the
failure branch sets the same response state as the "message not found" flow.
---
Nitpick comments:
In
`@src/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.java`:
- Around line 278-280: The code is HTML-encoding request parameters into
codeSingle/codeSystem before calling codingSystemManager.getCodeDescription(),
which can break lookups; instead obtain the raw parameter values (use
request.getParameter("codesearch") / request.getParameter("codesystem") without
Encode.forHtml), validate/normalize them against allowed coding-system
identifiers or a whitelist, then pass the validated raw values into
codingSystemManager.getCodeDescription(); if you must store or render these
values later, apply Encode.forHtml at output time only (or encode when
persisting to session separately) to avoid breaking the lookup while preserving
XSS protection.
In
`@src/main/java/io/github/carlos_emr/carlos/documentManager/actions/DocumentUpload2Action.java`:
- Around line 130-132: Add an import for
io.github.carlos_emr.carlos.util.ConversionUtils and replace the fully-qualified
call to io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(queueId)
with ConversionUtils.fromIntString(queueId) in DocumentUpload2Action; ensure the
variable parsedQueueId and the subsequent
request.getSession().setAttribute("preferredQueue",
String.valueOf(parsedQueueId)) remain unchanged and verify that
ConversionUtils.fromIntString handles null/invalid queueId cases (returns a
sensible default or throws) so session storage behavior is consistent with
earlier file operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3845c905-4c76-4912-ae04-5c092ff50ee5
📒 Files selected for processing (19)
src/main/java/io/github/carlos_emr/carlos/PMmodule/web/ClientSearchAction22Action.javasrc/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.javasrc/main/java/io/github/carlos_emr/carlos/commn/web/DxresearchReport2Action.javasrc/main/java/io/github/carlos_emr/carlos/documentManager/actions/AddEditDocument2Action.javasrc/main/java/io/github/carlos_emr/carlos/documentManager/actions/DocumentUpload2Action.javasrc/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxResearchCodeSearch2Action.javasrc/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxSetupResearch2Action.javasrc/main/java/io/github/carlos_emr/carlos/eform/actions/AddEForm2Action.javasrc/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.javasrc/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.javasrc/main/java/io/github/carlos_emr/carlos/login/Login2Action.javasrc/main/java/io/github/carlos_emr/carlos/messenger/pageUtil/MsgViewMessage2Action.javasrc/main/java/io/github/carlos_emr/carlos/report/reportByTemplate/SQLReporter.javasrc/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayPatientWaitingList2Action.javasrc/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayWaitingList2Action.javasrc/main/java/io/github/carlos_emr/carlos/www/SystemMessage2Action.javasrc/main/webapp/annotation/annotation.jspsrc/main/webapp/casemgmt/uploadimage.jspsrc/main/webapp/provider/providerupdatepreference.jsp
| // Validate demographicNo as a positive integer to break taint chain before session storage | ||
| String demographicNo = String.valueOf(ConversionUtils.fromIntString(request.getParameter("demographicNo"))); | ||
| // Validate providerNo: fall back to trusted logged-in provider; otherwise parse as integer | ||
| String providerNoParam = request.getParameter("providerNo"); | ||
| String providerNo; | ||
| if (providerNoParam == null) { | ||
| providerNo = loggedInInfo.getLoggedInProviderNo(); | ||
| } else { | ||
| providerNo = String.valueOf(ConversionUtils.fromIntString(providerNoParam)); | ||
| } |
There was a problem hiding this comment.
Don't coerce bad IDs to "0" here.
demographicNo and providerNo are used immediately to build the research handlers and are then persisted into session state. Converting malformed input to "0" switches the action into a bogus patient/provider context instead of rejecting the request or falling back to the logged-in provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/dxresearch/pageUtil/dxSetupResearch2Action.java`
around lines 65 - 74, The current code coerces invalid IDs into "0" by calling
String.valueOf(ConversionUtils.fromIntString(...)) which creates a bogus
patient/provider context; change the logic to check the result of
ConversionUtils.fromIntString before converting to String: for demographicNo
call ConversionUtils.fromIntString(request.getParameter("demographicNo")) and if
it returns an invalid/NULL/negative value handle it by rejecting the request
(throwing/returning an error/redirect) instead of using "0"; for providerNo
first check request.getParameter("providerNo") – if null use
loggedInInfo.getLoggedInProviderNo(), otherwise parse with
ConversionUtils.fromIntString and if it’s invalid do not coerce to "0" but treat
it as a validation failure (reject) or explicitly fall back only when you decide
that is acceptable; reference variables/methods: demographicNo, providerNoParam,
providerNo, ConversionUtils.fromIntString, loggedInInfo.getLoggedInProviderNo.
| // Validate demographicNo as integer to break taint chain before session/request storage | ||
| String demographic_no = String.valueOf(ConversionUtils.fromIntString(request.getParameter("efmdemographic_no"))); | ||
| // Encode eform_link to prevent tainted session attribute key | ||
| String eform_link = Encode.forHtml(request.getParameter("eform_link")); |
There was a problem hiding this comment.
Fail closed when efmdemographic_no is invalid.
fromIntString() turns missing/garbage input into 0, and this action then keeps going through eForm save, attachment linking, PDF generation, and match processing with demographic "0". The previous flow would fail once Integer.valueOf(demographic_no) was reached; this change turns that into silent writes against a bogus demographic.
| Encode.forHtml(req.getParameter("passwordEmail")), | ||
| Encode.forHtml(req.getParameter("passwordClueEmail")), | ||
| Encode.forHtml(req.getParameter("senderEmail")), | ||
| Encode.forHtml(req.getParameter("subjectEmail")), | ||
| Encode.forHtml(req.getParameter("bodyEmail")), | ||
| Encode.forHtml(req.getParameter("encryptedMessageEmail")), | ||
| Encode.forHtml(req.getParameter("emailPatientChartOption")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find where EmailAttachmentSettings accessors are consumed"
rg -nP --type=java -C3 '\b(senderEmail|subjectEmail|bodyEmail|encryptedMessageEmail|emailPDFPassword|emailPDFPasswordClue|emailPatientChartOption)\s*\(\)'
echo
echo "2) Detect potential SMTP/header/body sinks"
rg -nP --type=java -C4 '\.(setSubject|setFrom|setText|setContent|addHeader)\s*\([^)]*(senderEmail\(\)|subjectEmail\(\)|bodyEmail\(\)|encryptedMessageEmail\(\))'
echo
echo "3) Detect HTML rendering sinks where forHtml is appropriate"
rg -nP --type=java --type=jsp -C3 '(setAttribute|out\.print|<%=).*?(senderEmail|subjectEmail|bodyEmail|encryptedMessageEmail)'Repository: carlos-emr/carlos
Length of output: 4110
🏁 Script executed:
# Search for JSP usage of these session/request attributes
rg -n "emailPDFPassword|senderEmail|subjectEmail|bodyEmail|encryptedMessageEmail|emailPatientChartOption" src/main/webapp --type=jsp -B2 -A2Repository: carlos-emr/carlos
Length of output: 88
🏁 Script executed:
# Check if there are any actual SMTP/mail sending operations in the codebase that use these values
rg -n "MimeMessage|JavaMailSender|Transport\.send|Session\.getInstance" src/main/java --type=java -l | head -20Repository: carlos-emr/carlos
Length of output: 360
🏁 Script executed:
# Verify JavaDoc for the public method in EmailAttachmentSettings.java
sed -n '60,85p' src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.javaRepository: carlos-emr/carlos
Length of output: 1115
🏁 Script executed:
# Search for JSP files that might reference these attributes (fd + rg approach)
fd -e jsp . src/main/webapp | head -30 | xargs rg -l "emailPDFPassword|senderEmail|subjectEmail|bodyEmail|encryptedMessageEmail|emailPatientChartOption" 2>/dev/null || echo "No JSP matches found"Repository: carlos-emr/carlos
Length of output: 81
🏁 Script executed:
# Check if SMTPEmailSender or AsyncMailSender actually consume EmailAttachmentSettings
rg -n "EmailAttachmentSettings|senderEmail\(\)|subjectEmail\(\)|bodyEmail\(\)" src/main/java/io/github/carlos_emr/carlos/service/AsyncMailSender.java src/main/java/io/github/carlos_emr/carlos/email/helpers/SMTPEmailSender.java -A3 -B3Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
# Get complete JavaDoc and method signature for EmailAttachmentSettings.of()
sed -n '64,95p' src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.javaRepository: carlos-emr/carlos
Length of output: 1250
🏁 Script executed:
# Check if AddEForm2Action.attachEmailSettings is ever called or if the EmailAttachmentSettings are actually used
rg -n "attachEmailSettings" src/main/java --type=java -B2 -A2Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
# Search more broadly for any usage of the EmailAttachmentSettings class itself
rg -n "new EmailAttachmentSettings|EmailAttachmentSettings\.of" src/main/java --type=javaRepository: carlos-emr/carlos
Length of output: 649
JavaDoc missing @since tag required per coding guidelines.
The OWASP Encoder import and Encode.forHtml() usage at trust-boundary entry (session storage) is appropriate. The method JavaDoc should include the @since tag per the coding guideline that "All public classes and methods MUST have comprehensive JavaDoc with @param, @return, @throws, and @since tags."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/email/core/EmailAttachmentSettings.java`
around lines 105 - 111, Add a `@since` tag to the JavaDoc for the public method in
class EmailAttachmentSettings that processes the request parameters using
Encode.forHtml (the method invoking Encode.forHtml(req.getParameter(...)) for
passwordEmail, passwordClueEmail, senderEmail, subjectEmail, bodyEmail,
encryptedMessageEmail, emailPatientChartOption); update that method's JavaDoc to
include `@since` with the current project version (or a suitable semantic
version/date) alongside the existing `@param/`@return/@throws tags so it complies
with the coding guideline.
| // Encode groupName to break taint chain before using it in bean construction and session storage | ||
| String groupName = Encode.forHtml(request.getParameter("groupName")); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how groupName is used in EctValidation and EctMeasurementTypesBeanHandler
ast-grep --pattern $'class EctValidation {
$$$
getCssPath($$$) {
$$$
}
$$$
}'
ast-grep --pattern $'class EctMeasurementTypesBeanHandler {
$$$
EctMeasurementTypesBeanHandler($_, $_) {
$$$
}
$$$
}'Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Find the files containing these classes
find . -type f -name "*.java" | xargs grep -l "class EctValidation"
find . -type f -name "*.java" | xargs grep -l "class EctMeasurementTypesBeanHandler"
find . -type f -name "EctSetupMeasurements2Action.java"Repository: carlos-emr/carlos
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Read EctSetupMeasurements2Action.java to trace groupName flow
wc -l "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java"
head -100 "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java" | tail -40Repository: carlos-emr/carlos
Length of output: 2321
🏁 Script executed:
#!/bin/bash
# Read EctValidation.java to see getCssPath implementation
wc -l "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctValidation.java"
cat -n "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctValidation.java" | head -150Repository: carlos-emr/carlos
Length of output: 6226
🏁 Script executed:
#!/bin/bash
# Search for getCssPath method in EctValidation.java
grep -n "getCssPath" "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctValidation.java"Repository: carlos-emr/carlos
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Read the getCssPath method implementation
sed -n '239,260p' "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctValidation.java"Repository: carlos-emr/carlos
Length of output: 1530
🏁 Script executed:
#!/bin/bash
# Read more of the getCssPath method
sed -n '239,290p' "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctValidation.java"Repository: carlos-emr/carlos
Length of output: 2614
🏁 Script executed:
#!/bin/bash
# Read EctMeasurementTypesBeanHandler to see what it does with groupName
wc -l "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/bean/EctMeasurementTypesBeanHandler.java"
head -80 "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/bean/EctMeasurementTypesBeanHandler.java"Repository: carlos-emr/carlos
Length of output: 3438
🏁 Script executed:
#!/bin/bash
# Find the init(groupName, demo) method
grep -n "public boolean init(String groupName" "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/bean/EctMeasurementTypesBeanHandler.java"Repository: carlos-emr/carlos
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Read the init(groupName, demo) method
sed -n '95,160p' "./src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/bean/EctMeasurementTypesBeanHandler.java"Repository: carlos-emr/carlos
Length of output: 3105
🏁 Script executed:
#!/bin/bash
# Verify if there are any tests that would catch this functional break
find . -type f -name "*EctSetupMeasurements2ActionTest*" -o -name "*Test*.java" | xargs grep -l "EctSetupMeasurements2Action" 2>/dev/nullRepository: carlos-emr/carlos
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if there are any example/valid groupName values in the codebase to understand the format
rg "groupName\s*=" --type java -A 2 | head -40Repository: carlos-emr/carlos
Length of output: 3933
🏁 Script executed:
#!/bin/bash
# Check if groupName values in database might contain special characters
rg "setGroupName|new MeasurementGroup" --type java -A 1 -B 1 | head -50Repository: carlos-emr/carlos
Length of output: 5055
🏁 Script executed:
#!/bin/bash
# Double-check if groupName might ever be output to HTML in the JSP
grep -r "groupName" src/main/webapp/encounter/oscarMeasurements/ --include="*.jsp" 2>/dev/null | head -20Repository: carlos-emr/carlos
Length of output: 3232
🏁 Script executed:
#!/bin/bash
# Check if AddMeasurementGroup.jsp properly encodes groupName before output
cat -n "src/main/webapp/encounter/oscarMeasurements/AddMeasurementGroup.jsp" | grep -A 2 -B 2 "groupName"Repository: carlos-emr/carlos
Length of output: 1504
🏁 Script executed:
#!/bin/bash
# Check if any of the JSPs use Encode.forHtml or c:out for groupName
rg "Encode.forHtml.*groupName|c:out.*groupName" src/main/webapp/encounter/oscarMeasurements/ --include="*.jsp"Repository: carlos-emr/carlos
Length of output: 502
🏁 Script executed:
#!/bin/bash
# Check if any of the JSPs use Encode.forHtml or c:out for groupName
rg "Encode.forHtml.*groupName|c:out.*groupName" src/main/webapp/encounter/oscarMeasurements/ -t jspRepository: carlos-emr/carlos
Length of output: 88
Remove HTML encoding from groupName at input; apply context-specific encoding only at output.
Encode.forHtml() converts special characters (e.g., & to &) and should only be applied when outputting to HTML contexts. Applying it here before database operations breaks downstream lookups: EctValidation.getCssPath() calls dao.findByGroupName(inputGroupName) and EctMeasurementTypesBeanHandler.init() calls mgDao.findByName(groupName). If a groupName in the database is "Test&Unit", encoding it before the query causes the lookup to fail.
Instead, validate the input at the boundary (using allowlist pattern for identifier characters), then apply Encode.forHtml() only in JSPs where groupName is output (AddMeasurementGroup.jsp lines 97, 101–103, 135, etc.) using <c:out value='${groupName}'/> or Encode.forHtml in the JSP context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java`
around lines 70 - 71, Remove the premature HTML-encoding of the request
parameter in EctSetupMeasurements2Action (the String groupName =
Encode.forHtml(...)) so the raw identifier is used for downstream lookups
(EctValidation.getCssPath -> dao.findByGroupName and
EctMeasurementTypesBeanHandler.init -> mgDao.findByName); instead validate the
raw request value at input with an allowlist pattern for identifier characters
and store/propagate the validated raw groupName, and move context-specific
encoding to the view layer (use <c:out value='${groupName}'/> or Encode.forHtml
in AddMeasurementGroup.jsp where groupName is rendered).
| // find the program id; parse as integer to validate and break taint chain before session storage | ||
| String programId = String.valueOf( | ||
| io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(request.getParameter("id"))); | ||
|
|
There was a problem hiding this comment.
Preserve the request-attribute fallback for programId.
String.valueOf(...) never returns null, so Line 133's request.getAttribute("id") fallback is now dead. Paths like override_restriction() that forward back into view() via request attributes can now load program "0" instead of the intended program.
Suggested fix
- String programId = String.valueOf(
- io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(request.getParameter("id")));
+ String programId = null;
+ String programIdParam = request.getParameter("id");
+ if (programIdParam != null) {
+ int parsedProgramId =
+ io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(programIdParam);
+ if (parsedProgramId > 0) {
+ programId = String.valueOf(parsedProgramId);
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.java`
around lines 122 - 125, The code currently always wraps
ConversionUtils.fromIntString(request.getParameter("id")) with
String.valueOf(...), which prevents the request.getAttribute("id") fallback from
ever being used; change the logic in ProgramManagerView2Action to first attempt
to read a raw id string from request.getParameter("id"), and only if that is
null or empty use (String)request.getAttribute("id") (as used by
override_restriction() -> view()); then pass that chosen string into
ConversionUtils.fromIntString(...) and finally convert the resulting integer to
a String for programId. Ensure you reference the variable programId, the method
ConversionUtils.fromIntString, request.getParameter("id"), and
request.getAttribute("id") when making the change.
| // Validate programId and clientId as integers to break taint chain | ||
| String programId = String.valueOf( | ||
| io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(request.getParameter("id"))); | ||
| String clientId = String.valueOf( | ||
| io.github.carlos_emr.carlos.util.ConversionUtils.fromIntString(request.getParameter("clientId"))); |
There was a problem hiding this comment.
Reject invalid id / clientId before the admission write path.
These two conversions now collapse bad input to "0" and then flow into getDependentsList(0) and processAdmission(0, ...). This path mutates admission state, so malformed requests should be rejected instead of being normalized into a sentinel ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.java`
around lines 282 - 286, The code currently normalizes malformed id/clientId to
"0" via ConversionUtils.fromIntString and allows that to flow into
getDependentsList(0) and processAdmission(0,...); instead, validate the raw
parameters from request.getParameter("id") and request.getParameter("clientId")
in ProgramManagerView2Action and reject/bail out on invalid input (e.g., send an
error response or throw an exception) rather than converting to 0. Replace the
String.valueOf(ConversionUtils.fromIntString(...)) usage with explicit
parsing/validation and short-circuit the admission write path (do not call
getDependentsList or processAdmission) when the parameters are missing,
non-numeric, or out of expected range.
| // store this for display; encode free-text notes to break taint chain before session storage | ||
| this.setServiceRestriction(e.getRestriction()); | ||
|
|
||
| request.getSession().setAttribute("programId", programId); | ||
| request.getSession().setAttribute("admission.dischargeNotes", dischargeNotes); | ||
| request.getSession().setAttribute("admission.admissionNotes", admissionNotes); | ||
| request.getSession().setAttribute("admission.dischargeNotes", Encode.forHtml(dischargeNotes)); | ||
| request.getSession().setAttribute("admission.admissionNotes", Encode.forHtml(admissionNotes)); |
There was a problem hiding this comment.
Don't HTML-encode notes before storing them for the override flow.
override_restriction() reads these session values back and passes them into processAdmission(...), so encoding here will persist <, &, etc. into admission/discharge notes whenever the override path is used. Encode on the JSP/rendering path instead, or keep separate display-only attributes. As per coding guidelines, use context-appropriate OWASP Encoder methods for user input output: Encode.forHtml() for HTML body, Encode.forHtmlAttribute() for HTML attributes, Encode.forJavaScript() for JS contexts, Encode.forJavaScriptAttribute() for JS in HTML attributes, Encode.forCssString() for CSS, Encode.forUri()/Encode.forUriComponent() for URLs.
Suggested fix
- request.getSession().setAttribute("admission.dischargeNotes", Encode.forHtml(dischargeNotes));
- request.getSession().setAttribute("admission.admissionNotes", Encode.forHtml(admissionNotes));
+ request.getSession().setAttribute("admission.dischargeNotes", dischargeNotes);
+ request.getSession().setAttribute("admission.admissionNotes", admissionNotes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/PMmodule/web/admin/ProgramManagerView2Action.java`
around lines 315 - 320, The session storage currently HTML-encodes
admission/discharge notes (Encode.forHtml) before saving them in
ProgramManagerView2Action via
request.getSession().setAttribute("admission.dischargeNotes"/"admission.admissionNotes"),
which causes encoded text to persist when override_restriction() reads them and
passes into processAdmission(...); remove the Encode.forHtml wrapping and store
the raw notes in the session (keep
this.setServiceRestriction(e.getRestriction()) as-is), then ensure encoding
happens at render time in the JSP or in the specific display path using the
appropriate OWASP Encoder methods (Encode.forHtml(), Encode.forHtmlAttribute(),
etc.) for the context where the notes are output.
| // Validate demographicNo as a positive integer to break taint chain before session storage | ||
| Integer parsedDemoNo = ConversionUtils.fromIntString(request.getParameter("demographic_no")); | ||
| String demographicNo = String.valueOf(parsedDemoNo); |
There was a problem hiding this comment.
Handle null return from ConversionUtils.fromIntString().
ConversionUtils.fromIntString() returns null for invalid/missing input. String.valueOf(null) produces the literal string "null", which would be passed to getDemographic() and WLPatientWaitingListBeanHandler, likely causing lookup failures or exceptions.
Consider adding a null check with a sensible default or error response:
🛡️ Proposed fix to handle null
// Validate demographicNo as a positive integer to break taint chain before session storage
Integer parsedDemoNo = ConversionUtils.fromIntString(request.getParameter("demographic_no"));
+ if (parsedDemoNo == null || parsedDemoNo <= 0) {
+ throw new RuntimeException("Invalid demographic_no parameter");
+ }
String demographicNo = String.valueOf(parsedDemoNo);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayPatientWaitingList2Action.java`
around lines 62 - 64,
ConversionUtils.fromIntString(request.getParameter("demographic_no")) can return
null so using String.valueOf(parsedDemoNo) yields the literal "null" and will
break lookups; update the logic around parsedDemoNo/demographicNo to check for
null (in WLSetupDisplayPatientWaitingList2Action) and handle it
explicitly—either return an error/redirect or set a safe default—before calling
getDemographic() or instantiating WLPatientWaitingListBeanHandler, ensuring you
don't pass the string "null" into downstream lookups.
| if (request.getParameter("waitingListId") != null) { | ||
| waitingListId = request.getParameter("waitingListId"); | ||
| // Validate waitingListId as an integer to break taint chain before session storage | ||
| int parsedWlId = ConversionUtils.fromIntString(request.getParameter("waitingListId")); | ||
| waitingListId = parsedWlId > 0 ? String.valueOf(parsedWlId) : ""; |
There was a problem hiding this comment.
Reject invalid waitingListId before continuing.
This block normalizes bad input to "", but the update path later only checks waitingListId != null, so malformed requests can still reach WLWaitingListUtil.updateWaitingListRecord(...) / rePositionWaitingList(...) with an empty ID instead of failing closed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/io/github/carlos_emr/carlos/waitinglist/pageUtil/WLSetupDisplayWaitingList2Action.java`
around lines 98 - 101, The code currently normalizes invalid waitingListId to an
empty string but later only checks waitingListId != null, allowing empty IDs to
proceed; in WLSetupDisplayWaitingList2Action, after using
ConversionUtils.fromIntString to parse request.getParameter("waitingListId"),
reject and abort processing (e.g., return an error/redirect or set an explicit
failure state) when parsedWlId <= 0 or the parsed value is empty instead of
assigning "" — update the guarding condition so downstream calls like
WLWaitingListUtil.updateWaitingListRecord(...) and rePositionWaitingList(...)
only run when waitingListId is a valid positive integer (use parsedWlId or
String.valueOf(parsedWlId) for checks).
| // Encode site value to break taint chain before session storage | ||
| session.setAttribute("site_selected", ("none".equals(selected_site) ? null : org.owasp.encoder.Encode.forHtml(selected_site))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how site_selected session attribute is used downstream
rg -n "site_selected" --type=jsp --type=java -C3 | head -50Repository: carlos-emr/carlos
Length of output: 88
🏁 Script executed:
rg "site_selected" -C3Repository: carlos-emr/carlos
Length of output: 2473
🏁 Script executed:
# Search specifically for getAttribute calls on site_selected
rg "getAttribute.*site_selected|site_selected.*getAttribute" -C3Repository: carlos-emr/carlos
Length of output: 1484
🏁 Script executed:
# Also search the file being reviewed to understand context better
cat -n src/main/webapp/provider/providerupdatepreference.jsp | head -130 | tail -30Repository: carlos-emr/carlos
Length of output: 2008
🏁 Script executed:
# Look at appointmentprovideradminday.jsp to see how selectedSite is used after retrieval
rg -A 10 "get site providers list" src/main/webapp/provider/appointmentprovideradminday.jspRepository: carlos-emr/carlos
Length of output: 723
🏁 Script executed:
# Also check the full context of selectedSite usage in appointmentprovideradminday.jsp
sed -n '1,100p' src/main/webapp/provider/appointmentprovideradminday.jsp | grep -n "selectedSite\|siteDao\|site"Repository: carlos-emr/carlos
Length of output: 43
🏁 Script executed:
# Check if site identifiers typically contain special characters
rg "getProviderNoBySiteLocation|getGroupBySiteLocation" --type=java -B5 -A5 | head -40Repository: carlos-emr/carlos
Length of output: 3830
🏁 Script executed:
# Look at the actual SiteDao implementation to understand the lookup
fd -e java -path "*/SiteDao*" | xargs cat | head -100Repository: carlos-emr/carlos
Length of output: 229
🏁 Script executed:
# Find SiteDao implementation file
find . -name "*SiteDao*.java" -type fRepository: carlos-emr/carlos
Length of output: 524
🏁 Script executed:
# Look for getProviderNoBySiteLocation implementation
rg "getProviderNoBySiteLocation" --type=java -A8Repository: carlos-emr/carlos
Length of output: 3969
🏁 Script executed:
# Check SiteDaoImpl to see the full implementation
cat src/main/java/io/github/carlos_emr/carlos/commn/dao/SiteDaoImpl.java | grep -A10 "getProviderNoBySiteLocation\|getGroupBySiteLocation"Repository: carlos-emr/carlos
Length of output: 1221
🏁 Script executed:
# Check if there are any example site names or constraints on what site.name can be
rg "site.name|site_name" src/main/java/io/github/carlos_emr/carlos/commn/model/ -B2 -A2 | head -50Repository: carlos-emr/carlos
Length of output: 1088
Remove Encode.forHtml() from session storage—encoding before database lookups breaks site identifier matching.
The value is stored with HTML encoding (Encode.forHtml()) but then passed to siteDao.getProviderNoBySiteLocation(selectedSite) and siteDao.getGroupBySiteLocation(selectedSite), which query where s.name = ?1 against the database. If a site name contains special characters (e.g., "Town&Country"), the encoded value ("Town&Country") will not match the database record, causing the lookup to fail silently.
Encoding should be applied at output time (when rendering HTML), not at storage time. Store the raw site identifier in the session and apply Encode.forHtml() only when displaying the value in HTML markup. Alternatively, verify that site names are constrained to alphanumeric characters with no special characters that require HTML encoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/provider/providerupdatepreference.jsp` around lines 113 -
114, Remove HTML encoding before storing the site identifier in session: stop
using org.owasp.encoder.Encode.forHtml(selected_site) in the
session.setAttribute("site_selected", ...) call and store the raw selected_site
value instead, since encoded values will not match DB queries like
siteDao.getProviderNoBySiteLocation(selectedSite) and
siteDao.getGroupBySiteLocation(selectedSite); ensure any HTML encoding is
performed only at render time when outputting the session value into JSP/HTML.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Michael Yingbull <michael@maplecreekmedical.ca>
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if ("inbox".equals(from) || "sent".equals(from) || "trash".equals(from) || "messenger".equals(from)) { | ||
| validatedFrom = from; | ||
| } | ||
| request.getSession().setAttribute("from", validatedFrom); |
Check notice
Code scanning / SnykCode
Trust Boundary Violation Note
| String orderKey = orderBy.startsWith("!") ? orderBy.substring(1) : orderBy; | ||
| if ("status".equals(orderKey) || "from".equals(orderKey) || "subject".equals(orderKey) | ||
| || "date".equals(orderKey) || "sentto".equals(orderKey) || "linked".equals(orderKey)) { | ||
| request.getSession().setAttribute("orderBy", orderBy); |
Check notice
Code scanning / SnykCode
Trust Boundary Violation Note
Summary by CodeRabbit
Release Notes