Skip to content

Conversation

@everettbu
Copy link
Contributor

Test 7

Closes #37428

Signed-off-by: Alexander Schwartz <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR implements HTML sanitization for translated message resources across Keycloak's theme system. The implementation adds a comprehensive validation layer using OWASP HTML Sanitizer to ensure that internationalized message properties files only contain safe HTML content.

Core Implementation

The central component is the enhanced VerifyMessageProperties.java class in the theme-verifier module, which now includes HTML sanitization policies using OWASP HTML Sanitizer. The system defines two main policies:

  • POLICY_NO_HTML: Strips all HTML content for messages where the English source contains no HTML
  • POLICY_SOME_HTML: Allows only safe HTML elements (<br>, <p>, <strong>, <b>) for messages where HTML is present in the English source

The verification process compares each translated message against its English counterpart to ensure HTML consistency. Special handling is implemented for anchor tags to prevent URL injection through translations.

Testing Infrastructure

Comprehensive test coverage has been added with new test resource files:

  • illegalHtmlTag_en.properties: Tests detection of malformed HTML (unclosed <div> tag)
  • noHtml_en.properties and noHtml_de.properties: Test HTML rejection where none should exist
  • changedAnchor_en.properties and changedAnchor_de.properties: Test anchor tag validation between languages
  • duplicateKeys_en.properties: Renamed and standardized existing duplicate key test

Mass Translation Updates

The PR includes systematic updates across 25+ language files to standardize HTML formatting:

  • Converting <br/> to <br /> (adding space before closing slash) for XHTML compliance
  • Fixing malformed HTML tags (missing closing brackets, extra spaces)
  • Removing unauthorized HTML content like external links in TOTP setup instructions
  • Correcting HTML entity issues and parameter reference bugs

Integration with Build Process

Two new Maven dependencies were added to the theme-verifier:

  • OWASP Java HTML Sanitizer (20240325.1) for security validation
  • Apache Commons Text (1.13.0) for HTML entity processing

The theme verifier runs during build time to validate all message properties files, ensuring only sanitized content reaches production.

Confidence score: 2/5

• This PR contains critical implementation issues that could cause build failures and localization bugs
• The implementation has potential infinite loops, hardcoded assumptions, and contains at least one case where Italian text replaced Lithuanian content
• Files misc/theme-verifier/src/main/java/org/keycloak/themeverifier/VerifyMessageProperties.java, themes/src/main/resources-community/theme/base/account/messages/messages_lt.properties, and themes/src/main/resources-community/theme/base/login/messages/messages_lt.properties need immediate attention

44 files reviewed, 8 comments

Edit Code Review Bot Settings | Greptile

# See the License for the specific language governing permissions and
# limitations under the License.
#
key=Some <a href="http://example.com">link</a> No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Missing final newline - should end with a newline character according to .editorconfig

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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This line contains Italian text in a Lithuanian translation file. Should be Lithuanian text: 'Installa una delle seguenti applicazioni sul tuo cellulare:' should be replaced with proper Lithuanian translation.

Suggested change
totpStep1=Installa una delle seguenti applicazioni sul tuo cellulare:
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.

# See the License for the specific language governing permissions and
# limitations under the License.
#
key=Some <b>HTML</b> No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: missing final newline according to .editorconfig rules

# See the License for the specific language governing permissions and
# limitations under the License.
#
key=No HTML No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: missing newline at end of file, violates .editorconfig rule insert_final_newline = true

Comment on lines +58 to +61
PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder()
.allowElements(
"br", "p", "strong", "b"
).toFactory();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: instance fields should be private static final for thread safety and memory efficiency

Suggested change
PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder()
.allowElements(
"br", "p", "strong", "b"
).toFactory();
private static final PolicyFactory POLICY_SOME_HTML = new org.owasp.html.HtmlPolicyBuilder()
.allowElements(
"br", "p", "strong", "b"
).toFactory();

/**
* 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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: method name has typo: santizeAnchors should be sanitizeAnchors

Suggested change
private String santizeAnchors(String key, String value, String englishValue) {
private String sanitizeAnchors(String key, String value, String englishValue) {

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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Critical localization error: Italian text replaced Lithuanian translation for TOTP setup instructions. This breaks the user experience for Lithuanian users and removes essential download links for authenticator apps.

Suggested change
loginTotpStep1=Installa una delle seguenti applicazioni sul tuo cellulare:
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.

Comment on lines +83 to +84
<version>1.13.0</version>
<scope>compile</scope>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The <scope>compile</scope> is redundant since compile is the default scope

Suggested change
<version>1.13.0</version>
<scope>compile</scope>
<version>1.13.0</version>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants