Skip to content

Fix security notices #8474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Fix security notices #8474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cad1a17
Fix security notices
dwainm Mar 26, 2024
42f1ec0
Add changelog file
dwainm Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/fix-multi-currency-phpcs-notices
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: dev

Escaping error logs and ignoring noticese where there are no issues.
8 changes: 5 additions & 3 deletions includes/multi-currency/MultiCurrency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ public function update_single_currency_settings( string $currency_code, string $
if ( ! is_numeric( $manual_rate ) || 0 >= $manual_rate ) {
$message = 'Invalid manual currency rate passed to update_single_currency_settings: ' . $manual_rate;
Logger::error( $message );
throw new InvalidCurrencyRateException( $message, 'wcpay_multi_currency_invalid_currency_rate', 500 );
throw new InvalidCurrencyRateException( esc_html( $message ), 'wcpay_multi_currency_invalid_currency_rate', 500 );
}
update_option( 'wcpay_multi_currency_manual_rate_' . $currency_code, $manual_rate );
}
Expand Down Expand Up @@ -935,7 +935,7 @@ public function get_raw_conversion( float $amount, string $to_currency, string $
if ( 0 >= $from_currency_rate ) {
$message = 'Invalid rate for from_currency in get_raw_conversion: ' . $from_currency_rate;
Logger::error( $message );
throw new InvalidCurrencyRateException( $message, 'wcpay_multi_currency_invalid_currency_rate', 500 );
throw new InvalidCurrencyRateException( esc_html( $message ), 'wcpay_multi_currency_invalid_currency_rate', 500 );
}

$amount = $amount * ( $to_currency_rate / $from_currency_rate );
Expand Down Expand Up @@ -1019,6 +1019,8 @@ public function display_geolocation_currency_update_notice() {
$notice_id = md5( $message );

echo '<p class="woocommerce-store-notice demo_store" data-notice-id="' . esc_attr( $notice_id . 2 ) . '" style="display:none;">';
// No need to escape here as the function called handles it.
// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
echo \WC_Payments_Utils::esc_interpolated_html(
$message,
Comment on lines +1022 to 1025
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I would've expected this to get caught because of the following lines in phpcs.xml.dist:

woocommerce-payments/phpcs.xml.dist

Lines 73 to 77 in 9b6a7da

<rule ref="WordPress.Security.EscapeOutput">
<properties>
<property name="customEscapingFunctions" type="array" value="WC_Payments_Utils,esc_interpolated_html" />
</properties>
</rule>

I wonder if we're not adding that correctly 🤔
It'd be neat if we didn't need the ignore. I'll see if I can look into this as part of #8460 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @reykjalin, good catch there and thanks for looking into it. I wasn't aware of that rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is actually an issue with the ruleset and not necessarily our config (although I think the config is wrong as well):

See WordPress/WordPress-Coding-Standards#1176 and WordPress/WordPress-Coding-Standards#2370.

I think the config is wrong based on this example.

Either way, I guess this can wait until the next time we update the PHPCS sniffs since all those issues and PRs are still open 🫠

[
Expand Down Expand Up @@ -1623,7 +1625,7 @@ public function is_initialized(): bool {
private function log_and_throw_invalid_currency_exception( $method, $currency_code, $code = 500 ) {
$message = 'Invalid currency passed to ' . $method . ': ' . $currency_code;
Logger::error( $message );
throw new InvalidCurrencyException( $message, 'wcpay_multi_currency_invalid_currency', $code );
throw new InvalidCurrencyException( esc_html( $message ), 'wcpay_multi_currency_invalid_currency', esc_html( $code ) );
}

/**
Expand Down
Loading