This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Sanitize error message html #7145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The release ZIP for this PR is accessible via: |
|
Size Change: +41.1 kB (+5%) 🔍 Total Size: 934 kB
ℹ️ View Unchanged
|
hsingyuc
force-pushed
the
fix/woopay-1130
branch
from
9de1905 to
6c71ebe
Compare
|
Hey @hsingyuc could you check the first step of your testing instructions - looks like there's some shorthand there that didn't parse correctly and I want to be sure we're getting the right instructions. Thanks! |
|
@opr Thank you for pointing that out! I've updated the instructions. |
Hey, I don't see the updated URL, could you let me know what PR you're referring to in step 1? Thanks! |
|
@opr I think it's working now, can you try again? Thank you! |
hsingyuc
force-pushed
the
fix/woopay-1130
branch
from
9fff5da to
01b4a0f
Compare
opr
approved these changes
Sep 15, 2022
| @@ -230,6 +230,7 @@ | |||
| "config": "3.3.7", | |||
| "dataloader": "2.1.0", | |||
| "dinero.js": "1.9.1", | |||
| "dompurify": "^2.4.0", | |||
There was a problem hiding this comment.
Looks like dompurify already exists in the node_modules directory - we have it from @woocommerce/data. I don't think it's a problem to add it here.
There was a problem hiding this comment.
It is not redundant if it becomes a dependency for this project, and then it should definitely be added here. Removing @woocommerce/data or it stopping being a dependency for it won't bubble up into problems here
We could make use also of |
3 tasks
wavvves
approved these changes
Sep 21, 2022
nielslange
reviewed
Sep 23, 2022
There was a problem hiding this comment.
@hsingyuc I just added a comment regarding the use of sanitize() as I'm uncertain if the current implementation will work as expected.
Comment on lines
+15
to
+22
| 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 } ), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
@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 } ), | |
| }; | |
| }; |
There was a problem hiding this comment.
@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' ]
}
|
Merged this despite false positive of CI jobs failing. The main culprit was the linter not finding a report file: This is unlikely to be due to this PR, although in retrospective it might be related to the change in |
This was referenced Sep 23, 2022
7 tasks
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 12, 2022
Sanitizing the merchant store link in the error message added in WooPay when in some cases the customer could run into an issue where we need to tell them an error occurred and they have to go back to the merchant store and re-initialize WooPay to fix it. Because previously we were only expecting strings, the text was not sanitized.
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 12, 2022
Sanitizing the merchant store link in the error message added in WooPay when in some cases the customer could run into an issue where we need to tell them an error occurred and they have to go back to the merchant store and re-initialize WooPay to fix it. Because previously we were only expecting strings, the text was not sanitized.
senadir
pushed a commit
to senadir/woocommerce-blocks
that referenced
this pull request
Nov 20, 2022
Sanitizing the merchant store link in the error message added in WooPay when in some cases the customer could run into an issue where we need to tell them an error occurred and they have to go back to the merchant store and re-initialize WooPay to fix it. Because previously we were only expecting strings, the text was not sanitized.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is sanitizing the merchant store link in the error message added in this PR. Because in some (rare) cases the customer could run into an issue where we need to tell them an error occurred and they have to go back to the merchant store and reinitialize WooPay to fix the issue. In these cases, it'd be great to include a link back to the merchant store.
Fixes 1130-gh-Automattic/woopay
Related PR 1287-gh-Automattic/woopay
Accessibility
prefers-reduced-motionOther Checks
Screenshots
Testing
In src/Checkout/PaymentsHandler.php, comment or remove the if around the exception:
Automated Tests
Changes in this PR are covered by Automated Tests.
Do not include in the Testing Notes
WooCommerce Visibility
Performance Impact
Changelog
WooPay: fixed a compatibility issue with some error messages shown by WooPay.