search

Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
87 stars 33 forks source link

Replace esc url with ignore comments #440

Closed james-allan closed 1 year ago

james-allan commented 1 year ago

Description

This PR reverses the changes in previous commits to escape these URLs. Escaping URLs returned from functions and passed to filters breaks compatibility making the URL impossible for third-party code to add additional query args or remove existing args.

See this comment https://github.com/woocommerce/woocommerce-subscriptions/pull/4510#issuecomment-1535934966

I've looked over the URLs returned from these functions and where they are used and confirmed they are escaped at output or final usage.

How to test this PR

This PR reverses previous changes and adds inline comments instead. With that in mind, there's no need to test the places these URLs are used. I'd recommend:

  1. Confirming that these changes don't reintroduce semgrep warnings.
    • Install semgrep eg brew install semgrep.
    • Clone git@github.com:Automattic/wpscan-semgrep-rules.git
    • cd into the wpscan-semgrep-rules repo.
    • Run the following command: 
semgrep -c audit {wp_path}/wp-content/plugins/woocommerce-subscriptions-core/
  1. Look over the previous changes.
    • I've listed all the PRs we merged here: p1683523589400989/1683283676.180859-slack-C02BW3Z8SHK

Product impact

james-allan commented 1 year ago

Note: there's a php linting issue because of a loose comparison on shipping rate comparison.

Screenshot 2023-05-10 at 7 40 35 am 
$package['rates'] == $standard_packages[ $package_index ]['rates']

Given this involves comparing an array of shipping rates, trying to resolve that strict compare isn't something I'd like to do as part of this PR.

james-allan commented 1 year ago

I noticed there was still errors so I fixed these by moving the // nosemgrep to the return statement.

Good catch. I reran the command locally and replicated the issue was still being flagged on the pay url instance. Thanks for pushing 979a7c0. I've confirmed that resolves the semgrep warnings.