[LS-81] fix(markdown-utils): change sanitization process + add unescape

[seaerchin]

Problem

Previously, there was inconsistent behaviour caused by sanitization on the backend. This is because of dompurify's sanitization config, where it will automatically html encode certain special characters if it detects that there is a html tag.

This process affects not just content within the tag, but the string as a whole. For example,

const x = "& <b>&something</b>"

will have both ampersands encoded even though the first one is outside of the b-tag.

Closes LS-81

Solution

In order to make sure that sanitization takes place properly, this PR establishes an invariant, as follows:
frontmatter content is never html encoded.

This is chosen over html encoding all content in our frontmatter due to the existence of the 3 special pages (homepage/nav/contact-us), where users are able to input html (:sadge:)

In order to preserve this property, a few rules have to be followed (done alr at present)

1. devs never call sanitize directly for markdown files but through a given interface (<convert|retrieve>DataFromMarkdown).
2. the respective conversion/retrieval functions separately sanitize frontmatter/body
3. we run unescape after sanitizing frontmatter.

This allows us

Testing

• navigate to the CMS and choose a testing site
• make the page have special character in frontmatter (can be done through editing the page directly on github if lazy)
• rename a page to have a special character in the title
• check that clicking settings on the page does not show a html encoded title

Notes
This change might be destructive - existing pages w html encoded properties in their frontmatter will have them unescaped. I'm not entirely sure how this impacts things like permalink etc but it's also possible to guarantee only for the title property

[seaerchin]

@alexanderleegs tagging you separately in case there is anything that isn't back-compat

[kishore03109]

This change might be destructive - existing pages w html encoded properties in their frontmatter will have them unescaped. I'm not entirely sure how this impacts things like permalink etc but it's also possible to guarantee only for the title property

hmm, if we are not sure the possible implications of this, is there a reason why we don't make this change only for the title property then to reduce surface area of bugs?

[kishore03109]

Nit: This solution escapes all html characters, wdyt about only escaping &, since that is the most common case that we are encountering at the moment?

[seaerchin]

the common case isn't the only case - this means that the same bug can appear, just with a different character that's escaped. in the event that it happens, we'd have to expend eng resources to dig through + fix so i'd rather just escape all

[kishore03109]

Do you think there are going to be additional security concerns that we might have with allowing all html for all frontmatter?
Considering we have CSP headers + this is already the case for some pages, it seems ok, but just checking in

[seaerchin]

this PR doesn't allow/disallow html, it just escapes encoded html present in frontmatter. the sanitization invariant is still preserved (front matter still sanitised)

[alexanderleegs]

creating new pages creates this commented out thing at the moment!

[alexanderleegs]

@alexanderleegs tagging you separately in case there is anything that isn't back-compat

I think this is fine, we previously didn't do any html encoding for the special pages either as far as i know?

[seaerchin]

creating new pages creates this commented out thing at the moment!

does this actually impact editing experience? this is injected due to sanitize encountering an empty body and injecting it into a document. if it's concerning, we could always avoid sanitization if it's an empty string

[kishore03109]

nit:

does this actually impact editing experience? this is injected due to sanitize encountering an empty body and injecting it into a document. if it's concerning, we could always avoid sanitization if it's an empty string

this does seem confusing at the first glance... could we add a test case to show this behaviour is expected?

[alexanderleegs]

creating new pages creates this commented out thing at the moment!

does this actually impact editing experience? this is injected due to sanitize encountering an empty body and injecting it into a document. if it's concerning, we could always avoid sanitization if it's an empty string

Could we put in the check for empty string then? As a user creating a new page, having something you didn't input immediately show up in your editing view probably isn't ideal