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

astro-rss: Generate feed with proper XML escaping #5550

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Dec 7, 2022

Changes

Generating XML using string concatenation is dangerous and can lead to underescaping.

Just to preemptively dismiss some excuses for this unsafety 😛:

  • “But most of the content was surrounded with with <![CDATA[…]]>, so it was fine!” Wrong. The content might contain ]]> itself and break out of the <![CDATA[ early.
  • “But URLs don’t need to be escaped, so it was fine!” Wrong. & commonly appears in URLs and needs to be escaped for XML or HTML serialization.
  • “But we were validating the output XML, so it was fine!” Wrong. A bug that’s caught at generation time is still a bug; also, some cases of underescaping could lead to incorrect output that isn’t invalid.
  • “But there’s no user-generated content for an attacker to control, so it was fine!” Wrong. Underescaping is still a bug even when it’s not a security vulnerability.

So let’s take the fast-xml-parser library that we were using to validate our hand-rolled XML, and use it to properly generate XML instead.

Testing

I updated the existing tests to compare the XML using chai-xml, so that the snapshotted XML strings didn’t need to change. This verifies that the new output is semantically equivalent.

Docs

This doesn’t need any documentation changes. I even preserve support for the existing customData option by parsing and re-serializing the XML string.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2022

🦋 Changeset detected

Latest commit: beeef3a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks for contributing!

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.

3 participants