Add HTML sanitizer for translated message resources
keycloak/keycloak#37429
Golden recall: 50%
Extra findings: 6
Golden Comments (1/2 valid found, 2 N/A)
| # | Comment | Severity | Status | FriendlyReviewer Detail |
|---|---|---|---|---|
| #1 | Italian instead of Lithuanian in messages_lt.properties |
N/A | N/A | Out of scope: translation content (Italian vs Lithuanian). FriendlyReviewer is asked to review code, not to validate which natural language a string is written in. An LLM has linguistic knowledge and could detect this if tasked as a translation-QA step, but that's not what the code review prompt asks for. |
| #2 | totpStep1 uses Traditional Chinese terms in Simplified Chinese file (zh_CN) |
N/A | N/A | Same — translation content, not a code bug. FR is scoped to code review, not translation QA. An LLM could detect Traditional vs Simplified Chinese if specifically instructed, but that task is outside the code review prompt. |
| #3 | Anchor sanitization logic consumes English matcher groups without proper validation — if a translation has more anchors than English, the validation is incorrect | LOW | ✗ Missed | FR found related issues on the HTML logic (case-sensitive regex, containsHtml() gaps) but did not specifically identify this matcher group mismatch between EN and translations. |
| #4 | Method name santizeAnchors should be sanitizeAnchors (missing 'i') |
LOW | ✓ Found | FR: "Typo in method name: santizeAnchors → sanitizeAnchors." Identical finding. |
Supplementary Findings (13 findings)
| Finding | File | Severity | Legitimate? |
|---|---|---|---|
verifySafeHtml() throws RuntimeException uncaught — bypasses Maven Mojo exception handling | VerifyMessageProperties.java:74 | HIGH | ✓ Critical bug — silent crash |
Encoding mismatch: PropertyResourceBundle (ISO-8859-1) vs Files.readString() (UTF-8) — files read twice with different encodings | VerifyMessageProperties.java:64 | HIGH | ✓ Real data corruption bug |
English file path derivation via replaceAll fragile — fails for properties without locale suffix | VerifyMessageProperties.java:71 | MEDIUM | ✓ Path resolution flaw |
verifyNoDuplicateKeys ignores ! comment lines, : separator, and backslash line continuations — incomplete .properties parsing | VerifyMessageProperties.java:177 | MEDIUM | ✓ Parsing bug — false negatives on duplicate keys |
containsHtml() regex case-sensitive — does not detect tags like <BR> or <Div> | VerifyMessageProperties.java:149 | MEDIUM | ✓ False negatives on HTML tags |
| Missing test: valid-HTML scenario (translation with allowed HTML, no errors expected) | VerifyMessagePropertiesTest.java:42 | MEDIUM | ✓ Coverage gap |
| Missing test: negative case with no HTML in either file | VerifyMessagePropertiesTest.java:47 | MEDIUM | ✓ Coverage gap |
PolicyFactory fields instance-level but stateless — should be private static final | VerifyMessageProperties.java:55 | LOW | ✓ Convention |
replaceAll(Pattern.quote(...)) should use replace() for literal replacements | VerifyMessageProperties.java:96 | LOW | ✓ Simplification |
templateHelp regex only matches uppercase identifiers — extend to case-insensitive | VerifyMessageProperties.java:82 | LOW | ✓ Potential false negatives |
| Missing unit tests for key-specific normalization paths | VerifyMessagePropertiesTest.java:42 | LOW | ✓ Coverage gap |
Test isolation: duplicateKeys shares fixture with HTML verification — fragile | VerifyMessagePropertiesTest.java:61 | LOW | ✓ Fragile |
| OWASP dependency without explicit scope in pom.xml | pom.xml:76 | LOW | ✓ Convention |
Honest Analysis
FriendlyReviewer's full review
As posted on the PR — the complete output from FriendlyReviewer.
Hi there 👋
| 🌥️ Tech |
|---|
| 2 high, 5 medium, 7 low |
The Merge Request introduces an HTML sanitizer for translated message resources in the Keycloak Theme Verifier Maven plugin. The core concept is sound, but several issues need to be addressed: (1) a critical bug where verifySafeHtml() throws RuntimeException uncaught, bypassing Maven's Mojo exception handling; (2) encoding inconsistency between PropertyResourceBundle (ISO-8859-1) and Files.readString() (UTF-8); (3) gaps in .properties parsing in verifyNoDuplicateKeys (missing ! comments, : separator, line continuations); (4) case-sensitive HTML tag detection missing uppercase tags; (5) missing test coverage for valid-HTML scenarios, negative cases, key normalization paths; and (6) minor issues like typo in method naming, redundant file re-reading, and fragile path derivation. These findings collectively indicate that while the feature is valuable, it requires significant refinement to avoid runtime failures and ensure correctness.
These might need a close look
- 🔴 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L74)
verifySafeHtml() throws RuntimeException, not caught by verify() which only catches IOException. Wrap in MojoExecutionException to follow Maven plugin conventions. - 🔴 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L64)
Encoding mismatch: FileInputStream/PropertyResourceBundle uses ISO-8859-1 while file already read with Files.readString() (UTF-8). Reuse already-read content or ensure consistent encoding.
Worth checking
- 🟡 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L71)
English file path derivation via replaceAll is fragile; fails for base properties without locale suffix. Add fallback or validation. - 🟡 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L177)
verifyNoDuplicateKeys ignores!comment lines,:separator, and backslash line continuations required by .properties format. Fix parsing to handle these. - 🟡 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L149)
containsHtml() uses case-sensitive regex; does not detect uppercase tags like<BR>or<Div>. Use Pattern.CASE_INSENSITIVE. - 🟡 misc/theme-verifier/src/test/java/org/keycloak/themeverifier/VerifyMessagePropertiesTest.java (L42)
Missing test for valid-HTML scenario (translation with allowed HTML, no errors expected). Add test with matching safe HTML in English source. - 🟡 misc/theme-verifier/src/test/java/org/keycloak/themeverifier/VerifyMessagePropertiesTest.java (L47)
Missing negative test case for no HTML in either file (should produce no messages). Add such test.
Small things (take or leave)
- 🔵 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L55)
PolicyFactory fields are instance-level but stateless; make private static final. - 🔵 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L119)
Typo in method name: santizeAnchors -> sanitizeAnchors. - 🔵 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L96)
Use replace() instead of replaceAll(Pattern.quote(...)) for literal removals (e.g., style attribute in email path). - 🔵 misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (L82)
templateHelp regex only matches uppercase identifiers (e.g., CLAIM.<NAME>); extend to case-insensitive or generic pattern. - 🔵 misc/theme-verifier/src/test/java/org/keycloak/themeverifier/VerifyMessagePropertiesTest.java (L42)
Missing targeted unit tests for key-specific normalization paths: templateHelp, optimizeLookupHelp, timePeriodUnit, error-invalid-multivalued-size. - 🔵 misc/theme-verifier/src/test/java/org/keycloak/themeverifier/VerifyMessagePropertiesTest.java (L61)
Test isolation concern: duplicateKeys test shares fixture with HTML verification; consider separate fixtures. - 🔵 misc/theme-verifier/pom.xml (L76)
OWASP dependency missing explicit scope; add <scope>compile</scope> for consistency.
Of the 4 golden comments, 2 are out of scope for a code review tool: they concern translation content (Italian vs Lithuanian, Traditional vs Simplified Chinese). FriendlyReviewer is asked to review code — not to validate whether a translation file uses the correct natural language. An LLM has linguistic knowledge and could potentially detect Italian text in a Lithuanian file if given that specific instruction, but that's a translation-QA task, not a code review. On the 2 remaining code-level bugs, FriendlyReviewer found 1 out of 2: the typo
santizeAnchors(#4) was identified, but the anchor matcher group mismatch (#3) was not specifically flagged, though FriendlyReviewer found related issues on the HTML detection logic.The 13 supplementary findings are where FriendlyReviewer excelled. Two HIGH-severity bugs stand out: the uncaught
RuntimeExceptioninverifySafeHtml()that bypasses Maven's Mojo exception handling, and the encoding mismatch betweenPropertyResourceBundle(ISO-8859-1) andFiles.readString()(UTF-8) that can corrupt translated data.The case-sensitive
containsHtml()regex and incomplete.propertiesparsing are both legitimate bugs that would produce false negatives. The missing test coverage for valid-HTML and negative scenarios shows FriendlyReviewer evaluating test completeness.