Fix/31582 special characters in password on checkout page lead to not working account #43777

wavvves · 2024-01-18T10:56:44Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR introduces wp_slash() on $_POST['password'] to mimic the behavior of default account creation on registration during checkout via submitting a password.
On the default account creation, passwords are being slashed before encryption. Lack of this step would prevent a new user from subsequently logging in after creating an account during checkout while submitting the correct password

Closes #31582 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Set up WooCommerce using the settings from the above printscreen on /wp-admin/admin.php?page=wc-settings&tab=account
On an incognito browser window, place an order using the shortcode checkout page, and create a new account while doing it using a password containing the " character
Go to My Account, log out, and try to log back in using the new user credentials
Verify that the login process was done successfully

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Comment

github-actions · 2024-01-18T10:57:25Z

Hi @mikejolley, @tarunvijwani,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

github-actions · 2024-01-18T11:11:22Z

Test Results Summary

Commit SHA: 72c4a64

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 39s
E2E Tests	281	0	0	18	0	299	6m 43s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

mikejolley

I'm not sure this covers all use cases. I understand its targeting shortcode checkout, but wc_create_new_customer is used in several places across the codebase.

Do you think we should move this change within wc_create_new_customer? And does this need some kind of test in place to confirm behaviour and avoid future regressions?

Additionally, does this code assume the POST variable was unslashed already? Is that the case everywhere wc_create_new_customer is used?

plugins/woocommerce/includes/class-wc-checkout.php

wavvves · 2024-01-18T18:32:39Z

I'm not sure this covers all use cases. I understand its targeting shortcode checkout, but wc_create_new_customer is used in several places across the codebase.

Do you think we should move this change within wc_create_new_customer? And does this need some kind of test in place to confirm behaviour and avoid future regressions?

Additionally, does this code assume the POST variable was unslashed already? Is that the case everywhere wc_create_new_customer is used?

It took a while to debug this; on the default account registration, the password arrives slashed to wc_create_new_customer, whereas here, it is not. Actually, it seems that the password is always encrypted after slashing. Using unslashed data should not be a problem since any malicious payload is destroyed after encryption. Still, I strongly think we should keep encrypting slashed passwords, as changing that function seems to have a higher risk of breaking all logins.

I wouldn't test for this behaviour, I would rather do E2E tests that does guest checkout with account creation, check everything works and includes a check that logs in with supplied credentials after. That can be done in a separate issue, let me check what E2E tests are already in place regarding this ;)

…-checkout-page-lead-to-not-working-account

mikejolley · 2024-01-22T12:05:52Z

Ok, but can we at least track where the unslash is happening @wavvves? I cannot see it happening when wc_create_new_customer is called. It would be useful to confirm where the unslash first happens so it can be documented clearly in the code why the slash is needed. Or we can remove the unslash at source.

…-checkout-page-lead-to-not-working-account

wavvves · 2024-01-29T15:40:23Z

plugins/woocommerce/includes/class-wc-checkout.php

