Keycloak / PR #27

Add HTML sanitizer for translated message resources

keycloak/keycloak#37429
Golden recall: 100% Extra findings: 11

Golden Comments (4/4 found)

# Comment Severity Status FriendlyReviewer Detail
#1 The translation is in Italian instead of Lithuanian. This should be translated to Lithuanian to match the file's locale (messages_lt.properties). MEDIUM ✓ Found Both occurrences are cited. πŸ”΄ "The totpStep1 value is written in Italian (Installa una delle seguenti applicazioni sul tuo cellulare:) in the Lithuanian file, while surrounding keys are Lithuanian. This is a copy/paste error that renders Italian to Lithuanian users" (themes/.../base/account/messages/messages_lt.properties L101) and πŸ”΄ "loginTotpStep1 is written in Italian (Installa una delle seguenti applicazioni sul tuo cellulare:) in the Lithuanian login file. This is a copy/paste from the Italian locale, rendering Italian to Lithuanian users on the TOTP setup screen" (themes/.../base/login/messages/messages_lt.properties L71)
#2 The totpStep1 value uses Traditional Chinese terms in the Simplified Chinese file (zh_CN), which is likely incorrect for this locale. Please verify the locale-appropriate translation. MEDIUM ✓ Found 🟑 "The totpStep1 value uses Traditional Chinese characters (ζ‰‹ζ©Ÿ, 應用程式) in a Simplified Chinese (zh_CN) file. The rest of the file uses Simplified (e.g. ζ‰‹ζœΊ, 应用). Rewrite in Simplified Chinese for consistency" (messages_zh_CN.properties L112)
#3 The anchor sanitization logic has a potential issue where it consumes English matcher groups without proper validation. If the translated text has more anchor tags than the English text, this could lead to incorrect validation results. LOW ✓ Found Found β€” partial coverage, documented. πŸ”΅ "The anchor-removal loop using replaceFirst while the matcher continues over the original string is fragile" (VerifyMessageProperties.java L160). The review flags the same fragile mechanism at the golden's exact location: the loop consumes English matcher groups via replaceFirst without validating the correspondence with the translated text, which is precisely the defect that yields incorrect validation results when a translation has more anchors than the English reference. The concrete scenario (more anchors in the translation than in English) is not named explicitly, but the mechanism that produces it is identified at the same line.
#4 The method name 'santizeAnchors' should be 'sanitizeAnchors' (missing 'i'). LOW ✓ Found πŸ”΅ "Method santizeAnchors is misspelled (should be sanitizeAnchors)" (VerifyMessageProperties.java L160)

Supplementary Findings (11 findings)

Finding File Severity Legitimate?
The standalone project uses the Maven-distribution default surefire version (pre-JUnit 5) and declares only junit-jupiter-api without junit-jupiter-engine β€” the new VerifyMessagePropertiesTest tests will likely never execute theme-verifier/pom.xml:93 HIGH ✓ Real: the feature's entire validation would silently not run in CI
verifySafeHtml() loads bundles via new PropertyResourceBundle(fis), which decodes as ISO-8859-1 β€” Keycloak message files are UTF-8, so non-Latin values (Arabic, Chinese, Cyrillic, etc.) are corrupted before sanitization VerifyMessageProperties.java:65 MEDIUM ✓ Real: UTF-8 values corrupted before the HTML check, mojibake in anchor matching and errors
If no English counterpart file is found by regex, FileInputStream fails and an uncaught RuntimeException propagates through ThemeVerifierMojo.execute(), aborting the whole build with a raw stack trace VerifyMessageProperties.java:76 MEDIUM ✓ Real: build aborts with an unidentifiable failure
All four tests assert that a message IS reported; there are no negative (false-positive) cases verifying that valid translations pass cleanly. Also untested: the normalizeValue special cases for templateHelp, optimizeLookupHelp, and {n,choice,...} VerifyMessagePropertiesTest.java:34 MEDIUM ✓ Real: a regression flagging every legitimate translation would go undetected
normalizeValue hardcodes content-specific rules for templateHelp, optimizeLookupHelp, linkExpirationFormatter.timePeriodUnit*, error-invalid-multivalued-size, and an email RTL style strip β€” drifts from the documented contract "translation HTML must be a subset of english HTML" VerifyMessageProperties.java:106 MEDIUM ✓ Real: contract divergence requiring a new branch for every future message with angle brackets
error-invalid-multivalued-size uses Java MessageFormat syntax ({0}, {2,choice,...}) while the React bundles' same-key translations use ICU/i18next double-brace syntax ({{0}}) β€” i18next does not recognize Java MessageFormat; the en locale would render literal tokens account-ui/messages_en.properties:188 MEDIUM ✓ Real: broken interpolation in the React account/admin apps
The four webauthn-error-* keys use <br /> in the EN reference; if HtmlPolicyBuilder.allowElements("br") normalizes to <br> or <br/>, these keys and translations will be reported as "Illegal HTML" and break the build login/messages_en.properties:487 MEDIUM ✓ Borderline: depends on the sanitizer's canonical <br /> output, which the tests do not verify
The fixture value Some <div>tag</div is missing the closing > β€” it still triggers "Illegal HTML" but does not cleanly isolate the intended disallowed-div scenario illegalHtmlTag_en.properties:16 LOW ✓ Real: fixture does not isolate the scenario it claims to test
identityProviderLinkBodyHtml contains a stray leading "n" in the final paragraph: ...<p>nHvis du ikke vil forbinde... (should be "Hvis") email/messages_da.properties:9 LOW ✓ Real typo on a modified line (pre-existing, correct while touching the line)
emailVerificationBodyHtml contains a stray leading "n" in the last paragraph: ...<p>nJei paskyros nekurte... (should be "Jei") email/messages_lt.properties:3 LOW ✓ Real typo on a modified line (pre-existing)
identityProviderLinkBodyHtml begins with ...<p>Ε½as pageidauja... β€” the word is missing its leading "Ka" (should be "KaΕΎkas pageidauja...") email/messages_lt.properties:6 LOW ✓ Real typo on a modified line (pre-existing)

