Skip to content

enhancement(settings): hide Subscription Token value on settings page - #4324

Merged
felipeelia merged 8 commits into
10up:developfrom
faisalahammad:fix/4305-hide-subscription-token
Aug 17, 2026
Merged

enhancement(settings): hide Subscription Token value on settings page#4324
felipeelia merged 8 commits into
10up:developfrom
faisalahammad:fix/4305-hide-subscription-token

Conversation

@faisalahammad

@faisalahammad faisalahammad commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

The Subscription Token for ElasticPress.io users was displayed in plain text on the Settings page. This change hides the value by using a password field, it preserves the existing token when the form is submitted without a new value, and it adds a checkbox to remove the stored token when needed.

Fixes #4305

Changes

includes/partials/settings-page.php

Before:

<input <?php if ( defined( 'EP_CREDENTIALS' ) && EP_CREDENTIALS ) : ?>disabled<?php endif; ?> type="text" value="<?php echo esc_attr( $credentials['token'] ); ?>" name="ep_credentials[token]" id="ep_token">

After:

<input <?php if ( defined( 'EP_CREDENTIALS' ) && EP_CREDENTIALS ) : ?>disabled<?php endif; ?> type="password" value="" autocomplete="off" placeholder="<?php echo esc_attr( $credentials['token'] ? '••••••••' : '' ); ?>" name="ep_credentials[token]" id="ep_token">
<?php if ( ! defined( 'EP_CREDENTIALS' ) || ! EP_CREDENTIALS ) : ?>
	<p>
		<label for="ep_remove_token">
			<input type="checkbox" name="ep_remove_token" id="ep_remove_token" value="1">
			<?php esc_html_e( 'Remove the saved subscription token', 'elasticpress' ); ?>
		</label>
	</p>
<?php endif; ?>

Why: Type password alone was not enough because the browser could still expose the value via DevTools. Leaving the value empty and using a placeholder means the token is never sent back to the browser. The remove-token checkbox lets subscribers clear the stored token without editing wp_options. It is only rendered when the EP_CREDENTIALS constant is not set, since any change in the ep_credentials option is ignored in that case.

includes/classes/Screen/Settings.php

Before:

if ( isset( $post['ep_credentials'] ) ) {
    $credentials = Utils\sanitize_credentials( $post['ep_credentials'] );
    Utils\update_option( 'ep_credentials', $credentials );
}

After:

if ( isset( $post['ep_credentials'] ) && ( ! defined( 'EP_CREDENTIALS' ) || ! EP_CREDENTIALS ) ) {
    if ( ! empty( $post['ep_remove_token'] ) ) {
        $username    = isset( $post['ep_credentials']['username'] )
            ? sanitize_text_field( $post['ep_credentials']['username'] )
            : ( $this->prev_ep_credentials['username'] ?? '' );
        $credentials = [
            'username' => $username,
            'token'    => '',
        ];
    } else {
        $credentials = Utils\sanitize_credentials( $post['ep_credentials'] );

        // Preserve the existing token if the field was left empty (it is always empty on load).
        if ( empty( $credentials['token'] ) ) {
            $credentials['token'] = $this->prev_ep_credentials['token'];
        }
    }

    Utils\update_option( 'ep_credentials', $credentials );
}

Why: Since the token input is always empty on page load, an empty POST value means the user did not change it. This prevents overwriting the stored token with an empty value on every save. The ep_remove_token checkbox switches that behavior to clearing the token instead. The whole block is skipped when EP_CREDENTIALS is set, so a crafted POST cannot persist the wp-config token into wp_options.

assets/css/dashboard.css

Before:

.ep-credentials .form-table td input,
.ep-credentials .form-table td select,
.ep-credentials-general .form-table td input,
.ep-credentials-general .form-table td select {
	width: 250px;
}

After:

.ep-credentials .form-table td input:not([type="checkbox"]),
.ep-credentials .form-table td select,
.ep-credentials-general .form-table td input:not([type="checkbox"]),
.ep-credentials-general .form-table td select {
	width: 250px;
}

Why: The unfiltered input selector stretched the remove-token checkbox to 250px, the same width as the text fields. Excluding checkbox inputs restores its natural size while the other fields keep their width.

Testing

Test 1: Token is not exposed in the page

  1. Load ElasticPress > Settings with an existing token.
  2. Inspect the token input element.
  3. Confirm value="" and that the actual token is not in the DOM.

Result: Token not exposed in page source.

Test 2: Save without changing the token

  1. Load the Settings page.
  2. Click Save Changes without entering anything in the token field.
  3. Verify the token still works.

Result: Existing token is preserved.

Test 3: Update the token

  1. Enter a new value in the token field.
  2. Save Changes.
  3. Verify the new token is stored.

Result: New token is saved correctly.

Test 4: Remove the stored token

  1. Tick "Remove the saved subscription token".
  2. Save Changes.
  3. Verify the stored token is now empty and the username is preserved.

Result: Stored token is cleared.

Test 5: Checkbox under EP_CREDENTIALS

  1. Define EP_CREDENTIALS in wp-config.php.
  2. Reload the Settings page.
  3. Confirm the remove-token checkbox is not rendered.

