Displaying the case definition text with the actual format.#13937
Open
KarnaiahPesula wants to merge 3 commits into
Open
Displaying the case definition text with the actual format.#13937KarnaiahPesula wants to merge 3 commits into
KarnaiahPesula wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughCaseDataForm replaces URL-only linkification with a combined regex detecting HTML tag fragments or http(s) URLs, unescapes HTML entities, sanitizes with a controlled Jsoup Safelist via HtmlHelper, preserves safe tag fragments, and converts plain-text URLs into secure external links. ChangesText Sanitization and URL Linkification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java`:
- Around line 1663-1668: The code in CaseDataForm that appends matcher.group(1)
(htmlTag) directly into result (the htmlTag/result.append path) allows XSS;
change it to allowlist safe tags only (e.g., div, span, p, br, b, i, ul, li) and
only append the tag if htmlTag's name and attributes match that allowlist,
otherwise escape or strip the tag before appending; locate the logic around
matcher.group(1)/htmlTag/result.append and enforce the allowlist check (or
replace manual checks with a call to an HTML sanitizer such as OWASP Java HTML
Sanitizer) so no untrusted tags/attributes (like script or on* handlers) are
passed through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c9f8774-acf7-4cb2-a819-7090d1d785cc
📒 Files selected for processing (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java`:
- Around line 1674-1684: The current sanitization in CaseDataForm (the htmlTag
handling that checks lowerTag.contains("onclick"/"onerror"/"onload") and then
calls escapeHtml(htmlTag) or result.append(htmlTag)) is incomplete and allows
many XSS bypasses; replace this custom blocklist with a robust sanitizer: call
HtmlHelper.cleanHtmlRelaxed(htmlTag, HtmlHelper.EVENTACTION_WHITELIST) (or the
existing jsoup Safelist-based helper) for each fragment instead of manual string
checks, ensuring all attributes matching /^on\w+/ and dangerous URL schemes like
data: and vbscript: are removed; update the code path that currently uses
escapeHtml(htmlTag) / result.append(htmlTag) to use the cleaned output and
remove the manual lowerTag contains checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5832b922-b002-4a46-98b9-36c052717003
📒 Files selected for processing (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java (1)
1682-1690:⚠️ Potential issue | 🟠 Major | ⚡ Quick winURLs with ampersands will be double-encoded and break.
After Jsoup sanitization,
&in text becomes&. When a URL likehttps://example.com?a=1&b=2is matched from sanitized text, it's alreadyhttps://example.com?a=1&b=2. ApplyingescapeHtml()again produceshttps://example.com?a=1&b=2, breaking the link.🐛 Proposed fix to handle pre-encoded URLs
} else if (url != null) { // It's a plain-text URL. Wrap it in your custom blue link styling. - String escapedUrl = escapeHtml(url); + // URL from sanitized text may already have HTML entities - decode first, then encode + String decodedUrl = url.replace("&", "&").replace("<", "<").replace(">", ">").replace(""", "\"").replace("&`#39`;", "'"); + String escapedUrl = escapeHtml(decodedUrl); result.append("<a href=\"") .append(escapedUrl) .append("\" target=\"_blank\" rel=\"noopener noreferrer\" style=\"color: `#197de1`; text-decoration: underline;\">") .append(escapedUrl) .append("</a>"); }🤖 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 `@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java` around lines 1682 - 1690, The matched URL from sanitized text may already contain HTML entities (e.g., "&") and calling escapeHtml(url) double-encodes ampersands; in CaseDataForm (the block that builds the "<a href=...>" using escapeHtml(url)) first unescape any existing HTML entities (e.g., via StringEscapeUtils.unescapeHtml4 or equivalent) to get the raw URL, then escape for HTML once for the href attribute and link text; ensure you use the unescaped-then-escaped value for both the href and the displayed text so ampersands are not double-encoded and the link remains valid.
🤖 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.
Inline comments:
In `@sormas-api/src/main/java/de/symeda/sormas/api/utils/HtmlHelper.java`:
- Around line 66-69: The new overload cleanHtmlRelaxed(String htmlText, Safelist
whitelist) lacks a null check for htmlText and will NPE in Jsoup.clean(); update
the method to mirror the other helpers (e.g., cleanHtml, cleanI18nString) by
returning null immediately if htmlText is null before creating
Document.OutputSettings or calling Jsoup.clean, keeping the existing use of
Safelist and OutputSettings.
---
Outside diff comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java`:
- Around line 1682-1690: The matched URL from sanitized text may already contain
HTML entities (e.g., "&") and calling escapeHtml(url) double-encodes
ampersands; in CaseDataForm (the block that builds the "<a href=...>" using
escapeHtml(url)) first unescape any existing HTML entities (e.g., via
StringEscapeUtils.unescapeHtml4 or equivalent) to get the raw URL, then escape
for HTML once for the href attribute and link text; ensure you use the
unescaped-then-escaped value for both the href and the displayed text so
ampersands are not double-encoded and the link remains valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfa85a35-ac63-4b50-89e2-3a817ee9d343
📒 Files selected for processing (2)
sormas-api/src/main/java/de/symeda/sormas/api/utils/HtmlHelper.javasormas-ui/src/main/java/de/symeda/sormas/ui/caze/CaseDataForm.java
Comment on lines
+66
to
+69
| public static String cleanHtmlRelaxed(String htmlText, Safelist whitelist) { | ||
| Document.OutputSettings outputSettings = new Document.OutputSettings().prettyPrint(false); | ||
| return Jsoup.clean(htmlText, "", whitelist, outputSettings); | ||
| } |
There was a problem hiding this comment.
Missing null check for htmlText parameter.
The new overload doesn't handle null input, unlike all other methods in this class (e.g., cleanHtml, cleanI18nString, the other cleanHtmlRelaxed). Passing null will cause a NullPointerException from Jsoup.clean().
🛡️ Proposed fix to add null handling
public static String cleanHtmlRelaxed(String htmlText, Safelist whitelist) {
+ if (htmlText == null) {
+ return "";
+ }
Document.OutputSettings outputSettings = new Document.OutputSettings().prettyPrint(false);
return Jsoup.clean(htmlText, "", whitelist, outputSettings);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static String cleanHtmlRelaxed(String htmlText, Safelist whitelist) { | |
| Document.OutputSettings outputSettings = new Document.OutputSettings().prettyPrint(false); | |
| return Jsoup.clean(htmlText, "", whitelist, outputSettings); | |
| } | |
| public static String cleanHtmlRelaxed(String htmlText, Safelist whitelist) { | |
| if (htmlText == null) { | |
| return ""; | |
| } | |
| Document.OutputSettings outputSettings = new Document.OutputSettings().prettyPrint(false); | |
| return Jsoup.clean(htmlText, "", whitelist, outputSettings); | |
| } |
🤖 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 `@sormas-api/src/main/java/de/symeda/sormas/api/utils/HtmlHelper.java` around
lines 66 - 69, The new overload cleanHtmlRelaxed(String htmlText, Safelist
whitelist) lacks a null check for htmlText and will NPE in Jsoup.clean(); update
the method to mirror the other helpers (e.g., cleanHtml, cleanI18nString) by
returning null immediately if htmlText is null before creating
Document.OutputSettings or calling Jsoup.clean, keeping the existing use of
Safelist and OutputSettings.
raulbob
approved these changes
May 26, 2026
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
2 participants
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.
Fixes #
Summary by CodeRabbit