Skip to content

Update UID only for WooCommerce cookies #29542

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.

Sign up for GitHub

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

Jump to bottom
Merged
roykho merged 7 commits into from
Mar 31, 2021
Merged

Update UID only for WooCommerce cookies #29542

roykho merged 7 commits into from
Mar 31, 2021

Conversation

@claudiosanches
Copy link
Contributor

@claudiosanches claudiosanches commented Mar 30, 2021

All Submissions:

Changes proposed in this Pull Request:

We need to filter into nonce_user_logged_out to prevent CSRF attacks for unregistered users, but we introduced some issues since changing this nonce will also invalidate all cookies because it relies in the customer UID.
This PR proposes to only change WooCommerce's cookies, so it doesn't affect 3rd party integrations, unless they are incorrectly using woocommerce as a prefix for any cookie or nonce.

Closes #23682.

How to test the changes in this Pull Request:

I created a plugin to help test how nonces behave: https://gist.github.com/claudiosanches/71e4a1282599509720d72927d10a94ee
But you can also test and follow the same steps in #28364 to test with " WordPress Popular Posts".

  1. Install and enable our test plugin: https://gist.github.com/claudiosanches/71e4a1282599509720d72927d10a94ee
  2. Set the "Test nonce behavior" widget to any sidebar of your theme (just make sure you can access in WooCommerce's catalog page, you may have to change to a theme like Twenty Twenty-One or Storefront, I have tested with both just in case too).
  3. Go to /wp-admin/admin.php?page=wc-settings&tab=products and "Enable AJAX add to cart buttons on archives".
  4. Open a new incognito window in your browser and starts go your store's catalog page.
  5. By clicking in Test Request it will display true in the bottom of the widget because a of successful request.
  6. Now add any product to the cart, and note that is still working the Test Request button.
  7. Refresh your catalog page and click in Test Request again, you'll see now an authentication error from the REST API, go to any page and check that Test Request returns the same error every time now.
  8. Close your incognito window, apply this patch and start again from step 3.
  9. Note that now our Test Request works as expected and always returns true, even after completing a checkout.

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 - Prevent cart to reset all nonce in front-end.

@claudiosanches claudiosanches added this to the 5.3.0 milestone Mar 30, 2021
@claudiosanches claudiosanches requested review from a team and roykho and removed request for a team March 30, 2021 17:38
@claudiosanches claudiosanches mentioned this pull request Mar 30, 2021
Closed
6 tasks
roykho
roykho reviewed Mar 31, 2021
View reviewed changes
Copy link
Contributor

@roykho roykho left a comment

Choose a reason for hiding this comment

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

Tested fine with the steps outlined in the PR. I also further tested Bookings Availability to make sure the REST API is working there.

Left some comments for improvement.

includes/class-wc-session-handler.php
$customer_id = (string) get_current_user_id();
}

return $customer_id;
Copy link
Contributor

@roykho roykho Mar 31, 2021

Choose a reason for hiding this comment

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

Return type is stated as string and only line 203 enforces that even though 201 may already be string. So perhaps remove (string) from line 203 and put it on line 206 return (string) $customer_id would be better?

Copy link
Contributor Author

@claudiosanches claudiosanches Mar 31, 2021

Choose a reason for hiding this comment

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

Only get_current_user_id() returns int, that's why I didn't want to cast string for everything by default.

Copy link
Contributor Author

@claudiosanches claudiosanches Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

See generate_customer_id() it returns always string, even when we need to use get_current_user_id() we cast string too.

@claudiosanches claudiosanches requested a review from roykho March 31, 2021 22:34
roykho
roykho approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@roykho roykho left a comment

Choose a reason for hiding this comment

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

LGTM!

@roykho roykho merged commit 8b6e4ac into trunk Mar 31, 2021
@roykho roykho deleted the fix/23682.1 branch March 31, 2021 23:05
@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 Mar 31, 2021
@tammullen tammullen added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Apr 20, 2021
@claudiosanches claudiosanches mentioned this pull request May 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@roykho roykho roykho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto]

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

nonce_user_logged_out filter breaks REST API cookie auth

5 participants

@claudiosanches @roykho @tammullen @woocommercebot