Open
Conversation
4 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @phc007, your pull request is larger than the review limit of 150000 diff characters
Contributor
📝 WalkthroughWalkthroughAdds i18n keys for the demographic "add" flow across five locale bundles and refactors two JSPs (add.jsp, add-form-clinical.jsp) to use Bootstrap layout, encoded client-side i18n, updated form controls, calendar hookups, and improved validation/encoding for referral/consent fields. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request modernizes the patient demographic addition interface by migrating the layout from legacy tables to Bootstrap 5 and externalizing hardcoded JavaScript strings into localized resource bundles. The changes include refactoring add.jsp and add-form-clinical.jsp to use responsive grid systems and standard Bootstrap form components. Feedback focuses on critical security vulnerabilities where dynamic content—including specialist names, provider info, program names, and extended demographic labels—is rendered without proper OWASP encoding, posing XSS risks. Additionally, there is a style guide violation regarding the preferred EL encoding function and a concern about fragile layout coupling caused by unbalanced tags across JSP fragments.
| %> | ||
| </select> | ||
| </div> | ||
| </div><%-- end cross-file row --%> |
Contributor
There was a problem hiding this comment.
Closing a div that was opened in a different file (add-form-personal.jsp) creates a tight coupling between fragments and makes the layout fragile. It is recommended to keep tags balanced within each JSP fragment to improve maintainability and prevent layout breaks if fragments are reused or reordered.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp (1)
32-32: 🧹 Nitpick | 🔵 TrivialUnused OWASP encoder taglib import.
After this refactor every encoded output in the file uses
<carlos:encode …>(lines 96, 112, 130, 145, 269), so theetaglib declared here is no longer referenced. Per coding guidelines,<e:forXxx>/${e:forXxx(...)}should not be used in this codebase anyway, so the import can simply be dropped.♻️ Proposed cleanup
-<%@ taglib uri="owasp.encoder.jakarta.advanced" prefix="e" %> <%@ taglib uri="carlos" prefix="carlos" %>As per coding guidelines: "OWASP encoding is MANDATORY for all user inputs - use null-safe CARLOS wrappers … do NOT use
<e:forXxx>,${e:forXxx(...)}, orEncode.forXxx(...)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp` at line 32, Remove the unused OWASP encoder taglib directive by deleting the JSP taglib line that declares prefix "e" (the <%@ taglib uri="owasp.encoder.jakarta.advanced" prefix="e" %> directive) since all encoding now uses the carlos:encode wrapper (e.g., the instances referenced at lines with carlos:encode), and then search the JSP for any remaining references to the "e" prefix to ensure no uses remain before committing.src/main/webapp/WEB-INF/jsp/demographic/add.jsp (2)
31-37: 🛠️ Refactor suggestion | 🟠 MajorMissing
carlostaglib declaration.The companion fragment
add-form-clinical.jsp(line 33) declares<%@ taglib uri="carlos" prefix="carlos" %>and uses<carlos:encode>throughout. Once thei18nblock is migrated to CARLOS wrappers (see comment on lines 10/117–131) — or to drive any future${carlos:forHtml(...)}usage on this page — the master page will need the same taglib declaration.🧩 Proposed addition
<%@ taglib uri="jakarta.tags.core" prefix="c" %> +<%@ taglib uri="carlos" prefix="carlos" %> <c:set var="ctx" value="${ pageContext.request.contextPath }"/>As per coding guidelines: "All JSP views must use CARLOS null-safe encoding wrappers in all output contexts: HTML body, HTML attributes, JavaScript strings, CSS, and URL parameters; import
<%@ taglib uri="carlos" prefix="carlos" %>."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp` around lines 31 - 37, The add.jsp page is missing the CARLOS taglib declaration required for using carlos:encode (and future carlos:forHtml) that add-form-clinical.jsp relies on; add the taglib directive for CARLOS (declare the carlos taglib with prefix "carlos") alongside the existing taglib lines in add.jsp so all carlos:encode usages resolve and CARLOS wrappers are available across this master page.
467-475: 🧹 Nitpick | 🔵 TrivialConsider refactoring
ignoreDuplicates()to use async/await instead ofjQuery.ajaxSetup({async: false}).The synchronous XHR pattern (line 467–475) is deprecated and blocks the UI thread. While synchronous XHR remains supported in all major browsers today, it is slated for eventual removal. When browsers deprecate it further,
jQuery.ajaxSetup({async: false})may become a no-op, causing theconfirm()to fire after the form submit decision is made, makingignorealwaystrue. A Promise-based refactor using async/await would be more future-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp` around lines 467 - 475, The code uses synchronous XHR via jQuery.ajaxSetup({async: false}) inside the duplicate check; refactor ignoreDuplicates() into an async function that awaits the jQuery.post call (or returns a Promise) instead of toggling global ajaxSetup, then set the local ignore variable from the awaited response before proceeding to confirm(i18n.confirmDuplicatePatient) (use data.hasDuplicates to decide). Remove jQuery.ajaxSetup calls and ensure callers either await ignoreDuplicates() or handle the returned Promise so the confirm runs before the form submission decision.
🤖 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/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp`:
- Line 100: Remove the duplicate trailing empty <option> elements that follow
the provider loops in the staff and roster_enrolled_to select blocks (the stray
<option value=""></option> after the doctors loop in the staff <select> and the
similar one after the roster_enrolled_to loop); keep only the initial
placeholder empty option (the leading empty option already present) and delete
the redundant empty option nodes so the select lists do not show duplicate blank
entries.
- Around line 254-262: The roster_enrolled_to doctor <select> is emitting
provider names raw via p.getFormattedName(), violating the CARLOS null-safe
encoding rule; update the option label generation inside the loop over
providerDao.getActiveProvidersByRole("doctor") so that p.getFormattedName() is
wrapped with the same <carlos:encode … context="html"/> encoder used in the
other care-team selects (keep id and value as <%=docProviderNo%>), ensuring the
displayed provider name is encoded and null-safe consistent with the project's
encoding policy.
- Around line 578-606: The dropdown population currently leaves bedP as an empty
Program[] so residential programs never appear; replace the stub by fetching the
provider's active programs the same way as edit-form-clinical.jsp does: call
ProgramManager.getProgramDomain(_pvid) (or equivalent existing helper) to
populate bedP after obtaining _pvid from loggedInInfo, then proceed to filter
out oscarProgram (from programDao.getProgramByName("OSCAR")) and render the
options; ensure bedP is a non-null list/array before looping and keep the
existing no-programs fallback when bedP is empty and oscarProgram is null.
In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp`:
- Around line 635-638: The opening <form> start tag for the demographic form
(id/name "adddemographic", action "DemographicAddRecord") is missing its
terminating ">", so the included fragments add-form-personal.jsp and
add-form-clinical.jsp are being swallowed into the start tag; fix by adding the
missing ">" to the <form ...> start tag in add.jsp (the form opens there) and
then consider moving the corresponding closing </form> out of
add-form-clinical.jsp so the form is opened and closed in the same file for
structural safety.
- Around line 273-276: The code reads
selectBox.options[selectBox.selectedIndex].value for
document.adddemographic.month_of_birth and date_of_birth without validating
selectedIndex, which can be -1 and throw a TypeError; update the logic in the
block that sets mm and dd to first check selectBox.selectedIndex >= 0 (or that
selectBox.options[selectBox.selectedIndex] exists) before accessing .value and
provide a safe fallback (e.g., empty string or null) when no selection exists so
the submit pipeline and subsequent i18n alerts won't be interrupted; apply this
check for both month_of_birth and date_of_birth assignments (variables mm and
dd).
- Line 10: Replace the direct use of OWASP's Encode with CARLOS's null-safe
wrapper: change the JSP import from org.owasp.encoder.Encode to the SafeEncode
wrapper and update every call to Encode.forJavaScript(...) inside the i18n map
(the eleven occurrences currently using Encode.forJavaScript) to
SafeEncode.forJavaScript(...). Ensure you only modify the import and those
Encode.forJavaScript invocations so null translation values are handled without
throwing NullPointerException.
---
Outside diff comments:
In `@src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp`:
- Line 32: Remove the unused OWASP encoder taglib directive by deleting the JSP
taglib line that declares prefix "e" (the <%@ taglib
uri="owasp.encoder.jakarta.advanced" prefix="e" %> directive) since all encoding
now uses the carlos:encode wrapper (e.g., the instances referenced at lines with
carlos:encode), and then search the JSP for any remaining references to the "e"
prefix to ensure no uses remain before committing.
In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp`:
- Around line 31-37: The add.jsp page is missing the CARLOS taglib declaration
required for using carlos:encode (and future carlos:forHtml) that
add-form-clinical.jsp relies on; add the taglib directive for CARLOS (declare
the carlos taglib with prefix "carlos") alongside the existing taglib lines in
add.jsp so all carlos:encode usages resolve and CARLOS wrappers are available
across this master page.
- Around line 467-475: The code uses synchronous XHR via
jQuery.ajaxSetup({async: false}) inside the duplicate check; refactor
ignoreDuplicates() into an async function that awaits the jQuery.post call (or
returns a Promise) instead of toggling global ajaxSetup, then set the local
ignore variable from the awaited response before proceeding to
confirm(i18n.confirmDuplicatePatient) (use data.hasDuplicates to decide). Remove
jQuery.ajaxSetup calls and ensure callers either await ignoreDuplicates() or
handle the returned Promise so the confirm runs before the form submission
decision.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed0aa613-2f40-4a10-9da8-bd35685e456a
📒 Files selected for processing (8)
src/main/resources/oscarResources_en.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_pl.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add-form-personal.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: build
- GitHub Check: check-containers
- GitHub Check: Semgrep SAST Analysis
- GitHub Check: Build and analyze
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{jsp,java}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use OWASP Encoder for ALL user input output: in JSP use
${e:forHtml(value)}with taglib<%@ taglib uri="owasp.encoder.jakarta" prefix="e" %>, in Java useEncode.forHtml(value), with context-specific variants (forHtmlAttribute,forJavaScript,forCssString,forUri,forUriComponent)
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
**/*.jsp
📄 CodeRabbit inference engine (.github/instructions/jsp-security.instructions.md)
**/*.jsp: Every JSP file MUST include OWASP Encoder output encoding for ALL user data
Add required taglib with URIowasp.encoder.jakarta(Jakarta EE) to every new JSP file:<%@ taglib uri="owasp.encoder.jakarta" prefix="e" %>
Use${e:forHtml(value)}for HTML body output encoding, NOT<c:out>orfn:escapeXml()
Use${e:forHtmlAttribute(value)}for HTML attribute encoding
Use${e:forJavaScript(value)}for JavaScript string encoding
Use${e:forJavaScriptAttribute(value)}for JavaScript in HTML attribute encoding
Use${e:forCssString(value)}for CSS string encoding
Use${e:forUri(value)}for URL path encoding
Use${e:forUriComponent(value)}for URL parameter encoding
CSRF tokens are auto-injected by CSRFGuard 4.5. Do NOT add manual CSRF hidden inputs to JSP files
Avoid using<c:out value="${...}" />in new JSP code; use${e:forHtml(...)}instead
Avoid usingfn:escapeXml()in new JSP code; use${e:forHtml(...)}instead
NEVER output user data without encoding in JSP files; avoid raw${variable}without OWASP Encoder functions
**/*.jsp: All JSP views must use CARLOS null-safe encoding wrappers in all output contexts: HTML body, HTML attributes, JavaScript strings, CSS, and URL parameters; import<%@ taglib uri="carlos" prefix="carlos" %>
Any JSP that performs AJAX POSTs reading the CSRF-TOKEN must either contain a<form action="<real URL>" method="post">or include<%@ include file="/WEB-INF/jspf/csrf-token.jspf" %>
For new JSP views, place them under/WEB-INF/jsp/**and create Struts action entry points; do NOT expose JSP files directly as public endpoints; use extensionless routes like/login,/form/setupSelect,/eform/efmshowform_data
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
{README.md,docs/**/*.md,src/main/webapp/**/*.jsp,src/main/webapp/**/*.html}
📄 CodeRabbit inference engine (GEMINI.md)
Refer to this system as 'CARLOS EMR' or 'CARLOS' in all user-facing content (UI, documentation, README files, issue templates, workflows)
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
{src/main/webapp/**/*.jsp,src/main/java/io/github/carlos_emr/carlos/*/web/**Action.java}
📄 CodeRabbit inference engine (GEMINI.md)
Use OWASP Encoder for ALL user inputs:
Encode.forHtml()for Java code, or${e:forHtml()}EL function for JSP (prefer EL functions in JSP)
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
src/main/webapp/**/*.jsp
📄 CodeRabbit inference engine (GEMINI.md)
src/main/webapp/**/*.jsp: Include JSP taglib declaration<%@ taglib uri="owasp.encoder.jakarta" prefix="e" %>once per JSP that uses EL encoding functions
Use context-appropriate OWASP encoding: HTML body${e:forHtml()}, HTML attribute${e:forHtmlAttribute()}, JavaScript${e:forJavaScript()}, CSS${e:forCssString()}, URL${e:forUri()}or${e:forUriComponent()}
Add comprehensive JSP comment blocks after copyright headers with purpose, features, parameters, and@sincedocumentation
Prefer JSP EL encoding functions${e:...()}over scriptlets; use JSP with JSTL/EL for cleaner, context-aware encoding
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
{src/main/java/io/github/carlos_emr/carlos/**/*.java,src/main/webapp/**/*.jsp}
📄 CodeRabbit inference engine (GEMINI.md)
PHI (Patient Health Information) must NEVER be logged or exposed in error messages
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
**/*.{jsp,jspf,java}
📄 CodeRabbit inference engine (CLAUDE.md)
OWASP encoding is MANDATORY for all user inputs - use null-safe CARLOS wrappers:
<carlos:encode>tag,${carlos:forXxx()}EL functions, orSafeEncode.forXxx()Java methods; do NOT use<e:forXxx>,${e:forXxx(...)}, orEncode.forXxx(...)
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
**/*.{java,jsp,jspf}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve all existing copyright headers in modified files; do NOT remove existing copyright notices as this violates GPL; optionally add modification notices for functional changes; use the CARLOS project header (see docs/copyright-header-carlos.md) for new files
Files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
🧠 Learnings (44)
📓 Common learnings
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 650
File: src/main/resources/oscarResources_pl.properties:0-0
Timestamp: 2026-03-17T16:16:33.344Z
Learning: Repo policy: In carlos-emr/carlos, i18n wording refinements (e.g., strings in src/main/resources/oscarResources_*.properties) are out of scope for feature PRs and should be tracked/fixed separately.
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 716
File: src/main/webapp/WEB-INF/eform-assets/editControl2.js:476-476
Timestamp: 2026-03-24T17:34:17.471Z
Learning: In `carlos-emr/carlos`, the file `src/main/webapp/WEB-INF/eform-assets/editControl2.js` contains implicit global variables (undeclared `i` in for-loops such as `block()`, and `new_top`/`new_left` in `toggleTempBin()`) that are pre-existing upstream OSCAR code, not introduced by this PR. Do not flag these as new issues in future reviews of this file unless the surrounding code is actively modified.
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1967
File: src/main/java/io/github/carlos_emr/carlos/billings/ca/on/pageUtil/BillingShortcutPg1DataAssembler.java:516-525
Timestamp: 2026-04-25T22:33:40.512Z
Learning: In `carlos-emr/carlos`, the `BillingShortcutPg1DataAssembler` (and potentially other ON billing assemblers) bake static HTML markup (`<br>`, `<b>`, `<font color='...'>`) directly into their `errorMessage`/`warningMessage`/`msg` fields. No user-controlled data flows into these strings — they are entirely server-composed. The refactor to move markup into the JSP view and apply CARLOS encoding to plain-text message output is a known deferred polish item (not a security issue). Do not re-flag this pattern as an XSS or encoding violation in PRs that have not yet landed the markup-extraction refactor.
📚 Learning: 2026-03-17T16:16:27.811Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 650
File: src/main/resources/oscarResources_pl.properties:0-0
Timestamp: 2026-03-17T16:16:27.811Z
Learning: In the repo carlos-emr/carlos, i18n wording refinements for files matching src/main/resources/oscarResources_*.properties are out of scope for feature PRs. Track and fix these refinements separately (e.g., via issues or a dedicated i18n PR) so feature changes aren’t blocked by localization wording edits.
Applied to files:
src/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_en.propertiessrc/main/resources/oscarResources_pl.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_pt_BR.properties
📚 Learning: 2026-04-05T00:19:28.926Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1048
File: src/main/webapp/oscarMDS/PatientSearch.jsp:384-384
Timestamp: 2026-04-05T00:19:28.926Z
Learning: In `src/main/webapp/oscarMDS/PatientSearch.jsp`, the `nextform` pagination form's default `action` should point to `DemographicSearch.do` (demographic search/pagination endpoint), NOT to `PatientSearch.jsp`. The `PatientSearch.jsp` page serves the MDS lab patient-matching flow; the `nextform` buttons (prev/next page) belong to the demographic search results pagination flow and are correctly handled by `DemographicSearch.do`. Do not suggest `PatientSearch.jsp` as the fallback action for `nextform` in this file.
Applied to files:
src/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_en.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T00:04:12.481Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1358
File: src/main/webapp/oscarMDS/documentsInQueues.jsp:0-0
Timestamp: 2026-04-08T00:04:12.481Z
Learning: In `src/main/webapp/oscarMDS/documentsInQueues.jsp` (carlos-emr/carlos), the `page-header-bar` hidden inputs for `providerNo`, `searchProviderNo`, `lname`, `fname`, and `hnum` are already properly encoded with `Encode.forHtmlAttribute()` for HTML attribute contexts. The corresponding JavaScript variable assignments (`var providerNo` and `var searchProviderNo`) are also properly encoded with `Encode.forJavaScript()`. Do not flag these as missing OWASP encoding — the correct context-appropriate encoding is already applied.
Applied to files:
src/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-14T03:43:20.664Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1671
File: src/main/webapp/WEB-INF/jsp/provider/appointmentprovideradminday.jsp:529-529
Timestamp: 2026-04-14T03:43:20.664Z
Learning: Repo: carlos-emr/carlos — In provider schedule JSPs (e.g., src/main/webapp/WEB-INF/jsp/provider/appointmentprovideradminday.jsp), several client-side URL constructions concatenate dynamic values (e.g., newGroupNo, siteName, providername) directly into .do query strings without URI-encoding. This is a pre-existing OSCAR-era pattern and not introduced by recent .jsp → .do migrations. Treat as non-blocking in migration PRs; comprehensive URI-encoding hardening is tracked in issue `#1681`.
Applied to files:
src/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-02-21T01:06:58.978Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 443
File: src/main/resources/oscarResources_en.properties:34-57
Timestamp: 2026-02-21T01:06:58.978Z
Learning: In user-facing properties files (e.g., src/main/resources/oscarResources_en.properties), avoid introducing new brand domains (e.g., a new carlosgalaxy domain) in strings. When updating links for branding (such as loginApplication.image.logoText), prefer a neutral, stable link like the GitHub repo with https, or omit the link entirely if no suitable neutral target exists. Ensure all updates keep branding consistent and do not introduce new external domains in UI strings.
Applied to files:
src/main/resources/oscarResources_fr.propertiessrc/main/resources/oscarResources_en.propertiessrc/main/resources/oscarResources_pl.propertiessrc/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_pt_BR.properties
📚 Learning: 2026-03-17T21:22:43.854Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 667
File: src/main/webapp/form/demographicMeasurementModal.jsp:242-242
Timestamp: 2026-03-17T21:22:43.854Z
Learning: In src/main/webapp/form/demographicMeasurementModal.jsp (JSP, Java EE / OSCAR EMR), the showMeasurementDialog() function accepts bodyHtml built in displayDemographicMeasurements(). Most of bodyHtml comes from server JSON (this.dataField, this.measuringInstruction), which yingbull considers safe. However, the string also includes document.getElementById(elementId).value (a live DOM form-field value) unsanitized into an HTML attribute context before being assigned to innerHTML. This is technically a self-XSS / attribute-injection vector, but yingbull has assessed it as acceptable risk (self-XSS only, authenticated users). Do not flag this as a critical XSS unless new untrusted input sources are added to the bodyHtml construction.
Applied to files:
src/main/resources/oscarResources_en.properties
📚 Learning: 2026-04-25T22:33:40.512Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1967
File: src/main/java/io/github/carlos_emr/carlos/billings/ca/on/pageUtil/BillingShortcutPg1DataAssembler.java:516-525
Timestamp: 2026-04-25T22:33:40.512Z
Learning: In `carlos-emr/carlos`, the `BillingShortcutPg1DataAssembler` (and potentially other ON billing assemblers) bake static HTML markup (`<br>`, `<b>`, `<font color='...'>`) directly into their `errorMessage`/`warningMessage`/`msg` fields. No user-controlled data flows into these strings — they are entirely server-composed. The refactor to move markup into the JSP view and apply CARLOS encoding to plain-text message output is a known deferred polish item (not a security issue). Do not re-flag this pattern as an XSS or encoding violation in PRs that have not yet landed the markup-extraction refactor.
Applied to files:
src/main/resources/oscarResources_es.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-03-24T17:34:17.471Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 716
File: src/main/webapp/WEB-INF/eform-assets/editControl2.js:476-476
Timestamp: 2026-03-24T17:34:17.471Z
Learning: In `carlos-emr/carlos`, the file `src/main/webapp/WEB-INF/eform-assets/editControl2.js` contains implicit global variables (undeclared `i` in for-loops such as `block()`, and `new_top`/`new_left` in `toggleTempBin()`) that are pre-existing upstream OSCAR code, not introduced by this PR. Do not flag these as new issues in future reviews of this file unless the surrounding code is actively modified.
Applied to files:
src/main/resources/oscarResources_es.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-04-14T03:43:01.925Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1671
File: src/main/webapp/WEB-INF/jsp/provider/infirmaryviewprogramlist.jspf:21-21
Timestamp: 2026-04-14T03:43:01.925Z
Learning: In `src/main/webapp/WEB-INF/jsp/provider/infirmaryviewprogramlist.jspf` (carlos-emr/carlos), the unencoded emission of `session.getAttribute("infirmaryView_OscarQue")` directly into `href` HTML attributes (lines ~118, ~127) and into JavaScript URL strings via `providerurlStringJs` is a pre-existing OSCAR-era pattern, not introduced by any individual PR. The required fix (applying `Encode.forHtmlAttribute()` for href contexts and `Encode.forJavaScript()` for JS string contexts) is tracked as a dedicated security-hardening follow-up in issue `#1681`. Do not flag this as a new issue introduced by PRs that merely change `.jsp` URL suffixes to `.do`.
Applied to files:
src/main/resources/oscarResources_es.propertiessrc/main/resources/oscarResources_pt_BR.propertiessrc/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-17T21:22:37.959Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 667
File: src/main/webapp/form/demographicMeasurementModal.jsp:242-242
Timestamp: 2026-03-17T21:22:37.959Z
Learning: In src/main/webapp/form/demographicMeasurementModal.jsp, showMeasurementDialog() builds bodyHtml from server data (this.dataField, this.measuringInstruction) and unsafely inlines a live DOM value (document.getElementById(elementId).value) into an HTML attribute context before assigning to innerHTML. This is a self-XSS/attribute-injection vector, but it has been assessed as acceptable risk given authenticated users. Do not flag as a critical XSS unless new untrusted input sources are introduced to bodyHtml. Review guidance should be: (1) Treat this as a narrow self-XSS risk, not a general XSS issue; (2) Do not escalate unless you add new untrusted inputs; (3) Document the rationale in code comments and/or review notes; (4) If future changes add untrusted sources, re-evaluate and consider sanitization, stricter encoding, or replacing innerHTML construction with safer DOM manipulation; (5) Consider a minimal CI guard that flags additions of new untrusted inputs to the bodyHtml constructio...
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-23T01:03:12.952Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 715
File: src/main/webapp/provider/appointmentprovideradminday.jsp:2610-2633
Timestamp: 2026-03-23T01:03:12.952Z
Learning: In this repo, avoid changing only a single caller that invokes `SearchDemographic.do` from GET to POST. If the request method for `SearchDemographic.do` needs to change, do it as one coordinated, global change across *all* JSP callers (so request semantics stay consistent) and include required CSRF token handling as part of that same migration. Do not introduce a mixed GET/POST pattern for this endpoint across the codebase.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-20T16:41:26.583Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1607
File: src/main/webapp/WEB-INF/jsp/messenger/generatePreviewPDF.jsp:183-186
Timestamp: 2026-04-20T16:41:26.583Z
Learning: In `carlos-emr/carlos`, `RxPatientData.getPatient(String loggedInInfo, String demographic_no)` (called in `src/main/webapp/messenger/generatePreviewPDF.jsp` and likely other JSPs) always returns a non-null `RxPatientData.Patient` wrapper object even when the underlying demographic lookup finds no record. Therefore the `request.getSession().setAttribute("Patient", patient)` call unconditionally overwrites any previous session value on every page load — there is no stale-Patient-context risk. Do not flag the absence of a `removeAttribute("Patient")` null-branch as a session-staleness bug in this codebase.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T00:54:25.889Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1358
File: src/main/webapp/documentManager/showDocument.jsp:446-446
Timestamp: 2026-04-08T00:54:25.889Z
Learning: In `src/main/webapp/documentManager/showDocument.jsp` (and `src/main/webapp/lab/CA/ALL/labDisplay.jsp`) in carlos-emr/carlos, the async `fetchCsrfToken()` / CSRFGuard servlet fetch pattern for populating `input[name="CSRF-TOKEN"]` elements is the established codebase pattern. The ~50-200ms race window between page load and token hydration is considered acceptable because: (1) the consequence is a rejected request, not a security bypass, and (2) interactive user actions virtually never occur within that window. A tracking issue has been created to investigate migrating to the hidden-form auto-injection pattern (`<form method="post" style="display:none;"></form>`) as an alternative. Do not flag the async fetch pattern as a race condition requiring immediate fix in these files.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-06T03:30:57.332Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1116
File: src/main/webapp/billing/CA/BC/billingTeleplanCorrectionWCB.jsp:50-51
Timestamp: 2026-04-06T03:30:57.332Z
Learning: In `src/main/webapp/billing/CA/BC/billingTeleplanCorrectionWCB.jsp` (carlos-emr/carlos), the file is 700+ lines and entirely scriptlet-based. When adding OWASP output encoding in a security-fix PR, it is acceptable and expected to use scriptlet-style `Encode.forHtml(...)` / `Encode.forHtmlAttribute(...)` / `Encode.forJavaScript(...)` calls via `<%= ... %>` rather than converting to the OWASP Encoder EL taglib (`${e:forHtml()}`). The EL taglib migration is a separate refactoring effort and should not be flagged as a blocker on security fix PRs targeting this file.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-10T03:19:35.297Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-10T03:19:35.297Z
Learning: Applies to src/main/java/io/github/carlos_emr/carlos/**/*2Action.java : Session access pattern: use `LoggedInInfo.getLoggedInInfoFromSession(request)` to retrieve logged-in provider context for security checks
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-04-07T20:28:16.270Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: .github/instructions/action-security.instructions.md:0-0
Timestamp: 2026-04-07T20:28:16.270Z
Learning: Applies to **/*2Action.java : MANDATORY Security Check (FIRST operation in every method): Call LoggedInInfo.getLoggedInInfoFromSession(request) and use securityInfoManager.hasPrivilege() to check privileges before any operation, throwing SecurityException if privilege check fails
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-03-27T22:41:39.520Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 604
File: src/main/webapp/tickler/ticklerMain.jsp:766-770
Timestamp: 2026-03-27T22:41:39.520Z
Learning: In `src/main/webapp/tickler/ticklerMain.jsp` (and similar JSP files in carlos-emr/carlos), the multisite provider dropdown uses a JS object `_providers[siteId]` whose values are HTML `<option>` fragment strings embedded inside JavaScript string literals. The correct double-encoding pattern for this nested context is: `Encode.forJavaScript(Encode.forHtmlAttribute(p.getProviderNo()))` for the `value` attribute and `Encode.forJavaScript(Encode.forHtml(p.getLastName()))` / `Encode.forJavaScript(Encode.forHtml(p.getFirstName()))` for the option text content. The inner encoder (forHtmlAttribute / forHtml) handles the HTML layer; the outer encoder (forJavaScript) handles the JS string layer. Do not flag this double-encoding as incorrect or suggest replacing it with forJavaScriptBlock — this is the approved OWASP pattern for HTML fragments inside JS string literals.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-04-06T15:22:52.578Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1147
File: src/main/webapp/demographic/demographicappthistory.jsp:0-0
Timestamp: 2026-04-06T15:22:52.578Z
Learning: For JSP views under src/main/webapp, when looking up multisite attributes from a map (e.g., `siteBgColor`) keyed by a location that may have become inactive/deleted, don’t assume the key exists. Guard against null/empty results before rendering. Pattern: fetch the mapped `String[]` (e.g., `String[] sbc = siteBgColor.get(appointment.getLocation());`), derive `siteColor` and `siteLabel` with null/length checks (fall back to `""` for CSS color and to the location label when the array is missing/too short), then render with `Encode.forCssString(siteColor)` and `Encode.forHtml(siteLabel)` to avoid NPEs and incorrect output.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-07T04:31:40.822Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1070
File: src/main/webapp/encounter/oscarMeasurements/adminFlowsheet/FlowsheetEditor.jsp:102-102
Timestamp: 2026-04-07T04:31:40.822Z
Learning: In `src/main/webapp/encounter/oscarMeasurements/adminFlowsheet/FlowsheetEditor.jsp` (carlos-emr/carlos), the `loadFlowsheet()`, `loadTypes()`, and `loadPreventionTypes()` functions build table rows and `<option>` elements entirely using jQuery DOM APIs (`$('<tr>')`, `$('<td>').text(...)`, `$('<option>').attr(...).text(...)`, etc.). These are inherently safe against DOM XSS — no HTML string concatenation is used. Do not flag `.text()` or `.attr()` jQuery DOM construction in this file as XSS sinks; they are the accepted safe pattern here.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-13T21:49:09.631Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1670
File: src/main/webapp/provider/appointmentprovideradminday.jsp:2043-2047
Timestamp: 2026-04-13T21:49:09.631Z
Learning: In `carlos-emr/carlos`, tickler popup/link callers follow a two-tier routing pattern based on the required privilege:
- **Indicator/icon callers** (e.g., the `!` tickler alert on the appointment schedule in `appointmentprovideradminday.jsp`) use `ViewTicklerMain.do?demoview=...` — this only requires `_tickler r`, matching the original `ticklerMain.jsp` oscarSec check at line 58.
- **Full demographic tickler panel callers** (e.g., demographic edit pages, case management navigation) use `ViewTicklerDemoMain.do?demoview=...` — this requires `_tickler w`, matching `ticklerDemoMain.jsp` oscarSec at line 37.
Do NOT suggest changing indicator/icon callers from `ViewTicklerMain.do` to `ViewTicklerDemoMain.do`, as that would silently lock out read-only providers from the tickler-indicator icon. The mixed routing is a known pre-existing inconsistency tracked in issue `#1672` for future consolidation.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-03-23T23:25:23.075Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 716
File: src/main/webapp/eform/displayAttachedFiles.jsp:48-68
Timestamp: 2026-03-23T23:25:23.075Z
Learning: In carlos-emr/carlos, for eForm AJAX endpoints (e.g., displayAttachedFiles.jsp, displayAttachedFiles.jsp), checking `_eform` read privilege via `securityInfoManager.hasPrivilege(loggedInInfo, "_eform", "r", null)` (passing null for demographicNo) is the intended and documented authorization gate for non-patient-specific checks. Record-level fdid ownership validation (joining fdid to a demographic) is NOT required by the project's security design. Do not flag the absence of per-fdid ownership checks in eForm JSP endpoints as a security issue; the authenticated + privileged user requirement is the accepted risk boundary.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp
📚 Learning: 2026-03-15T15:52:50.897Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 644
File: src/main/webapp/admin/updateDrugref.jsp:63-70
Timestamp: 2026-03-15T15:52:50.897Z
Learning: Guideline: For JSPs (any .jsp) that perform fetch() with POST and rely on CSRF tokens not auto-injected by CSRFGuard 4.5, insert a hidden form into the page body: <form method="post" style="display:none;"></form>. This causes CSRFGuardScriptInjectionFilter to inject the CSRF-TOKEN hidden input into that form, making the token available as document.querySelector('input[name="CSRF-TOKEN"]') for inclusion in fetch() request bodies. If getCsrfToken() returns '' as a fallback, ensure it is accompanied by a clear console.warn and that the hidden form guarantees token injection. Apply this pattern to JSPs where manual CSRF token integration is required.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-06T03:31:35.622Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1116
File: src/main/webapp/billing/CA/BC/genTAS00.jsp:181-183
Timestamp: 2026-04-06T03:31:35.622Z
Learning: When using the OWASP Java Encoder (`Encode.*`) in JSP, select the encoder based on the actual JavaScript context:
- Use `Encode.forJavaScript()` when inserting data into a JavaScript string literal (e.g., values embedded in `'...'?` or inside a `javascript:` href that contains JS code).
- Use `Encode.forJavaScriptAttribute()` only when the JavaScript appears inside an HTML event handler attribute (e.g., `onclick="..."`).
Do not flag `Encode.forJavaScript()` as incorrect merely because it contains JavaScript; only flag it if the target location is an HTML event attribute requiring `forJavaScriptAttribute()`.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-27T22:41:03.205Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 604
File: src/main/webapp/tickler/ticklerMain.jsp:87-88
Timestamp: 2026-03-27T22:41:03.205Z
Learning: In JSPs, it can be intentional for a Java scriptlet variable (e.g., `isDemoView` inside `<% ... %>`) and a separately named `pageContext` attribute (e.g., `pageContext.setAttribute("hasDemoView", isDemoView)`) to coexist. The scriptlet variable is used within `<% %>` code, while the `pageContext` attribute is accessed via JSTL/EL (e.g., `${hasDemoView}`). These are different namespaces—if the `pageContext` attribute is set before use, do not flag `${...}` as an unresolved/broken reference just because a similarly named scriptlet variable exists with a different name.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-27T23:09:08.560Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 724
File: src/main/webapp/billing/CA/BC/billingDeleteWithoutNo.jsp:148-148
Timestamp: 2026-03-27T23:09:08.560Z
Learning: In this codebase’s JSP/JS usage of `BroadcastChannel`, follow the established convention: wrap both the `new BroadcastChannel(...)` instantiation and the `postMessage(...)` call in a `try/catch` block with an empty (or comment-only) catch handler like `catch (e) { /* BroadcastChannel not supported */ }`. Do not re-run the `BroadcastChannel` logic inside the catch block. This repository’s convention does not use feature detection such as `typeof BroadcastChannel === 'undefined'` for this guard.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-27T23:09:13.720Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 724
File: src/main/webapp/billing/CA/CLINICAID/billingDeleteWithoutNo.jsp:77-77
Timestamp: 2026-03-27T23:09:13.720Z
Learning: When using `BroadcastChannel` in this repo, follow the established guard convention: wrap `BroadcastChannel` message posting in a `try/catch` and use an empty (or comment-only) `catch` body (e.g., `catch (e) { /* BroadcastChannel not supported */ }`). Do not use `typeof BroadcastChannel !== 'undefined'` as the primary guard; keep this pattern consistent across all JSP files in `src/main/webapp/**`—especially the JSP billing files—and for any code that posts `BroadcastChannel` messages.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-06T03:31:14.839Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1116
File: src/main/webapp/billing/CA/BC/support/billingfeeitem.jsp:138-143
Timestamp: 2026-04-06T03:31:14.839Z
Learning: In this repository’s JSP files, when outputting untrusted values that will be embedded inside JavaScript string literals within HTML event handler attributes (e.g., inside `onclick="..."` or any attribute whose value contains JavaScript code with values surrounded by single or double quote delimiters), prefer `Encode.forJavaScript()` as the recommended encoder for the JavaScript-string-literal context. Do not suggest replacing it with `Encode.forJavaScriptAttribute()` for these event-handler/JS-string-literal contexts; both are safe, but `forJavaScript()` is the general-purpose recommended choice for values placed in JS string literals.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T02:45:55.926Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1358
File: src/main/webapp/documentManager/showDocument.jsp:481-481
Timestamp: 2026-04-08T02:45:55.926Z
Learning: In this codebase’s JSPs (carlos-emr/carlos), it is an established convention to pass the numeric `docId` unquoted to `ForwardSelectedRows(docId + ':DOC', ...)` (i.e., relying on JS type coercion such as `123 + ":DOC"` → `"123:DOC"`). Do not flag these calls as inconsistent or fragile for missing quotes; do not “fix” by quoting only some occurrences, since that would create inconsistency. Ensure the value used for any JavaScript-context encoding is handled by adding/verifying `Encode.forJavaScriptAttribute(docId)` for `docId` where applicable before embedding it in JavaScript/HTML attributes.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T20:06:19.316Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1483
File: src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java:88-88
Timestamp: 2026-04-08T20:06:19.316Z
Learning: In `src/main/java/io/github/carlos_emr/carlos/encounter/oscarMeasurements/pageUtil/EctSetupMeasurements2Action.java` (Struts2 action, Java, carlos-emr/carlos), the `demographicNo` request parameter in the `else` branch of `execute()` is intentionally processed via `ConversionUtils.fromIntString(request.getParameter("demographicNo"))`, which silently returns `0` for null/non-numeric input. This is an accepted design choice: existing workflows may pass no value or an invalid value and rely on the default-to-0 fallback. Do NOT flag this as missing strict numeric validation or suggest replacing it with a `\\d+` regex guard + early return, as that could break those workflows.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-10T03:19:35.297Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-10T03:19:35.297Z
Learning: Applies to src/main/webapp/**/*.jsp : Use context-appropriate OWASP encoding: HTML body `${e:forHtml()}`, HTML attribute `${e:forHtmlAttribute()}`, JavaScript `${e:forJavaScript()}`, CSS `${e:forCssString()}`, URL `${e:forUri()}` or `${e:forUriComponent()}`
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-10T03:19:35.297Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-10T03:19:35.297Z
Learning: Applies to {src/main/webapp/**/*.jsp,src/main/java/io/github/carlos_emr/carlos/*/web/**Action.java} : Use OWASP Encoder for ALL user inputs: `Encode.forHtml()` for Java code, or `${e:forHtml()}` EL function for JSP (prefer EL functions in JSP)
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-10T03:19:35.297Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: GEMINI.md:0-0
Timestamp: 2026-04-10T03:19:35.297Z
Learning: Applies to src/main/webapp/**/*.jsp : Prefer JSP EL encoding functions `${e:...()}` over scriptlets; use JSP with JSTL/EL for cleaner, context-aware encoding
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-19T22:56:55.017Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T22:56:55.017Z
Learning: Applies to **/*.{jsp,jspf,java} : OWASP encoding is MANDATORY for all user inputs - use null-safe CARLOS wrappers: `<carlos:encode>` tag, `${carlos:forXxx()}` EL functions, or `SafeEncode.forXxx()` Java methods; do NOT use `<e:forXxx>`, `${e:forXxx(...)}`, or `Encode.forXxx(...)`
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-06T15:23:04.470Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1147
File: src/main/webapp/documentManager/documentBrowser.jsp:415-415
Timestamp: 2026-04-06T15:23:04.470Z
Learning: In `src/main/webapp/documentManager/documentBrowser.jsp` (carlos-emr/carlos), the file is heavily scriptlet-based. When adding OWASP output encoding in a security-fix PR, it is acceptable and expected to use scriptlet-style `Encode.forHtml(...)` / `Encode.forHtmlAttribute(...)` / `Encode.forJavaScript(...)` / `Encode.forJavaScriptAttribute(...)` calls via `<%= ... %>` rather than converting to the OWASP Encoder EL taglib (`${e:forHtml()}`). The EL taglib migration is a separate refactoring effort and should not be flagged as a blocker on security-fix PRs targeting this file.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-05T04:39:30.264Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1000
File: src/main/java/io/github/carlos_emr/carlos/report/pageUtil/Obec2Action.java:88-88
Timestamp: 2026-04-05T04:39:30.264Z
Learning: In `src/main/java/io/github/carlos_emr/carlos/report/pageUtil/Obec2Action.java` (carlos-emr/carlos), logging `startDate` (a `StrutsParameter` user input) at DEBUG level without `Encode.forJava()` in the `isDate()` validation-failure branch is considered an accepted minimal risk by the maintainer. Do not raise this as a blocking log-injection concern in future reviews of this file; flag it at most as a low-priority hardening note.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-02-25T21:12:00.588Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-25T21:12:00.588Z
Learning: Applies to {src/main/webapp/**/*.jsp,src/main/java/**/*2Action.java,src/main/java/**/web/*.java} : 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
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-19T22:56:55.017Z
Learnt from: CR
Repo: carlos-emr/carlos PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T22:56:55.017Z
Learning: Applies to **/*.jsp : All JSP views must use CARLOS null-safe encoding wrappers in all output contexts: HTML body, HTML attributes, JavaScript strings, CSS, and URL parameters; import `<%@ taglib uri="carlos" prefix="carlos" %>`
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-05T15:27:56.398Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1011
File: src/main/webapp/billing/CA/BC/wcbForms.jsp:0-0
Timestamp: 2026-04-05T15:27:56.398Z
Learning: In `src/main/webapp/billing/CA/BC/wcbForms.jsp` (carlos-emr/carlos), URL query parameter values embedded in onclick JavaScript attribute strings are pre-computed at the top of the scriptlet using layered encoding: `Encode.forJavaScriptAttribute(Encode.forUriComponent(StringUtils.noNull(value)))` for URL parameters (e.g., `encodedDemoNo`, `encodedBillingCode`, `encodedProvNo`), and `Encode.forJavaScriptAttribute(StringUtils.noNull(value))` for non-URL-parameter JS string values (e.g., `encodedDemoNoJs` used as a window name). Do not flag these pre-computed variables as missing URI encoding — the correct double-encoding is already applied. Integer DB primary key values like `wcb.getId()` returned as Java `int` carry zero XSS risk and do not require encoding.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-27T22:41:15.411Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 604
File: src/main/webapp/includes/global-head.jspf:0-0
Timestamp: 2026-03-27T22:41:15.411Z
Learning: In carlos-emr/carlos, `src/main/webapp/includes/global-head.jspf` intentionally provides only the jQuery UI CSS (`jquery-ui-1.14.2.min.css`), NOT the jQuery UI JS. Pages that need jQuery UI widgets (e.g., autocomplete, datepicker) must include `jquery-ui-1.14.2.min.js` explicitly in their own `<head>`. Do not flag missing or per-page jQuery UI JS includes as redundant/duplicate loads in this codebase.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-03-23T22:56:40.922Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 717
File: src/main/java/io/github/carlos_emr/carlos/commn/web/SearchDemographicAutoComplete2Action.java:108-112
Timestamp: 2026-03-23T22:56:40.922Z
Learning: In carlos-emr/carlos, the 4 non-jQuery callers of SearchDemographic.do (incomingDocs.jsp, MultiPageDocDisplay.jsp, FlowsheetAdd.jsp, quickBillingBC.jsp) use YUI AutoComplete with minQueryLength: 3, so a blank searchStr should never reach the server under normal use. The non-jQuery response contract is {"results":[...]} (not bare []).
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-06T04:55:02.451Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1116
File: src/main/webapp/billing/CA/BC/adjustBill.jsp:459-460
Timestamp: 2026-04-06T04:55:02.451Z
Learning: In `src/main/webapp/billing/CA/BC/adjustBill.jsp` (carlos-emr/carlos), `UpdateDate` is initialized to `""` at the top of the scriptlet and is only assigned its real value (via `MyDateFormat.getMyStandardDate(bill.getUpdateDate())`) around line 500, which is after the "Last update:" header renders it. This causes the header to always display a blank last-update value. This is a pre-existing ordering bug (not introduced by any XSS-fix PR) and is tracked in issue `#1299`. Do not flag the `Encode.forHtml(UpdateDate)` call as incorrect — the encoding is safe and correct; only the assignment ordering is wrong.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T00:07:39.186Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1358
File: src/main/webapp/lab/CA/ALL/labDisplay.jsp:942-957
Timestamp: 2026-04-08T00:07:39.186Z
Learning: In `src/main/webapp/lab/CA/ALL/labDisplay.jsp` (and `src/main/webapp/documentManager/showDocument.jsp`) in carlos-emr/carlos, the CSRF token is intentionally populated via an async IIFE that fetches `/<contextPath>/csrfguard`, parses `masterTokenValue` from the returned JavaScript, and sets it on all `input[name="CSRF-TOKEN"]` elements. This dynamic fetch pattern is working as designed. Do not flag this as a race condition or suggest replacing it with a hidden form injection approach for these files.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
📚 Learning: 2026-04-08T00:07:39.165Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1358
File: src/main/webapp/documentManager/showDocument.jsp:446-446
Timestamp: 2026-04-08T00:07:39.165Z
Learning: In `src/main/webapp/documentManager/showDocument.jsp` (carlos-emr/carlos), the `<input type="hidden" name="CSRF-TOKEN" value="">` inside `acknowledgeForm_<%=docId%>` is intentionally empty. It is hydrated asynchronously at page load by a self-invoking JS function that fetches `/csrfguard`, parses `masterTokenValue` via regex, and populates all `input[name="CSRF-TOKEN"]` elements. Do not flag this empty value as missing CSRF protection — the token is filled by JS before user interaction in normal page load scenarios.
Applied to files:
src/main/webapp/WEB-INF/jsp/demographic/add.jsp
Comment on lines
273
to
+276
| var selectBox = document.adddemographic.month_of_birth; | ||
| var mm = selectBox.options[selectBox.selectedIndex].value | ||
| var mm = selectBox.options[selectBox.selectedIndex].value; | ||
| selectBox = document.adddemographic.date_of_birth; | ||
| var dd = selectBox.options[selectBox.selectedIndex].value | ||
| var dd = selectBox.options[selectBox.selectedIndex].value; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
selectedIndex can be -1 here.
document.adddemographic.month_of_birth.selectedIndex / date_of_birth.selectedIndex can be -1 if the select has no rendered/selectable option (e.g. while the form is partially populated by autofill or the include hasn't rendered yet), and selectBox.options[-1].value throws TypeError, killing the submit pipeline before any of the i18n alert()s fire. Pre-existing pattern, but easy to harden:
- var mm = selectBox.options[selectBox.selectedIndex].value;
+ var mm = selectBox.selectedIndex >= 0 ? selectBox.options[selectBox.selectedIndex].value : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp` around lines 273 - 276, The
code reads selectBox.options[selectBox.selectedIndex].value for
document.adddemographic.month_of_birth and date_of_birth without validating
selectedIndex, which can be -1 and throw a TypeError; update the logic in the
block that sets mm and dd to first check selectBox.selectedIndex >= 0 (or that
selectBox.options[selectBox.selectedIndex] exists) before accessing .value and
provide a safe fallback (e.g., empty string or null) when no selection exists so
the submit pipeline and subsequent i18n alerts won't be interrupted; apply this
check for both month_of_birth and date_of_birth assignments (variables mm and
dd).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 8 files
Confidence score: 2/5
- There is a high-confidence security risk in
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp: referral doctor names are rendered without context-specific HTML escaping, which can allow markup/script injection from database values. - Because this is a concrete, user-facing XSS vector (severity 8/10, confidence 9/10), the merge risk is high until output encoding is corrected for both attribute and content contexts.
- Pay close attention to
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp- ensure referral doctor values are properly escaped before rendering in option text/attributes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp">
<violation number="1" location="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp:179">
P1: Escape referral doctor names for HTML attribute/content before rendering this option; unescaped DB values can inject markup.
(Based on your team's feedback about context-specific HTML encoding in JSP attribute vs content contexts.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant User
participant Browser as Browser (Client-Side JS)
participant JSP as JSP Engine (Server-Side)
participant RB as Resource Bundles (.properties)
participant DB as Database (DAOs)
Note over User,DB: Demographic Registration Flow (i18n enabled)
User->>Browser: Access "Add Demographic" Page
Browser->>JSP: GET /demographic/add.jsp
rect rgb(240, 248, 255)
Note right of JSP: Server-Side Localization
JSP->>JSP: Get request locale (EN, ES, FR, etc.)
JSP->>RB: NEW: Load oscarResources for locale
RB-->>JSP: Return localized string bundles
JSP->>DB: Fetch active doctors/nurses/specialists
DB-->>JSP: Provider lists
end
JSP->>JSP: CHANGED: Render HTML using <fmt:message> labels
JSP->>JSP: NEW: Construct 'i18n' JSON object for JavaScript alerts
JSP-->>Browser: Return HTML + Localized JS Strings
Note over Browser: User interacts with form
alt Validation Failure (e.g., Invalid DOB or HIN)
Browser->>Browser: NEW: JavaScript validation triggered (aSubmit)
Browser->>Browser: Access i18n[key] for localized message
Browser-->>User: alert/confirm(localized message)
else Validation Success
Browser->>JSP: POST form data
JSP->>DB: Save new Demographic Record
DB-->>JSP: Success
JSP-->>Browser: Redirect to record view
end
Note over Browser,RB: NEW: Support for EN, ES, FR, PL, PT-BR bundles
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Failed to generate code suggestions for PR |
Updated JSP file to use encoding functions for HTML attributes and content to prevent XSS vulnerabilities. Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
move from isolated year month date to consistent flatpickr input (relegating historic ones to hidden fields) Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Added required attributes and validation messages for various input fields in the personal information form. Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Updated form validation to use Bootstrap styles and removed unnecessary alerts. Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Collaborator
Author
|
@yingbull its ready to review |
Contributor
There was a problem hiding this comment.
Sorry @phc007, your pull request is larger than the review limit of 150000 diff characters
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp`:
- Around line 176-199: The option value is HTML attribute context so replace
Encode.forJavaScript(...) with Encode.forHtmlAttribute(...) for the <option
value> (reference: r_doctor and vecRef/prop) and add a data-referral-no="<%=
Encode.forHtmlAttribute(prop.getProperty("referral_no","")) %>" attribute on
each <option>; then simplify the changeRefDoc() function to read the selected
option element (document.forms[1].r_doctor.options[selectedIndex]) and use its
dataset.referralNo (or getAttribute('data-referral-no')) to set
document.forms[1].r_doctor_ohip.value, removing the server-side loop in the
script that attempted string matching.
In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp`:
- Around line 555-573: The validation script runs before the .needs-validation
form exists so no submit handler is bound and the novalidate form bypasses
checks; fix by delaying binding until the DOM is ready (e.g., wrap the existing
IIFE in a DOMContentLoaded/window.load listener or move the script to run after
the form) so document.querySelectorAll('.needs-validation') finds the form and
attaches the submit listener, and ensure the submit handler invokes the existing
aSubmit() (or calls the same duplicate/HIN/DOB/postal/roster validation flow)
when form.checkValidity() passes so those checks run before actual submission.
- Around line 534-550: The parseDateField function is writing to non-existent
IDs and using the wrong suffix (_day) for the legacy field (_date); update
parseDateField to locate the legacy companion inputs by name (e.g.
document.querySelector(`[name="${fieldId}_year"]`) / `${fieldId}_month` /
`${fieldId}_date`) instead of getElementById, check for null before assigning
values, and set the date part into the `${fieldId}_date` element (not
`${fieldId}_day`); keep the input split logic and only assign when the target
element exists to avoid null.value errors.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18a1daf7-1547-443f-b044-5bed354da6ef
📒 Files selected for processing (3)
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add-form-personal.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
Contributor
There was a problem hiding this comment.
3 issues found across 8 files
Confidence score: 1/5
- Merge risk is critical because
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsphas a confirmed JSP compilation failure (Encodenot imported), which is likely merge-blocking for that page. - There is also a high-impact encoding bug in
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp:Encode.forJavaScriptis used in an HTML attribute context; switching toSafeEncode.forHtmlAttributeis needed to avoid incorrect output handling. src/main/webapp/WEB-INF/jsp/demographic/add.jspappears to initialize Bootstrap validation before forms exist in the DOM, so.needs-validationhandlers are skipped whilenovalidatedisables native validation, creating a concrete user-facing validation regression.- Pay close attention to
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspandsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp- compile failure and form validation wiring need correction before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/WEB-INF/jsp/demographic/add.jsp">
<violation number="1" location="src/main/webapp/WEB-INF/jsp/demographic/add.jsp:556">
P2: The Bootstrap validation handler runs before the form is in the DOM, so `.needs-validation` forms are never wired up and `novalidate` disables native validation.</violation>
</file>
<file name="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp">
<violation number="1" location="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp:98">
P0: JSP compilation error: `Encode` is not imported. Use the fully qualified `io.github.carlos_emr.carlos.utility.SafeEncode` instead.
(Based on your team's feedback about using the SafeEncode null-safe wrappers in JSPs.) [FEEDBACK_USED]</violation>
<violation number="2" location="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp:181">
P1: Using `Encode.forJavaScript` inside an HTML attribute is incorrect. Additionally, `Encode` is not imported. Use `SafeEncode.forHtmlAttribute` instead.
(Based on your team's feedback about context-specific HTML encoding and using SafeEncode.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant User
participant Browser as Browser (UI/JS)
participant Server as Server (JSP Engine)
participant i18n as Resource Bundles (oscarResources)
participant Backend as Demographic Action
Note over User,Backend: Page Initialization Flow
User->>Server: GET /demographic/add.jsp
Server->>i18n: NEW: Fetch localized JS strings (EN/ES/FR/PL/PT-BR)
i18n-->>Server: Return alert/confirm message templates
Server->>Server: CHANGED: Encode HTML attributes using OWASP Encoder (XSS Fix)
Server-->>Browser: Return rendered HTML with NEW: Bootstrap 5 styles
Note over User,Backend: Data Input & Validation Flow
User->>Browser: Select DOB / Roster Date
Browser->>Browser: NEW: Initialize flatpickr date component
Browser->>Browser: NEW: Synchronize single date input to hidden legacy Y/M/D fields
User->>Browser: Submit Form
Browser->>Browser: NEW: Bootstrap client-side validation check
alt Validation Fails (e.g. Invalid Postal Code/Sex)
Browser->>Browser: NEW: Fetch localized message from i18n JS object
Browser-->>User: Display localized Bootstrap alert
else Duplicate Check
Browser->>Browser: NEW: confirmDuplicatePatient (i18n)
User-->>Browser: Confirm creation
end
Note over Browser,Backend: Submission Flow
Browser->>Server: POST /demographic/add2.action
Server->>Backend: Map request parameters (including NEW: roster_date)
Backend-->>User: Success/Redirect to Demographic Record
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Failed to generate code suggestions for PR |
Refactor date input handling and improve form validation. Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp">
<violation number="1" location="src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp:190">
P0: The refactor left stale lines after `changeRefDoc()`, so the script contains orphan JSP/JS code (`<% } %>` and `refNo`) and can fail to compile/execute.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Collaborator
Author
|
@claude the JSP doesn't seem to compile |
Collaborator
Author
|
@codex the jsp doesn't seem to compile in testing |
|
To use Codex here, create a Codex account and connect to github. |
yingbull
previously approved these changes
Apr 27, 2026
Collaborator
|
I'll sort out the compile issue unless you grab it before merge. |
Collaborator
|
@copilot Resolve the compile issue on the jsps involved in this PR. |
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/b6d92603-b127-44b3-9ca5-e4932c2266f7 Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Contributor
Addressed in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Sorry @phc007, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Failed to generate code suggestions for PR |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp (1)
355-365:⚠️ Potential issue | 🟠 MajorEncode PHU lookup values before rendering.
LookupListItem.valueandLookupListItem.labelare emitted raw here. Those are lookup-list data, not constants, so a malformed or hostile entry can break the attribute/body context in this<select>.🛡️ Proposed fix
- <option value="<%=llItem.getValue()%>" <%=selected%>><%=llItem.getLabel()%></option> + <option value="<%= SafeEncode.forHtmlAttribute(llItem.getValue()) %>" <%= selected %>><%= SafeEncode.forHtml(llItem.getLabel()) %></option>As per coding guidelines: "All JSP views must use CARLOS null-safe encoding wrappers in all output contexts: HTML body, HTML attributes, JavaScript strings, CSS, and URL parameters."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp` around lines 355 - 365, The PHU lookup item values and labels are emitted raw into an HTML attribute and body via llItem.getValue() and llItem.getLabel(); wrap both outputs with the CARLOS null-safe encoding helpers for HTML attribute and HTML body contexts respectively (use the attribute encoder for the option value and the HTML/body encoder for the option text) while preserving the selected logic that checks llItem.getValue() against defaultPhu; update the option rendering inside the loop that iterates LookupListItem (llItem) so all outputs are encoded before being written to the page.
♻️ Duplicate comments (1)
src/main/webapp/WEB-INF/jsp/demographic/add.jsp (1)
679-694:⚠️ Potential issue | 🟠 MajorReattach
aSubmit()to the submit pipeline.This listener only runs native
checkValidity(). Because the form isnovalidateand there is no other visibleonsubmit, the duplicate-patient, HIN, DOB, postal-code, roster-date, and iframe-submit logic inaSubmit()never executes. Also note: once you wireaSubmit()back in, the stalecheckResidentStatus()/rsidbranch needs to be removed or guarded, becauseadd-form-clinical.jspno longer renders that field.🐛 Proposed fix
Array.from(forms).forEach(form => { form.addEventListener('submit', event => { - if (!form.checkValidity()) { + const valid = form.checkValidity() && aSubmit() + if (!valid) { event.preventDefault() event.stopPropagation() } form.classList.add('was-validated') }, false) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp` around lines 679 - 694, Reattach the existing aSubmit() handler into the form submission flow so its custom checks (duplicate-patient, HIN, DOB, postal-code, roster-date and iframe-submit) run instead of only native checkValidity(); e.g. have the DOMContentLoaded listener call or delegate to aSubmit on submit (ensure you still preventDefault/stopPropagation as needed and add the 'was-validated' class after aSubmit completes) so novalidate forms invoke your bespoke logic; also remove or guard the stale checkResidentStatus()/rsid branch inside aSubmit (or any code path it calls) because add-form-clinical.jsp no longer renders rsid.
🤖 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/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp`:
- Around line 287-290: The patient status options are rendered raw from
demographicDao.search_ptstatus() and need OWASP HTML encoding to prevent XSS;
update the option generation in add-form-clinical.jsp to wrap the option value
with SafeEncode.forHtmlAttribute(status) and the option label with
SafeEncode.forHtml(status) (matching the roster-status approach) so both the
value attribute and displayed text are null-safe and encoded; keep the loop over
demographicDao.search_ptstatus() and only replace the raw <%=status%> usages
with the two SafeEncode calls.
- Around line 558-579: The radio inputs currently use safeConsentTypeKey for
their name, causing a mismatch with server-side reads
(request.getParameter(consentType.getType())); change the two radio input name
attributes to use rawConsentTypeKey (name="${ rawConsentTypeKey }") while
keeping id and label for attributes using safeConsentTypeKey
(id="optin_${safeConsentTypeKey}", for="optin_${safeConsentTypeKey}" etc.);
leave the clear button and the consentClearBtn call using safeConsentTypeKey as
they target DOM ids.
---
Outside diff comments:
In `@src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp`:
- Around line 355-365: The PHU lookup item values and labels are emitted raw
into an HTML attribute and body via llItem.getValue() and llItem.getLabel();
wrap both outputs with the CARLOS null-safe encoding helpers for HTML attribute
and HTML body contexts respectively (use the attribute encoder for the option
value and the HTML/body encoder for the option text) while preserving the
selected logic that checks llItem.getValue() against defaultPhu; update the
option rendering inside the loop that iterates LookupListItem (llItem) so all
outputs are encoded before being written to the page.
---
Duplicate comments:
In `@src/main/webapp/WEB-INF/jsp/demographic/add.jsp`:
- Around line 679-694: Reattach the existing aSubmit() handler into the form
submission flow so its custom checks (duplicate-patient, HIN, DOB, postal-code,
roster-date and iframe-submit) run instead of only native checkValidity(); e.g.
have the DOMContentLoaded listener call or delegate to aSubmit on submit (ensure
you still preventDefault/stopPropagation as needed and add the 'was-validated'
class after aSubmit completes) so novalidate forms invoke your bespoke logic;
also remove or guard the stale checkResidentStatus()/rsid branch inside aSubmit
(or any code path it calls) because add-form-clinical.jsp no longer renders
rsid.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fff78c38-96e6-48c6-93dc-4d2ad0dda3cb
📒 Files selected for processing (2)
src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jspsrc/main/webapp/WEB-INF/jsp/demographic/add.jsp
Comment on lines
+558
to
+579
| <c:set var="rawConsentTypeKey" value="${ consentType.type }"/> | ||
| <c:set var="safeConsentTypeKey" value="${ fn:replace(rawConsentTypeKey, ' ', '_') }"/> | ||
| <div class="row mb-2 align-items-start privacyConsentRow" id="privacyConsentRow_${count.index}"> | ||
| <div class="col-sm-2 text-end"> | ||
| <span class="fw-bold">${carlos:forHtml(consentType.name)}</span> | ||
| </div> | ||
| <div class="col-sm-6"> | ||
| ${carlos:forHtml(consentType.description)} | ||
| </td> | ||
|
|
||
| <td id="consentStatusDate" style="width:31%;vertical-align:top;"> | ||
| <input type="radio" | ||
| name="${ consentType.type }" | ||
| id="optin_${ consentType.type }" | ||
| value="0" | ||
| /> | ||
| <label for="optin_${ consentType.type }"><fmt:message key="demographic.demographicaddrecordhtm.optIn"/></label> | ||
| <input type="radio" | ||
| name="${ consentType.type }" | ||
| id="optout_${ consentType.type }" | ||
| value="1" | ||
| /> | ||
| <label for="optout_${ consentType.type }"><fmt:message key="demographic.demographicaddrecordhtm.optOut"/></label> | ||
| <input type="button" | ||
| name="clearRadio_${consentType.type}_btn" | ||
| onclick="consentClearBtn('${consentType.type}')" value="<fmt:message key='demographic.demographicaddrecordhtm.clear'/>"/> | ||
|
|
||
| </td> | ||
|
|
||
| </tr> | ||
| </div> | ||
| <div class="col-sm-4" id="consentStatusDate"> | ||
| <div class="form-check form-check-inline"> | ||
| <input type="radio" name="${ safeConsentTypeKey }" id="optin_${safeConsentTypeKey}" value="0" class="form-check-input"/> | ||
| <label class="form-check-label" for="optin_${ safeConsentTypeKey }"><fmt:message key="demographic.demographicaddrecordhtm.optIn"/></label> | ||
| </div> | ||
| <div class="form-check form-check-inline"> | ||
| <input type="radio" name="${ safeConsentTypeKey }" id="optout_${safeConsentTypeKey}" value="1" class="form-check-input"/> | ||
| <label class="form-check-label" for="optout_${ safeConsentTypeKey }"><fmt:message key="demographic.demographicaddrecordhtm.optOut"/></label> | ||
| </div> | ||
| <input type="button" class="btn btn-outline-secondary btn-sm" | ||
| name="clearRadio_${safeConsentTypeKey}_btn" | ||
| onclick="consentClearBtn('${safeConsentTypeKey}')" value="<fmt:message key='demographic.demographicaddrecordhtm.clear'/>"/> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
src/main/java/io/github/carlos_emr/carlos/demographic/pageUtil/DemographicAddRecord2Action.java \
src/main/java/io/github/carlos_emr/carlos/demographic/pageUtil/DemographicUpdate2Action.java
do
echo "=== $f ==="
rg -n -C2 'ConsentType|patientConsentManager|getParameter\(' "$f"
echo
doneRepository: carlos-emr/carlos
Length of output: 35057
Keep the raw consent type for the radio name attribute.
The Java actions (DemographicAddRecord2Action, DemographicUpdate2Action) read consent parameters using request.getParameter(consentType.getType()), where consentType.getType() returns the raw consent type with spaces preserved. Using safeConsentTypeKey (with spaces replaced by underscores) for the radio name will cause a parameter mismatch: the form will post "Health_Records" but the action will request "Health Records", resulting in a null and binding failure.
Use rawConsentTypeKey for the radio name, and keep safeConsentTypeKey only for id and for attributes where spaces are problematic in HTML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/webapp/WEB-INF/jsp/demographic/add-form-clinical.jsp` around lines
558 - 579, The radio inputs currently use safeConsentTypeKey for their name,
causing a mismatch with server-side reads
(request.getParameter(consentType.getType())); change the two radio input name
attributes to use rawConsentTypeKey (name="${ rawConsentTypeKey }") while
keeping id and label for attributes using safeConsentTypeKey
(id="optin_${safeConsentTypeKey}", for="optin_${safeConsentTypeKey}" etc.);
leave the clear button and the consentClearBtn call using safeConsentTypeKey as
they target DOM ids.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Signed-off-by: phc007 <11081221+phc007@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/webapp/WEB-INF/jsp/demographic/add.jsp">
<violation number="1" location="src/main/webapp/WEB-INF/jsp/demographic/add.jsp:670">
P2: The new `onsubmit="return aSubmit()"` path causes `eform_iframe` to be submitted twice (once in `aSubmit`, again in `checkFormTypeIn`), which can trigger duplicate side effects on submit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| <jsp:include page="/demographic/ViewZdemographicFullTitleSearch" /> | ||
|
|
||
| <form method="post" id="adddemographic" name="adddemographic" action="DemographicAddRecord" novalidate class="needs-validation" onsubmit="return aSubmit()" autocomplete="off"> |
Contributor
There was a problem hiding this comment.
P2: The new onsubmit="return aSubmit()" path causes eform_iframe to be submitted twice (once in aSubmit, again in checkFormTypeIn), which can trigger duplicate side effects on submit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/webapp/WEB-INF/jsp/demographic/add.jsp, line 670:
<comment>The new `onsubmit="return aSubmit()"` path causes `eform_iframe` to be submitted twice (once in `aSubmit`, again in `checkFormTypeIn`), which can trigger duplicate side effects on submit.</comment>
<file context>
@@ -667,7 +667,7 @@
<jsp:include page="/demographic/ViewZdemographicFullTitleSearch" />
- <form method="post" id="adddemographic" name="adddemographic" action="DemographicAddRecord" novalidate class="needs-validation">
+ <form method="post" id="adddemographic" name="adddemographic" action="DemographicAddRecord" novalidate class="needs-validation" onsubmit="return aSubmit()" autocomplete="off">
<jsp:include page="add-form-personal.jsp"/>
</file context>
|
Collaborator
|
@copilot Verify each finding against the current code and only fix it if needed. In |
Agent-Logs-Url: https://github.com/carlos-emr/carlos/sessions/f385ff14-19cb-4a13-bc42-058d4f5567a7 Co-authored-by: yingbull <8680161+yingbull@users.noreply.github.com>
Contributor
Addressed in
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Fixes #1992
How Was This Tested?
Screenshots
before
after
with invalid inputs
base form
Checklist
git commit -s)Summary by cubic
Completes i18n for Demographic Add (Linear 1992), unifies DOB/roster to one
flatpickrfield with stricter checks, and hardens JSPs against XSS. Fixes JSP compile errors so the add flow builds cleanly.Bug Fixes
Refactors
flatpickracross personal and clinical; kept hidden Y/M/D for compatibility.changeRefDoc; streamlined Bootstrap validation; encoded attributes/content inadd-form-clinical.jsp,add-form-personal.jsp, andadd.jsp.Written for commit c1efee9. Summary will update on new commits. Review in cubic