Honest Analysis

FriendlyReviewer found all 4 golden comments of this PR. The two MEDIUM goldens (Italian text in the Lithuanian files, Traditional Chinese in zh_CN) are found with the exact files and keys named; for the Italian case both occurrences are cited (totpStep1 in the account bundle and loginTotpStep1 in the login bundle), which is more complete than the reference comment. The typo golden (santizeAnchors β†’ sanitizeAnchors) is found verbatim.

The anchor-sanitization golden deserves nuance. The review flags the fragile mechanism β€” the anchor-removal loop using replaceFirst while the matcher continues over the original string β€” at the exact line where the golden sits (L160), but it does not name the concrete failure scenario (more anchors in the translation than in the English text leading to incorrect validation results). The mechanism that produces the failure is identified at the precise location; the specific consequence is not spelled out. This page counts it as found, with the partiality documented in the Detail cell.

The 11 supplementary findings are a mixed bag. The strongest is the pom.xml surefire configuration: the new JUnit 5 tests are very likely never executed, which means the feature's entire validation silently does nothing β€” arguably more damaging than the goldens themselves. The UTF-8/ISO-8859-1 misdecoding (L65) and the Java MessageFormat vs ICU divergence in the React bundles (L188) are also substantive. The low end is noisier: three pre-existing typos on modified lines (nHvis, nJei, Ε½as) and a malformed fixture are legitimate but minor, and the <br /> round-trip item (L487) is borderline since it depends on the sanitizer's canonical output, which the review itself could not confirm from the tests.

Overall verdict: full golden coverage on a mostly-cosmetic PR, with the best supplementary finding (untested tests) outranking the goldens in importance. The review's blocking stance is defensible on the test-execution issue alone; the main criticism is that the severity mix (3 high, 7 medium, 5 low) leans heavy for items that are largely locale typos and fixture nits.

FriendlyReviewer's full review

As posted on the PR β€” the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there πŸ‘‹

πŸŒ₯️ Tech 🌀️ Feat
3 high, 7 medium, 5 low 1 mitigated

The MR introduces an HTML sanitizer for translated message resources and updates numerous locale bundles to conform to the new policy. The core verification logic is conceptually sound and most translation updates align with the sanitizer's allowed-tag and anchor-equality rules. However, the review surfaced several blocking concerns: the new JUnit 5 tests are very likely to never execute because the standalone pom pins surefire to a pre-Jupiter default and lacks junit-jupiter-engine; the verifier reads message files with PropertyResourceBundle (ISO-8859-1), corrupting UTF-8 values; and two Lithuanian locale files (account and login themes) contain Italian text for the TOTP step message. Additional issues include placeholder-syntax divergence in the React bundles, a risk that the sanitizer may not round-trip <br /> as written in the English reference, missing negative test cases, and several minor fixture/code-quality problems. Recommend changes before merge.

These might need a close look

  • πŸ”΄ misc/theme-verifier/pom.xml (L93)
    The standalone project uses the Maven-distribution default surefire version (pre-JUnit 5) and declares only junit-jupiter-api without junit-jupiter-engine or junit-platform-surefire-provider. The new VerifyMessagePropertiesTest tests will likely never execute, leaving the entire feature's validation untested. Add an explicit surefire 2.22.0+ and the Jupiter engine dependency, and confirm tests run in CI.
  • πŸ”΄ themes/.../base/account/messages/messages_lt.properties (L101)
    totpStep1 value is written in Italian (Installa una delle seguenti applicazioni sul tuo cellulare:) in the Lithuanian file, while surrounding keys are Lithuanian. This is a copy/paste error that renders Italian to Lithuanian users. Replace with a Lithuanian translation.
  • πŸ”΄ themes/.../base/login/messages/messages_lt.properties (L71)
    loginTotpStep1 is written in Italian (Installa una delle seguenti applicazioni sul tuo cellulare:) in the Lithuanian login file. This is a copy/paste from the Italian locale, rendering Italian to Lithuanian users on the TOTP setup screen. Replace with a correct Lithuanian translation.

