Skip to content
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

Always sanitize coupon code to prevent inconsistent between admins and shop owners #27140

Merged
merged 16 commits into from Aug 7, 2020

Conversation

claudiosanches
Copy link
Contributor

@claudiosanches claudiosanches commented Jul 27, 2020

All Submissions:

Changes proposed in this Pull Request:

By default WordPress will run kses_init_filters() if the current user can't use unfiltered HTML, applying the wp_filter_kses() function to the title_save_pre.
That's why admins can save a coupon code like a&a without getting converted into a&a.

To fix the inconsistent I'm changing to always apply the same sanitation for all users on WooCommerce, also should be safer to always sanitize.
And it also includes a upgrade routine to fix all codes at once.

Closes #23655.

How to test the changes in this Pull Request:

  1. Create two coupons with the following codes using an admin:
  • a&a
  • b&b
  1. Now create more two coupons using a shop manager:
  • c&c
  • d&d
  1. Check that with the shop manager ampersand gets encoded.
  2. With the shop manager edit the coupon with code a&a and note that the ampersand got encoded making impossible to restore.
  3. Apply this patch, edit or create coupons with any user and see that ampersands always get encoded.
  4. Added some products to the cart and try use your coupons created in the previous steps.
  5. Also try apply coupons with both formats: a&a and a&a.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - Coupon code inconsistent between admins and shop owners.

By default it only get sanitized when editing the coupon with an user
that doesn't have unfiltered_html capability.
This makes match with WP sanitization for post_title.
WP sanitize post_title using kses_init_filters() when
the current user can't use unfiltered HTML.
@claudiosanches claudiosanches added status: needs review priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. labels Jul 27, 2020
@claudiosanches claudiosanches added this to the 4.5.0 milestone Jul 27, 2020
@vedanshujain vedanshujain added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Jul 28, 2020
@vedanshujain
Copy link
Contributor

Adding high impact label because this will existing coupon codes.

@claudiosanches
Copy link
Contributor Author

@vedanshujain not sure if it's "high impact", since there's a migration script, and will also accept the old format too, since will got converted before searching in the database.

Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the intention is to get to a state where code everywhere had run through wp_filter_kses I have a few questions/suggestions:

  1. We have a method wc_format_coupon_code, perhaps it will be good to add wp_filter_kses there as well.
  2. Should we batch the update function for shops that have large amount of coupons (incase running the request takes more than 30s).
  3. Do we really need to run the update command for every coupon code, I think if we do it only where old_code !== new_code, it will be a good improvement.

wdyt?

@vedanshujain
Copy link
Contributor

@claudiosanches agree with high impact assessment, removing the label.

@vedanshujain vedanshujain removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Jul 29, 2020
@claudiosanches
Copy link
Contributor Author

claudiosanches commented Jul 30, 2020

@vedanshujain

We have a method wc_format_coupon_code, perhaps it will be good to add wp_filter_kses there as well.

It already gets applied there by the woocommerce_coupon_code filter. See:

add_filter( 'woocommerce_coupon_code', 'html_entity_decode' );
add_filter( 'woocommerce_coupon_code', 'wc_sanitize_coupon_code' );
add_filter( 'woocommerce_coupon_code', 'wc_strtolower' );

I'm added this code to the wc_sanitize_coupon_code() function because this is the expected output of sanitize_post_field( 'post_title', $value, 0, 'db' ) if you are using an user that have unfiltered_html capabilities.

Do we really need to run the update command for every coupon code, I think if we do it only where old_code !== new_code, it will be a good improvement.

It will solve problems later, like for querying coupons where we should be search for a&a while in the database it's still a&a.

@claudiosanches
Copy link
Contributor Author

claudiosanches commented Jul 30, 2020

Should we batch the update function for shops that have large amount of coupons (incase running the request takes more than 30s).

I just implemented it, now will update 10 coupons for each run.

If you like to test the migration script, you can install the "coupon-generator-for-woocommerce" plugin to generate how many coupons you like, you can install using wp plugin install --activate coupon-generator-for-woocommerce.
Then you need to update the $version to 4.5.0 in includes/class-woocommerce.php, it will trigger the banner to migrate the database to 4.5.0.

Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I have a couple more suggestions, let me know what you think.

includes/wc-update-functions.php Outdated Show resolved Hide resolved
includes/wc-update-functions.php Outdated Show resolved Hide resolved
@claudiosanches
Copy link
Contributor Author

claudiosanches commented Jul 30, 2020

@vedanshujain I just made the changes, if you like to test again in additional to change the $version to 4.5.0 you should also set the follow:

wp option update woocommerce_db_version "4.4.0"
wp option update woocommerce_version "4.4.0"

It will force to trigger the upgrade again.

Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I have left some comments but they are suggestions rather blockers, so approving the PR anyway.

includes/wc-update-functions.php Outdated Show resolved Hide resolved
@claudiosanches
Copy link
Contributor Author

@vedanshujain thank you very much for reviewing and for all the tips 🤗

@Konamiman Konamiman self-requested a review August 7, 2020 10:40
@Konamiman Konamiman merged commit 76cf1e4 into master Aug 7, 2020
@Konamiman Konamiman deleted the fix/23790 branch August 7, 2020 10:42
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Aug 7, 2020
@juliaamosova juliaamosova added status: testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. release: add changelog Mark all PRs that have not had their changelog entries added. [auto]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coupon code with ampersand is inconsistent between logged in user and guest
5 participants