Skip to content

Don't write self-closing tag with empty attributes#151

Merged
grafana-dee merged 1 commit intomicrocosm-cc:mainfrom
Gusted:self-closing-tag-bug
Oct 3, 2022
Merged

Don't write self-closing tag with empty attributes#151
grafana-dee merged 1 commit intomicrocosm-cc:mainfrom
Gusted:self-closing-tag-bug

Conversation

@Gusted
Copy link
Contributor

@Gusted Gusted commented Oct 1, 2022

  • Currently the code will write self-closing tags with empty attributes to the output, even when the element isn't allowed to have empty attributes.
  • Move to break to one outer scope to fix it.

- Currently the code will write self-closing tags with empty attributes
to the output, even when the element isn't allowed to have empty
attributes.
- Move to `break` to one outer scope to fix it.
@grafana-dee
Copy link
Contributor

Thanks, this is a good catch and clearly a long-time minor bug.

For the people from Snyk, etc it is not a security bug in that this resolves an issue that wouldn't have led to an XSS.

What this bug is... it's tiny, but some elements are meaningful in HTML without attributes, for example the <hr/> has no attributes and will draw a horizontal rule. But other elements are meaningless without attributes as they result in a no-op... and in those cases where sanitization would've removed all attributes, then the element should also be removed.

This bug meant that a policy that added an element via allowing an attribute... where the attribute was not actually in the input... then the element was still meaningless and should've been removed.

The bug produced a no-op... hence no risk. But still, a bug is a bug and this fix correctly aligns the output with the desired state, which is to remove empty elements that are meaningless without attributes.

Thanks @Gusted for the change.

@grafana-dee grafana-dee merged commit 07c5693 into microcosm-cc:main Oct 3, 2022
@Gusted Gusted deleted the self-closing-tag-bug branch October 3, 2022 12:54
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