Ensure URLs in dispute order note are urlencoded before passed to add_query_arg() to follow best practice

[Jinksi]

URLs query string parameters used for links to the transaction details screen from disputed order notes are not URL-encoded.

add_query_arg() does not encode the parameter value. It expects the parameter value already encoded which currently, it is not url-encoded.

// current
-/wp-admin/admin.php?page=wc-admin&path=/payments/transactions/details&id=ch_123

// preferred
+/wp-admin/admin.php?page=wc-admin&path=%2Fpayments%2Ftransactions%2Fdetails&id=ch_123

Note

Nothing breaks due to this issue. It's just that it's not a best practice that was picked up from #7087 (comment).

When @Jinksi tried to url-encode it, a new problem arose #7087 (comment). Hence, it needs further investigation as to why that is.

• WC_Payments_Order_Service::compose_dispute_url
• WC_Payments_Webhook_Processing_Service::process_webhook_dispute_updated

[haszari]

non-browser-compliant URLs.

Is this an issue? As I understand it this string should be escaped correctly. The chance of it being exploited is probably low, so this might be a linter / best practice issue.

[Jinksi]

non-browser-compliant URLs.

Thanks @haszari, I am incorrect in that statement so I've removed it. I had in my mind that all reserved characters need to be percent-encoded when being used in a query string.

But / and ? are allowed within the query string – all other reserved characters should be percent-encoded.

From https://www.rfc-editor.org/rfc/rfc3986#section-3.4:

The characters slash ("/") and question mark ("?") may represent data within the query component.

...as query components are often used to carry identifying information in the form of "key=value" pairs and one frequently used value is a reference to another URI, it is sometimes better for usability to avoid percent-encoding those characters.

[haszari]

@Jinksi can you test or investigate to confirm if there's impact/risk here? Not clear from the description, I figure someone needs to explore.

• If this is breaking something or is a security risk, let's prioritise.
• If this is not affecting anything and no risk we can close.

[Jinksi]

IMO there is no real impact or risk here.

@shendy-a8c can I get your opinion on this? Uou noticed this as a problem in code review #7087 (comment) and may have a better idea of the impact this could have.

[shendy-a8c]

I can't see any risk or impact. It's just that it's not a best practice.
As I stated in #7087 (comment), add_query_arg() does not encode the parameter value. It expects the parameter value is already encoded which currently, the string in the code is not url-encoded.

[haszari]

Thanks @shendy-a8c – based on that this is a mini tech debt issue and we should fix it when we get a chance. Setting as low priority 👍🏻

[haszari]

@shendy-a8c can you update the description and title so it's super clear what the problem is, and what the outcome/goal is? So it's easy for anyone to pick up and action 🚀

[haszari]

Upgrading to medium priority as this is a code best practice that we should do. If this is impractical or not needed we can close.