Automated Test: feature-html-sanitizer-implementation #306
+343
−105
Conversation
This file contains 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
Closes #37428 Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
📝 WalkthroughWalkthroughThis pull request introduces HTML safety verification into the theme verifier, standardizes HTML formatting across localization files, adds new TOTP authentication flow messages across multiple locales, updates error message pluralization logic using ICU format, corrects HTML structure issues in email templates, and adds dependencies for HTML sanitization. Changes
Sequence DiagramsequenceDiagram
participant VerifyProcess as Verify Process
participant PropsLoader as Properties Loader
participant Sanitizer as HTML Sanitizer
participant Comparator as Value Comparator
participant Reporter as Report Generator
VerifyProcess->>PropsLoader: Load base properties file
PropsLoader-->>VerifyProcess: File contents
VerifyProcess->>PropsLoader: Load English fallback bundle
PropsLoader-->>VerifyProcess: English properties
VerifyProcess->>Sanitizer: For each property key
Sanitizer->>Sanitizer: Normalize value
Sanitizer->>Sanitizer: Check if contains HTML
alt HTML Detected
Sanitizer->>Sanitizer: Sanitize with policy
Sanitizer->>Sanitizer: Unescape entities
end
Sanitizer-->>VerifyProcess: Sanitized value
VerifyProcess->>Comparator: Compare original vs sanitized
alt Mismatch Found
Comparator->>Comparator: Detect illegal HTML or anchor changes
Comparator-->>Reporter: Report difference
Reporter->>Reporter: Generate contextual error message
end
Reporter-->>VerifyProcess: Validation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (1)
46-56:⚠️ Potential issue | 🟠 Major
RuntimeExceptionfromverifySafeHtml()escapes theIOExceptioncatch block.
verify()catchesIOExceptionon Line 52, butverifySafeHtml()throwsRuntimeException(Lines 70, 79), which will propagate uncaught. This is related to the English file loading issue flagged above — if that's addressed with graceful handling, this concern is resolved.
🤖 Fix all issues with AI agents
In `@misc/theme-verifier/pom.xml`:
- Around line 75-85: Update the vulnerable dependencies in the POM: bump
artifactId owasp-java-html-sanitizer to version 20260102.1 and add an explicit
<scope>compile</scope> for the owasp-java-html-sanitizer dependency; upgrade
artifactId commons-text to 1.15.0 (or newer) to pull a safe commons-lang3, or
alternatively add a dependencyManagement override for groupId org.apache.commons
/ artifactId commons-lang3 to at least 3.18.0 (preferably 3.20.0) to eliminate
CVE-2025-48924; locate the dependencies by their artifactId entries
(owasp-java-html-sanitizer and commons-text) and apply the version/scope changes
or add the commons-lang3 override in dependencyManagement.
In
`@misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java`:
- Around line 58-63: The fields POLICY_SOME_HTML and POLICY_NO_HTML (and
HTML_TAGS) are declared as package-private instance fields but are immutable
constants; change their declarations to be private static final and keep their
current initializers, e.g., update the declarations for POLICY_SOME_HTML and
POLICY_NO_HTML to "private static final PolicyFactory ..." and likewise make
HTML_TAGS a private static final (preserving its type and initializer) so they
are class-level constants.
- Around line 150-162: Rename the method from santizeAnchors to sanitizeAnchors
and fix the matcher desync by not mutating the string while iterating its
Matcher; either collect all anchor matches first via ANCHOR_PATTERN for both
value and englishValue and validate pairs, then perform removals, or after each
replaceFirst update the Matcher (recreate matcher =
ANCHOR_PATTERN.matcher(value)) and similarly reset englishMatcher when
appropriate; ensure you reference ANCHOR_PATTERN, matcher, englishMatcher,
replaceFirst and messages in the fix so anchors are validated against
englishValue before removal and the messages list is populated correctly.
- Around line 99-111: The diff can throw StringIndexOutOfBounds when the
computed prefix length (start) and suffix length (end) overlap; clamp end before
taking substrings in VerifyMessageProperties: compute end = Math.min(end,
Math.max(0, sanitized.length() - start), Math.max(0, value.length() - start))
(or equivalent) so sanitized.substring(start, sanitized.length() - end) and
value.substring(start, value.length() - end) never get beginIndex > endIndex,
then continue adding the message to messages using those safe substrings
(referencing variables start, end, sanitized, value, messages, key, file).
- Around line 73-80: The code in VerifyMessageProperties that builds englishFile
and then loads it into bundleEnglish currently throws a RuntimeException on
IOException which escapes verify(); instead catch the IOException around the
FileInputStream/PropertyResourceBundle creation and handle it like other
verification errors: add a descriptive error entry to the messages collection
referencing the englishFile (or a message key) and return early from verify(),
or alternatively wrap the IOException in a MojoExecutionException before
rethrowing so the error follows the existing Mojo flow; update the bundleEnglish
usage to account for the early-return or wrapped exception.
In `@misc/theme-verifier/src/test/resources/illegalHtmlTag_en.properties`:
- Line 17: The property value for the entry starting with "key=Some
<div>tag</div" has a malformed closing tag (it reads "</div" missing the
trailing '>'); either correct it to a well-formed disallowed tag ("key=Some
<div>tag</div>") or, if the intent is to test a malformed-tag case, add an
explicit comment or separate fixture entry (e.g., one file/value for malformed
"</div" and another for the well-formed "<div>tag</div>") so both scenarios are
covered and unambiguous.
In
`@themes/src/main/resources-community/theme/base/account/messages/messages_lt.properties`:
- Line 101: The totpStep1 entry in the Lithuanian locale file currently contains
Italian text; open the messages_lt.properties and replace the totpStep1 value
(key: totpStep1) with the correct Lithuanian translation, e.g. "Įdiekite vieną
iš šių programėlių savo mobiliajame telefone:", ensuring the string matches
surrounding Lithuanian entries and preserves any required property file
escaping/encoding.
In
`@themes/src/main/resources-community/theme/base/account/messages/messages_zh_CN.properties`:
- Line 112: The value for the message key totpStep1 uses Traditional Chinese
characters; change the string to Simplified Chinese to match the zh_CN locale
and neighboring entries (e.g., replace "在您的手機上安裝以下應用程式之一：" with the Simplified
form "在您的手机上安装以下应用程序之一：" in the messages_zh_CN.properties file so the key
totpStep1 consistently uses Simplified characters).
In
`@themes/src/main/resources-community/theme/base/email/messages/messages_da.properties`:
- Line 9: The localized property identityProviderLinkBodyHtml contains a stray
leading character "n" before "Hvis"; remove the unwanted "n" so the sentence
begins "Hvis" (i.e., change "nHvis du ikke vil..." to "Hvis du ikke vil...") to
correct the typo and ensure the HTML message renders properly.
In
`@themes/src/main/resources-community/theme/base/email/messages/messages_lt.properties`:
- Line 3: The property emailVerificationBodyHtml contains two typos: change "Jei
tao buvote" to "Jei tai buvote" and remove the stray leading "n" before "Jei
paskyros" so it reads "<p>Jei paskyros..." ensuring the HTML string matches the
plain-text version and correct Lithuanian text.
- Line 6: Fix the truncated leading word in the properties value for the key
identityProviderLinkBodyHtml: replace the incorrect "žas pageidauja" with the
correct "Kažkas pageidauja" (so the sentence begins "Kažkas pageidauja
susieti...") to match the plain-text counterpart and correct the typo.
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_ca.properties`:
- Around line 423-427: Normalize the spacing between the HTML line break and the
placeholder across the listed message keys: update webauthn-error-registration
and webauthn-error-api-get or webauthn-error-auth-verification and
webauthn-error-register-verification so they all use the same convention (either
"<br /> {0}" or "<br />{0}"); pick one style and apply it to all four keys to
ensure consistency.
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_ka.properties`:
- Around line 172-174: The messages for webauthn-error-registration and
webauthn-error-api-get (and any other occurrences like the duplicates mentioned)
use "<br />{0}" without a space; update these message values to use "<br /> {0}"
(i.e., add a single space after the <br />) so they match other locales'
formatting consistency—locate the keys webauthn-error-registration and
webauthn-error-api-get (and any duplicate occurrences) in messages_ka.properties
and insert the space after the break tag.
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_lt.properties`:
- Line 71: The loginTotpStep1 message is in Italian; replace the Italian value
for the loginTotpStep1 key with the correct Lithuanian translation Įdiekite
vieną iš šių programėlių savo mobiliajame telefone: in the
messages_lt.properties file, save the file with UTF-8 encoding, run a quick
build/test to ensure the properties load correctly, and commit with a clear
message like "Fix Lithuanian translation for loginTotpStep1".
🧹 Nitpick comments (2)
themes/src/main/resources-community/theme/base/email/messages/messages_uk.properties (1)
6-6: HTML structure fix looks correct, but anchor text is inconsistent with sibling templates.The consolidation into
<p><a href="{0}">{0}</a></p>is well-formed HTML. However, every other*BodyHtmlkey in this file uses descriptive Ukrainian text as the anchor label (e.g.,"Посилання для скидання паролю"on Line 15), whereas this entry displays the raw URL{0}as the link text. Consider aligning with the established pattern for a consistent user experience across email templates.✏️ Optional: use descriptive anchor text to match other templates
-emailUpdateConfirmationBodyHtml=<p>Щоб оновити адресу електронної пошти {1} облікового запису {2}, натисніть на посилання нижче</p><p><a href="{0}">{0}</a></p><p>Це посилання дійсне протягом {3}.</p><p>Якщо Ви не бажаєте застосувати ці зміни, просто проігноруйте цей лист.</p> +emailUpdateConfirmationBodyHtml=<p>Щоб оновити адресу електронної пошти {1} облікового запису {2}, натисніть на посилання нижче</p><p><a href="{0}">Посилання для підтвердження оновлення адреси електронної пошти</a></p><p>Це посилання дійсне протягом {3}.</p><p>Якщо Ви не бажаєте застосувати ці зміни, просто проігноруйте цей лист.</p>misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java (1)
132-135: TODO: Move RTL information for emails.This TODO indicates pending work to properly handle RTL styles in email templates rather than stripping them during verification.
Would you like me to open an issue to track the RTL email style migration?
Comment on lines
+75
to
+85
| <dependency> | ||
| <groupId>com.googlecode.owasp-java-html-sanitizer</groupId> | ||
| <artifactId>owasp-java-html-sanitizer</artifactId> | ||
| <version>20240325.1</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-text</artifactId> | ||
| <version>1.13.0</version> | ||
| <scope>compile</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pom.xml" -path "*/misc/theme-verifier/*" -type fRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 116
🏁 Script executed:
cat -n misc/theme-verifier/pom.xml | sed -n '70,90p'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 900
🌐 Web query:
owasp-java-html-sanitizer 20240325.1 maven latest version
💡 Result:
The latest Maven Central release of com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer is 20260102.1 (released Jan 3, 2026). [1][2]
If you’re currently on 20240325.1, that is an older release (Mar 25, 2024). [3]
Maven:
<dependency>
<groupId>com.googlecode.owasp-java-html-sanitizer</groupId>
<artifactId>owasp-java-html-sanitizer</artifactId>
<version>20260102.1</version>
</dependency>Sources
[1] https://repo1.maven.org/maven2/com/googlecode/owasp-java-html-sanitizer/owasp-java-html-sanitizer/
[2] https://mvnrepository.com/artifact/com.googlecode.owasp-java-html-sanitizer/owasp-java-html-sanitizer/20260102.1
[3] https://repo1.maven.org/maven2/com/googlecode/owasp-java-html-sanitizer/owasp-java-html-sanitizer/20240325.1/
🌐 Web query:
commons-text 1.13.0 CVE vulnerabilities security
💡 Result:
commons-text 1.13.0 — CVE / security status (as of Feb 2026)
- Direct CVEs in
org.apache.commons:commons-text1.13.0: None listed by the Apache Commons Text project. The project’s published security page only calls out CVE-2022-42889, which affects versions prior to 1.10.0—so 1.13.0 is not in the affected range. (commons.apache.org) - Transitive/dependency CVEs impacting a default 1.13.0 build:
commons-text1.13.0 has a compile dependency onorg.apache.commons:commons-lang3:3.17.0, which is flagged as vulnerable. (mvnrepository.com)- The relevant issue is CVE-2025-48924 (Apache Commons Lang): an uncontrolled recursion / StackOverflowError (DoS) risk in
ClassUtils.getClass(...)on very long inputs, affecting commons-lang3 < 3.18.0. (nvd.nist.gov)
- The relevant issue is CVE-2025-48924 (Apache Commons Lang): an uncontrolled recursion / StackOverflowError (DoS) risk in
Recommended remediation
- Best: Upgrade to a newer Commons Text release (e.g., commons-text 1.15.0), which depends on commons-lang3 3.20.0 (not in the vulnerable range for CVE-2025-48924). (commons.apache.org)
- Alternative: Keep commons-text 1.13.0 but override/pin
commons-lang3to 3.18.0+ in your dependency management (Maven/Gradle). (nvd.nist.gov)
Sources
[1] Apache Commons Text — Security Vulnerabilities (commons.apache.org)
[2] MvnRepository — commons-text 1.13.0 dependencies (shows commons-lang3 3.17.0) (mvnrepository.com)
[3] NVD — CVE-2025-48924 (Apache Commons Lang) (nvd.nist.gov)
[4] Apache Commons Text — Project Dependencies (1.15.0 uses commons-lang3 3.20.0) (commons.apache.org)
Citations:
- 1: https://commons.apache.org/proper/commons-text/security.html
- 2: https://mvnrepository.com/artifact/org.apache.commons/commons-text/1.13.0/dependencies
- 3: https://nvd.nist.gov/vuln/detail/CVE-2025-48924?utm_source=openai
- 4: https://commons.apache.org/proper/commons-text/dependencies.html?utm_source=openai
- 5: https://nvd.nist.gov/vuln/detail/CVE-2025-48924?utm_source=openai
- 6: https://commons.apache.org/proper/commons-text/security.html
- 7: https://mvnrepository.com/artifact/org.apache.commons/commons-text/1.13.0/dependencies
- 8: https://nvd.nist.gov/vuln/detail/CVE-2025-48924?utm_source=openai
- 9: https://commons.apache.org/proper/commons-text/dependencies.html?utm_source=openai
Critical: Outdated and vulnerable dependencies require immediate attention.
The owasp-java-html-sanitizer version 20240325.1 is outdated (current: 20260102.1, released Jan 3, 2026). More critically, commons-text 1.13.0 introduces a transitive dependency on commons-lang3 3.17.0, which is vulnerable to CVE-2025-48924 (uncontrolled recursion / DoS in ClassUtils.getClass(...)).
Fixes required:
- Upgrade
commons-textto1.15.0or newer (which depends oncommons-lang3 3.20.0), OR overridecommons-lang3to3.18.0+in dependency management. - Upgrade
owasp-java-html-sanitizerto20260102.1or later.
Additionally, the owasp-java-html-sanitizer dependency lacks an explicit <scope>compile</scope> declaration for consistency with commons-text.
🤖 Prompt for AI Agents
In `@misc/theme-verifier/pom.xml` around lines 75 - 85, Update the vulnerable
dependencies in the POM: bump artifactId owasp-java-html-sanitizer to version
20260102.1 and add an explicit <scope>compile</scope> for the
owasp-java-html-sanitizer dependency; upgrade artifactId commons-text to 1.15.0
(or newer) to pull a safe commons-lang3, or alternatively add a
dependencyManagement override for groupId org.apache.commons / artifactId
commons-lang3 to at least 3.18.0 (preferably 3.20.0) to eliminate
CVE-2025-48924; locate the dependencies by their artifactId entries
(owasp-java-html-sanitizer and commons-text) and apply the version/scope changes
or add the commons-lang3 override in dependencyManagement.
Comment on lines
+58
to
+63
| PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder() | ||
| .allowElements( | ||
| "br", "p", "strong", "b" | ||
| ).toFactory(); | ||
|
|
||
| PolicyFactory POLICY_NO_HTML = new org.owasp.html.HtmlPolicyBuilder().toFactory(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Policy fields should be private static final.
POLICY_SOME_HTML, POLICY_NO_HTML use constant naming conventions but are package-private instance fields. They are immutable after construction and shared across all uses — they should be private static final. Same applies to HTML_TAGS on Line 139.
♻️ Proposed fix
- PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder()
+ private static final PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder()
.allowElements(
"br", "p", "strong", "b"
).toFactory();
- PolicyFactory POLICY_NO_HTML = new org.owasp.html.HtmlPolicyBuilder().toFactory();
+ private static final PolicyFactory POLICY_NO_HTML = new org.owasp.html.HtmlPolicyBuilder().toFactory();And for Line 139:
- Pattern HTML_TAGS = Pattern.compile("<[a-z]+[^>]*>");
+ private static final Pattern HTML_TAGS = Pattern.compile("<[a-z]+[^>]*>");📝 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
| PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder() | |
| .allowElements( | |
| "br", "p", "strong", "b" | |
| ).toFactory(); | |
| PolicyFactory POLICY_NO_HTML = new org.owasp.html.HtmlPolicyBuilder().toFactory(); | |
| private static final PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder() | |
| .allowElements( | |
| "br", "p", "strong", "b" | |
| ).toFactory(); | |
| private static final PolicyFactory POLICY_NO_HTML = new org.owasp.html.HtmlPolicyBuilder().toFactory(); |
🤖 Prompt for AI Agents
In
`@misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java`
around lines 58 - 63, The fields POLICY_SOME_HTML and POLICY_NO_HTML (and
HTML_TAGS) are declared as package-private instance fields but are immutable
constants; change their declarations to be private static final and keep their
current initializers, e.g., update the declarations for POLICY_SOME_HTML and
POLICY_NO_HTML to "private static final PolicyFactory ..." and likewise make
HTML_TAGS a private static final (preserving its type and initializer) so they
are class-level constants.
Comment on lines
+73
to
+80
| PropertyResourceBundle bundleEnglish; | ||
| String englishFile = file.getAbsolutePath().replaceAll("resources-community", "resources") | ||
| .replaceAll("_[a-zA-Z-_]*\\.properties", "_en.properties"); | ||
| try (FileInputStream fis = new FileInputStream(englishFile)) { | ||
| bundleEnglish = new PropertyResourceBundle(fis); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("unable to read file " + englishFile, e); | ||
| } |
There was a problem hiding this comment.
Missing English file causes an uncaught RuntimeException.
If the English .properties counterpart doesn't exist (e.g., new locale-only key file, or path transformation mismatch), this throws a RuntimeException that bypasses the MojoExecutionException handling in verify(). Consider adding the error to messages and returning early, or wrapping it in MojoExecutionException, consistent with the rest of the verification flow.
🛡️ Proposed fix — graceful fallback
PropertyResourceBundle bundleEnglish;
String englishFile = file.getAbsolutePath().replaceAll("resources-community", "resources")
.replaceAll("_[a-zA-Z-_]*\\.properties", "_en.properties");
- try (FileInputStream fis = new FileInputStream(englishFile)) {
- bundleEnglish = new PropertyResourceBundle(fis);
- } catch (IOException e) {
- throw new RuntimeException("unable to read file " + englishFile, e);
+ File englishPath = new File(englishFile);
+ if (!englishPath.exists()) {
+ messages.add("English reference file not found: " + englishFile + " — skipping HTML safety check for " + file);
+ return;
+ }
+ try (FileInputStream fis = new FileInputStream(englishPath)) {
+ bundleEnglish = new PropertyResourceBundle(fis);
+ } catch (IOException e) {
+ messages.add("Unable to read English reference file " + englishFile + ": " + e.getMessage());
+ return;
}📝 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
| PropertyResourceBundle bundleEnglish; | |
| String englishFile = file.getAbsolutePath().replaceAll("resources-community", "resources") | |
| .replaceAll("_[a-zA-Z-_]*\\.properties", "_en.properties"); | |
| try (FileInputStream fis = new FileInputStream(englishFile)) { | |
| bundleEnglish = new PropertyResourceBundle(fis); | |
| } catch (IOException e) { | |
| throw new RuntimeException("unable to read file " + englishFile, e); | |
| } | |
| PropertyResourceBundle bundleEnglish; | |
| String englishFile = file.getAbsolutePath().replaceAll("resources-community", "resources") | |
| .replaceAll("_[a-zA-Z-_]*\\.properties", "_en.properties"); | |
| File englishPath = new File(englishFile); | |
| if (!englishPath.exists()) { | |
| messages.add("English reference file not found: " + englishFile + " — skipping HTML safety check for " + file); | |
| return; | |
| } | |
| try (FileInputStream fis = new FileInputStream(englishPath)) { | |
| bundleEnglish = new PropertyResourceBundle(fis); | |
| } catch (IOException e) { | |
| messages.add("Unable to read English reference file " + englishFile + ": " + e.getMessage()); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In
`@misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java`
around lines 73 - 80, The code in VerifyMessageProperties that builds
englishFile and then loads it into bundleEnglish currently throws a
RuntimeException on IOException which escapes verify(); instead catch the
IOException around the FileInputStream/PropertyResourceBundle creation and
handle it like other verification errors: add a descriptive error entry to the
messages collection referencing the englishFile (or a message key) and return
early from verify(), or alternatively wrap the IOException in a
MojoExecutionException before rethrowing so the error follows the existing Mojo
flow; update the bundleEnglish usage to account for the early-return or wrapped
exception.
Comment on lines
+99
to
+111
| if (!Objects.equals(sanitized, value)) { | ||
|
|
||
| // Strip identical characters from the beginning and the end to show where the difference is | ||
| int start = 0; | ||
| while (start < sanitized.length() && start < value.length() && value.charAt(start) == sanitized.charAt(start)) { | ||
| start++; | ||
| } | ||
| int end = 0; | ||
| while (end < sanitized.length() && end < value.length() && value.charAt(value.length() - end - 1) == sanitized.charAt(sanitized.length() - end - 1)) { | ||
| end++; | ||
| } | ||
|
|
||
| messages.add("Illegal HTML in key " + key + " for file " + file + ": '" + value.substring(start, value.length() - end) + "' vs. '" + sanitized.substring(start, sanitized.length() - end) + "'"); |
There was a problem hiding this comment.
Potential StringIndexOutOfBoundsException when prefix/suffix overlap.
If the matching prefix (start) and suffix (end) overlap (i.e., start + end >= sanitized.length()), sanitized.substring(start, sanitized.length() - end) will have beginIndex > endIndex, causing an exception. This can occur when the sanitizer strips content whose surrounding text characters coincidentally match.
🐛 Proposed fix — clamp end to avoid overlap
int end = 0;
- while (end < sanitized.length() && end < value.length() && value.charAt(value.length() - end - 1) == sanitized.charAt(sanitized.length() - end - 1)) {
+ while (end < sanitized.length() - start && end < value.length() - start && value.charAt(value.length() - end - 1) == sanitized.charAt(sanitized.length() - end - 1)) {
end++;
}🤖 Prompt for AI Agents
In
`@misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java`
around lines 99 - 111, The diff can throw StringIndexOutOfBounds when the
computed prefix length (start) and suffix length (end) overlap; clamp end before
taking substrings in VerifyMessageProperties: compute end = Math.min(end,
Math.max(0, sanitized.length() - start), Math.max(0, value.length() - start))
(or equivalent) so sanitized.substring(start, sanitized.length() - end) and
value.substring(start, value.length() - end) never get beginIndex > endIndex,
then continue adding the message to messages using those safe substrings
(referencing variables start, end, sanitized, value, messages, key, file).
Comment on lines
+150
to
+162
| private String santizeAnchors(String key, String value, String englishValue) { | ||
| Matcher matcher = ANCHOR_PATTERN.matcher(value); | ||
| Matcher englishMatcher = ANCHOR_PATTERN.matcher(englishValue); | ||
| while (matcher.find()) { | ||
| if (englishMatcher.find() && Objects.equals(matcher.group(), englishMatcher.group())) { | ||
| value = value.replaceFirst(Pattern.quote(englishMatcher.group()), ""); | ||
| } else { | ||
| messages.add("Didn't find anchor tag " + matcher.group() + " in original string"); | ||
| break; | ||
| } | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Typo: santizeAnchors → sanitizeAnchors.
Also, the Matcher on Line 151 is bound to the original value string, but value is reassigned on Line 155 via replaceFirst. On subsequent loop iterations, matcher.find() continues scanning the old string while value has changed, causing position desync when multiple anchors exist.
Consider rebuilding the matcher after each replacement, or collecting all anchors first and validating them separately from the removal step.
🐛 Proposed fix — rename and rebuild matcher
- private String santizeAnchors(String key, String value, String englishValue) {
- Matcher matcher = ANCHOR_PATTERN.matcher(value);
+ private String sanitizeAnchors(String key, String value, String englishValue) {
Matcher englishMatcher = ANCHOR_PATTERN.matcher(englishValue);
- while (matcher.find()) {
- if (englishMatcher.find() && Objects.equals(matcher.group(), englishMatcher.group())) {
+ Matcher matcher = ANCHOR_PATTERN.matcher(value);
+ while (matcher.find()) {
+ if (englishMatcher.find() && Objects.equals(matcher.group(), englishMatcher.group())) {
value = value.replaceFirst(Pattern.quote(englishMatcher.group()), "");
+ matcher = ANCHOR_PATTERN.matcher(value);
} else {
messages.add("Didn't find anchor tag " + matcher.group() + " in original string");
break;📝 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
| private String santizeAnchors(String key, String value, String englishValue) { | |
| Matcher matcher = ANCHOR_PATTERN.matcher(value); | |
| Matcher englishMatcher = ANCHOR_PATTERN.matcher(englishValue); | |
| while (matcher.find()) { | |
| if (englishMatcher.find() && Objects.equals(matcher.group(), englishMatcher.group())) { | |
| value = value.replaceFirst(Pattern.quote(englishMatcher.group()), ""); | |
| } else { | |
| messages.add("Didn't find anchor tag " + matcher.group() + " in original string"); | |
| break; | |
| } | |
| } | |
| return value; | |
| } | |
| private String sanitizeAnchors(String key, String value, String englishValue) { | |
| Matcher englishMatcher = ANCHOR_PATTERN.matcher(englishValue); | |
| Matcher matcher = ANCHOR_PATTERN.matcher(value); | |
| while (matcher.find()) { | |
| if (englishMatcher.find() && Objects.equals(matcher.group(), englishMatcher.group())) { | |
| value = value.replaceFirst(Pattern.quote(englishMatcher.group()), ""); | |
| matcher = ANCHOR_PATTERN.matcher(value); | |
| } else { | |
| messages.add("Didn't find anchor tag " + matcher.group() + " in original string"); | |
| break; | |
| } | |
| } | |
| return value; | |
| } |
🤖 Prompt for AI Agents
In
`@misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java`
around lines 150 - 162, Rename the method from santizeAnchors to sanitizeAnchors
and fix the matcher desync by not mutating the string while iterating its
Matcher; either collect all anchor matches first via ANCHOR_PATTERN for both
value and englishValue and validate pairs, then perform removals, or after each
replaceFirst update the Matcher (recreate matcher =
ANCHOR_PATTERN.matcher(value)) and similarly reset englishMatcher when
appropriate; ensure you reference ANCHOR_PATTERN, matcher, englishMatcher,
replaceFirst and messages in the fix so anchors are validated against
englishValue before removal and the messages list is populated correctly.
| @@ -1,15 +1,15 @@ | |||
| emailVerificationSubject=El. pašto patvirtinimas | |||
| emailVerificationBody=Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą\n\n{0}\n\nŠi nuoroda galioja {1} min.\n\nJei paskyros nekūrėte, tuomet ignuoruokite šį laišką. | |||
| emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tao buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href=LT"{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>nJei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p> | |||
| emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tao buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href="{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>nJei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p> | |||
There was a problem hiding this comment.
Two text corruptions introduced in emailVerificationBodyHtml.
"Jei tao buvote"→ should be"Jei tai buvote"(compare with the plain-text version on Line 2)."<p>nJei paskyros"→ should be"<p>Jei paskyros"— a straynwas prepended toJei.
Both will be visible to end users receiving this email in Lithuanian.
Proposed fix
-emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tao buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href="{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>nJei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p>
+emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href="{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>Jei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p>📝 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
| emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tao buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href="{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>nJei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p> | |
| emailVerificationBodyHtml=<p>Paskyra {2} sukurta naudojant šį el. pašto adresą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą</p><p><a href="{0}">{0}</a></p><p>Ši nuoroda galioja {1} min.</p><p>Jei paskyros nekūrėte, tuomet ignuoruokite šį laišką.</p> |
🤖 Prompt for AI Agents
In
`@themes/src/main/resources-community/theme/base/email/messages/messages_lt.properties`
at line 3, The property emailVerificationBodyHtml contains two typos: change
"Jei tao buvote" to "Jei tai buvote" and remove the stray leading "n" before
"Jei paskyros" so it reads "<p>Jei paskyros..." ensuring the HTML string matches
the plain-text version and correct Lithuanian text.
| identityProviderLinkSubject=Sąsaja {0} | ||
| identityProviderLinkBody=Kažas pageidauja susieti Jūsų "{1}" paskyrą su "{0}" {2} naudotojo paskyrą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą norėdami susieti paskyras\n\n{3}\n\nŠi nuoroda galioja {4} min.\n\nJei paskyrų susieti nenorite, tuomet ignoruokite šį laišką. Jei paskyras susiesite, tuomet prie {1} galėsiste prisijungti per {0}. | ||
| identityProviderLinkBodyHtml=<p>žas pageidauja susieti Jūsų <b>{1}</b> paskyrą su <b>{0}</b> {2} naudotojo paskyrą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą norėdami susieti paskyras</p><p><a href=LT"{3}">{3}</a></p><p>Ši nuoroda galioja {4} min.</p><p>Jei paskyrų susieti nenorite, tuomet ignoruokite šį laišką. Jei paskyras susiesite, tuomet prie {1} galėsiste prisijungti per {0}.</p> | ||
| identityProviderLinkBodyHtml=<p>žas pageidauja susieti Jūsų <b>{1}</b> paskyrą su <b>{0}</b> {2} naudotojo paskyrą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą norėdami susieti paskyras</p><p><a href="{3}">{3}</a></p><p>Ši nuoroda galioja {4} min.</p><p>Jei paskyrų susieti nenorite, tuomet ignoruokite šį laišką. Jei paskyras susiesite, tuomet prie {1} galėsiste prisijungti per {0}.</p> |
There was a problem hiding this comment.
Truncated sentence start in identityProviderLinkBodyHtml.
The message begins with "žas pageidauja" — the leading "Kaž" was dropped. The plain-text counterpart on Line 5 has "Kažas pageidauja" (itself likely a typo for "Kažkas"). At minimum, restore the prefix to match the plain-text version; ideally fix to "Kažkas".
Proposed fix (matches plain-text wording)
-identityProviderLinkBodyHtml=<p>žas pageidauja susieti Jūsų <b>{1}</b> paskyrą su <b>{0}</b> {2} naudotojo paskyrą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą norėdami susieti paskyras</p><p><a href="{3}">{3}</a></p><p>Ši nuoroda galioja {4} min.</p><p>Jei paskyrų susieti nenorite, tuomet ignoruokite šį laišką. Jei paskyras susiesite, tuomet prie {1} galėsiste prisijungti per {0}.</p>
+identityProviderLinkBodyHtml=<p>Kažkas pageidauja susieti Jūsų <b>{1}</b> paskyrą su <b>{0}</b> {2} naudotojo paskyrą. Jei tai buvote Jūs, tuomet paspauskite žemiau esančią nuorodą norėdami susieti paskyras</p><p><a href="{3}">{3}</a></p><p>Ši nuoroda galioja {4} min.</p><p>Jei paskyrų susieti nenorite, tuomet ignoruokite šį laišką. Jei paskyras susiesite, tuomet prie {1} galėsiste prisijungti per {0}.</p>🤖 Prompt for AI Agents
In
`@themes/src/main/resources-community/theme/base/email/messages/messages_lt.properties`
at line 6, Fix the truncated leading word in the properties value for the key
identityProviderLinkBodyHtml: replace the incorrect "žas pageidauja" with the
correct "Kažkas pageidauja" (so the sentence begins "Kažkas pageidauja
susieti...") to match the plain-text counterpart and correct the typo.
Comment on lines
+423
to
+427
| webauthn-error-registration=No s''ha pogut registrar la vostra clau de seguretat.<br /> {0} | ||
| webauthn-error-api-get=No s''ha pogut identificar amb la clau de seguretat.<br /> {0} | ||
| webauthn-error-different-user=L''usuari identificat primer no és l''autenticat per la clau de seguretat. | ||
| webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br/>{0} | ||
| webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br/>{0} | ||
| webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br />{0} | ||
| webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br />{0} |
There was a problem hiding this comment.
Inconsistent spacing between <br /> and {0} within the same file.
Lines 423–424 use <br /> {0} (with a space), while lines 426–427 use <br />{0} (without a space). Since this PR's purpose is to normalize HTML formatting, these should be consistent.
Proposed fix
-webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br />{0}
-webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br />{0}
+webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br /> {0}
+webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br /> {0}📝 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
| webauthn-error-registration=No s''ha pogut registrar la vostra clau de seguretat.<br /> {0} | |
| webauthn-error-api-get=No s''ha pogut identificar amb la clau de seguretat.<br /> {0} | |
| webauthn-error-different-user=L''usuari identificat primer no és l''autenticat per la clau de seguretat. | |
| webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br/>{0} | |
| webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br/>{0} | |
| webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br />{0} | |
| webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br />{0} | |
| webauthn-error-registration=No s''ha pogut registrar la vostra clau de seguretat.<br /> {0} | |
| webauthn-error-api-get=No s''ha pogut identificar amb la clau de seguretat.<br /> {0} | |
| webauthn-error-different-user=L''usuari identificat primer no és l''autenticat per la clau de seguretat. | |
| webauthn-error-auth-verification=El resultat de l''autenticació amb clau de seguretat no és vàlid.<br /> {0} | |
| webauthn-error-register-verification=El resultat del registre amb clau de seguretat no és vàlid.<br /> {0} |
🤖 Prompt for AI Agents
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_ca.properties`
around lines 423 - 427, Normalize the spacing between the HTML line break and
the placeholder across the listed message keys: update
webauthn-error-registration and webauthn-error-api-get or
webauthn-error-auth-verification and webauthn-error-register-verification so
they all use the same convention (either "<br /> {0}" or "<br />{0}"); pick one
style and apply it to all four keys to ensure consistency.
Comment on lines
+172
to
+174
| webauthn-error-registration=თქვენი Passkey-ის რეგისტრაცია ჩავარდა.<br />{0} | ||
| passkey-unsupported-browser-text=Passkey-ის მხარდაჭერა ამ ბრაუზერს არ აქვს. სცადეთ სხვა ან დაუკავშირდით თქვენს ადმინისტრატორს. | ||
| webauthn-error-api-get=Passkey-ით ავთენტიკაცია ჩავარდა.<br/>{0} | ||
| webauthn-error-api-get=Passkey-ით ავთენტიკაცია ჩავარდა.<br />{0} |
There was a problem hiding this comment.
Cross-locale formatting inconsistency: missing space after <br />.
All four changed lines use <br />{0} (no space), while other locales in this PR (Korean, Traditional Chinese, Croatian, Spanish) consistently use <br /> {0} (with a space). For consistency across the normalization effort, consider adding the space.
Proposed fix
-webauthn-error-registration=თქვენი Passkey-ის რეგისტრაცია ჩავარდა.<br />{0}
+webauthn-error-registration=თქვენი Passkey-ის რეგისტრაცია ჩავარდა.<br /> {0}
-webauthn-error-api-get=Passkey-ით ავთენტიკაცია ჩავარდა.<br />{0}
+webauthn-error-api-get=Passkey-ით ავთენტიკაცია ჩავარდა.<br /> {0}
-webauthn-error-auth-verification=Passkey-ით ავთენტიკაციის შედეგი არასწორია.<br />{0}
+webauthn-error-auth-verification=Passkey-ით ავთენტიკაციის შედეგი არასწორია.<br /> {0}
-webauthn-error-register-verification=Passkey-ის რეგისტრაციის შედეგი არასწორია.<br />{0}
+webauthn-error-register-verification=Passkey-ის რეგისტრაციის შედეგი არასწორია.<br /> {0}Also applies to: 451-452
🤖 Prompt for AI Agents
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_ka.properties`
around lines 172 - 174, The messages for webauthn-error-registration and
webauthn-error-api-get (and any other occurrences like the duplicates mentioned)
use "<br />{0}" without a space; update these message values to use "<br /> {0}"
(i.e., add a single space after the <br />) so they match other locales'
formatting consistency—locate the keys webauthn-error-registration and
webauthn-error-api-get (and any duplicate occurrences) in messages_ka.properties
and insert the space after the break tag.
| @@ -68,7 +68,7 @@ country=Šalis | |||
| emailVerified=El. pašto adresas patvirtintas | |||
| gssDelegationCredential=GSS prisijungimo duomenų delegavimas | |||
|
|
|||
| loginTotpStep1=Įdiekite <a href="https://freeotp.github.io/" target="_blank">FreeOTP</a> arba Google Authenticator savo įrenginyje. Programėlės prieinamos <a href="https://play.google.com">Google Play</a> ir Apple App Store. | |||
| loginTotpStep1=Installa una delle seguenti applicazioni sul tuo cellulare: | |||
There was a problem hiding this comment.
Wrong language: Italian text in Lithuanian locale file.
The loginTotpStep1 value is in Italian ("Installa una delle seguenti applicazioni sul tuo cellulare:") instead of Lithuanian. The rest of the file is in Lithuanian. This will cause Lithuanian users to see a single Italian sentence during TOTP setup.
The correct Lithuanian translation would be along the lines of:
Įdiekite vieną iš šių programėlių savo mobiliajame telefone:
Proposed fix
-loginTotpStep1=Installa una delle seguenti applicazioni sul tuo cellulare:
+loginTotpStep1=Įdiekite vieną iš šių programėlių savo mobiliajame telefone:📝 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
| loginTotpStep1=Installa una delle seguenti applicazioni sul tuo cellulare: | |
| loginTotpStep1=Įdiekite vieną iš šių programėlių savo mobiliajame telefone: |
🤖 Prompt for AI Agents
In
`@themes/src/main/resources-community/theme/base/login/messages/messages_lt.properties`
at line 71, The loginTotpStep1 message is in Italian; replace the Italian value
for the loginTotpStep1 key with the correct Lithuanian translation Įdiekite
vieną iš šių programėlių savo mobiliajame telefone: in the
messages_lt.properties file, save the file with UTF-8 encoding, run a quick
build/test to ensure the properties load correctly, and commit with a clear
message like "Fix Lithuanian translation for loginTotpStep1".
Sign in
to join this conversation on GitHub.
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.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Localization