Result: Checkbox only appears when the constant is not set.

Screenshots

Before:
image

After:
image

- Change token input from type=text to type=password
- Remove token value from HTML, use masked placeholder instead
- Preserve existing token in DB when field submitted empty
- Update description text to reflect new behavior

Fixes 10up#4305

@burhandodhy burhandodhy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Please add tests to cover these changes.

- Guard credentials update when EP_CREDENTIALS defined (prevent crafted POST leaking wp-config token to wp_options)
- Reuse prev_ep_credentials instead of redundant get_epio_credentials read
- Remove dead ternary branch
- Add tests for token preserve-on-empty and new-token save
- Add CHANGELOG Security entry

Refs 10up#4324

@faisalahammad faisalahammad left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up to address feedback before review. Pushed in 4e0092a:

  • Leak guard: when EP_CREDENTIALS is defined, a crafted POST could persist the wp-config token into wp_options in plaintext. The credentials update block now short-circuits when the constant is set, consistent with the field's existing disabled attribute.
  • Redundant read removed: reuse $this->prev_ep_credentials (already fetched at the top of action_admin_init) instead of calling get_epio_credentials() again.
  • Dead ternary branch removed inside the isset guard.
  • Tests added (TestSettings): token preserved on empty POST, new token saved. Both skip when EP_CREDENTIALS is defined.
  • CHANGELOG: Security entry under Unreleased.

composer run lint clean, composer run test-single-site -- --filter TestSettings passes (9 tests).

Ready for review.

@faisalahammad

Copy link
Copy Markdown
Contributor Author

Pushed follow-up addressing feedback before review (commit 4e0092a). Leak guard for EP_CREDENTIALS, redundant read removed, dead branch removed, tests + CHANGELOG added. Lint clean, 9/9 TestSettings pass. Ready for review @felipeelia.

@felipeelia felipeelia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@faisalahammad in addition to the autocomplete attribute change, can we also add a playwright test for this?

Also, any ideas on how a user could "remove" the stored value and make it empty in the database if needed?

Comment thread includes/partials/settings-page.php Outdated
@faisalahammad

Copy link
Copy Markdown
Contributor Author

Playwright spec added in tests/e2e/src/specs/settings-token.spec.ts (commit pending push, already in zip). Three tests: token not in DOM, empty save preserves stored token, new token is saved. Spec skips when IS_EPIO_ENVIRONMENT=1 so CI stays local-only.

For the second question, three options ordered by what I would recommend:

  1. Separate "Clear token" button next to the field. New admin-post action with nonce + capability check that calls Utils update_option( 'ep_credentials', [ 'username' => creds[username], 'token' => '' ] ). Discoverable, no risk to the normal save flow, matches the expectation of a non-technical ElasticPress.io subscriber.

  2. A "remove token" checkbox next to the field. When ticked, the empty POST becomes a signal to clear instead of preserve. Smallest UI change, but the semantics need a short helper text and the user has to understand the checkbox.

  3. WP-CLI command (wp elasticpress reset-token). Cleanest for ops-driven sites where the deployer is the admin, but it does not answer the UI path you asked about.

My pick is option 1 if you want a UI answer. Happy to implement it in this PR or a follow-up, your call.

@faisalahammad faisalahammad left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed felipeelia review feedback (2026-08-06):

  • autocomplete: switched to autocomplete="off" per the inline suggestion.
  • Playwright spec: added tests/e2e/src/specs/settings-token.spec.ts covering token not in DOM, empty save preserves stored token, new token is saved. Spec skips when IS_EPIO_ENVIRONMENT=1.
  • UX question: replied with three options (separate Clear button / remove checkbox / WP-CLI command) and recommended option 1 in a top-level PR comment. No code change for it without maintainer pick.

Re-requesting review.

@faisalahammad

Copy link
Copy Markdown
Contributor Author

@felipeelia @burhandodhy addressed the review feedback (autocomplete fix + new Playwright spec) and replied to the UX question with three options. Ready for another look when you have time.

Switch the Subscription Token input autocomplete attribute to off and add a Playwright spec covering the hidden token behavior: the value is never rendered, an empty save preserves the stored token, and a new value is saved.

Addresses PR feedback.

Refs 10up#4324
@felipeelia felipeelia added this to the 5.3.4 milestone Aug 7, 2026
@felipeelia

Copy link
Copy Markdown
Member

@faisalahammad let's go with option number 2 please.

Add a checkbox on the ElasticPress settings page to clear the stored
subscription token. When unchecked, an empty token POST still preserves
the stored value (existing behavior). When checked, the token is cleared
from wp_options while the username is preserved.

Props @faisalahammad
Fixes 10up#4324
@faisalahammad

Copy link
Copy Markdown
Contributor Author

