Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Sanitize error message html #7145

Merged
merged 7 commits into from
Sep 23, 2022
Merged

Sanitize error message html #7145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a732841
Sanitize error mesage html
hsingyuc Sep 13, 2022
edbbfd3
Add dompurify as dependency
hsingyuc Sep 13, 2022
6c71ebe
Rebase and fix package lock file
hsingyuc Sep 14, 2022
01b4a0f
Add link style to notices
hsingyuc Sep 15, 2022
611fd11
Merge branch 'trunk' into fix/woopay-1130
hsingyuc Sep 20, 2022
630803b
Merge branch 'trunk' into fix/woopay-1130
hsingyuc Sep 21, 2022
458eb07
Merge branch 'trunk' into fix/woopay-1130
hsingyuc Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions assets/js/base/context/providers/store-notices/components/store-notices-container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,25 @@
* External dependencies
*/
import PropTypes from 'prop-types';
import { useDispatch, useSelect } from '@wordpress/data';
import classnames from 'classnames';
import { Notice } from 'wordpress-components';
import { useDispatch, useSelect } from '@wordpress/data';

import { sanitize } from 'dompurify';
/**
* Internal dependencies
*/
import './style.scss';
import { useStoreNoticesContext } from '../context';

const ALLOWED_TAGS = [ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ];
const ALLOWED_ATTR = [ 'target', 'href', 'rel', 'name', 'download' ];

const sanitizeHTML = ( html ) => {
return {
__html: sanitize( html, { ALLOWED_TAGS, ALLOWED_ATTR } ),
};
};
Comment on lines +15 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsingyuc While working on #7147, I was reusing this part. When looking at ALLOWED_TAGS and ALLOWED_ATTR, I noticed that they're not working as expected. https://github.com/cure53/DOMPurify#can-i-configure-dompurify shows the following example:

// allow only <b> and <q> with style attributes
var clean = DOMPurify.sanitize(dirty, {ALLOWED_TAGS: ['b', 'q'], ALLOWED_ATTR: ['style']});

Looking at the implementation above, I'd say that the current implementation would lead to this function call:

const sanitizeHTML = ( html ) => {
	return {
		__html: sanitize( html, { 
			[ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ],
			[ 'target', 'href', 'rel', 'name', 'download' ]
		} ),
	};
};

I believe the function should be called like this instead:

const sanitizeHTML = ( html ) => {
	return {
		__html: sanitize( html, { 
			ALLOWED_TAGS: [ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ],
			ALLOWED_ATTR: [ 'target', 'href', 'rel', 'name', 'download' ]
		} ),
	};
};

If that assumption is correct, I suggest the following change:

Suggested change
const ALLOWED_TAGS = [ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ];
const ALLOWED_ATTR = [ 'target', 'href', 'rel', 'name', 'download' ];
const sanitizeHTML = ( html ) => {
return {
__html: sanitize( html, { ALLOWED_TAGS, ALLOWED_ATTR } ),
};
};
const tags = [ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ];
const attr = [ 'target', 'href', 'rel', 'name', 'download' ];
const sanitizeHTML = ( html ) => {
return {
__html: sanitize( html, { ALLOWED_TAGS: tags, ALLOWED_ATTR: attr } ),
};
};

cc: @opr and @wavvves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nielslange, in ES6, it converts to below. If converted to the way you wrote, I believe we will get an error.

{ 
    ALLOWED_TAGS:   [ 'a', 'b', 'em', 'i', 'strong', 'p', 'br' ]
}


const getWooClassName = ( { status = 'default' } ) => {
switch ( status ) {
case 'error':
Expand Down Expand Up @@ -53,7 +62,7 @@ export const StoreNoticesContainer = ( {
<div className={ wrapperClass }>
{ regularNotices.map( ( props ) => (
<Notice
key={ 'store-notice-' + props.id }
key={ `store-notice-${ props.id }` }
{ ...props }
className={ classnames(
'wc-block-components-notices__notice',
Expand All @@ -65,7 +74,11 @@ export const StoreNoticesContainer = ( {
}
} }
>
{ props.content }
<span
dangerouslySetInnerHTML={ sanitizeHTML(
props.content
) }
/>
</Notice>
) ) }
</div>
Expand All @@ -84,3 +97,5 @@ StoreNoticesContainer.propTypes = {
} )
),
};

export default StoreNoticesContainer;
3 changes: 3 additions & 0 deletions assets/js/base/context/providers/store-notices/components/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
margin: 0;
display: flex;
flex-wrap: nowrap;
a {
text-decoration: underline;
}
.components-notice__dismiss {
background: transparent none;
padding: 0;
Expand Down
Loading