fix: per-patient RxSessionBean isolation to prevent medication cross-…#479
fix: per-patient RxSessionBean isolation to prevent medication cross-…#479yingbull wants to merge 1 commit into
Conversation
…contamination Port of openo-beta/open-o PR #2261 ("Fix: missing meds when saving - LEGACY BUG") originally authored by LiamStanziani. When multiple prescription tabs are open simultaneously for different patients, a single shared "RxSessionBean" session attribute causes medications to disappear or mix between patients. This fix introduces per-patient session keys (RxSessionBean_<demographicNo>) with a servlet filter that swaps the correct bean into the legacy key per request, maintaining backward compatibility. Changes: - RxSessionBean: Add static getFromSession/saveToSession methods with per-patient keys - RxSessionFilter: New servlet filter for per-request bean swapping (web.xml mapped) - rxSessionInterceptor.js: AJAX interceptor injecting demographicNo into all Rx requests - CaseManagementView2Action, RxChoosePatient2Action, RxShowAllergy2Action: Use new session methods instead of direct session attribute access - RxWriteScript2Action: Add error handling for randomId parsing, String demographicNo setter - ListDrugs.jsp: Multi-tab patient lookup from request parameter - SearchDrug3.jsp, ViewScript.jsp, ViewScript2.jsp: Include interceptor, add resetStash, pass demographicNo through forms and iframes - generatePreviewPDF.jsp: Use getFromSession/saveToSession Original PR: openo-beta/Open-O#2261 Original Author: LiamStanziani https://claude.ai/code/session_01D1MB9CvCDC4XaJGCXgxtB3
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @yingbull, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural improvement to the prescription (Rx) module by implementing per-patient session isolation for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a crucial fix for preventing medication data cross-contamination between different patient tabs. It achieves this by isolating the RxSessionBean on a per-patient basis in the HTTP session, using a unique key derived from the patient's demographic number.
The implementation is robust, employing a combination of a new Servlet Filter (RxSessionFilter) to transparently manage session state for legacy code, and a JavaScript AJAX interceptor (rxSessionInterceptor.js) to ensure patient context is passed in client-side requests. The changes are propagated consistently across various Actions and JSPs.
The code is well-structured and includes important enhancements for error handling, particularly in RxWriteScript2Action. My review includes a couple of suggestions to further improve maintainability and debuggability by reducing code duplication and adding logging for suppressed exceptions. Overall, this is a high-quality and important contribution to the stability of the application.
| try { | ||
| randomIdInt = Integer.parseInt(randomId); | ||
| } catch (NumberFormatException e) { | ||
| logger.error("Invalid randomId parameter: {}", Encode.forJava(randomId)); | ||
| response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
| response.setContentType("application/json;charset=UTF-8"); | ||
| ObjectNode errorResponse = objectMapper.createObjectNode(); | ||
| errorResponse.put("error", "Invalid prescription identifier."); | ||
| response.getOutputStream().write(errorResponse.toString().getBytes(StandardCharsets.UTF_8)); | ||
| return null; | ||
| } | ||
| RxPrescriptionData.Prescription rx = bean.getStashItem2(randomIdInt); | ||
| if (rx == null) { | ||
| logger.error("Prescription not found in stash for randomId: {}. " + | ||
| "Session may have been reset or prescription was not properly staged.", | ||
| Encode.forJava(randomId)); | ||
| response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
| response.setContentType("application/json;charset=UTF-8"); | ||
| ObjectNode errorResponse = objectMapper.createObjectNode(); | ||
| errorResponse.put("error", "Prescription not found. Please refresh and try again."); | ||
| response.getOutputStream().write(errorResponse.toString().getBytes(StandardCharsets.UTF_8)); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The error handling logic for invalid randomId and for a null prescription object is duplicated. The code for setting the HTTP status, content type, and writing the JSON error response is nearly identical in both the catch block and the if (rx == null) block. This duplication can be avoided by extracting the response logic into a private helper method. This will make the code more maintainable and reduce redundancy.
Consider adding a helper method like this to the class:
private String sendErrorResponse(String errorMessage) throws IOException {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
response.setContentType("application/json;charset=UTF-8");
ObjectNode errorResponse = objectMapper.createObjectNode();
errorResponse.put("error", errorMessage);
response.getOutputStream().write(errorResponse.toString().getBytes(StandardCharsets.UTF_8));
return null;
}Then you can simplify the updateDrug method as shown in the suggestion.
int randomIdInt;
try {
randomIdInt = Integer.parseInt(randomId);
} catch (NumberFormatException e) {
logger.error("Invalid randomId parameter: {}", Encode.forJava(randomId));
return sendErrorResponse("Invalid prescription identifier.");
}
RxPrescriptionData.Prescription rx = bean.getStashItem2(randomIdInt);
if (rx == null) {
logger.error("Prescription not found in stash for randomId: {}. " +
"Session may have been reset or prescription was not properly staged.",
Encode.forJava(randomId));
return sendErrorResponse("Prescription not found. Please refresh and try again.");
}| } catch (NumberFormatException e) { | ||
| // Keep default value (0) if parse fails | ||
| } |
There was a problem hiding this comment.
Swallowing exceptions without logging can make debugging difficult. While the intent here may be to ignore invalid input for demographicNo, it would be beneficial to log the NumberFormatException at a DEBUG or WARN level. This would provide visibility into malformed requests without cluttering logs in production if set to a higher level.
} catch (NumberFormatException e) {
// Keep default value (0) if parse fails
logger.debug("Could not parse demographicNo '{}'. Keeping previous value.", RHS, e);
}There was a problem hiding this comment.
Pull request overview
This PR implements per-patient session isolation for the prescription (Rx) module to prevent medication data cross-contamination when multiple patient medication tabs are open simultaneously. The solution uses demographicNo-keyed session storage (RxSessionBean_<demographicNo>) with a servlet filter that swaps the correct bean before each request, maintaining backward compatibility with legacy code that reads the single RxSessionBean session attribute.
Changes:
- Introduced
RxSessionFilterto swap per-patient RxSessionBeans based on demographicNo request parameters - Added client-side AJAX interceptor (rxSessionInterceptor.js) to inject demographicNo into all Rx-related requests
- Enhanced RxSessionBean with static methods for per-patient session management (getFromSession/saveToSession)
- Updated key Rx actions and JSPs to use new session management pattern
- Added defensive error handling in RxWriteScript2Action for missing prescriptions in stash
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| RxSessionFilter.java | NEW: Servlet filter that swaps per-patient RxSessionBean into legacy session key before each Rx request |
| RxSessionBean.java | Added static methods (getFromSession/saveToSession) for per-patient session storage with dual-key strategy |
| rxSessionInterceptor.js | NEW: Client-side interceptor adds demographicNo to Prototype.js, jQuery AJAX calls, and Rx forms |
| web.xml | Registered RxSessionFilter for /oscarRx/*, /CaseManagementView.do, and generatePreviewPDF.jsp |
| RxWriteScript2Action.java | Added error handling for missing prescriptions and String-based demographicNo setter for Struts2 |
| RxShowAllergy2Action.java | Migrated to use RxSessionBean.getFromSession/saveToSession pattern |
| RxChoosePatient2Action.java | Migrated to use RxSessionBean.getFromSession/saveToSession pattern |
| CaseManagementView2Action.java | Migrated to use RxSessionBean.getFromSession/saveToSession pattern |
| ViewScript2.jsp | Added rxSessionInterceptor.js, encoded demographicNo in iframe src and hidden form field |
| ViewScript.jsp | Added rxSessionInterceptor.js, encoded demographicNo in iframe src and hidden form field |
| SearchDrug3.jsp | Added rxSessionInterceptor.js, proper encoding for demographicNo in forms and JavaScript |
| ListDrugs.jsp | Enhanced to retrieve patient data by demographicNo parameter with proper fallback logic |
| generatePreviewPDF.jsp | Updated to use per-patient RxSessionBean.getFromSession/saveToSession pattern |
| <% if (bean.getStashSize() > 0) { %> | ||
| <iframe id='preview' name='preview' width=420px height=890px | ||
| src="oscarRx/Preview2.jsp?scriptId=<%=bean.getStashItem(0).getScript_no()%>&rePrint=<%=reprint%>&pharmacyId=<%=request.getParameter("pharmacyId")%>" | ||
| src="oscarRx/Preview2.jsp?scriptId=<%=bean.getStashItem(0).getScript_no()%>&rePrint=<%=reprint%>&pharmacyId=<%=request.getParameter("pharmacyId")%>&demographicNo=<%=Encode.forUriComponent(Integer.toString(bean.getDemographicNo()))%>" |
There was a problem hiding this comment.
The src attribute for the iframe embeds request.getParameter("pharmacyId") directly inside a quoted HTML attribute without any HTML encoding, which allows an attacker who controls pharmacyId to inject malicious markup or JavaScript by including a quote and additional attributes or event handlers. For example, a crafted pharmacyId containing " onload="alert(1) would break out of the URL and execute script in the clinician’s browser. Ensure pharmacyId is treated as untrusted input by HTML-encoding it for attribute context (or strictly validating and using a trusted numeric ID value) before inserting it into the page.
| src="oscarRx/Preview2.jsp?scriptId=<%=bean.getStashItem(0).getScript_no()%>&rePrint=<%=reprint%>&pharmacyId=<%=request.getParameter("pharmacyId")%>&demographicNo=<%=Encode.forUriComponent(Integer.toString(bean.getDemographicNo()))%>" | |
| src="oscarRx/Preview2.jsp?scriptId=<%=bean.getStashItem(0).getScript_no()%>&rePrint=<%=reprint%>&pharmacyId=<%=Encode.forUriComponent(request.getParameter("pharmacyId"))%>&demographicNo=<%=Encode.forUriComponent(Integer.toString(bean.getDemographicNo()))%>" |
Reviewer's GuideImplements per-patient RxSessionBean isolation and multi-tab safety across the Rx workflow by introducing a session filter, a JS interceptor, and consistently propagating demographicNo through server and client code, while adding defensive checks and small UX fixes around stash management and prescription updates. Sequence diagram for Rx request with per-patient session isolationsequenceDiagram
actor User
participant Browser as Browser_rxSessionInterceptor
participant Server as ServletContainer
participant Filter as RxSessionFilter
participant Action as RxAction_or_JSP
participant Session as HttpSession
User->>Browser: Trigger Rx action (e.g. save, preview)
note over Browser: rxSessionInterceptor.js adds demographicNo
Browser->>Server: HTTP request with demographicNo
Server->>Filter: doFilter(req,res,chain)
Filter->>Session: getAttribute(RxSessionBean)
Filter->>Filter: parseDemographicNo(demographicNo)
alt demographicNo > 0
Filter->>Session: getAttribute(RxSessionBean_<demographicNo>)
alt per-patient bean exists
Filter->>Session: setAttribute(RxSessionBean, perPatientBean)
else explicit demographicNo but no bean
Filter->>Session: removeAttribute(RxSessionBean)
end
else demographicNo missing
Filter->>Session: use legacy RxSessionBean as fallback
end
Filter-->>Server: chain.doFilter()
Server->>Action: dispatch (execute/view/JSP)
Action->>Session: getAttribute(RxSessionBean)
note right of Action: Now bound to the correct patient
Action-->>Browser: Response (HTML/JSON)
Class diagram for per-patient RxSessionBean isolation and filterclassDiagram
class RxSessionBean {
<<Serializable>>
- Logger logger
- String SESSION_KEY_PREFIX
- String LEGACY_SESSION_KEY
- String providerNo
- int demographicNo
- String view
+ String getSessionKey(int demographicNo)
+ RxSessionBean getFromSession(HttpSession session, int demographicNo)
+ RxSessionBean getFromSession(HttpServletRequest request, int demographicNo)
+ void saveToSession(HttpSession session, RxSessionBean bean)
+ void saveToSession(HttpServletRequest request, RxSessionBean bean)
+ String getProviderNo()
+ void setProviderNo(String RHS)
+ int getDemographicNo()
+ void setDemographicNo(String RHS)
+ String getView()
+ void setView(String RHS)
}
class RxSessionFilter {
<<Filter>>
- Logger logger
- String LEGACY_KEY
+ void init(FilterConfig filterConfig)
+ void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
+ void destroy()
- int parseDemographicNo(String param)
}
class RxShowAllergy2Action {
+ String execute()
- void reorder(HttpServletRequest request)
}
class RxChoosePatient2Action {
+ String execute()
+ String getDemographicNo()
}
class CaseManagementView2Action {
+ String view()
}
class HttpSession
class HttpServletRequest
RxSessionFilter --> RxSessionBean : uses getSessionKey
RxSessionFilter --> HttpSession : accesses attributes
RxSessionFilter --> HttpServletRequest : reads parameters
RxShowAllergy2Action --> RxSessionBean : getFromSession(HttpSession,int)
RxShowAllergy2Action --> RxSessionBean : saveToSession(HttpSession,RxSessionBean)
RxChoosePatient2Action --> RxSessionBean : getFromSession(HttpServletRequest,int)
RxChoosePatient2Action --> RxSessionBean : saveToSession(HttpServletRequest,RxSessionBean)
CaseManagementView2Action --> RxSessionBean : getFromSession(HttpServletRequest,int)
CaseManagementView2Action --> RxSessionBean : saveToSession(HttpServletRequest,RxSessionBean)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Opened #1826 capturing design-pattern observations from unmerged upstream work by @chitrankd on the same underlying problem (Open-O branch The observations sketch a two-phase architectural follow-up — inner-class extraction, then moving Full rationale and suggested sequencing in #1826. |
Summary by Sourcery
Implement per-patient Rx session isolation to support multiple concurrent patient medication tabs without cross-contamination, while maintaining backward compatibility with existing session usage.
Bug Fixes:
Enhancements:
Documentation: