-
Notifications
You must be signed in to change notification settings - Fork 0
Add HTML sanitizer for translated message resources #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature-html-sanitizer-baseline
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,17 @@ | |
| <version>2.2</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <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> | ||
|
Comment on lines
+75
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainValidate sanitizer and commons-text versions against your platform baseline Adding 🌐 Web query: 💡 Result: Summary (checked Nov 22, 2025):
Recommendations
Sources
If you want, I can: (a) check your project’s dependency tree for transitive versions, or (b) fetch distro-specific advisories (e.g., Red Hat, Debian) for these artifacts. Citations:
Upgrade commons-text to 1.14.0 for latest stability owasp-java-html-sanitizer:20240325.1 is the current upstream release, with no public high/critical CVE found for that version. However, commons-text:1.13.0 is not the latest stable release; 1.14.0 was released in July 2025. While 1.13.0 is not vulnerable to CVE-2022-42889 (Text4Shell), upgrading to 1.14.0 aligns with best practices for dependency currency. 🤖 Prompt for AI Agents |
||
| </dependencies> | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,15 +17,22 @@ | |
| package org.keycloak.themeverifier; | ||
|
|
||
| import org.apache.maven.plugin.MojoExecutionException; | ||
| import org.owasp.html.PolicyFactory; | ||
|
|
||
| import java.io.BufferedReader; | ||
| import java.io.File; | ||
| import java.io.FileInputStream; | ||
| import java.io.IOException; | ||
| import java.io.StringReader; | ||
| import java.nio.file.Files; | ||
| import java.util.ArrayList; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.MissingResourceException; | ||
| import java.util.Objects; | ||
| import java.util.PropertyResourceBundle; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| public class VerifyMessageProperties { | ||
|
|
||
|
|
@@ -41,12 +48,129 @@ public List<String> verify() throws MojoExecutionException { | |
| try { | ||
| String contents = Files.readString(file.toPath()); | ||
| verifyNoDuplicateKeys(contents); | ||
| verifySafeHtml(); | ||
| } catch (IOException e) { | ||
| throw new MojoExecutionException("Can not read file " + file, e); | ||
| } | ||
| return messages; | ||
| } | ||
|
|
||
| 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 void verifySafeHtml() { | ||
| PropertyResourceBundle bundle; | ||
| try (FileInputStream fis = new FileInputStream(file)) { | ||
| bundle = new PropertyResourceBundle(fis); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("unable to read file " + file, e); | ||
| } | ||
|
|
||
| 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); | ||
| } | ||
|
|
||
|
Comment on lines
+65
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainEnglish-bundle resolution is convention-based; ensure it matches all target layouts
This works for the tests and for conventional
🏁 Script executed: #!/bin/bash
# List all *.properties files this module normally verifies to check that
# each non-en file has a matching *_en.properties alongside it.
fd '\.properties$' -t f |
gawk '
/_([A-Za-z-]+)\.properties$/ {
base=$0; sub(/_[A-Za-z-]+\.properties$/, "", base);
en=base "_en.properties";
if (system("[ -f \"" en "\" ]") != 0) {
print "Missing English counterpart for: " $0 " (expected " en ")"
}
}
'Length of output: 36611 Fix English-bundle lookup to handle non-standard naming conventions The verification script confirms that
Over 130+ localized properties files in themes/, js/apps/, and testsuite/ directories do not follow the assumed try (FileInputStream fis = new FileInputStream(englishFile)) {
bundleEnglish = new PropertyResourceBundle(fis);
} catch (IOException e) {
// Fall back to current bundle if English variant is unavailable
bundleEnglish = bundle;
}Or make the convention configurable and fail explicitly with 🤖 Prompt for AI Agents |
||
| bundle.getKeys().asIterator().forEachRemaining(key -> { | ||
| String value = bundle.getString(key); | ||
| value = normalizeValue(key, value); | ||
| String englishValue = getEnglishValue(key, bundleEnglish); | ||
| englishValue = normalizeValue(key, englishValue); | ||
|
|
||
| value = santizeAnchors(key, value, englishValue); | ||
|
|
||
| // Only if the English source string contains HTML we also allow HTML in the translation | ||
| PolicyFactory policy = containsHtml(englishValue) ? POLICY_SOME_HTML : POLICY_NO_HTML; | ||
| String sanitized = policy.sanitize(value); | ||
|
|
||
| // Sanitizer will escape HTML entities for quotes and also for numberic tags like '<1>' | ||
| sanitized = org.apache.commons.text.StringEscapeUtils.unescapeHtml4(sanitized); | ||
| // Sanitizer will add them when there are double curly braces | ||
| sanitized = sanitized.replace("<!-- -->", ""); | ||
|
|
||
| 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) + "'"); | ||
| } | ||
|
|
||
| }); | ||
| } | ||
|
|
||
| private String normalizeValue(String key, String value) { | ||
| if (key.equals("templateHelp")) { | ||
| // Allow "CLAIM.<NAME>" here | ||
| value = value.replaceAll("CLAIM\\.<[A-Z]*>", ""); | ||
| } else if (key.equals("optimizeLookupHelp")) { | ||
| // Allow "<Extensions>" here | ||
| value = value.replaceAll("<Extensions>", ""); | ||
| } else if (key.startsWith("linkExpirationFormatter.timePeriodUnit") || key.equals("error-invalid-multivalued-size")) { | ||
| // The problem is the "<" that appears in the choice | ||
| value = value.replaceAll("\\{[0-9]+,choice,[^}]*}", "..."); | ||
| } | ||
|
|
||
| // Unescape HTML entities, as we later also unescape HTML entities in the sanitized value | ||
| value = org.apache.commons.text.StringEscapeUtils.unescapeHtml4(value); | ||
|
|
||
| if (file.getAbsolutePath().contains("email")) { | ||
| // TODO: move the RTL information for emails | ||
| value = value.replaceAll(Pattern.quote(" style=\"direction: rtl;\""), ""); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| Pattern HTML_TAGS = Pattern.compile("<[a-z]+[^>]*>"); | ||
|
|
||
| private boolean containsHtml(String englishValue) { | ||
| return HTML_TAGS.matcher(englishValue).find(); | ||
| } | ||
|
|
||
| private static final Pattern ANCHOR_PATTERN = Pattern.compile("</?a[^>]*>"); | ||
|
|
||
| /** | ||
| * Allow only those anchor tags from the source key to also appear in the target key. | ||
| */ | ||
| 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; | ||
| } | ||
|
Comment on lines
+145
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainAnchor sanitization logic works; improve naming and diagnostics The
Two minor polish suggestions:
Behavior need not change; this is about debuggability. 🌐 Web query: 💡 Result: Short answer: Yes — OWASP Java HTML Sanitizer provides built-in handling for safe anchors (via its LINKS sanitizer and HtmlPolicyBuilder allowUrlProtocols/requireRelNofollow options), and common practices used by OWASP and other sanitizers match the usual checks your custom anchor comparison should do: whitelist schemes, normalize/percent-decode and compare schemes/hosts, block dangerous schemes (javascript:, data:), and add rel="nofollow"/rel="noopener" for external/target=_blank links. See sources below for the exact APIs and behaviour. Key points to align your comparator with common practice:
Sources: If you want, paste your custom anchor-comparison code and I’ll check it line-by-line against these practices. I need to understand the actual purpose of this method within the codebase to determine if the OWASP best practices are relevant. Let me check the usage context. 🏁 Script executed: #!/bin/bash
# Find method calls to santizeAnchors to understand its usage context
rg -nP '\bsantizeAnchors\s*\(' misc/theme-verifier/src/main/java/org/keycloak/themeverifier/ -A 3 -B 3Length of output: 2058 🏁 Script executed: #!/bin/bash
# Check class-level documentation and overall structure
cat misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java | head -100Length of output: 3871 🏁 Script executed: #!/bin/bash
# Search for other sanitization methods in the file to understand scope
rg -nP 'private.*String.*\(' misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java -A 2Length of output: 534 Fix typo in method name and enhance error diagnostics The Two fixes remain valid:
🤖 Prompt for AI Agents |
||
|
|
||
| private static String getEnglishValue(String key, PropertyResourceBundle bundleEnglish) { | ||
| String englishValue; | ||
| try { | ||
| englishValue = bundleEnglish.getString(key); | ||
| } catch (MissingResourceException ex) { | ||
| englishValue = ""; | ||
| } | ||
| return englishValue; | ||
| } | ||
|
|
||
| private void verifyNoDuplicateKeys(String contents) throws IOException { | ||
| BufferedReader bufferedReader = new BufferedReader(new StringReader(contents)); | ||
| String line; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # | ||
| # Copyright 2025 Red Hat, Inc. and/or its affiliates | ||
| # and other contributors as indicated by the @author tags. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| key=Some <a href="http://malicious.com">link</a> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # | ||
| # Copyright 2025 Red Hat, Inc. and/or its affiliates | ||
| # and other contributors as indicated by the @author tags. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| key=Some <a href="http://example.com">link</a> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # | ||
| # Copyright 2025 Red Hat, Inc. and/or its affiliates | ||
| # and other contributors as indicated by the @author tags. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| key=Some <div>tag</div |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # | ||
| # Copyright 2025 Red Hat, Inc. and/or its affiliates | ||
| # and other contributors as indicated by the @author tags. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| key=Some <b>HTML</b> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # | ||
| # Copyright 2025 Red Hat, Inc. and/or its affiliates | ||
| # and other contributors as indicated by the @author tags. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| key=No HTML |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,7 @@ revoke=Atšaukti įgaliojimą | |
|
|
||
| configureAuthenticators=Sukonfigūruotas autentifikatorius | ||
| mobile=Mobilus | ||
| totpStep1=Į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. | ||
| totpStep1=Installa una delle seguenti applicazioni sul tuo cellulare: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainFix language mismatch in Lithuanian localization. Line 101 contains Italian text ("Installa una delle seguenti applicazioni sul tuo cellulare:") instead of Lithuanian. This breaks the Lithuanian localization for TOTP setup. Correct this to the appropriate Lithuanian translation to match the file's language and align with the intent shown in lines 102–103. Verify the correct Lithuanian translation for the TOTP setup instruction. The current value should be replaced with a Lithuanian translation (e.g., something similar to the Norwegian or Swedish equivalents in other files). I'll verify the review comment by examining the actual file and comparing translations across language files. Length of output: 555 🏁 Script executed: #!/bin/bash
# Extract the content around line 101 from the Lithuanian file
sed -n '98,105p' "themes/src/main/resources-community/theme/base/account/messages/messages_lt.properties"Length of output: 393 🏁 Script executed: #!/bin/bash
# Find all messages_*.properties files to compare translations
find themes/src/main/resources-community/theme/base/account/messages -name "messages_*.properties" -type f | sortLength of output: 2618 🏁 Script executed: #!/bin/bash
# Extract the totpStep1 line from multiple language files to compare
for file in $(find themes/src/main/resources-community/theme/base/account/messages -name "messages_*.properties" -type f | sort); do
echo "=== $(basename $file) ==="
grep -n "^totpStep1=" "$file" || echo "NOT FOUND"
doneLength of output: 2988 🏁 Script executed: #!/bin/bash
# Check other Lithuanian message files for the totpStep1 key to see if there's a correct translation elsewhere
for file in $(find . -name "messages_lt.properties" -type f); do
echo "=== $(echo $file | sed 's|.*/||') ==="
grep -n "totpStep1=" "$file" || echo "NOT FOUND"
doneLength of output: 309 🏁 Script executed: #!/bin/bash
# Check if there are any other Lithuanian files with TOTP-related content
for file in $(find . -name "*_lt.properties" -o -name "*_lt*.properties" 2>/dev/null | grep -i totp); do
echo "=== $(echo $file | sed 's|.*/||') ==="
grep -i "totp" "$file" | head -5
doneLength of output: 0 🏁 Script executed: #!/bin/bash
# Look for any Italian content in Lithuanian files that shouldn't be there
grep -l "Installa una delle seguenti" themes/src/main/resources-community/theme/base/**/*_lt.properties 2>/dev/nullLength of output: 0 Fix language mismatch in Lithuanian localization at line 101 of 🤖 Prompt for AI Agents |
||
| totpStep2=Atidarykite programėlę ir nuskenuokite barkodą arba įveskite kodą. | ||
| totpStep3=Įveskite programėlėje sugeneruotą vieną kartą galiojantį kodą ir paspauskite Saugoti norėdami prisijungti. | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Broken properties entry: orphan line without key (parsing bug).
Line 3139 starts with plain text (“to the attribute...”) and will be parsed as key “to” with a long value, corrupting the bundle at runtime. Merge it with line 3138.
Apply this diff:
Run this repo-wide check for malformed .properties lines (missing '=' or ':' and not comments/blanks):
🏁 Script executed:
Length of output: 4613
🏁 Script executed:
Length of output: 368
🏁 Script executed:
Length of output: 540
🏁 Script executed:
Length of output: 1255
🏁 Script executed:
Length of output: 787
Confirmed: Critical parsing bug in line 3139 needs merging with line 3138.
The verification shows that line 3139 in
js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.propertieslacks a key-value separator and contains orphaned text that belongs to the previous line's value. The.propertiesparser will treat "to" as a separate key, corrupting the message bundle. The proposed diff correctly merges the two lines into a single property entry.The same issue exists in the Spanish translation at line 3145 of
messages_es.properties.🤖 Prompt for AI Agents