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 highlight.js XSS / unescaped HTML issue #438

Closed
wants to merge 2 commits into from

Conversation

mmattel
Copy link
Collaborator

@mmattel mmattel commented Mar 2, 2022

Fixes: #437 (Escape the HTML within code blocks)
References: #435 (Fix and improve highlighting with highlight.js)

When package.json was defined in the initial setup, highlight.js was not limited to the major version 9.

When the time passed by, highlight.js published two major release with twice a lot of backend changes which were incompatible with Antora. The crucial upgrade from v9 to v10 was made on 20 Dec 2020 with PR: ([Security] Bump highlight.js from 9.18.1 to 10.4.1). As there was no upgrade limitation set in package.json, CI reported a go and no other obstacles were known at the time of reading, the PR got merged.

Note that it is a good advice to regulary check the Antora Default UI for changes...

Funnily nobody identified the XSS / unescaped HTML issue which was printed in the browsers console...

With a discussion in the Antora channel Escape content in source block, it turned out which issue we have and that we have to revert to highlight.js v9 AND avoid any upgrade for the time being.

This PR:

  • Fixes the XSS issue by going back and fixing the version of highlight.js to v9.18.3 (supported by Antora)
  • Updates the pattern match regex for bash to be up-to-date with the upcoming v11 release
  • Adds and updates the keywords of bash to a more close version of v11

Note that the comments written are intentionally to avoid an accidentially update of highlight.js.

@jnweiger fyi (as discussed)

@mmattel mmattel added bug Something isn't working deployment labels Mar 2, 2022
@xoxys
Copy link
Contributor

xoxys commented Mar 2, 2022

Please let discuss this tomorrow... Sorry, but switching back to an unmaintained 2 years old version is a no-go.

Copy link
Contributor

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

Needs discussion

@mmattel
Copy link
Collaborator Author

mmattel commented Mar 2, 2022

Yes, let us discuss this tomorrow. I am not amused too...

@mmattel
Copy link
Collaborator Author

mmattel commented Mar 3, 2022

Summary of the discusson with @xoxys

We will close this PR and do not merge it.

  • The unescaped HTML report is present in all three highlight.js major versions (9, 10, 11) but it is not a highlight.js issue. Antora must sanitize/escape HTML properly before handing over data to highlight.js
  • Version 11 of highlight.js added a warning (+ a disable warning capability) if it finds unescaped HTML in a code block. Note that we use this disable warning capability with Fix and improve highlighting with highlight.js #435 - this is the reason why it came up by looking into the browser console.
  • It mostly affects '<', '>', and '&' in code examples like when using admin@<your-nas-IP> or <?php
  • Using an online XSS Scanner report(Light) tool did not report an XSS issue on an example page QNAP where unescaped HTML issues were reported
  • We do not have any user input except search where code can get injected. This means the security warning tells that there is an issue and it should be fixed on the Antora side, but it has minimal effect to us atm
  • It makes no sense to keep an 2 year old, outdated and unsupported release of highlight.js
  • With one known drawback. It is not only about highlighting example code, but there are other functionalities bound to highlighting which do not work correctly, see Upgrading highlight.js. CallOuts which are affected are currently not used in our documentation - therefore currently not an issue for us.
  • Implementing another highlighter like prism would be a lot of work, is time consuming and a resource blocker

@mmattel mmattel closed this Mar 3, 2022
@mmattel mmattel deleted the fix_highlight.js_xss_issue branch March 3, 2022 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escape the HTML within code blocks
2 participants