Skip to content

fix: variable sites shared across concurrent requests#2736

Open
boriskole wants to merge 6 commits into
carlos-emr:developfrom
boriskole:fix/2647-sites-variable-shared-across-concurrent-requests
Open

fix: variable sites shared across concurrent requests#2736
boriskole wants to merge 6 commits into
carlos-emr:developfrom
boriskole:fix/2647-sites-variable-shared-across-concurrent-requests

Conversation

@boriskole

@boriskole boriskole commented Jun 8, 2026

Copy link
Copy Markdown

Description

Moved the sites variable to a scriptlet block to ensure the data is not shared across concurrent requests anymore.

Related Issues

Fixes #2647

How Was This Tested?

Ran to verify

mvn -B -Pjspc package -DskipTests

Checklist

  • My commits are signed off for the DCO (git commit -s)
  • My commits follow Conventional Commits format, or I've written clear commit messages and will use the format next time
  • I have not included any patient data (PHI) in this PR
  • I have added tests for new functionality, or this change doesn't need new tests
  • I have read the contributing guide

Summary by cubic

Moved sites and bMultisites to per-request scope in schedulecreatedate.jsp to stop data leaking across concurrent requests; sites now defaults to Collections.emptyList(). Fixes #2647.

  • Bug Fixes
    • Scoped sites/bMultisites per request; load via SiteDao only when multi-site is enabled.
    • Tidied whitespace on the bgcolor attribute.

Written for commit 514ce1f. Summary will update on new commits.

Review in cubic

Signed-off-by: Boris <boris.kole@outlook.com>
@boriskole boriskole requested a review from yingbull as a code owner June 8, 2026 10:38
Copilot AI review requested due to automatic review settings June 8, 2026 10:38

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @boriskole, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adjusts multisite-related state in schedulecreatedate.jsp to be request-scoped (instead of JSP declaration fields) and cleans up HTML/JSP formatting in the schedule grid.

Changes:

  • Move sites / bMultisites from JSP declaration (<%! ... %>) to request-time scriptlet variables (<% ... %>).
  • Minor whitespace/line formatting change around the bgcolor table cell rendering.

Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp Outdated
Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors schedulecreatedate.jsp by moving the sites list and bMultisites flag declarations from the JSP declaration block to the scriptlet block, preventing shared state across requests. The review feedback suggests initializing sites to an empty list instead of null to avoid potential NullPointerExceptions when multisites are disabled.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp Outdated
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

JSP instance fields sites and bMultisites are removed from the declaration block and introduced as request-local variables (sites initialized to an empty list; bMultisites recomputed). A minor whitespace adjustment was made to the calendar grid <td> bgcolor expression.

Changes

Multisite Variable Scope Refactoring

Layer / File(s) Summary
Remove JSP instance declarations
src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
Deleted class-level sites and bMultisites declarations from the JSP declaration block.
Introduce request-local variables and recompute flag
src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
Added List<Site> sites = java.util.Collections.emptyList(); and recomputed boolean bMultisites = io.github.carlos_emr.carlos.commn.IsPropertiesOn.isMultisitesEnable(); inside the main scriptlet where rendering uses them.
Calendar cell markup formatting tweak
src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
Whitespace/line-break adjusted in the calendar grid <td> bgcolor attribute expression; no behavioral change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

  • #2677: Same pattern of moving JSP class-level vars to request-local scope in other schedule JSPs — related refactor.
  • #2647: Thread-safety bug report describing sites as a JSP instance variable causing data races; this PR addresses that issue.

Suggested reviewers

  • yingbull

Poem

