Skip to content

Add session ID handling and shop name to payment form template#156

Open
BushraAsif wants to merge 4 commits into
mainfrom
make-checkout-session-backward-compatible
Open

Add session ID handling and shop name to payment form template#156
BushraAsif wants to merge 4 commits into
mainfrom
make-checkout-session-backward-compatible

Conversation

@BushraAsif
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@shahbaz-mehar shahbaz-mehar left a comment

Choose a reason for hiding this comment

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

Changes have been reviewed.

'stylingclass' => $payment_style,
'amount' => $postData['amount'],
'shop_logo' => _PS_IMG_ . Configuration::get('PS_LOGO'),
'shop_name' => Configuration::get('PS_SHOP_NAME'),
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.

We are pushing the shop name to the frontend, which potentially can do XSS if not sanitised.

Is the ShopName sanitised when being inserted in the configuration?

Or even better, maybe we should sanitise it now to ensure how it is stored does not open us for this vulnerability?

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.

@emicha , here we are only reading the value from configuration and sending it to the template.

In PrestaShop, the standard approach is to escape at output depending on the context, which is already done here with {$shop_name|escape:'html').

views/templates/front/payment_form_independent.tpl

If we sanitize it in PHP here, it could cause issues like double escaping or problems in other contexts.

I agree it would be better to validate the shop name when it is saved in configuration, so it is safe everywhere.

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.

4 participants