Three security fixes in an open-source EMR

CARLOS is an open-source electronic medical record system. Real clinics run it, and the data inside it is protected health information. Over about six weeks I worked on three security issues in it: a stored XSS in the document edit form, PHI leaking into application logs from a REST endpoint, and a mutation firing as a GET request, which bypassed CSRF protection and put clinical note text in the URL. Two are merged. The third is approved and waiting on a second reviewer.

I found all three on the project's issue tracker. I didn't discover them. What I did was reproduce each one locally, trace it to the lines responsible, fix it in the pattern the codebase already used, and get it through maintainer review.

I picked the security issues because that's the direction I'm actually trying to go. I have Security+, Network+, and A+, and I take the cybersecurity coursework at Maryland because the work interests me, not because it's a box to check. What I want to be doing is building the defensive side of user-facing software: the encoding, the auth, the parts that decide whether a bug is an inconvenience or a breach. Picking an easier issue would have taught me the contribution workflow. Picking these taught me the workflow and the thing I actually care about.

On the AI assistance

This was CodePath's AI 301, which is about working in large unfamiliar codebases with AI assistance. CARLOS is a Java EMR with years of history in it and I had never seen a line of it.

I used AI for orientation: finding the relevant file, understanding what a JSP taglib was doing, getting my bearings in code I had no context for. I did the reproduction, the verification, and the review responses myself, because those are the parts where being wrong actually costs something. The alert firing in my browser was mine. The curl against the tickler endpoint with the session cookie was mine. Assistance narrowed where to look. It didn't decide whether the fix worked.

Getting it running

Before any of the fixes I had to get CARLOS running locally, and that took longer than the first fix did.

The devcontainer build hung on step 15 of 37, installing Playwright browsers. Playwright had nothing to do with what I was fixing, so I commented those lines out of .devcontainer/development/Dockerfile and ran docker-compose up -d --build directly instead of going through the VS Code devcontainer flow. Then make install got OOM killed. Setting export MAVEN_OPTS="-Xmx1512m -Xms512m" before running it fixed that.

That environment carried all three contributions.

Fix 1: stored XSS in the document edit form

Issue #2315. addedithtmldocument.jsp rendered five database-sourced values straight into the page with no encoding. Anything stored in those fields executed in the browser of whoever opened the document edit form.

To reproduce it I created an HTML document with <img src=x onerror=alert(1)> in the Source Author field, saved, and reopened it for editing. Page source showed value="<img src=x onerror=alert(1)>", the raw payload sitting unencoded in an HTML attribute. The alert fired.

The fix looks trivial and mostly is. The project already ships a carlos:encode taglib and it was already used on other fields in the same file. These five just never got wrapped.

The part worth explaining is that they don't all get wrapped the same way. The five locations sat in three different output contexts, and the encoding has to match the context the value lands in:

Line 287 is the one that makes the point. It renders into a JavaScript array literal inside a <script> block, and <script> is a raw-text element, meaning the HTML parser does not decode character references inside it. So HTML-escaping that value causes two problems at once.

It corrupts the data, because an entity like ' never gets decoded back to an apostrophe. It just sits there as literal text and users see ' in their dropdown. And it fails to secure anything, because HTML escaping targets the wrong character set. It doesn't neutralize a backslash, and it doesn't handle the JavaScript line terminators U+2028 and U+2029, either of which can break out of a string literal the JS parser is reading.

The javaScriptBlock context hex-escapes to sequences like \x26, \/, and \-, which the JS parser actually understands. That's why the encoder has separate contexts at all. The escaping has to match the parser that reads the output, not the one that produced it.

Verification was manual, since an output-encoding change doesn't map cleanly onto a unit test. I re-ran the original payload and confirmed page source now showed <img src=x onerror=alert(1)> and the alert no longer fired. For the two provider-name fields I could only verify visually, since they're DB-managed and not free text through this form. I said so in the PR rather than claiming coverage I didn't have.

I also hit a pre-existing bug while testing. No value at all survives a submit and re-edit on the Source Facility field, payload or not. Unrelated to my fix and out of scope for the issue, so I noted it and left it.

PR: carlos-emr/carlos#2927, merged.