🐰 I hopped from fields to local ground,
No shared-list tangles to be found,
Each request now gets its own treat,
Clean scopes, neat code — a carrot sweet! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: moving the 'sites' variable from JSP class-level to request-local scope to prevent concurrent request data sharing.
Linked Issues check ✅ Passed The PR addresses issue #2647 by moving 'sites' and 'bMultisites' from JSP declaration block to request-local variables, directly matching the issue's primary objective to fix the thread-safety data race.
Out of Scope Changes check ✅ Passed All changes are scoped to issue #2647: moving variables to per-request scope and a minor whitespace formatting adjustment to the bgcolor attribute are both in scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description clearly explains the change: moving the sites variable from a shared JSP member field to a request-local scriptlet variable to fix data sharing across concurrent requests, addressing issue #2647.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Boris <boris.kole@outlook.com>
@boriskole boriskole requested a review from Copilot June 8, 2026 10:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp Outdated
Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
%>
<%

List<Site> sites = Collections.emptyList();
Comment thread src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
Signed-off-by: Boris <boris.kole@outlook.com>
@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@cubic-dev-ai review

@cubic-dev-ai

cubic-dev-ai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai review
@cubic-dev-ai review

@Ben-Heerema I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@Ben-Heerema

Ben-Heerema commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

@coderabbitai review please?

@Ben-Heerema

Ben-Heerema commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

@boriskole awesome work! Idk why coderabbit isn't working so I won't request yingbull until I get that sorted but everything else is checking out! Thanks for your help :)

Note: I think sonar is failing due to missing secrets (token) so that is expected.

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@yingbull this PR looks good.

