Skip to content

fix(isFloat): use locale decimal separator for min/max comparison#2734

Open
tan7vir wants to merge 1 commit into
validatorjs:masterfrom
tan7vir:fix/isfloat-locale-decimal-separator
Open

fix(isFloat): use locale decimal separator for min/max comparison#2734
tan7vir wants to merge 1 commit into
validatorjs:masterfrom
tan7vir:fix/isfloat-locale-decimal-separator

Conversation

@tan7vir
Copy link
Copy Markdown

@tan7vir tan7vir commented Jun 3, 2026

What

isFloat validates the format of a number against the locale's decimal
separator (decimal[options.locale]), but the numeric value used for the
min / max / lt / gt range checks was parsed with a hardcoded comma
replacement:

const value = parseFloat(str.replace(',', '.'));

That only normalizes . and ,. For locales whose separator is neither of
those, the replace is a no-op, so parseFloat stops at the separator and
silently drops the fractional part.

The Arabic locale (ar) and the Farsi locales (fa-IR, fa-AF) use ٫
(U+066B, Arabic decimal separator). So parseFloat('3٫14'.replace(',', '.'))
returns 3, not 3.14, and every range comparison for those locales is
wrong — in both directions.

Reproduction (against the published validator)

const validator = require('validator');
const ar = '٫'; // U+066B

// 3.14 is within [3.1, 4] — should be true
validator.isFloat('3٫14', { locale: 'ar', min: 3.1, max: 4 }); // => false  (false negative)

// 9.99 exceeds max 9 — should be false
validator.isFloat('9٫99', { locale: 'ar', max: 9 });           // => true   (false positive)

The false-positive case is the concerning one: a max used to enforce a
limit silently accepts out-of-range input for these locales.

Fix

Replace the locale's actual decimal separator before parsing:

const decimalSeparator = options.locale ? decimal[options.locale] : '.';
const value = parseFloat(str.replace(decimalSeparator, '.'));

Comma locales (de-DE, fr-FR, …) and the default . behavior are
unchanged; format validation is untouched.

Tests

Added an isFloat case for the ar locale with min/max bounds in
test/validators.test.js. It fails on the old code (the existing locale
tests only covered format validation, not range checks) and passes with the
fix. Full validators suite passes (249 passing).

isFloat builds its format regex from the locale's decimal separator
(decimal[options.locale]), but the numeric value used for the min/max/lt/gt
range checks was parsed with a hardcoded comma replacement:

  parseFloat(str.replace(',', '.'))

For locales whose separator is neither '.' nor ',', the replacement was a
no-op, so parseFloat stopped at the separator and dropped the fractional
part. With the Arabic separator U+066B, parseFloat('3٫14') returned 3
instead of 3.14, corrupting every range comparison for the ar and fa-*
locales -- producing both false negatives (valid values rejected) and
false positives (out-of-range values accepted).

Replace the locale's actual decimal separator before parsing. Format
validation and all other locales are unchanged.
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7fdc788) to head (9fa67f2).

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2734   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          114       114           
  Lines         2587      2588    +1     
  Branches       656       657    +1     
=========================================
+ Hits          2587      2588    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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