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.
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
Create pending renewal order (improve safety/error handling) #768
Create pending renewal order (improve safety/error handling) #768
Changes from all commits
25fc0ef
8427755
598bf55
5b78cbc
36ce48f
467409c
2deaa0f
72c4a08
10d531c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I've been pondering whether to send this feedback as it feels very minor... 😅
While testing these changes, I noticed that the

wcs_create_renewal_order()
function already adds a similar order note to this one, and so on success, you have two very similar order notes added which feels unnecessary:The
Create pending renewal order requested by admin action.
order note was purposefully added to the start of the function in https://github.com/woocommerce/woocommerce-subscriptions/pull/3462 and I believe it served two purposes:exit;
🙈 )trunk
Here's what I'm thinking
Curious to hear your thoughts on this!
The other idea I had was to remove the initial order note at the start and instead pass "Create pending renewal order requested by admin action." as the $note param when calling
$subscription->update_status( 'on-hold', $note );
🤔 but this also introduces inconsistencies with the similar "Process Renewal" action.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.
Agreed, except (and here I'm probably missing a trick) I don't think the original
Order #XYZ created to record renewal
ever disappeared. At least, I can't seem to replicate what I see in your screenshot from this branch. Perhaps we're each approaching this in a slightly different way? 🤔Nonetheless, I agree with the concept, and committed f9114d0, which should give us the admin notice (top of screen) alongside the existing order note:
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.
Btw, very glad to get notes like this. I think this is how we remove some of the papercuts from the UX (or, in this case, it's how we avoid adding new papercuts, so to speak).
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.
Oh sorry I probably should've shared how I was testing this 😅
I'm not sure if this helps but here's a longer screenshot:
trunk
With the "requested by admin action" note being added at the start of the process, it acts as a separator but also adds context for why this renewal was created.
For that reason, as well as keeping this action consistent with the "Process Renewal" action, I think we should add back this order note which was removed.
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.
(Edit: ignore this comment, I see what you mean now.)
Hmm. I wasn't missing this before, and on trying to replicate once more I do still get the requested by admin action note:
Or perhaps besides a subtle difference in testing, a further commit addressed this?
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.
☝🏼 Ignore!
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 removing this
esc_html
mean we should be escaping every other use ofwcs_add_admin_notice()
within Subscriptions and the other extensions that use it (WooPayments, Gifting Subscriptions).I could be wrong by my understanding is that just having
wp_kses_post
is only okay if we trust the content, but given these messages are typically passed through__()
,wp_kses_post()
won't be enough as it can still allow inline javascript through :(I'm sorry for putting you on this rollercoaster! I didn't even consider this escaping links issue when suggesting putting links in the admin notices 😭
What are your thoughts given the above?
I'm thinking we roll back the idea of including a link in the notice, but then that means we don't have a need for the new
notify()
function you added because I'm assuming we'll still want to add links to the order notes? Oh man 🙈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.
Who doesn't enjoy rollercoasters? 🎢 ...
I think the links are useful—the user experience feels better—so it would be great to keep them if we can do so safely. Drawing some amount of inspiration from Woo Core's HtmlSanitizer, what if continue to use Kses, but with a tighter set of rules than we get from
wp_kses_post()
?We could expand this over time if, for instance, we wished to allow
<em>
,<strong>
, etc. However...I'm not tied to the new
notify()
method, we can drop it if it turns out to be problematic and we can also drop the link from the admin notice if that feels better/safer.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.
That would still allow bad URLs like this wouldn't it?
Maybe we can go a step further and manually escape all hrefs for example?
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 with so many things in WordPress, this isn't fixed in stone, but assuming defaults that wouldn't be possible (because
javascript
is not one of the allowed protocols). We'd get back some HTML looking like this: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.
TIL! Thanks @barryhughes for sharing! I guess if some malicious actor has access to filter 'kses_allowed_protocols' then we're in deep trouble, unless another plugin adds it unknowing that is poses security risks.
In any case, given the above, I think I'm fine to stick with either
wp_kses( $sanitized_message, $allow_links );
or leave it withwp_kses_post()
since my concerns were mainly injecting JS/some other script.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.
Massively overthinking this no-doubt, but
wcs_add_admin_notice()
could be updated to support an array of links. Using modern syntax only to keep the example concise:The output might then look a little like this:
Escaping being done as late as possible, from within
wcs_display_admin_notices()
.To be clear, though, I'm just jamming on possibilities rather than pushing this as my preferred choice. I think it's fairly reasonable, and especially secure, but I also think using Kses is fine, or we could do as you suggest with a regex, or we could just drop the links from these messages (if anything, that's my least prefered option, because I think we lose something if we do, but it's definitely not a deal breaker).
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.
Just wanted to chime in to say I ❤️ this change. I was looking at #757 and I think that error wouldn't have occurred with this change.
I wonder if it would be worth to report these kind of failures somewhere. I'm thinking that if there are 2 or 3+ hooks which use this filter correctly, the changes they make would be overridden by 1 poorly written piece of code that returns a bool or null etc.
We don't have a good mechanism for doing this kind of thing but I'm thinking something like a WC log entry or something.
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.
I'd definitely be up for developing this concept a little further (and agree there is lots of value in logging when things go wrong).