Fix 2: PHI in the application logs

Issue #2982. TicklerWebService.java called MiscUtils.getLogger().info(json.toString()) in three mutation endpoints, writing the full raw request body to the application log at INFO level on every request.

The complete and delete payloads carry tickler IDs. The update payload is worse, since it includes message, taskAssignedTo, and serviceDate, which is clinical task content. Any log aggregator or monitoring tool ingesting those logs becomes a second, unmanaged store of patient-correlating data.

The Tickler UI went through a different code path than the REST endpoints, so I couldn't reproduce through the interface. I pulled JSESSIONID out of DevTools and hit the endpoints directly with curl while tailing catalina.out:

INFO rest.TicklerWebService (TicklerWebService.java:276) - {"ticklers":[1]}

One detail I noticed while reproducing: the log call fires before any business logic runs, so even a request that fails validation still leaks its full body.

The project already had a convention for this. PR #2611 established LogSafe.sanitize() for identifiers that genuinely need logging. I replaced all three raw calls with operational metadata at DEBUG level: what happened, how many ticklers, success or failure, and a sanitized ID where an ID was actually needed.

DEBUG rather than INFO was deliberate. The safe metadata stays suppressed in production and is there when someone turns debug logging on.

The bug in my own fix

Gemini Code Assist flagged that I was calling json.has() without checking whether json was null, so an empty or malformed request body would have thrown an NPE. That's a crash bug I introduced in a patch whose entire purpose was making an endpoint safer.

It wasn't a nitpick and I didn't argue with it. I added the guards in a follow-up commit. The lesson stuck harder than a style comment would have: defensive code is exactly the code that runs on malformed input, so it's the code that most needs its own guards. I write them upfront now instead of waiting for a reviewer.

PR: carlos-emr/carlos#3136, merged.

Fix 3: a mutation firing as GET

Issue #2795. saveNoteDialog() in ticklerMain.jsp called jQuery.ajax() without a method: key. jQuery defaults to GET. So saving a clinical note, which is a write, went out as a GET request.

Two things follow from that, and they're independent.

CSRFGuard only validates tokens on POST, PUT, DELETE, and PATCH. A mutation arriving as GET is never checked, which means an <img src="..."> tag on any page the user visits can trigger a note save.

And GET parameters live in the URL. So value, the free-text clinical note, and demographicNo, the patient ID, went into the query string, which Tomcat's access log records verbatim. The same data also lands in browser history and any proxy in between.

CaseManagementEntry?method=ticklerSaveNote&value=...&demographicNo=3&ticklerNo=1245

The fix is one line: method: "POST". No backend change was needed, since CaseManagementEntry already handled POST, and two sibling functions in the same file, openNoteDialog() at line 547 and saveView() at line 662, were already doing it correctly. This one had just been missed.

Finding the trigger took longer than the fix. The vulnerable call fired from a Save button inside the note dialog, not the note icon I kept clicking.

Calling a CI failure someone else's

After rebasing onto latest develop, the tests check started failing in the billing/Hibernate DB setup. My change was one line, in a JSP, touching no Java, no ORM, and nothing near billing. There's no path from that edit to a Hibernate setup failure. The failure also appeared only after picking up new commits from develop, not from anything on my branch.

So I flagged it to the maintainer as likely pre-existing rather than digging for a fault in my own change. Getting that call wrong in either direction costs something. Chase a failure that isn't yours and you burn a week. Wave off one that is and you ship a regression.

PR: carlos-emr/carlos#3298, open. Approved by one maintainer, and the repo requires two approving reviews from reviewers with write access before merge. Twenty-three of twenty-eight checks pass, including DCO sign-off, Semgrep SAST, and the project's own encode-lint.

What I'd do differently

On the XSS fix I patched exactly the five locations the issue named. What I should have done is check whether the same pattern existed elsewhere: same file, same taglib, other JSPs rendering database values without encoding. The issue reported five instances. It didn't establish that there were only five.

Fixing what's reported closes a ticket. Looking for variants of the same mistake closes a class of bug. CARLOS even runs an encode-lint check in CI, so there was probably a mechanical way to go looking. That's the habit I'd bring to the next one.