New methods for excluding elements with specific missing or empty attributes

[forum-is]

We recently came across a problem where an img-element with missing src-attribute caused our PDF engine to break. As the current version of the html sanitizer only contains a method for disallowing elements without any attributes, we suggest adding methods to target a specific missing/empty attribute.
As a convenience we also included a method to check for a non-matching regex pattern.

[jmanico]

Thank you very, very much for submitting a patch with your report. More from Mike soon!

Aloha,

Jim Manico
@manicode

On Oct 9, 2015, at 4:53 PM, FORUM Gesellschaft für Informationssicherheit mbH notifications@github.com wrote:

We recently came across a problem where an img-element with missing src-attribute caused our PDF engine to break. As the current version of the html sanitizer only contains a method for disallowing elements without any attributes, we suggest adding methods to target a specific missing/empty attribute.
As a convenience we also included a method to check for a non-matching regex pattern.

You can view, comment on, or merge this pull request online at:

#45

Commit Summary

+Added HtmlPolicyBuilder methods for excluding elements with specific missing or emtpy attributes
Reverted changes
added methods for excluding elements with specific empty or missing attributes or elements that do NOT match a pattern
File Changes

M src/main/java/org/owasp/html/HtmlPolicyBuilder.java (49)
Patch Links:

https://github.com/OWASP/java-html-sanitizer/pull/45.patch
https://github.com/OWASP/java-html-sanitizer/pull/45.diff
—
Reply to this email directly or view it on GitHub.

[mikesamuel]

Right now, we can say

    myPolicyBuilder.allowAttributes("src", ...).onElements("img")

and if I understand your goal, that is problematic because it allows but does not require src="..." on <img ...>.

I'd prefer

    myPolicyBuilder.withAttributes("src", ...).required().onElements("img")

which allows mixing required() into the existing flow by which elements are associated with attributes instead of creating a new API.

[mikesamuel]

This change seems to be missing any tests. Could you at least check that it works with the kind of policy you want to work.

[mikesamuel]

We recently came across a problem where an img-element with missing src-attribute caused our PDF engine to break. As the current version of the html sanitizer only contains a method for disallowing elements without any attributes, we suggest adding methods to target a specific missing/empty attribute.
As a convenience we also included a method to check for a non-matching regex pattern.

What PDF engine is this? If you don't maintain it then it might be worth filing a bug with them as well.

Should this change also require src="..." on <img> in Sanitizers.IMAGES?

[forum-is]

Hello Mike,

sorry for the wait.

Our goal was indeed to be able to require certain attributes when their element would make no sense or as in this case even cause harm without them, which was not possible with the current allowAttributes()-implementation.

Your proposed mimic of a chainable required() seems indeed a cleaner approach. Yet my understanding of the project code is currently not sufficient to change the implementation to that within reasonable effort. I will however try and provide a test case to show the validity of the current implementation shortly. Maybe then you could refactor it to the required()-mimic as you find the time.

We use Apache FOP to export PDFs from our web application.

As you mention, it might actually be useful to require the attribute in the standard sanitizer so others do not run into that kind of issue unsuspectingly. At least no valid use case comes to mind for an img without a src that would forbid it from being required.

Best regards,
Sebastian

[forum-is]

Hello Mike,

I added a TestCase for the disallowWithoutAttribute() functionality that represents the constellation that caused us problems (arbitrary alt, missing src).

Best regards,
Sebastian

[jmanico]

It looks like this is mostly resolved. @mikesamuel - can we merge in the test case or resolve this pull request somehow? It does not look like major work is needed to resolve this..

[jmanico]

Bump @mikesamuel ?

[mikesamuel]

Sorry for ghosting. I'll take a look shortly.

[mikesamuel]

{dis,}allowWithoutAttributes were reworked recently. https://github.com/OWASP/java-html-sanitizer/blob/main/change_log.md says

Release 20200615.1

• Change .and when combining two policies to respect explicit skipIfEmpty decisions.

which affected

java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java

Lines 314 to 343 in ca40697

	/**
	* Assuming the given elements are allowed, allows them to appear without
	* attributes.
	*
	* @see #DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR
	* @see #disallowWithoutAttributes
	*/
	public HtmlPolicyBuilder allowWithoutAttributes(String... elementNames) {
	invalidateCompiledState();
	for (String elementName : elementNames) {
	elementName = HtmlLexer.canonicalElementName(elementName);
	skipIssueTagMap.put(elementName, HtmlTagSkipType.DO_NOT_SKIP);
	}
	return this;
	}
	/**
	* Disallows the given elements from appearing without attributes.
	*
	* @see #DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR
	* @see #allowWithoutAttributes
	*/
	public HtmlPolicyBuilder disallowWithoutAttributes(String... elementNames) {
	invalidateCompiledState();
	for (String elementName : elementNames) {
	elementName = HtmlLexer.canonicalElementName(elementName);
	skipIssueTagMap.put(elementName, HtmlTagSkipType.SKIP);
	}
	return this;
	}

IIUC, what you need is a way to drop specific elements that are missing some core attributes.

The following passes for me, which uses an ElementPolicy to reject <img> tags that don't have "src" in a key position. It may spuriously drop <img alt="src" src="foo.png"> but works as a POC.

    String input = "<img src=foo.png><img alt=bar>";
    PolicyFactory imgOkWithSrc = new HtmlPolicyBuilder()
            .allowElements(
                    ((elementName, attrs) -> (attrs.indexOf("src") & 1) == 0 ? elementName : null),
                    "img"
            )
            .allowAttributes("alt", "src").onElements("img")
            .toFactory();
    assertEquals("<img src=\"foo.png\" />", imgOkWithSrc.sanitize(input));