Done in commit ad2a25b (option 2).

  • Checkbox markup: includes/partials/settings-page.php:140, inside the ep_admin_show_credentials block, under the token input.
  • Branch logic: includes/classes/Screen/Settings.php — when ep_remove_token is truthy, ep_credentials is saved with empty token and the posted/previous username is preserved. Unchecked falls through to the existing preserve-on-empty-POST path from 4e0092a.
  • EP_CREDENTIALS constant still short-circuits the whole credentials update, so the checkbox has no effect when credentials are locked in wp-config.
  • Tests: tests/php/screen/TestSettings.php (test_action_admin_init_remove_token_checkbox_clears_stored_token) and tests/e2e/src/specs/settings-token.spec.ts ("Checking the remove token checkbox clears the stored value"). Both skip when EP_CREDENTIALS is defined.

Lint clean, headless checks green. Re-requesting review.

@faisalahammad

Copy link
Copy Markdown
Contributor Author

Implemented and pushed (commit ad2a25b). Cannot dismiss the CHANGES_REQUESTED review or re-request review from this account (external contributor perms), so flagging here for visibility — @felipeelia could you re-request review when you have a moment? Reply with any follow-up and I will push fixes.

- merge origin/develop into fix/4305-hide-subscription-token (PR 10up#4338 price filter tax fix)
- auto-fix: run phpcbf on includes/classes/ElementorUtils.php:111 equals-align warning
- tests: wrap IS_EPIO_ENVIRONMENT in try/finally for isolation in 3 Settings tests

Errors fixed:
- PHPCS: equals sign not aligned correctly; expected 1 space but found 6 spaces
- PHPUnit: testPriceFilterWithTax float drift (100.99000000000001 vs 100.99)

Refs 10up#4324
@faisalahammad

Copy link
Copy Markdown
Contributor Author

CI Fix Summary — 9 failures resolved

Root cause: branch was stale relative to develop. Rebased the PR by merging origin/develop (which already contained PR #4338 — the WooCommerce price-filter tax float-drift fix and its new testPriceFilterWithTax test). CI runs on the merge-ref (base + head), so the stale branch's old Products.php failed the newer develop test.

# File Error Fix
1 includes/classes/ElementorUtils.php:111 PHPCS: equals sign not aligned (expected 1 space, found 6) phpcbf auto-fix
2 includes/classes/Feature/WooCommerce/Products.php PHPUnit testPriceFilterWithTax: 100.99 vs 100.99000000000001, boost 2 vs 2.0 merged develop's wc_format_decimal() in get_price_filter_tax_adjustment()
3 tests/php/screen/TestSettings.php CodeRabbit minor: env var leak between tests wrap IS_EPIO_ENVIRONMENT in try/finally

Tests: PHPCS clean, php -l clean, ESLint clean, PHPUnit on CI (needs wp-env). Verification sub-agents confirmed no token-render regressions and no PHP 7.4/WP/WC deprecations.

@felipeelia

Copy link
Copy Markdown
Member

@faisalahammad it seems e2e tests are not passing. Did you check those locally? Are they passing for you?

Toggle the EP_HOST wp-config constant in beforeAll/afterAll so the seeded
ep_host option drives is_epio(), rendering the credentials row on the
non-EPIO tab. Gate the two save tests on a live EPIO connection, since
saving calls get_elasticsearch_info( true ) and an unreachable seeded host
triggers reset_settings() which wipes the credentials.
@faisalahammad

faisalahammad commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@felipeelia fixed and verified locally (commit 8f27751ff): settings-token.spec.ts = 2 passed / 2 skipped / 0 failed on the non-EPIO matrix. The two skipped save-tests need a live EPIO connection — saving calls get_elasticsearch_info( true ) and an unreachable host triggers reset_settings(), wiping credentials — so they now test.skip(!isEpIo()) and run on the EPIO CI matrix. Remaining @Group1 failures are cold-ES noise (0 settings-token in failedTests); ESLint clean. Happy to mock the EPIO host if skipping those two is not acceptable.

@felipeelia

Copy link
Copy Markdown
Member

@faisalahammad we are almost there, but there are a couple of key things we need to address:

  1. The style of the checkbox is completely off:
image
  1. We don't need to display the checkbox when the value is coming from the EP_CREDENTIALS constant, as any change in the ep_credentials option will not be used in that case.

We will likely need to handle EP.io tests ourselves, so if you can address those two changes, we can handle that later.

- Exclude checkbox inputs from the 250px width rule applied to the credentials form tables, so the remove-token checkbox renders at its natural size
- Render the remove-token checkbox only when the EP_CREDENTIALS constant is not set, matching the backend behavior

Addresses PR feedback.

Refs 10up#4324
@faisalahammad

Copy link
Copy Markdown
Contributor Author

@felipeelia both changes are done in 47ae331.

  1. Checkbox style: the 250px width rule in dashboard.css was applying to all input types, so the checkbox got stretched. It now excludes checkbox inputs (assets/css/dashboard.css). The token field and the other inputs keep the same width.
  2. The checkbox is not rendered anymore when the EP_CREDENTIALS constant is defined (includes/partials/settings-page.php). The backend already skipped the option update in that case, so no change was needed there.

Left the EP.io tests for your side as you suggested. Ready for another look when you have time.

@felipeelia
felipeelia merged commit 5fb23da into 10up:develop Aug 17, 2026
24 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hide Subscription Token value

3 participants