Commit 0b6c602dd56 for woocommerce
commit 0b6c602dd5666baf556e21beb80cbe93c9f93d9d
Author: Vlad Olaru <vlad.olaru@automattic.com>
Date: Mon Mar 23 15:35:14 2026 +0200
Fix settings password fields losing characters that look like percent-encoded sequences (#63595)
* fix(settings): Preserve percent-encoded characters in password fields
WordPress's `sanitize_text_field()` strips sequences matching
`/%[a-f0-9]{2}/i` as a security measure against URL-encoded HTML
injection. This is appropriate for text labels but destroys legitimate
characters in passwords — e.g. `NlP4%EcCx}Na` became `NlP4Cx}Na`.
Add a dedicated `password` case to the settings save switch that uses
`wp_strip_all_tags()` instead of `wc_clean()`. This still strips HTML
tags (security) while preserving percent sequences in passwords.
Refs WOOPLUG-6157
* test(settings): Move option cleanup to tearDown for failure-safe test isolation
Previously, delete_option() calls were at the end of each test body.
If an assertion failed, cleanup was skipped, leaving stale options in
the database for the remainder of the test run.
Also fixes an imprecise assertion message that referenced '%E' instead
of the actual '%Ec' sequence in the test password.
Refs WOOPLUG-6157
* fix(settings): guard password field sanitization against null raw values
Previously, password fields used `wp_strip_all_tags(trim($raw_value))`
which coerced null to empty string. When a password field is absent from
POST data, $raw_value is null and the save loop should skip it (via the
`is_null($value) → continue` check downstream). Without the null guard,
a missing field would overwrite the stored password with an empty string.
Also removes the redundant outer `trim()` since `wp_strip_all_tags()`
already trims internally.
Refs WOOPLUG-6157
* chore: add changelog entry for password field percent-encoding fix
Refs WOOPLUG-6157
* fix(settings): guard password field sanitization against non-scalar values
wp_strip_all_tags() expects a string, but a crafted POST request could
send an array value for a password field (e.g. field_name[]=foo),
causing a TypeError. Treat non-scalar values the same as null — skip
the option update rather than passing them to wp_strip_all_tags().
Mirrors the defensive pattern used by wc_clean() in the default case.
Refs WOOPLUG-6157
* test: verify missing password field does not overwrite existing option
Add regression test that seeds an option with a known password value,
calls save_fields() with data that omits the password field, and
asserts the original value is preserved.
Refs WOOPLUG-6157
* style: fix equals sign alignment in settings test file
Align assignment operators with surrounding statements per PHPCS
Generic.Formatting.MultipleStatementAlignment rule.
Refs WOOPLUG-6157
* fix(settings): use is_string guard for password field sanitization
PHPStan flagged that is_scalar() allows bool|float|int through to
wp_strip_all_tags() which expects string. Simplify to is_string()
check since POST values are always strings when valid, and treat
everything else as null (skip the option update).
Refs WOOPLUG-6157
* docs: add pre-push branch lint check to CLAUDE.md
Document lint:changes:branch as a required step before pushing.
This catches issues like alignment warnings that per-file linting
misses, since it compares the full branch diff against trunk.
Refs WOOPLUG-6157
* test: fix missing-password test to exercise the correct code path
The test_save_fields_does_not_overwrite_missing_password_field test was
passing $data = array(), which hit the empty($data) early return at
line 925 — the password null guard was never reached. The test passed
for the wrong reason.
Fix by including a second text field in $data so save_fields enters
the foreach loop, while the password key remains absent from POST data.
Also add a test for the is_string() array-injection guard, verifying
that crafted POST arrays (field[]=foo) are rejected and the existing
password value is preserved.
Refs WOOPLUG-6157
* fix: use minimal sanitization for password fields to avoid truncation
WC_Settings_API::validate_password_field() deliberately uses only
trim() + stripslashes() with a comment: "No input sanitization is used
to avoid corrupting passwords."
The password case added to WC_Admin_Settings::save_fields() used
wp_strip_all_tags() instead, which calls PHP's strip_tags() internally.
strip_tags() treats a lone '<' as the start of a malformed HTML tag and
drops everything from the '<' onward (e.g. "pass<word" becomes "pass"),
silently corrupting stored API keys and SMTP passwords.
Replace wp_strip_all_tags() with trim( stripslashes() ) to align with
the existing WC_Settings_API pattern. The type guard (is_string check)
and null-preservation for absent fields remain unchanged.
Refs WOOPLUG-6157
* fix: remove redundant stripslashes from password field sanitization
save_fields() already calls wp_unslash() on $data values (line 946/949)
before the switch statement, which undoes WordPress's magic quotes. The
extra stripslashes() in the password case was double-stripping, so a
password like "abc\def" would be corrupted to "abcdef".
Remove stripslashes() and leave only trim(), since wp_unslash() has
already handled the slash removal. Add a test that verifies literal
backslashes survive the round-trip (using wp_slash() on test input to
simulate WordPress's magic quotes on $_POST data).
Refs WOOPLUG-6157
* refactor: shorten password field comment per review feedback
Reduce verbosity of the password case comment while keeping all
key context: null semantics, why aggressive sanitization is skipped,
the wp_unslash() note, and the validate_password_field() reference.
Remove brittle line number references.
Refs WOOPLUG-6157
diff --git a/CLAUDE.md b/CLAUDE.md
index 36a0be56faf..2bb69d3d30c 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -105,6 +105,16 @@ composer exec -- phpstan analyse path/to/modified/File.php --memory-limit=2G
PHPStan failures often indicate the need to update the baseline file (`phpstan-baseline.neon`). If your fix resolves a previously baselined error, remove the corresponding entry from the baseline.
+### Pre-push Checks
+
+**Before pushing**, run the branch-level lint to catch issues across all commits on the branch (e.g. alignment warnings that per-file linting misses):
+
+```sh
+pnpm --filter=@woocommerce/plugin-woocommerce lint:changes:branch
+```
+
+This compares the full branch diff against trunk and runs `phpcs-changed` on it. Fix any warnings before pushing.
+
**NEVER create a PR without changelog entries.** Each package modified in the monorepo requires its own changelog entry. Run for each affected package:
```sh
diff --git a/plugins/woocommerce/changelog/fix-WOOPLUG-6157-settings-password-percent-encoding b/plugins/woocommerce/changelog/fix-WOOPLUG-6157-settings-password-percent-encoding
new file mode 100644
index 00000000000..f515ef803c8
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-WOOPLUG-6157-settings-password-percent-encoding
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Fix settings password fields silently stripping characters that resemble percent-encoded sequences.
diff --git a/plugins/woocommerce/includes/admin/class-wc-admin-settings.php b/plugins/woocommerce/includes/admin/class-wc-admin-settings.php
index 268a7be6b0d..eb10d479b86 100644
--- a/plugins/woocommerce/includes/admin/class-wc-admin-settings.php
+++ b/plugins/woocommerce/includes/admin/class-wc-admin-settings.php
@@ -985,6 +985,14 @@ if ( ! class_exists( 'WC_Admin_Settings', false ) ) :
case 'relative_date_selector':
$value = wc_parse_relative_date_option( $raw_value );
break;
+ case 'password':
+ // Non-string or absent → null so the option is skipped, not overwritten.
+ // Only trim — no wp_strip_all_tags() or wc_clean() which would corrupt
+ // passwords containing '<' or percent-like sequences.
+ // $raw_value is already wp_unslash()ed upstream, so no stripslashes() needed.
+ // Matches WC_Settings_API::validate_password_field() behavior.
+ $value = is_string( $raw_value ) ? trim( $raw_value ) : null;
+ break;
default:
$value = wc_clean( $raw_value );
break;
diff --git a/plugins/woocommerce/tests/php/includes/admin/class-wc-admin-settings-test.php b/plugins/woocommerce/tests/php/includes/admin/class-wc-admin-settings-test.php
new file mode 100644
index 00000000000..176c01566a2
--- /dev/null
+++ b/plugins/woocommerce/tests/php/includes/admin/class-wc-admin-settings-test.php
@@ -0,0 +1,222 @@
+<?php
+declare( strict_types = 1 );
+
+/**
+ * Tests for WC_Admin_Settings.
+ *
+ * @package WooCommerce\Tests\Admin
+ */
+class WC_Admin_Settings_Test extends WC_Unit_Test_Case {
+
+ /**
+ * Option names used in tests, cleaned up in tearDown().
+ *
+ * @var string[]
+ */
+ private array $option_names_to_clean = array();
+
+ /**
+ * Clean up options after each test to ensure test isolation even on assertion failure.
+ */
+ public function tearDown(): void {
+ foreach ( $this->option_names_to_clean as $option_name ) {
+ delete_option( $option_name );
+ }
+ $this->option_names_to_clean = array();
+ parent::tearDown();
+ }
+
+ /**
+ * @testdox Should preserve percent-encoded sequences in password fields.
+ */
+ public function test_save_fields_preserves_percent_encoded_chars_in_password_fields(): void {
+ $option_name = 'test_password_with_percent';
+ $this->option_names_to_clean[] = $option_name;
+ $password = 'NlP4%EcCx}Na';
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ $data = array(
+ $option_name => $password,
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( $password, get_option( $option_name ), 'Password with %Ec sequence should be preserved' );
+ }
+
+ /**
+ * @testdox Should preserve HTML-like characters in password field values.
+ *
+ * Password fields use minimal sanitization (trim + stripslashes only) to avoid corrupting
+ * passwords and API keys, matching WC_Settings_API::validate_password_field(). Characters
+ * like '<' and '>' are valid in secrets and must not be stripped or escaped.
+ */
+ public function test_save_fields_preserves_html_like_chars_in_password_fields(): void {
+ $option_name = 'test_password_html_preserve';
+ $this->option_names_to_clean[] = $option_name;
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ $data = array(
+ $option_name => '<b>bold</b>secret%E0pass',
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( '<b>bold</b>secret%E0pass', get_option( $option_name ), 'HTML-like characters should be preserved in password fields' );
+ }
+
+ /**
+ * @testdox Should preserve a lone '<' in password field values without truncation.
+ *
+ * PHP's strip_tags() treats a lone '<' as the start of a malformed HTML tag and drops
+ * everything from the '<' onward (e.g. "abc<def" becomes "abc"). Password fields must
+ * not use strip_tags() or wp_strip_all_tags() for this reason.
+ */
+ public function test_save_fields_preserves_lone_less_than_in_password_fields(): void {
+ $option_name = 'test_password_lone_lt';
+ $this->option_names_to_clean[] = $option_name;
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ $data = array(
+ $option_name => 'pass<word123',
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( 'pass<word123', get_option( $option_name ), 'A lone < must not truncate the password' );
+ }
+
+ /**
+ * @testdox Should preserve literal backslashes in password field values.
+ *
+ * $raw_value is already wp_unslash()ed before reaching the password case,
+ * so no additional stripslashes() should be applied — doing so would strip
+ * legitimate backslashes from API keys and secrets.
+ */
+ public function test_save_fields_preserves_backslashes_in_password_fields(): void {
+ $option_name = 'test_password_backslash';
+ $this->option_names_to_clean[] = $option_name;
+ $password = 'abc\\def';
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ // save_fields() calls wp_unslash() on $data values, matching how it handles $_POST.
+ // WordPress adds magic quotes to $_POST via wp_magic_quotes(), so we must wp_slash()
+ // to simulate real form submission — otherwise wp_unslash() eats real backslashes.
+ $data = array(
+ $option_name => wp_slash( $password ),
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( $password, get_option( $option_name ), 'Literal backslashes must not be stripped from passwords' );
+ }
+
+ /**
+ * @testdox Should trim whitespace from password field values.
+ */
+ public function test_save_fields_trims_whitespace_from_password_fields(): void {
+ $option_name = 'test_password_trim';
+ $this->option_names_to_clean[] = $option_name;
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ $data = array(
+ $option_name => ' my%20password ',
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( 'my%20password', get_option( $option_name ), 'Password should be trimmed but percent sequences preserved' );
+ }
+
+ /**
+ * @testdox Should not overwrite an existing password option when the field is absent from POST data.
+ */
+ public function test_save_fields_does_not_overwrite_missing_password_field(): void {
+ $option_name = 'test_password_missing';
+ $other_option = 'test_other_field';
+ $this->option_names_to_clean[] = $option_name;
+ $this->option_names_to_clean[] = $other_option;
+ $original_password = 'existing%25secret';
+ update_option( $option_name, $original_password );
+
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ array(
+ 'id' => $other_option,
+ 'type' => 'text',
+ ),
+ );
+ // $data includes another field but intentionally omits the password field.
+ $data = array( $other_option => 'some value' );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( $original_password, get_option( $option_name ), 'Existing password should not be overwritten when field is absent from POST data' );
+ }
+
+ /**
+ * @testdox Should ignore array values for password fields and preserve the existing option.
+ */
+ public function test_save_fields_ignores_array_value_for_password_field(): void {
+ $option_name = 'test_password_array_injection';
+ $this->option_names_to_clean[] = $option_name;
+ $original_password = 'existing_secret';
+ update_option( $option_name, $original_password );
+
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'password',
+ ),
+ );
+ $data = array( $option_name => array( 'injected' ) );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( $original_password, get_option( $option_name ), 'Array values should be rejected and existing password preserved' );
+ }
+
+ /**
+ * @testdox Should still sanitize text fields with wc_clean as before.
+ */
+ public function test_save_fields_still_sanitizes_text_fields(): void {
+ $option_name = 'test_text_field';
+ $this->option_names_to_clean[] = $option_name;
+ $options = array(
+ array(
+ 'id' => $option_name,
+ 'type' => 'text',
+ ),
+ );
+ $data = array(
+ $option_name => '<b>bold</b> text',
+ );
+
+ WC_Admin_Settings::save_fields( $options, $data );
+
+ $this->assertSame( 'bold text', get_option( $option_name ), 'Text fields should still go through wc_clean' );
+ }
+}