Support for @media rule
#553
Conversation
There was a problem hiding this comment.
I'm going to be in full agreement with @kwwall here that we should NOT be turning on more than the bare minimum as a safe default. As a security professional, I want the principle of least privilege to be in play and we should only be allowlisting CSS iff the person using antisamy has a requirement for it. This is why by default, antisamy doesn't for example, allow HTML in attributes.
I don't have deep experience with CSS, while I've had some occasional brushes with it, I've been mostly a backend developer for my career.
There was a problem hiding this comment.
Unfortunately, I cannot see any comments made by @kwwall, so I don't know what you're referring to exactly.
I guess you are concerned about the additions to the default antisamy.xml policy?
The main goal this PR is trying to achieve is to generally support the parsing and sanitizing of @media rules. In it's current state the batik-css parser only seems to support a rather old version of the @media rule. More importantly though, Antisamy's CssHandler.java does not implement startMedia() or endMedia():
/*
* (non-Javadoc)
*
* @see org.w3c.css.sac.DocumentHandler#startMedia(org.w3c.css.sac.SACMediaList)
*/
@Override
public void startMedia(SACMediaList media) throws CSSException {
// CSS2 Media declaration - ignore this for now
}This is what this PR is trying to tackle.
Whether all possible media queries should be allowed per default is up to discussion.
I definitely would be fine with putting the lax rules of this PR in the anythinggoes.xml policy, instead of the default one.
There was a problem hiding this comment.
I understand the concern. As it is too much new policy definition for a feature that is better not to be used (CSS parsing) it will be better to move the policy changes to anythinggoes.xml and expand the policy accordingly in the tests for what is needed, as in other test cases that extend the default policy.
There was a problem hiding this comment.
In general I think this is a good implementation of media which I believe it was requested before (and is very common) but I cannot find a specific issue to link. Of course it involves too many new parts and the majority of them should be inside Batik CSS and not in AntiSamy but it is the only way it can be done "fast". In fact, my recommendation is to suggest to implement it there, probably most of the code could be reused.
In my opinion, considering the comments of others it should be a non default feature, even though it is pretty restricted to specific values and simple regular expressions. Just to discourage people on using HTML filters for CSS as a miracle solution against XSS.
| exp = parseTerm(null); | ||
| } | ||
| if (current != LexicalUnits.RIGHT_BRACE) { | ||
| throw createCSSParseException("right.brace"); |
There was a problem hiding this comment.
Does not seem consistent with the LEFT_BRACE expection above.
| } | ||
| styleSheet.append(query.getMediaType()); | ||
| for (CssMediaFeature feature : query.getMediaFeatures()) { | ||
| styleSheet.append(" and ("); |
There was a problem hiding this comment.
Seeing the hardcoded "and" makes me realize I don't identify the or operator posibility when building these features. Or maybe other logical valid combinations. Am I missing something?
There was a problem hiding this comment.
I understand the concern. As it is too much new policy definition for a feature that is better not to be used (CSS parsing) it will be better to move the policy changes to anythinggoes.xml and expand the policy accordingly in the tests for what is needed, as in other test cases that extend the default policy.
| @@ -2755,4 +2757,55 @@ public void testGithubIssue546FaultyPercentagesGetFilteredByRegex() throws ScanE | |||
| assertEquals(expectedCleanHtml, crDom.getCleanHTML()); | |||
| assertEquals(expectedCleanHtml, crSax.getCleanHTML()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testGithubIssue552() throws ScanException, PolicyException { | |||
There was a problem hiding this comment.
I like the test variety. However I don't see any with or or with content between { }. I think it is relevant to test the operators and content as all of that is in this proposed implementation.
Possible implementation for #552 .
Please feel free to review.