Skip to:

  1. Skip to Jira Navigation
  2. Skip to Side Navigation
  3. Skip to Main Content

Issues

Go to all issues
Select view

Select search mode

 
50 of

@SafeHtml(whitelistType = WhiteListType.NONE) allow <td>, <tr>

Fixed

Description

I use '@SafeHtml' on PsersonDto like this:

PersonDto.java

... @NotBlank @SafeHtml(whitelistType = WhiteListType.NONE) private String firstName; ...

And validate this on Spring Controller like this:

RegistrationController.java

... @RequestMapping(value = "/registrationUser", method = RequestMethod.POST) public String registrationUser(@Valid PersonDto personDto, BindingResult result) { logger.debug("registrationUser"); logger.debug("personDto: {}", personDto); if (result.hasErrors()) { for (ObjectError error : result.getAllErrors()) { logger.error("error: {}", error.getDefaultMessage()); } return formViewName; } else { personManager.registrationPerson(personDto); return completedViewName; } } ...

When I test, <script>, <b> tag is not allowed (result.hasErrors() is true), but <td>, <tr> is allowed. (result.hasErrors() is false)

When I use 'NONE' option, whitelist should allows onl text nodes. Is this hibernate validator's bug or Spring framework's bug?

Environment

None

Details

Assignee

Reporter

Bug Testcase Reminder (view)

Bug reports should generally be accompanied by a test case!

Bug Testcase Reminder (edit)

Bug reports should generally be accompanied by a test case!

Participants

Gunnar Morling
p
Richard Walker

Components

Fix versions

Affects versions

Priority

Created March 5, 2014 at 12:25 PM
Updated April 6, 2017 at 6:19 AM
Resolved April 24, 2014 at 8:53 PM

Activity

Gunnar MorlingApril 6, 2017 at 6:19 AM

Thanks for the investigation, ! As you say I'd prefer to leave things as they are then, so to not change semantics of the existing constraint.

Richard WalkerApril 6, 2017 at 12:41 AM

Yeah, not surprisingly it's more complicated than I had thought.

It was helpful for me to change the Validator code and run the test suite to see what happens.
(I'm currently working with 5.3.4.Final; it seems SafeHtmlValidator hasn't changed since this issue was resolved. I tried working with master branch, but a completely unrelated test currently fails, so I checked out 5.3.4.Final and did testing with that tag.)

Here's my modified/simplified SafeHtmlValidator:

/* * Hibernate Validator, declare and validate application constraints * * License: Apache License, Version 2.0 * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>. */ package org.hibernate.validator.internal.constraintvalidators.hv; import java.util.Iterator; import javax.validation.ConstraintValidator; import javax.validation.ConstraintValidatorContext; import org.jsoup.Jsoup; import org.jsoup.safety.Whitelist; import org.hibernate.validator.constraints.SafeHtml; /** * Validate that the string does not contain malicious code. * * It uses <a href="http://www.jsoup.org">JSoup</a> as the underlying parser/sanitizer library. * * @author George Gastaldi * @author Hardy Ferentschik */ public class SafeHtmlValidator implements ConstraintValidator<SafeHtml, CharSequence> { private Whitelist whitelist; @Override public void initialize(SafeHtml safeHtmlAnnotation) { switch ( safeHtmlAnnotation.whitelistType() ) { case BASIC: whitelist = Whitelist.basic(); break; case BASIC_WITH_IMAGES: whitelist = Whitelist.basicWithImages(); break; case NONE: whitelist = Whitelist.none(); break; case RELAXED: whitelist = Whitelist.relaxed(); break; case SIMPLE_TEXT: whitelist = Whitelist.simpleText(); break; } whitelist.addTags( safeHtmlAnnotation.additionalTags() ); for ( SafeHtml.Tag tag : safeHtmlAnnotation.additionalTagsWithAttributes() ) { whitelist.addAttributes( tag.name(), tag.attributes() ); } } @Override public boolean isValid(CharSequence value, ConstraintValidatorContext context) { if ( value == null ) { return true; } return Jsoup.isValid( value.toString(), whitelist ); } }

The "trouble" with this change is that one of the test suite methods for this issue then fails: SafeHtmlValidatorTest#testValidationOfValidFragment():

@Test @TestForIssue(jiraKey = "HV-873") public void testValidationOfValidFragment() throws Exception { descriptor.setValue( "whitelistType", WhiteListType.RELAXED ); assertTrue( getSafeHtmlValidator().isValid( "<td>1234qwer</td>", null ) ); }

It seems that "validity" now has a quite specific meaning for "body fragment": it means that the fragment must now also parse correctly if inserted directly inside a <body> tag. So although <td> is one of the permitted tags for "relaxed" mode, it has to be included inside <table><tr>....

So, to get the suite to pass, I had to replace the above test method with:

@Test @TestForIssue(jiraKey = "HV-873") public void testValidationOfInvalidFragment2() throws Exception { descriptor.setValue( "whitelistType", WhiteListType.RELAXED ); assertFalse( getSafeHtmlValidator().isValid( "<td>1234qwer</td>", null ) ); } @Test @TestForIssue(jiraKey = "HV-873") public void testValidationOfValidFragment() throws Exception { descriptor.setValue( "whitelistType", WhiteListType.RELAXED ); assertTrue( getSafeHtmlValidator().isValid( "<table><tr><td>1234qwer</td></tr></table>", null ) ); }

So maybe this is not a desirable change to make, after all.

Gunnar MorlingApril 5, 2017 at 7:43 AM

I'm not quite clear on the implications of the change you pointed to. Does it mean we wouldn't have to do that parsing-and-wrapping-in-a-document step anymore? If so, could you open a new issue, describing the suggested change for Hibernate Validator? Thanks!

Richard WalkerApril 5, 2017 at 6:46 AM

jsoup commit f44d6e64 of Thu Nov 24 14:36:13 2016 -0800 fixed the Jsoup#isValid() method to require body content and to require that the content parse correctly (e.g., end tags match start tags). Should now be good to use as-is.

Gunnar MorlingApril 23, 2014 at 7:21 AM

Flag notifications