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

Allow markdown in Project status description #4146

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

tomodwyer
Copy link
Member

@tomodwyer tomodwyer commented Feb 21, 2024

Closes #3338

This PR allows users to enter markdown in their Project Status Description. I have used the markdown library already used in jobserver/snippets.py.

Security

As HTML is valid in a Markdown file, we should protect against users adding <script> and <style> tags to their markdown. Whilst we do have a Content Security Policy on Job Server which would stop this, we should not allow it to be converted in to valid HTML.

The bleach library was recommended as part of a changelog entry in the Python Markdown library.

The library nh3 is used to clean the HTML before it is displayed in the template.

Screenshots

Before

After

This example shows the following text entered by a user, with an example of how a script tag would be rendered.

We aimed to explore general practice coding activity associated with the use of online consultation systems in terms of trends, COVID-19 effect, variation and quality. The pandemic accelerated work by the NHS in England to enable and stimulate use of online consultation systems across all practices, for improved access to primary care.

With the approval of NHS England, OpenSAFELY-TPP and OpenSAFELY-EMIS were used to query and analyse in situ records of electronic health record systems of over 53 million patients in over 6,400 practices, mainly in 2019-2020. SNOMED CT codes relevant to online consultation systems and written online consultations were identified. Coded events were described by volumes, practice coverage, trends pre- and post-COVID-19 and inter-practice and sociodemographic variation.

- [Blog link](https://nhsx.github.io/AnalyticsUnit/openSafely_onlineconsultations.html)
- [Pre-print link](https://www.medrxiv.org/content/10.1101/2023.01.25.23284428v1)
- Paper link TBD

<script>console.log("Hello world")</script>

@tomodwyer tomodwyer self-assigned this Feb 21, 2024
@tomodwyer tomodwyer marked this pull request as ready for review February 21, 2024 12:02
@StevenMaude
Copy link
Contributor

In principle, the actual code changes are fine 👍

However, Bleach is deprecated1, so not sure if we'd want to find something else. The opening comment in the linked thread from January 2023 says:

Bleach sits on top of--and heavily relies on--html5lib which is no longer in active development. It is increasingly difficult to maintain Bleach in that context and I think it's nuts to build a security library on top of a library that's not in active development. There are some options (switch to something else, take over html5lib, etc), I don't particularly like any of them. I think instead, someone new should explore the options with a brand new library and a fresh start.

Although later in the same thread, posted October 2023:

I will continue fixing security issues as they come up.

So: I'm not sure 🤷‍♂️

The something else that a few linked issues in the Bleach thread point to seem to be nh3, but that would require trying it out.

Footnotes

  1. I only discovered this from the PyPI page for Bleach. It's not obvious from the Python Markdown page that was linked in this PR.

@tomodwyer
Copy link
Member Author

@StevenMaude based on your findings, I've switched to nh3 which also means we're only adding one dependency, instead of two.

@StevenMaude
Copy link
Contributor

To add, I'd be very slightly wary of nh3 too, just because if we wanted to modify it or debug it, it might be trickier (it provides bindings to a Rust library).

But as we're most likely using it in this drop-in way, I think it'll be fine, and it's preferable to use something that's not end-of-life 👍

@tomodwyer tomodwyer merged commit 2462598 into main Feb 22, 2024
6 checks passed
@tomodwyer tomodwyer deleted the markdown-status-description branch February 22, 2024 09:02
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.

Support links in Project Status description
2 participants