Worth checking

  • 🟑 misc/theme-verifier/.../VerifyMessageProperties.java (L65)
    verifySafeHtml() loads bundles via new PropertyResourceBundle(fis), which decodes as ISO-8859-1. Keycloak message files are UTF-8, so non-Latin values (Arabic, Chinese, Cyrillic, etc.) are corrupted before sanitization. The HTML check still passes because multibyte bytes never map to ASCII </>, but anchor matching and error output use mojibake. Use a UTF-8 reader (e.g. Files.newBufferedReader(path, StandardCharsets.UTF_8)).
  • 🟑 misc/theme-verifier/.../VerifyMessageProperties.java (L76)
    If no English counterpart file is found by regex, FileInputStream fails and an uncaught RuntimeException propagates through ThemeVerifierMojo.execute(), aborting the whole build with a raw stack trace. Throw MojoExecutionException (or report via the messages collector) so failures are clean and identifiable.
  • 🟑 misc/theme-verifier/.../VerifyMessagePropertiesTest.java (L34)
    All four tests assert that a message IS reported; there are no negative (false-positive) cases verifying that valid translations pass cleanly. A regression flagging every legitimate translation would go undetected. Also untested: the normalizeValue special cases for templateHelp, optimizeLookupHelp, and {n,choice,...}.
  • 🟑 misc/theme-verifier/.../VerifyMessageProperties.java (L106)
    normalizeValue hardcodes content-specific rules for templateHelp, optimizeLookupHelp, linkExpirationFormatter.timePeriodUnit*, error-invalid-multivalued-size, and an email RTL style strip. This drifts from the documented contract "translation HTML must be a subset of english HTML" and will require a new branch for every future message with angle brackets. Document or generalize this divergence.
  • 🟑 js/apps/account-ui/.../messages_en.properties (L188)
    error-invalid-multivalued-size uses Java MessageFormat syntax ({0}, {2,choice,...}) while the React bundles' same-key translations use ICU/i18next double-brace syntax ({{0}}). The account/admin React apps interpolate with i18next, which does not recognize Java MessageFormat; the en locale would render literal tokens. Confirm the rendering path; if React, do not use the Java choice selector in the English source.
  • 🟑 themes/.../base/login/messages/messages_en.properties (L487)
    The four webauthn-error-* keys use <br /> in the EN reference. These EN files are themselves run through verifySafeHtml(), so the source value must equal the sanitizer's canonical output for <br />. If HtmlPolicyBuilder.allowElements("br") normalizes to <br> or <br/>, these keys and translations will be reported as "Illegal HTML" and break the build. Confirm the sanitizer round-trips <br /> exactly.
  • 🟑 themes/.../base/account/messages/messages_zh_CN.properties (L112)
    totpStep1 uses Traditional Chinese characters (ζ‰‹ζ©Ÿ, 應用程式) in a Simplified Chinese (zh_CN) file. The rest of the file uses Simplified (e.g. ζ‰‹ζœΊ, 应用). Rewrite in Simplified Chinese for consistency.
Small things (take or leave)
  • πŸ”΅ misc/theme-verifier/.../test/resources/illegalHtmlTag_en.properties (L16)
    The fixture value Some <div>tag</div is missing the closing >. It still triggers "Illegal HTML", but does not cleanly isolate the intended disallowed-div scenario because the malformed/unclosed </div is itself a difference. Use a clean value like Some <div>tag</div>.
  • πŸ”΅ misc/theme-verifier/.../VerifyMessageProperties.java (L160)
    Method santizeAnchors is misspelled (should be sanitizeAnchors). Also, POLICY_SOME_HTML/POLICY_NO_HTML are package-private instance fields and should be private static final to avoid rebuilding factories per instance. The anchor-removal loop using replaceFirst while the matcher continues over the original string is fragile though functionally correct for ordered matches.
  • πŸ”΅ themes/.../base/email/messages/messages_da.properties (L9)
    identityProviderLinkBodyHtml contains a stray leading "n" in the final paragraph: ...<p>nHvis du ikke vil forbinde... (should be "Hvis"). Pre-existing typo on a modified line; correct while touching the line.
  • πŸ”΅ themes/.../base/email/messages/messages_lt.properties (L3)
    emailVerificationBodyHtml contains a stray leading "n" in the last paragraph: ...<p>nJei paskyros nekurte... (should be "Jei"). Pre-existing typo on a modified line.
  • πŸ”΅ themes/.../base/email/messages/messages_lt.properties (L6)
    identityProviderLinkBodyHtml begins with ...<p>Ε½as pageidauja... β€” the word is missing its leading "Ka" (should be "KaΕΎkas pageidauja..."). Pre-existing typo on a modified line.

βœ… Feature-level checklist

Partially covered

  • ⚠️ Add a HTML sanitizer for translated message resources
    The sanitizer is implemented and most translation updates conform to its policy, but the JUnit 5 tests that validate the feature are likely not executed, the verifier misdecodes UTF-8 message values, and several locale files contain content errors. These issues prevent the requirement from being fully satisfied.