Skip to content

SL-378 Fix API password corruption and plaintext exposure on settings save#331

Open
TLabutis wants to merge 1 commit into
masterfrom
SL-378-api-password-special-chars-corruption
Open

SL-378 Fix API password corruption and plaintext exposure on settings save#331
TLabutis wants to merge 1 commit into
masterfrom
SL-378-api-password-special-chars-corruption

Conversation

@TLabutis

@TLabutis TLabutis commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Problem

On the settings page (2.0.3), a saved JSON API password containing &, <, > or " gets silently corrupted every time the settings are saved without re-typing it. The password_input template escapes the pre-filled value (regression from f7adabca), and on the next save that escaped value is submitted back and double-encoded. The corrupted password then fails auth with a 401 "Failed to fetch terminals", with no visible error in the back office. Purely alphanumeric passwords are unaffected.

Separately, the stored password was rendered in clear text in the page source.

Fix

  • Stop pre-filling the password into the value attribute (template renders value="").
  • Keep the stored password when the field is submitted empty (keepExistingPasswordWhenSubmittedEmpty).
  • Add autocomplete="off" and a "leave empty to keep the saved password" hint.

Verification (PS 9.1.3, module 2.0.3)

  • Save without touching the password: stored value preserved (before: Test&Pass&12 corrupted to Test&amp;Pass&12).
  • Type a new &-password: stored exactly as entered.
  • Password no longer present in page source.
  • Passes the PrestaShop module validator (no unescaped output); PHP 7.1 compatible.

Notes

Not yet confirmed as the cause of the current merchant 401 report - that is being verified separately on the merchant environment. This is a valid fix regardless.

Ticket: https://invertus.atlassian.net/browse/SL-378

… save

The password_input value was HTML-escaped in the template (regression from
f7adabc) and pre-filled from the stored value. On save without re-typing,
the escaped value was submitted back and double-encoded, silently corrupting
passwords containing & < > " and causing 401 errors. The stored password was
also exposed in clear text in the page source.

- Stop pre-filling the password into the value attribute
- Keep the stored password when the field is submitted empty
- Add autocomplete off and a 'leave empty to keep' hint

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request implements a mechanism to retain existing passwords when password fields are submitted empty, updating both the admin controller and the options template to support placeholders and autocomplete attributes. The review feedback suggests avoiding direct modification of the global $_POST superglobal by dynamically removing empty password fields from $this->fields_options instead. Additionally, it recommends using autocomplete="new-password" instead of autocomplete="off" to ensure modern browsers do not autofill saved credentials into the password fields.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +59 to +60
$this->keepExistingPasswordWhenSubmittedEmpty(SaferPayConfig::PASSWORD);
$this->keepExistingPasswordWhenSubmittedEmpty(SaferPayConfig::PASSWORD . SaferPayConfig::TEST_SUFFIX);

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.

medium

Instead of directly modifying the global $_POST superglobal (which is a code smell and can lead to unexpected side effects or issues with other modules/hooks), we can dynamically remove the password fields from $this->fields_options when they are submitted empty. This prevents PrestaShop's HelperOptions from updating them in the database, preserving the existing configuration values cleanly and safely.

        $this->ignoreEmptyPasswordFieldsFromOptions();

Comment on lines +88 to +95
private function keepExistingPasswordWhenSubmittedEmpty($key)
{
if (!isset($_POST[$key]) || $_POST[$key] !== '') {
return;
}

$_POST[$key] = \Configuration::get($key);
}

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.

medium

Refactor this helper method to dynamically remove the empty password fields from $this->fields_options instead of modifying the global $_POST superglobal. This is a much cleaner and safer approach that avoids side effects.

    private function ignoreEmptyPasswordFieldsFromOptions()
    {
        $keys = [
            SaferPayConfig::PASSWORD,
            SaferPayConfig::PASSWORD . SaferPayConfig::TEST_SUFFIX,
        ];

        foreach ($keys as $key) {
            if (Tools::getValue($key) === '') {
                foreach ($this->fields_options as &$category) {
                    if (isset($category['fields'][$key])) {
                        unset($category['fields'][$key]);
                    }
                }
            }
        }
    }

value="{$field['value']|escape:'htmlall':'UTF-8'}"
value=""
{if isset($field['placeholder']) && $field['placeholder']} placeholder="{$field['placeholder']|escape:'htmlall':'UTF-8'}"{/if}
{if isset($field['autocomplete']) && !$field['autocomplete']} autocomplete="off"{/if} />

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.

medium

Modern browsers (such as Chrome, Firefox, and Safari) often ignore autocomplete="off" on password fields to force password managers to work. To reliably prevent browsers from autofilling saved credentials into this field, use autocomplete="new-password" instead.

                    {if isset($field['autocomplete']) && !$field['autocomplete']} autocomplete="new-password"{/if} />

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.

1 participant