Copilot AI review requested due to automatic review settings June 11, 2026 16:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp:37

  • getSiteHTML(...) builds an HTML snippet using Site.bgColor and Site.shortName without any validation/encoding. Since these values come from the database/config (and are ultimately user-controlled via admin UI), this can enable CSS injection via the style attribute and XSS via the rendered text. This file already uses encoding elsewhere; it should apply the same hardening here (similar to scheduleflipview.jsp).
    private String getSiteHTML(String reason, List<Site> sites) {
        if (reason == null || reason.trim().length() == 0)
            return "";
        else
            return "<span style='background-color:" + ApptUtil.getColorFromLocation(sites, reason) + "'>" + ApptUtil.getShortNameFromLocation(sites, reason) + "</span>";

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/schedule/schedulecreatedate.jsp (1)

42-48: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Thread-safety fix is correct.

Moving sites from a JSP declaration (instance field) to a request-local scriptlet variable properly addresses the data race described in issue #2647. Initializing with Collections.emptyList() before conditional DAO population is a clean pattern.

Minor style note: java.util.Collections is redundant since line 62 imports java.util.*. Could simplify to Collections.emptyList().

Optional simplification
-    List<Site> sites = java.util.Collections.emptyList();
+    List<Site> sites = Collections.emptyList();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp` around lines 42
- 48, The thread-safety fix moved the previously shared JSP field into a
request-local variable 'sites' with initialization using
java.util.Collections.emptyList() and conditional population via bMultisites =
IsPropertiesOn.isMultisitesEnable() and SiteDao siteDao = (SiteDao)
WebApplicationContextUtils.getWebApplicationContext(application).getBean(SiteDao.class);
siteDao.getAllSites(); — simplify the style by replacing the fully-qualified
java.util.Collections.emptyList() with Collections.emptyList() (since
java.util.* is already imported) while keeping the same initialization and
conditional logic around 'bMultisites' and 'siteDao.getAllSites()'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp`:
- Around line 42-48: The thread-safety fix moved the previously shared JSP field
into a request-local variable 'sites' with initialization using
java.util.Collections.emptyList() and conditional population via bMultisites =
IsPropertiesOn.isMultisitesEnable() and SiteDao siteDao = (SiteDao)
WebApplicationContextUtils.getWebApplicationContext(application).getBean(SiteDao.class);
siteDao.getAllSites(); — simplify the style by replacing the fully-qualified
java.util.Collections.emptyList() with Collections.emptyList() (since
java.util.* is already imported) while keeping the same initialization and
conditional logic around 'bMultisites' and 'siteDao.getAllSites()'.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8205cf71-ea39-4949-87e3-8e2bb237ad04

📥 Commits

Reviewing files that changed from the base of the PR and between 56c494f and c3cfb5f.

📒 Files selected for processing (1)
  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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 use Encode.forHtml(value), with context-specific variants (forHtmlAttribute, forJavaScript, forCssString, forUri, forUriComponent)

Use OWASP Encoder null-safe CARLOS wrappers for all user input encoding: <carlos:encode> tag, ${carlos:forXxx()} EL functions, or SafeEncode.forXxx() Java methods. DO NOT use <e:forXxx>, ${e:forXxx()}, or Encode.forXxx() directly in new code.

Files:

  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.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 URI owasp.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> or fn: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 using fn: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: Add comprehensive JSP comment blocks after copyright headers documenting purpose, features, parameters, and @since for all JSP page templates
AJAX JSPs that read CSRF-TOKEN via document.querySelector('input[name="CSRF-TOKEN"]') must either contain at least one <form action="<real URL>" method="post"> OR include the canonical bootstrap fragment <%@ include file="/WEB-INF/jspf/csrf-token.jspf" %>

Files:

  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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 @since documentation
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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.jsp
**/*.{jsp,xml}

📄 CodeRabbit inference engine (CLAUDE.md)

JSP views for web pages must be placed under src/main/webapp/WEB-INF/jsp/** and actions should forward to internal WEB-INF paths, not public JSP files

Files:

  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
**/*

📄 CodeRabbit inference engine (CLAUDE.md)

Preserve all existing copyright headers in modified files; do not remove or change GPL version statements

Files:

  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
**/*.{java,jsp,xml,js,css}

📄 CodeRabbit inference engine (CLAUDE.md)

New files must use CARLOS project header as defined in docs/copyright-header-carlos.md

Files:

  • src/main/webapp/WEB-INF/jsp/schedule/schedulecreatedate.jsp
🧠 Learnings (10)
📚 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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.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/schedule/schedulecreatedate.jsp

@Ben-Heerema

Ben-Heerema commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for flagging the getSiteHTML(...) encoding concern. I agree it is a valid hardening issue, but it appears to be pre-existing and not introduced by this PR's request-scoping/thread-safety change.

I opened #2832 to track it separately: #2832

@Ben-Heerema Ben-Heerema marked this pull request as ready for review June 11, 2026 19:10
Copilot AI review requested due to automatic review settings June 11, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@yingbull I think this is good to go, the major bot review was an XSS/CSS concern but I made a separate ticket.

@boriskole

Copy link
Copy Markdown
Author

Hey, do I need to make some changes or is this PR fine?

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@cubic-dev-ai review
@coderabbitai review

@cubic-dev-ai

cubic-dev-ai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review
@coderabbitai review

@Ben-Heerema I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff
gh pr view 2736 --repo carlos-emr/carlos --json files,body,title

Length of output: 2607


🏁 Script executed:

#!/bin/bash
# Get the actual diff
gh pr diff 2736 --repo carlos-emr/carlos

Length of output: 1930


🏁 Script executed:

#!/bin/bash
# Check related JSP files for the same pattern
rg -n '<%!' --type=xml -g '*.jsp'

Length of output: 9634


🏁 Script executed:

#!/bin/bash
# Find and view the modified file plus related JSPs
fd -e jsp -i 'schedulecreatedate\|scheduleholidaysetting\|scheduleedittemplate' --exec cat {}

Length of output: 400


🏁 Script executed:

#!/bin/bash
# Find schedule-related JSPs and check for instance-field patterns
fd -e jsp . src/main/webapp/WEB-INF/jsp/schedule/ --exec echo {}

echo "---"
# Check scheduleholidaysetting, scheduleedittemplate and scheduledatepopup for mutable instance fields
for f in scheduleholidaysetting scheduleedittemplate scheduledatepopup scheduleflipview; do
  echo "=== Searching for $f ==="
  fd -e jsp -i "$f" --exec head -80 {}
done

Length of output: 16806


🏁 Script executed:

#!/bin/bash
# Check scheduledatepopup.jsp for the problematic pattern
cat src/main/webapp/WEB-INF/jsp/schedule/scheduledatepopup.jsp | head -100

Length of output: 4581


🏁 Script executed:

#!/bin/bash
# Look at the <%! %> blocks in schedule JSPs that might have mutable instance fields
rg -n 'private\s+\w.*;\s*$|<%!\s*\w' src/main/webapp/WEB-INF/jsp/schedule/

Length of output: 1266



@Ben-Heerema: The fix in schedulecreatedate.jsp is correct and well-implemented. The sites and bMultisites variables are now per-request locals, eliminating the data race. The getSiteHTML(...) helper method correctly stays in the <%! %> declaration block since it's a pure function with no mutable state. ✅

However, while reviewing the related schedule JSPs, I found two more files with the same class of bug that should be tracked as follow-up work:


🐛 scheduleflipview.jsp — identical issue (Lines 58–61)

<%!
    //multisite starts =====================
    private boolean bMultisites = io.github.carlos_emr.carlos.commn.IsPropertiesOn.isMultisitesEnable();
    private JdbcApptImpl jdbc = new JdbcApptImpl();  // shared stateful object
    private List<Site> sites;                          // data race — same bug as `#2647`
    private String[] curScheduleMultisite;             // data race

sites and curScheduleMultisite are mutable instance fields reassigned per request — exact same race condition fixed in this PR. jdbc being a shared stateful object is also risky.


🐛 scheduledatepopup.jsp — mutable instance field (Lines 32–33)

<%! boolean bMultisites = io.github.carlos_emr.carlos.commn.IsPropertiesOn.isMultisitesEnable(); %>
<%! String[] bgColors; %>  // mutable instance field — data race risk

bgColors as an instance field is a data race risk if reassigned per request.


Summary

File Status
schedulecreatedate.jsp ✅ Fixed by this PR
scheduleholidaysetting.jsp ✅ Clean (uses scriptlets only)
scheduleedittemplate.jsp ✅ Clean (uses scriptlets only)
scheduleflipview.jsp ❌ Same bug — needs follow-up
scheduledatepopup.jsp ❌ Mutable instance field — needs follow-up

Would you like me to open a GitHub issue to track the fixes needed in scheduleflipview.jsp and scheduledatepopup.jsp?

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@coderabbitai Thanks for checking. There are already tickets for those follow-ups: #2677 covers the scheduleflipview.jsp and scheduledatepopup.jsp JSP instance-variable/thread-safety issues, and #2832 covers the separate getSiteHTML(...) encoding/hardening concern.

No new ticket needed here; this PR should stay scoped to #2647.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Ben-Heerema: Understood — good to know those are already tracked. I'll keep future review commentary for this repo scoped accordingly. This PR looks clean and correctly addresses #2647. 🐇


✏️ Learnings added
Learnt from: Ben-Heerema
URL: https://github.com/carlos-emr/carlos/pull/2736

Timestamp: 2026-06-15T13:54:14.694Z
Learning: In the `carlos-emr/carlos` repository, the JSP instance-variable/thread-safety issues in `scheduleflipview.jsp` and `scheduledatepopup.jsp` are already tracked under issue `#2677`. Do not suggest opening a new issue for these when reviewing related PRs.

Learnt from: Ben-Heerema
URL: https://github.com/carlos-emr/carlos/pull/2736

Timestamp: 2026-06-15T13:54:14.694Z
Learning: In the `carlos-emr/carlos` repository, the `getSiteHTML(...)` XSS/encoding hardening concern in schedule JSPs is tracked under issue `#2832`. Do not suggest opening a new issue for this when reviewing related PRs.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@Ben-Heerema

Copy link
Copy Markdown
Collaborator

@boriskole it looks good as far as I can tell.
@yingbull ready for review

@Ben-Heerema Ben-Heerema added this to the June-2026-release milestone Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] schedulecreatedate.jsp: JSP instance variable 'sites' shared across concurrent requests — thread-safety data race (claude assist)

4 participants