fix: XSS vulnerability in the click event of a button

[dgdev1024]

Checklist:

• I have read freeCodeCamp's contribution guidelines.
• My pull request has a descriptive title (not a vague title like Update index.md)

A cross-site-scripting vulnerability was caught in the process of posting a comment on a saved book. Upon the comment being posted to the backend, the un-sanitized comment is 'unshifted' into the comments array, which is subsequently shown in the HTML. This pull request addresses the vulnerability by using the 'replace' method to replace any portion of the new comment matching the following regular expression, /(?\<\/?.+\>)/gi, with a blank string, removing much, if not all, of the vulnerability. See lines 74 and 75 of public/client.js for the fix.

Update: There is another XSS that pops up when there are books to fetch from the database, as their titles may also be unsanitized and also contain cross-site scripting. See line 13 for that fix.

[Solarc117]

I just caught this vulnerability, and was about to mention it. Nice job!

[raisedadead]

Hi, @freeCodeCamp/moderators

Can you help us verify the existing user stories and functionality is unchanged? If not we should be able to merge this in.

Thanks.

[ShaunSHamilton]

This does not address any XSS, as these are only client-side changes.

Also, this is a boilerplate repo - it is not in production anywhere (that this change will affect).

So, it is up to the Camper to implement XSS safety when building the API.

As for something we can do, our production example app could get an XSS patch. However, this again would need to be a server (api) solution, and should not rely on Regex. @dgdev1024 Would you be interested in opening a PR with such a change on our demo-projects repo?

Hope this is clear.

[SethFalco]

This does not address any XSS, as these are only client-side changes.

In my opinion, it's not accurate to say that being client-side means that it doesn't address an XSS vulnerability. In fact, there are libraries dedicated to this which work in the browser, like DOMPurify.

Most frontend frameworks sanitize on client-side before inserting variables too. This was also a common practice historically afaik, it's just less visible now. A benefit I can see to this is that if a malicious engineer, browser, or browser extension, were to redirect the request to fetch data from somewhere untrusted, the client will still sanitize the input.

React always sanitizes HTML unless you use the dangerouslySetInnerHTML attribute.

So, you can set HTML directly from React, but you have to type out dangerouslySetInnerHTML and pass an object with a __html key, to remind yourself that it’s dangerous.

— https://legacy.reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml

Angular always sanitized HTML unless you call bypassSecurityTrustHtml.

To systematically block XSS bugs, Angular treats all values as untrusted by default. When a value is inserted into the DOM from a template binding, or interpolation, Angular sanitizes and escapes untrusted values.

— https://angular.io/guide/security#angulars-cross-site-scripting-security-model

But indeed, it's a good idea to sanitize the content as early as possible on a trusted environment (i.e. the server), and using a reputable library for it.

The intricacies of HTML sanitization aren't always apparent. I just had to deal with a similar problem lately, and dealing with content that looks like HTML but isn't HTML can be a headache. For example, there is a movie called 1968 < 2018 > 2068.

Even on IMDB, it's written as 1968 2068 (< 2018 > missing) in the synopsis, likely due to poor sanitization.

"I love the movie 1968 < 2018 > 2068!".match(/(?:\<\/?.+\>)/) 
// False Positive: [ "< 2018 >" ]

Reference:

• fix: remove html tags in overviews jellyfin/jellycon#292

[ShaunSHamilton]

@SethFalco Quite right. Thank you for pointing that out. I was not thinking along those lines and missed the potential.

I think the main issue is still:

A benefit I can see to this is that if a malicious engineer, browser, or browser extension, were to redirect the request to fetch data from somewhere untrusted, the client will still sanitize the input.

If we are at this point, then the engineer/extension could/would just remove the client-side sanitization anyway. So, having client-side sanitation helps prevent simple attacks, but the minute something malicious has access to the DOM, there is nothing a web developer can do for the victim.

Server-side sanitation just ensures one man's vulnerability doesn't become everyone's vulnerability.

[SethFalco]

If we are at this point, then the engineer/extension could/would just remove the client-side sanitization anyway.

Valid, I'll concede. ^-^'

The objective was just to pick at the wording. I was wary a learner's takeaway from the comment might be that XSS can never be resolved with a client-side fix, while a lot of effort is actually put into doing just that.

Just to clarify, I entirely agree with your proposed changes.