Fix Stripe/Trans amount parser truncating locale-mismatched separators

stripe/controllers/FrmTransLiteActionsController.php

@@ -317,7 +317,6 @@ public static function prepare_amount( $amount, $atts = array() ) {
 
 		foreach ( (array) $amount as $a ) {
 			$this_amount = self::get_amount_from_string( $a );
-			self::maybe_use_decimal( $this_amount, $currency );
 			self::normalize_number( $this_amount, $currency );
 
 			$total += $this_amount;
@@ -364,35 +363,71 @@ private static function get_amount_from_string( $amount ) {
 	 *
 	 * @return void
 	 */
-	private static function maybe_use_decimal( &$amount, $currency ) {
-		if ( $currency['thousand_separator'] !== '.' ) {
-			return;
-		}
-
-		$amount_parts = explode( '.', $amount );
+	private static function normalize_number( &$amount, $currency ) {
+		$decimal_position = self::find_decimal_position( $amount, $currency );
 
-		if ( 2 !== count( $amount_parts ) ) {
-			return;
+		if ( false === $decimal_position ) {
+			$amount = str_replace( array( '.', ',' ), '', $amount );
+		} else {
+			$integer_part    = str_replace( array( '.', ',' ), '', substr( $amount, 0, $decimal_position ) );
+			$fractional_part = str_replace( array( '.', ',' ), '', substr( $amount, $decimal_position + 1 ) );
+			$amount          = $integer_part . '.' . $fractional_part;
 		}
 
-		$strlen           = strlen( $amount_parts[1] );
-		$used_for_decimal = $strlen === 1 || $strlen === 2;
-
-		if ( $used_for_decimal ) {
-			$amount = str_replace( '.', $currency['decimal_separator'], $amount );
-		}
+		$amount = number_format( (float) $amount, $currency['decimals'], '.', '' );
 	}
 
 	/**
+	 * Find the position of the amount's real decimal separator, or false if it has none (a
+	 * whole-number amount, possibly with thousands grouping).
+	 *
+	 * A user can type an amount in a different locale's format than the form's configured
+	 * currency expects (e.g. US-style "1,030.21" on a form whose currency configures '.' as
+	 * the thousand separator). Trusting the currency's configured separator in that case --
+	 * and blindly replacing every occurrence of it -- is what let this silently truncate:
+	 * when both '.' and ',' appear, or the same character repeats, only the rightmost
+	 * occurrence is ever the real decimal point; everything else gets stripped as grouping
+	 * noise by the caller.
+	 *
 	 * @param string $amount
 	 * @param array  $currency
 	 *
-	 * @return void
+	 * @return int|false
 	 */
-	private static function normalize_number( &$amount, $currency ) {
-		$amount = str_replace( $currency['thousand_separator'], '', $amount );
-		$amount = str_replace( $currency['decimal_separator'], '.', $amount );
-		$amount = number_format( (float) $amount, $currency['decimals'], '.', '' );
+	private static function find_decimal_position( $amount, $currency ) {
+		$last_dot   = strrpos( $amount, '.' );
+		$last_comma = strrpos( $amount, ',' );
+
+		if ( false !== $last_dot && false !== $last_comma ) {
+			return max( $last_dot, $last_comma );
+		}
+
+		if ( false !== $last_dot ) {
+			$present  = '.';
+			$position = $last_dot;
+		} elseif ( false !== $last_comma ) {
+			$present  = ',';
+			$position = $last_comma;
+		} else {
+			return false;
+		}
+
+		if ( $present === $currency['decimal_separator'] ) {
+			return $position;
+		}
+
+		// The lone separator matches the currency's thousand separator instead. A dot in a
+		// comma-decimal currency is ambiguous -- a single occurrence with a 1-2 digit tail
+		// still reads as a decimal point even though the currency expects '.' as its thousand
+		// separator. A comma in a dot-decimal currency is never ambiguous this way: it's
+		// always thousands grouping (e.g. "1,23" on a GBP form is 123, not 1.23).
+		if ( '.' !== $present || 1 !== substr_count( $amount, $present ) ) {
+			return false;
+		}
+
+		$tail_length = strlen( $amount ) - $position - 1;
+
+		return in_array( $tail_length, array( 1, 2 ), true ) ? $position : false;
 	}
 
 	/**

tests/phpunit/square/test_FrmSquareLiteAppController.php

@@ -135,7 +135,7 @@ public function amount_format_provider(): \Iterator {
 		yield 'BRL with a thousands dot' => array( 'brl', 'R$1.234,50', '1234.50', '123450' );
 
 		// A dot in a comma decimal currency is ambiguous. One or two trailing digits are
-		// read as a decimal, three are read as thousands. See maybe_use_decimal.
+		// read as a decimal, three are read as thousands. See find_decimal_position.
 		yield 'EUR dot with two digits is a decimal' => array( 'eur', '€20.00', '20.00', '2000' );
 		yield 'EUR dot with one digit is a decimal' => array( 'eur', '€1.5', '1.50', '150' );
 		yield 'EUR dot with three digits is thousands' => array( 'eur', '€1.234', '1234.00', '123400' );
@@ -145,6 +145,20 @@ public function amount_format_provider(): \Iterator {
 		// a shopper typing a European style amount into a GBP form is read as 123 pounds.
 		yield 'GBP comma is never a decimal' => array( 'gbp', '1,23', '123.00', '12300' );
 
+		// A shopper can type in a different locale's format than the form's configured
+		// currency expects. Both separators appearing together is unambiguous regardless of
+		// currency -- whichever one appears last is the real decimal point. Trusting the
+		// currency's configured separators here used to collide the two into one, silently
+		// truncating "1,030.21" to 1.03 (formidable-forms#3379).
+		yield 'EUR form with a US-style amount' => array( 'eur', '€1,030.21', '1030.21', '103021' );
+		yield 'GBP form with a EU-style amount' => array( 'gbp', '£1.030,21', '1030.21', '103021' );
+
+		// A repeated occurrence of whichever character turns out to be the decimal separator
+		// used to get blanket-replaced entirely, colliding into a second decimal point and
+		// truncating the value again just like the original bug -- only the rightmost
+		// occurrence is ever the real decimal point; earlier ones are grouping noise.
+		yield 'repeated decimal-separator character is still just noise' => array( 'eur', '1,234.567,89', '1234567.89', '123456789' );
+
 		// A currency with no fractional unit keeps the two paths identical, and rounds.
 		yield 'JPY with a thousands comma' => array( 'jpy', '1,234', '1234', '1234' );
 		yield 'JPY rounds away a decimal' => array( 'jpy', '1234.56', '1235', '1235' );

tests/phpunit/stripe/test_FrmTransLiteActionsController.php

@@ -43,39 +43,53 @@ private function get_fields_for_price( $action ) {
 	}
 
 	/**
-	 * @covers FrmTransLiteActionsController::maybe_use_decimal
+	 * @covers FrmTransLiteActionsController::find_decimal_position
 	 */
-	public function test_maybe_use_decimal() {
-		// We need a currency with a . thousands separator.
+	public function test_find_decimal_position() {
+		// A dot-thousands currency (e.g. EUR).
 		$currency = array(
 			'thousand_separator' => '.',
 			'decimal_separator'  => ',',
 		);
 
-		// Test with two decimal places.
-		$amount = '111.50';
-		$this->maybe_use_decimal( $amount, $currency );
-		$this->assertSame( '111,50', $amount );
+		// A single dot with a 1-2 digit tail reads as a decimal point even though this
+		// currency configures '.' as its thousand separator.
+		$this->assertSame( 3, $this->find_decimal_position( '111.50', $currency ) );
+		$this->assertSame( 3, $this->find_decimal_position( '111.5', $currency ) );
 
-		// Test with a single decimal place.
-		$amount = '111.5';
-		$this->maybe_use_decimal( $amount, $currency );
-		$this->assertSame( '111,5', $amount );
+		// Three digits after a single dot reads as thousands grouping instead.
+		$this->assertFalse( $this->find_decimal_position( '111.500', $currency ) );
 
-		// Test to make sure that three decimal places does not convert.
-		// It should be interpreted as thousands.
-		$amount = '111.500';
-		$this->maybe_use_decimal( $amount, $currency );
-		$this->assertSame( '111.500', $amount );
+		// Repeated dots and no comma at all is unambiguous thousands grouping.
+		$this->assertFalse( $this->find_decimal_position( '1.234.567', $currency ) );
+
+		// Both separators present is a different locale's format (e.g. a US-style
+		// "1,111.50" typed into a form with this dot-thousands currency) -- whichever
+		// separator appears last is the real decimal point, regardless of currency config.
+		$this->assertSame( 5, $this->find_decimal_position( '1,111.50', $currency ) );
+		$this->assertSame( 5, $this->find_decimal_position( '1.111,50', $currency ) );
+
+		// A repeated occurrence of the character that turns out to be the decimal separator
+		// doesn't move the split point off the rightmost occurrence (formidable-forms#3379:
+		// blindly replacing every occurrence of it is what caused the original truncation).
+		$this->assertSame( 9, $this->find_decimal_position( '1,234.567,89', $currency ) );
+
+		// A comma-thousands currency (e.g. GBP): a lone comma is always thousands grouping,
+		// never reinterpreted as a decimal point, regardless of its tail length.
+		$currency = array(
+			'thousand_separator' => ',',
+			'decimal_separator'  => '.',
+		);
+		$this->assertFalse( $this->find_decimal_position( '1,23', $currency ) );
 	}
 
 	/**
 	 * @param string $amount
 	 * @param array  $currency
 	 *
-	 * @return string
+	 * @return int|false
 	 */
-	private function maybe_use_decimal( &$amount, $currency ) {
-		return $this->run_private_method( array( 'FrmTransLiteActionsController', 'maybe_use_decimal' ), array( &$amount, $currency ) );
+	private function find_decimal_position( $amount, $currency ) {
+		return $this->run_private_method( array( 'FrmTransLiteActionsController', 'find_decimal_position' ), array( $amount, $currency ) );
 	}
 }