@@ -783,6 +783,9 @@ public function get_posted_data() {
 							$value = wc_sanitize_textarea( $value );
 							break;
 						case 'password':
+							if ( $data[ 'createaccount' ] && 'account_password' === $key ) {
+								$value = wp_slash( $value ); // Passwords are encrypted with slashes on account creation, so we need to slash here too.


Ok, but can we at least track where the unslash is happening @wavvves? I cannot see it happening when wc_create_new_customer is called. It would be useful to confirm where the unslash first happens so it can be documented clearly in the code why the slash is needed. Or we can remove the unslash at source.

@mikejolley unslashing is being done here on L767. Instead of adding an exception there I think it would be more readable and self-contained to be re-slashed on the switch. The tiny performance hit of just avoiding unslashing can be neglected IMO.
What do you think?

@mikejolley Here is the direct link to it: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/includes/class-wc-checkout.php#L767

Makes sense.

…-checkout-page-lead-to-not-working-account

mikejolley

Tests ok. Thanks for investigating.

… working account (#43777) * Fix registration during checkout * Added changelog. * Moved changelog. * Added correct spacing * Move password slashing into WC_Checkout:get_posted_data() * Lint fix

wavvves added 2 commits January 18, 2024 10:40

Fix registration during checkout

54a1d9c

Added changelog.

c9bd6d7

wavvves self-assigned this Jan 18, 2024

wavvves linked an issue Jan 18, 2024 that may be closed by this pull request

Special characters in password on checkout page lead to not working account #31582

Closed

5 tasks

woocommercebot requested review from a team and tarunvijwani and removed request for a team January 18, 2024 10:56

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jan 18, 2024

wavvves assigned tarunvijwani Jan 18, 2024

wavvves added type: bug The issue is a confirmed bug. and removed plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Jan 18, 2024

wavvves added focus: checkout Issues related to checkout page. focus: my account Issues related to my account page. labels Jan 18, 2024

Moved changelog.

5373b77

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jan 18, 2024

wavvves requested review from mikejolley and removed request for tarunvijwani January 18, 2024 10:59

mikejolley requested changes Jan 18, 2024

View reviewed changes

plugins/woocommerce/includes/class-wc-checkout.php Outdated Show resolved Hide resolved

Added correct spacing

c4c5d05

wavvves added 2 commits January 19, 2024 12:00

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

7effb84

…-checkout-page-lead-to-not-working-account

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

0ffa95c

…-checkout-page-lead-to-not-working-account

wavvves added 3 commits January 22, 2024 17:59

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

79eff76

…-checkout-page-lead-to-not-working-account

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

980fc04

…-checkout-page-lead-to-not-working-account

Move password slashing into WC_Checkout:get_posted_data()

3607745

wavvves commented Jan 29, 2024

View reviewed changes

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

3df4b12

…-checkout-page-lead-to-not-working-account

wavvves added 2 commits January 30, 2024 12:48

Lint fix

d261ac6

Merge branch 'trunk' into fix/31582-special-characters-in-password-on…

72c4a64

…-checkout-page-lead-to-not-working-account

wavvves requested review from mikejolley and tarunvijwani January 30, 2024 19:13

mikejolley approved these changes Jan 31, 2024

View reviewed changes

wavvves merged commit 1e81575 into trunk Jan 31, 2024
38 checks passed

wavvves deleted the fix/31582-special-characters-in-password-on-checkout-page-lead-to-not-working-account branch January 31, 2024 09:36

github-actions bot added this to the 8.7.0 milestone Jan 31, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Jan 31, 2024

nigeljamesstevenson added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/31582 special characters in password on checkout page lead to not working account #43777

Fix/31582 special characters in password on checkout page lead to not working account #43777

wavvves commented Jan 18, 2024

github-actions bot commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

mikejolley left a comment

wavvves commented Jan 18, 2024 •

edited

Loading

mikejolley commented Jan 22, 2024

wavvves Jan 29, 2024

tarunvijwani Jan 29, 2024

mikejolley Jan 30, 2024

mikejolley left a comment

Fix/31582 special characters in password on checkout page lead to not working account #43777

Fix/31582 special characters in password on checkout page lead to not working account #43777

Conversation

wavvves commented Jan 18, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Jan 18, 2024 • edited Loading

github-actions bot commented Jan 18, 2024 • edited Loading

Test Results Summary

mikejolley left a comment

Choose a reason for hiding this comment

wavvves commented Jan 18, 2024 • edited Loading

mikejolley commented Jan 22, 2024

wavvves Jan 29, 2024

Choose a reason for hiding this comment

tarunvijwani Jan 29, 2024

Choose a reason for hiding this comment

mikejolley Jan 30, 2024

Choose a reason for hiding this comment

mikejolley left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 18, 2024 •

edited

Loading

wavvves commented Jan 18, 2024 •

edited

Loading