Simplify regular expressions in i18n.t to make them non-polynomial #6350
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Placeholders start with a %, not a $.
Thanks to @chrisns who explained: > “You do not need both `.` and `\S+`. The ambiguity comes from their combination, which makes overlapping matching possible.” This change does alter the behaviour of the function. The use of `.\S+` in the current regex allows for a single leading space within the placeholder name (e.g. `%{ foo}`) because: - `.` matches ANY character except newline, *including* a space - `\S+` matches one or more *non-whitespace* characters (letters, digits, symbols, etc.) By removing the `.` in the regular expression a leading space is no longer permitted as part of the placeholder name. I don’t believe this was an intentional feature, and given the i18n library remains private and I think we can safely make this change as we know how it is used internally.
We don’t need the match that is returned by `String.prototype.match` – we just need a boolean that tells us whether `translation` matches the regex or not. Use `RegExp.prototype.test` which is the recommended method for this use case. [1] This also means we can get rid of an eslint-disable comment 🎉 [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match#:~:text=If%20you%20need%20to%20know%20if%20a%20string%20matches%20a%20regular%20expression%20RegExp%2C%20use%20RegExp.prototype.test().
Contributor
Author
|
As i18n.t is not currently public, I don't think this needs a changelog entry? Happy to be persuaded otherwise though 😄 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // eslint-disable-next-line @typescript-eslint/prefer-regexp-exec | ||
| if (translation.match(/%{(.\S+)}/)) { | ||
| // Check for %{} placeholders in the translation string | ||
| if (/%{(\S+)}/.test(translation)) { |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
JavaScript changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
index c88509283..8ce369cc7 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.js
@@ -199,7 +199,7 @@ class I18n {
n && (i = n)
}
if ("string" == typeof i) {
- if (i.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(i)) {
if (!e) throw new Error("i18n: cannot replace placeholders in string if no option data provided");
return this.replacePlaceholders(i, e)
}
@@ -209,7 +209,7 @@ class I18n {
}
replacePlaceholders(t, e) {
const i = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : void 0;
- return t.replace(/%{(.\S+)}/g, (function(t, n) {
+ return t.replace(/%{(\S+)}/g, (function(t, n) {
if (Object.prototype.hasOwnProperty.call(e, n)) {
const t = e[n];
return !1 === t || "number" != typeof t && "string" != typeof t ? "" : "number" == typeof t ? i ? i.format(t) : `${t}` : t
Action run for f0726b3 |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/all.bundle.js b/packages/govuk-frontend/dist/govuk/all.bundle.js
index f030cdff5..7c5a89783 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.js
@@ -394,7 +394,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -406,7 +406,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/all.bundle.mjs b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
index 82431b278..01eedcae4 100644
--- a/packages/govuk-frontend/dist/govuk/all.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/all.bundle.mjs
@@ -388,7 +388,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -400,7 +400,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
index e68960718..69bc7ad3e 100644
--- a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.js
@@ -313,7 +313,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -325,7 +325,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
index 871d3c9fa..5cc339ab0 100644
--- a/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/accordion/accordion.bundle.mjs
@@ -307,7 +307,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -319,7 +319,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.js b/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.js
index 1b6f35f96..10edea80b 100644
--- a/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.js
@@ -338,7 +338,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -350,7 +350,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.mjs
index df4f4ca7b..f9025a5ad 100644
--- a/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/character-count/character-count.bundle.mjs
@@ -332,7 +332,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -344,7 +344,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.js b/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.js
index 162c2521b..9bd6e374d 100644
--- a/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.js
@@ -313,7 +313,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -325,7 +325,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.mjs
index 4c46039c6..0e1a274c0 100644
--- a/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/exit-this-page/exit-this-page.bundle.mjs
@@ -307,7 +307,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -319,7 +319,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.js b/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.js
index 1e55e54b2..ea5757f9d 100644
--- a/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.js
@@ -318,7 +318,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -330,7 +330,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.mjs
index 02969bc75..5d89fecc7 100644
--- a/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/file-upload/file-upload.bundle.mjs
@@ -312,7 +312,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -324,7 +324,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.js b/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.js
index ffc98d501..e3439a9e5 100644
--- a/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.js
+++ b/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.js
@@ -318,7 +318,7 @@
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -330,7 +330,7 @@
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.mjs b/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.mjs
index f1061b82a..bbbe0059c 100644
--- a/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.mjs
+++ b/packages/govuk-frontend/dist/govuk/components/password-input/password-input.bundle.mjs
@@ -312,7 +312,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -324,7 +324,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
diff --git a/packages/govuk-frontend/dist/govuk/i18n.mjs b/packages/govuk-frontend/dist/govuk/i18n.mjs
index 6da10630e..3458a3231 100644
--- a/packages/govuk-frontend/dist/govuk/i18n.mjs
+++ b/packages/govuk-frontend/dist/govuk/i18n.mjs
@@ -20,7 +20,7 @@ class I18n {
}
}
if (typeof translation === 'string') {
- if (translation.match(/%{(.\S+)}/)) {
+ if (/%{(\S+)}/.test(translation)) {
if (!options) {
throw new Error('i18n: cannot replace placeholders in string if no option data provided');
}
@@ -32,7 +32,7 @@ class I18n {
}
replacePlaceholders(translationString, options) {
const formatter = Intl.NumberFormat.supportedLocalesOf(this.locale).length ? new Intl.NumberFormat(this.locale) : undefined;
- return translationString.replace(/%{(.\S+)}/g, function (placeholderWithBraces, placeholderKey) {
+ return translationString.replace(/%{(\S+)}/g, function (placeholderWithBraces, placeholderKey) {
if (Object.prototype.hasOwnProperty.call(options, placeholderKey)) {
const placeholderValue = options[placeholderKey];
if (placeholderValue === false || typeof placeholderValue !== 'number' && typeof placeholderValue !== 'string') {
Action run for f0726b3 |
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for f0726b3 |
Contributor
Author
|
The regular expression is still being flagged in one place 🤔 Putting this back in draft for now… |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a couple of GitHub Code Scanning alerts relating to the regular expressions we use being polynomial.
We're not particularly concerned about these alerts as the regexes should not be used against user-controlled data, but they're worth addressing anyway.
Thanks to @chrisns who explained:
This change does alter the behaviour of the function.
The use of
.\S+in the current regex allows for a single leading space within the placeholder name (e.g.%{ foo}) because:.matches ANY character except newline, including a space\S+matches one or more non-whitespace characters (letters, digits, symbols, etc.)By removing the
.in the regular expression a leading space is no longer permitted as part of the placeholder name.I don’t believe this was an intentional feature, and given the i18n library remains private and I think we can safely make this change as we know how it is used internally.
I've also fixed a couple of comments that incorrectly used
${}for the placeholder format, and switched to usingRegexp.prototype.testoverString.prototype.matchwhere we just want to know if the translation string matches the regex or not.