Backport security fixes from Woo 3.5.4+

[timbocode]

As mentioned by @bahiirwa on Slack, we should consider backporting any significant security fixes from WC 3.5.4+ that are still applicable to CC.

I've pulled a list of security related fixes from all WC releases since 3.5.3 up to and including 3.9.1 (https://github.com/woocommerce/woocommerce/blob/master/CHANGELOG.txt).

Each will need to be looked into further and a PR created for each fix as required.

---

3.8.0 - 2019-11-05

• Security - Add an exit after the redirect when checking author archive capabilities for customers. woocommerce/woocommerce@293d16f (PR Security fix: Exit after redirect in wc-user-functions.php #228)
• Security - Ensure 404 pages with single product urls cannot be exploited using Open Redirect. woocommerce/woocommerce@f7a5449 (PR Security fix: Add 404 check when creating add to cart url for single … #227)

---

3.7.1 - 2019-10-09

• Security - Add an exit after the redirect when checking author archive capabilities for customers. woocommerce/woocommerce@4d6ddf3 (PR Security fix: Exit after redirect in wc-user-functions.php #228)
• Security - Ensure 404 pages with single product urls cannot be exploited using Open Redirect. woocommerce/woocommerce@dc1438d (PR Security fix: Add 404 check when creating add to cart url for single … #227)

---

3.6.5 - 2019-07-02

• Security - Introduce file type check for tax rate importer. woocommerce/woocommerce@74b746d (PR Security fix: Introduce file type check for tax rate importer #222)
• Security - Added nonce check to CSV importer actions. woocommerce/woocommerce@fd7feb7 (PR Security fix: Added nonce check to CSV importer actions #221)

---

3.6.2 - 2019-04-24

• Fix - Fix security check on email template preview page. woocommerce/woocommerce@aa35486 (PR Security fix: Security check on email template preview page #220)

---

3.5.8 - 2019-04-16 woocommerce/woocommerce@3.5.7...3.5.8

• Security - Added escaping for states on the user profile screen. (PR Security fix: Backport security fixes from WC 3.5.8 #230)
• Security - Added escaping for PhotoSwipe captions. (PR Security fix: Backport security fixes from WC 3.5.8 #230)
• Security - Added escaping for SelectWoo selected options. (PR Security fix: Backport security fixes from WC 3.5.8 #230)

Includes Woo commits:
woocommerce/woocommerce@1ef2739, woocommerce/woocommerce@2ecf61a, woocommerce/woocommerce@c39e0dc, woocommerce/woocommerce@e0382c5, woocommerce/woocommerce@211d9bf, woocommerce/woocommerce@70610f5, woocommerce/woocommerce@67fa997

---

3.5.7 - 2019-03-19 woocommerce/woocommerce@3.5.6...3.5.7

• Security - Improved the way in which state fields are regenerated by JavaScript to ensure values are properly escaped. (PR Security fix: Backport security fixes from WC 3.5.7 #229)

Includes Woo commits:
woocommerce/woocommerce@b626945, woocommerce/woocommerce@a2dee22, woocommerce/woocommerce@0a10883, woocommerce/woocommerce@039915f

---

3.5.5 - 2019-02-20

• Security - Improved escaping for Photoswipe captions. woocommerce/woocommerce@b21bc41 (PR Security fix: Escape html entities before passing to WP/photoswipe #219)
• Security - Improved escaping for JSON attributes and structured data. woocommerce/woocommerce@cf24ff8 (PR Security fix: Improved escaping for JSON attributes and structured data #218)

---

Merge Order

1. PR Security fix: Improved escaping for JSON attributes and structured data #218
2. PR Security fix: Escape html entities before passing to WP/photoswipe #219
3. PR Security fix: Backport security fixes from WC 3.5.7 #229
4. PR Security fix: Backport security fixes from WC 3.5.8 #230
5. PR Security fix: Security check on email template preview page #220
6. PR Security fix: Added nonce check to CSV importer actions #221
7. PR Security fix: Introduce file type check for tax rate importer #222
8. PR Security fix: Add 404 check when creating add to cart url for single … #227
9. PR Security fix: Exit after redirect in wc-user-functions.php #228

---

List of files updated

View files

3.5.5

PR #218
includes/class-wc-structured-data.php
includes/wc-formatting-functions.php
includes/admin/class-wc-admin-pointers.php
includes/admin/class-wc-admin-setup-wizard.php
includes/admin/class-wc-admin-assets.php
includes/admin/helper/class-wc-helper.php
includes/admin/meta-boxes/views/html-product-data-variations.php
includes/admin/reports/class-wc-admin-report.php
includes/admin/reports/class-wc-report-coupon-usage.php
includes/admin/reports/class-wc-report-customers.php
includes/admin/reports/class-wc-report-sales-by-category.php
includes/admin/reports/class-wc-report-sales-by-product.php
includes/admin/reports/class-wc-report-sales-by-date.php
includes/libraries/action-scheduler/classes/ActionScheduler_wpPostStore.php
includes/widgets/class-wc-widget-layered-nav.php
includes/api/legacy/v1/class-wc-api-json-handler.php
includes/api/legacy/v2/class-wc-api-json-handler.php
includes/api/legacy/v3/class-wc-api-json-handler.php
templates/single-product/add-to-cart/variable.php
tests/unit-tests/util/class-wc-tests-core-functions.php

PR #219
includes/wc-template-functions.php

---

3.5.7

PR #229
assets/js/admin/meta-boxes-order.js
assets/js/admin/meta-boxes-order.min.js
assets/js/frontend/country-select.js
assets/js/frontend/country-select.min.js

---

3.5.8

PR #230
assets/js/admin/users.js
assets/js/admin/users.min.js
assets/js/frontend/single-product.js
assets/js/frontend/single-product.min.js
assets/js/selectWoo/selectWoo.full.js
assets/js/selectWoo/selectWoo.full.min.js
assets/js/selectWoo/selectWoo.js
assets/js/selectWoo/selectWoo.min.js
includes/admin/class-wc-admin-assets.php
includes/admin/class-wc-admin-setup-wizard.php
includes/class-wc-frontend-scripts.php

---

3.6.2

PR #220
includes/admin/class-wc-admin.php

---

3.6.5

PR #221
includes/admin/class-wc-admin-importers.php
includes/admin/importers/class-wc-product-csv-importer-controller.php

PR #222
includes/admin/importers/class-wc-product-csv-importer-controller.php
includes/admin/importers/class-wc-tax-rate-importer.php
includes/wc-conditional-functions.php

---

3.7.1 & 3.8.0

PR #227
includes/class-wc-product-simple.php

PR #228
includes/wc-user-functions.php

[bahiirwa]

This is well curated.

1. Can we make the list with check marks - [ ] so we can tick off a PR for each PR merged/fixed?
2. @timbocode Do you have the commit names (commit id) from your research so that we cherry-pick? It will save some time

[timbocode]

1. I've added the checkboxes...good idea.
2. I'll take a look to see what I can find. The info above is all that's available in the WC release notes so it's just a starting point.

Obviously we need to work from oldest (3.5.5) to newest (3.8.0).

[nylen]

Many of the commit IDs above are wrong (not part of the main WooCommerce history) so they can't be used as-is.

As a starting point we need links to the PRs where these commits were introduced (if there are any?)

Failing that, we need to use git log woo/master and search for the correct commit IDs for each change.

[bahiirwa]

@timbocode I have run through most of the commit IDs and I am finding challenges as they are unknown commits with exception of f7a5449

[timbocode]

Which commit IDs are wrong??

[bahiirwa]

I guess I have some hiccups local. I tried to delete the local repo and git clone again but no change. I will try a different computer or directory

[timbocode]

@nylen has also said some commits are wrong so I don't think it's anything specific to your installation. But I'd just like to know which ones are wrong because I haven't got much to go on at the moment.

[nylen]

Just had a minute to dig more deeply into this, there are two issues.

When using the command line (GitHub Desktop may have equivalent operations but I'm not sure what they are) we set up a link to the WooCommerce repository and pull in a commit as follows:

git remote add woo https://github.com/woocommerce/woocommerce
git fetch woo
git checkout -b my-security-fix
git cherry-pick COMMIT_ID

First issue: git fetch does not fetch tagged releases that are outside of a branch, by default.

There are two general ways to fix this:

• Use git fetch woo --tags to download the missing commits. After this, the commit IDs above should work correctly with git cherry-pick.
• When looking for commit IDs, look only through the branches in the Woo commit history. You can use this command to do this on the command line: git log --graph --oneline --decorate --remotes=woo Then type (for example) /Escape html entities before passing and you will find woocommerce/woocommerce@ffa230de9. This commit is reachable from the woo/release/4.0 branch (among others), while woocommerce/woocommerce@b21bc41 is only reachable from a few tags.

Second issue: the ranges 3.5.6...3.5.7 and 3.5.7...3.5.8 may include more than one commit. If they do, these need to be split out into proper commit IDs. This is easy enough to do for the person working on the changes, but just noting here because it might be unexpected.

[timbocode]

OK, I'm aware of 3.5.6...3.5.7 and 3.5.7...3.5.8. I thought it'd easy enough for whoever was dealing with these to pick off each commit individually.

To clone Woo locally, I used git clone https://github.com/woocommerce/woocommerce.git woocommerce .

I then did a git show command for each commit ID listed. e.g. git show b21bc41 and it showed the same woocommerce/woocommerce@b21bc41

And I've also started from scratch using the above but again without success. 😞

[nylen]

git clone fetches tags, git fetch does not. We need to use git remote add + git fetch here so that we can pull the Woo commits into an existing Classic Commerce.

This is the simplest way to fix this issue, no need to change the commit IDs:

git remote add woo https://github.com/woocommerce/woocommerce
git fetch woo
git fetch woo --tags # this line is the key

[nylen]

The commit IDs are fine, the problem was that we weren't downloading all of the history from the WooCommerce repository.

The steps I outlined above need to be run from within an existing Classic Commerce repository - this makes it possible to pull in the required commits using git cherry-pick.

More generally, I have a question about how to handle these security fixes. Which of these approaches should we choose?

1. Understand and test each of these fixes before merging them
2. Trust that the Woo team is doing a good job with security fixes and merge them

(1) is probably ideal, but (2) is what we do with ClassicPress core due to time and information limitations.

[timbocode]

Well first thing, I think we need to start with the earliest fix and work towards the latest. In other words, address issues in 3.5.5 first and then work towards 3.8.0. If that makes sense?

As for how to handle these sort of issues, I do think we need to try and at least have some understanding of what each fix does but as for applying them, I'm inclined to go for option (2).

The Woo team is much larger than ours, as is their user base, so effectively that means much real-world testing has already been done. I agree (1) would be the better option but we don't have time or resources.

[nylen]

address issues in 3.5.5 first and then work towards 3.8.0. If that makes sense?

Yep, sounds good to me.

The Woo team is much larger than ours, as is their user base, so effectively that means much real-world testing has already been done. I agree (1) would be the better option but we don't have time or resources.

Makes sense to me also.

[nylen]

We need to decide what level of review/testing is required for merging these. At a bare minimum I would propose the following:

• Passing build on Travis (this doesn't guarantee no issues, but it helps)
• Look at the diff resulting from each PR and make sure it's the same as the intended Woo commit

Additionally, anything we can do to test these changes using basic flows like creating an order is also a good idea. #193 (end-to-end tests) would give a bit more confidence here, but those need more work to get them running again.

[timbocode]

I agree we need to give more thought to testing. I've given each of the PRs I've done so far the "once over" as far as looking at the diff is concerned but a second pair of eyes would be useful. Everything so far has passed the Travis tests which is good but not idiot-proof. 🙄 None of the PRs are ready to be merged.

I'll need a bit of help if we're to update the automated tests.

[timbocode]

Just going back to the commit IDs above, if we take a look at "Ensure 404 pages with single product urls cannot be exploited using Open Redirect", this is found in 3.7.1 and 3.8.0. And in fact, if you search for "Add 404 check when creating add to cart url for single products.", three commits show up, all affecting file includes/class-wc-product-simple.php.

Using these commands:

git remote add woo https://github.com/woocommerce/woocommerce
git fetch woo
git fetch woo --tags
git log --graph --oneline --decorate --remotes=woo 
git log --oneline --remotes=woo --grep="Add 404 check when creating add to cart url for single products." -i

returns these three commits:

f7a5449 Add 404 check when creating add to cart url for single products.
dc1438d Add 404 check when creating add to cart url for single products.
badb135 Add 404 check when creating add to cart url for single products.

two of which I have listed above ( dc1438d and f7a5449 ). All 3 were committed on the same day.

badb135 and f7a5449 are identical. dc1438d seems to supersede both of the other two and it is how the code still is in 3.8.0 (I've compared includes/class-wc-product-simple.php in versions 3.5.3, 3.7.1 and 3.8.0).

So the question is, do we still need to do a commit for woocommerce/woocommerce@badb135 and woocommerce/woocommerce@f7a5449 or can we just do the one for woocommerce/woocommerce@dc1438d?

[nylen]

The important thing is how the code ends up at the end. So I looked at all three of these commits, and they all result in that section of code being the same after the commit is applied (as one would hope).

Given that, I don't see any need to apply all 3 commits, I would just pick the one that is easiest to apply (i.e. best matches the current CC code). It looks like that would be woocommerce/woocommerce@dc1438d as you said.

Tracking which WooCommerce commits we've already applied to CC is another question. For that purpose it might make sense to mark all 3 of these commits as "applied" in CC since that will be effectively true. We're not really set up to do that yet though, so I suggest we not worry about it for beta1. I've opened #226 to discuss this as a separate topic.

[timbocode]

Thanks James. This is covered in PR #227.

[nylen]

Copy/paste from #218 (review):

Do we need to create any further automated tests?

I think if we do the following then we will be about as ready as we can be:

• Compare the diffs against the corresponding WC commits
• Keep the current Travis build passing
• Do more extensive manual testing after all security fixes are merged

[bahiirwa]

@timbocode Could you add the checkboxes (#214 (comment)) to the Merge order as well to consider progress of this issue?

[timbocode]

@bahiirwa yes, I'll take a look. A lot to catch up on.