Skip to content
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

Fix : CSS Child Combinator Parsing Bug #297

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

subbudvk
Copy link
Contributor

241b4b8#diff-0a08f29a5b7867e56d6aa9f6abe035e32ee9411a8bc96afa9a6acff2a6d6f07fR338

The above commit was made to make parsing consistent with HTML5 Spec, but while this is being rewritten it looks like a regression was introduced when parsing for >

To ensure this entity > is part of a HTML Comment it seems to have been checked the previous two characters are - but with this commit only char - 2 is checked twice. So if a CSS Child combinator with - selector is used, it was treated was a error and as a result user CSS was badly stripped. Fixing this and adding a test for it.

Fixes #251

@subbudvk subbudvk changed the title Fix CSS Child Combinator Parsing Bug Fix : CSS Child Combinator Parsing Bug Dec 20, 2023
@subbudvk
Copy link
Contributor Author

@mikesamuel @jmanico

@@ -341,7 +341,7 @@ private static int checkHtmlCdataCloseable(
}
break;
case '>':
if (i >= 2 && sb.charAt(i - 2) == '-' && sb.charAt(i - 2) == '-') {
if (i >= 2 && sb.charAt(i - 2) == '-' && sb.charAt(i - 1) == '-') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

@@ -994,7 +994,7 @@ public static final void testTextareaIsNotTextArea() {
assertEquals("x<textArea>y</textArea>", textAreaPolicy.sanitize(input));
}

@Test
@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an opportunistic fix while merging.

+ "-->\n"
+ "</style>";
assertEquals(toSanitize, factory.sanitize(toSanitize));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the unit test.

@mikesamuel mikesamuel merged commit 6b3cebd into OWASP:main Jan 15, 2024
3 checks passed
@log2akshat
Copy link

@subbudvk Thanks for fixing the bug.

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.

Stripping off the contents when Child Combinator are present in the media queries
3 participants