Skip to content

Commit

Permalink
[localize] Make placeholder validation pass with different expressions (
Browse files Browse the repository at this point in the history 
#4530)
  • Loading branch information
@augustjk
augustjk committed Jan 31, 2024
1 parent 89c5bdf commit 258142d
Show file tree
Hide file tree
Showing 28 changed files with 162 additions and 26 deletions.
5 changes: 5 additions & 0 deletions .changeset/mighty-cars-confess.md
View file
@@ -0,0 +1,5 @@
---
'@lit/localize-tools': patch
---

Translated message validation that runs before the `build` step now disregards template literal expressions. This allow source code to have variables in expressions renamed while still keeping the same translations, or avoid errors that could happen from module import order changing which expression gets picked up first when multiple `msg()` calls with the same id have different expressions. This behavior is more consistent with how a translation unit is identified according to [how the message id is generated](https://lit.dev/docs/localization/overview/#id-generation).
60 changes: 47 additions & 13 deletions packages/localize-tools/src/messages.ts
View file
Expand Up @@ -4,8 +4,9 @@
* SPDX-License-Identifier: BSD-3-Clause
*/

import * as ts from 'typescript';
import ts from 'typescript';
import type {Locale} from './types/locale.js';
import {parseStringAsTemplateLiteral} from './typescript.js';

/**
* A message for translation.
Expand Down Expand Up @@ -111,17 +112,20 @@ export function sortProgramMessages(
* (no more, no less, no changes, but order can change).
*
* It is important to validate this condition because placeholders can contain
* arbitrary HTML and JavaScript template literal placeholder expressions, will
* be substituted back into generated executable source code. A well behaving
* localization process/tool would not allow any modification of these
* placeholders, but we can't assume that to be the case, so it is a potential
* source of bugs and attacks and must be validated.
* arbitrary HTML which will be substituted back into generated executable
* source code. A well behaving localization process/tool would not allow any
* modification of these placeholders, but we can't assume that to be the case,
* so it is a potential source of bugs and attacks and must be validated.
*
* JavaScript template expressions within placeholders are not validated since
* they are replaced by numbers in runtime mode, or the expression from the
* source code in transform mode.
*/
export function validateLocalizedPlaceholders(
programMessages: Message[],
localizedMessages: Map<Locale, Message[]>
): string[] {
const errors = [];
const errors: string[] = [];
const programMap = makeMessageIdMap(programMessages);
for (const [locale, messages] of localizedMessages) {
for (const localizedMsg of messages) {
Expand All @@ -135,22 +139,34 @@ export function validateLocalizedPlaceholders(
// Note that two identical placeholders could appear in the same template,
// and it matters how many of them there are, hence we use an array, not a
// set (might be good to implement some form of multiset here).
const remainingProgramPlaceholders = [];
const remainingProgramPlaceholders: Array<{
raw: string;
normalized: string;
}> = [];
for (const content of programMsg.contents) {
if (typeof content !== 'string') {
remainingProgramPlaceholders.push(content.untranslatable);
remainingProgramPlaceholders.push({
raw: content.untranslatable,
normalized: normalizeExpressionInTemplateString(
content.untranslatable
),
});
}
}

for (const content of localizedMsg.contents) {
if (typeof content !== 'string') {
const placeholder = content.untranslatable;
const index = remainingProgramPlaceholders.indexOf(placeholder);
const normalizedPlaceholder = normalizeExpressionInTemplateString(
content.untranslatable
);
const index = remainingProgramPlaceholders.findIndex(
({normalized}) => normalized === normalizedPlaceholder
);
if (index === -1) {
errors.push(
`Placeholder error in ${locale} ` +
`localization of ${localizedMsg.name}: ` +
`unexpected "${placeholder}"`
`unexpected "${content.untranslatable}"`
);
} else {
remainingProgramPlaceholders.splice(index, 1);
Expand All @@ -162,10 +178,28 @@ export function validateLocalizedPlaceholders(
errors.push(
`Placeholder error in ${locale} ` +
`localization of ${localizedMsg.name}: ` +
`missing "${placeholder}"`
`missing "${placeholder.raw}"`
);
}
}
}
return errors;
}

/**
* Given a template string, replace all expression with "expr". Used to compare
* static parts of the template string.
*
* e.g. `hello ${foo} world ${bar}` -> `hello ${expr} world ${expr}`
*/
function normalizeExpressionInTemplateString(templateString: string): string {
const template = parseStringAsTemplateLiteral(templateString);
if (ts.isNoSubstitutionTemplateLiteral(template)) {
return template.text;
}
let normalizedString = template.head.text;
for (const span of template.templateSpans) {
normalizedString += '${expr}' + span.literal.text;
}
return normalizedString;
}
2 changes: 0 additions & 2 deletions packages/localize-tools/src/tests/e2e/placeholder-errors.test.ts
View file
Expand Up @@ -14,8 +14,6 @@ e2eGoldensTest(
Placeholder error in es-419 localization of extra-expression: unexpected "\${alert("evil")}"
Placeholder error in es-419 localization of missing-expression: missing "\${name}"
Placeholder error in es-419 localization of changed-expression: unexpected "\${alert("evil") || name}"
Placeholder error in es-419 localization of changed-expression: missing "\${name}"
Placeholder error in es-419 localization of missing-html: missing "<b>"
Placeholder error in es-419 localization of missing-html: missing "</b>"
Placeholder error in es-419 localization of changed-html: unexpected "<blink>"
Expand Down
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/goldens/foo.ts
View file
Expand Up @@ -58,3 +58,7 @@ msg(html`<b>Hello</b>! Click <a href="${urlBase}/${urlPath}">here</a>!`);

// Escaped markup characters should remain escaped
msg(html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`);

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
2 changes: 2 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/goldens/tsout/es-419.ts
View file
Expand Up @@ -11,6 +11,7 @@ export const templates = {
h02c268d9b1fcb031: html`&lt;Hola<b>&lt;Mundo &amp; Amigos&gt;</b>!&gt;`,
h349c3c4777670217: html`[SALT] Hola <b>${0}</b>!`,
h3c44aff2d5f5ef6b: html`Hola <b>Mundo</b>!`,
h5e2d21ff71e6c8b5: html`<b>${0}</b> diferente`,
h82ccc38d4d46eaa9: html`Hola <b>${0}</b>!`,
h8d70dfec810d1eae: html`<b>Hola</b>! Clic <a href="${0}/${1}">aquí</a>!`,
h99e74f744fda7e25: html`Clic <a href="${0}">aquí</a>!`,
Expand All @@ -21,5 +22,6 @@ export const templates = {
s00ad08ebae1e0f74: str`Hola ${0}!`,
s03c68d79ad36e8d4: `described 0`,
s0f19e6c4e521dd53: `Mundo`,
s372f95c2b25986c8: str`${0} diferente`,
s8c0ec8d1fb9e6e32: `Hola Mundo!`,
};
2 changes: 2 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/goldens/tsout/zh_CN.ts
View file
Expand Up @@ -22,4 +22,6 @@ export const templates = {
s03c68d79ad36e8d4: `described 0`,
h8d70dfec810d1eae: html`<b>Hello</b>! Click <a href="${0}/${1}">here</a>!`,
h02c268d9b1fcb031: html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`,
s372f95c2b25986c8: str`Different ${0}`,
h5e2d21ff71e6c8b5: html`Different <b>${0}</b>`,
};
8 changes: 8 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/goldens/xliff/es-419.xlf
View file
Expand Up @@ -59,6 +59,14 @@
<source>&lt;Hello<ph id="0">&lt;b></ph>&lt;World &amp; Friends><ph id="1">&lt;/b></ph>!></source>
<target>&lt;Hola<ph id="0">&lt;b></ph>&lt;Mundo &amp; Amigos><ph id="1">&lt;/b></ph>!></target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <ph id="0">${user}</ph></source>
<target><ph id="0">${differentUser}</ph> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <ph id="0">&lt;b&gt;${user}&lt;/b&gt;</ph></source>
<target><ph id="0">&lt;b&gt;${differentUser}&lt;/b&gt;</ph> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/input/foo.ts
View file
Expand Up @@ -58,3 +58,7 @@ msg(html`<b>Hello</b>! Click <a href="${urlBase}/${urlPath}">here</a>!`);

// Escaped markup characters should remain escaped
msg(html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`);

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
8 changes: 8 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff-ph/input/xliff/es-419.xlf
View file
Expand Up @@ -59,6 +59,14 @@
<source>&lt;Hello<ph id="0">&lt;b></ph>&lt;World &amp; Friends><ph id="1">&lt;/b></ph>!></source>
<target>&lt;Hola<ph id="0">&lt;b></ph>&lt;Mundo &amp; Amigos><ph id="1">&lt;/b></ph>!></target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <ph id="0">${user}</ph></source>
<target><ph id="0">${differentUser}</ph> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <ph id="0">&lt;b&gt;${user}&lt;/b&gt;</ph></source>
<target><ph id="0">&lt;b&gt;${differentUser}&lt;/b&gt;</ph> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/goldens/foo.ts
View file
Expand Up @@ -58,3 +58,7 @@ msg(html`<b>Hello</b>! Click <a href="${urlBase}/${urlPath}">here</a>!`);

// Escaped markup characters should remain escaped
msg(html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`);

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
2 changes: 2 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/goldens/tsout/es-419.ts
View file
Expand Up @@ -11,6 +11,7 @@ export const templates = {
h02c268d9b1fcb031: html`&lt;Hola<b>&lt;Mundo &amp; Amigos&gt;</b>!&gt;`,
h349c3c4777670217: html`[SALT] Hola <b>${0}</b>!`,
h3c44aff2d5f5ef6b: html`Hola <b>Mundo</b>!`,
h5e2d21ff71e6c8b5: html`<b>${0}</b> diferente`,
h82ccc38d4d46eaa9: html`Hola <b>${0}</b>!`,
h8d70dfec810d1eae: html`<b>Hola</b>! Clic <a href="${0}/${1}">aquí</a>!`,
h99e74f744fda7e25: html`Clic <a href="${0}">aquí</a>!`,
Expand All @@ -21,5 +22,6 @@ export const templates = {
s00ad08ebae1e0f74: str`Hola ${0}!`,
s03c68d79ad36e8d4: `described 0`,
s0f19e6c4e521dd53: `Mundo`,
s372f95c2b25986c8: str`${0} diferente`,
s8c0ec8d1fb9e6e32: `Hola Mundo!`,
};
2 changes: 2 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/goldens/tsout/zh_CN.ts
View file
Expand Up @@ -22,4 +22,6 @@ export const templates = {
s03c68d79ad36e8d4: `described 0`,
h8d70dfec810d1eae: html`<b>Hello</b>! Click <a href="${0}/${1}">here</a>!`,
h02c268d9b1fcb031: html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`,
s372f95c2b25986c8: str`Different ${0}`,
h5e2d21ff71e6c8b5: html`Different <b>${0}</b>`,
};
8 changes: 8 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/goldens/xliff/es-419.xlf
View file
Expand Up @@ -59,6 +59,14 @@
<source>described 0</source>
<target>described 0</target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <x id="0" equiv-text="${user}"/></source>
<target><x id="0" equiv-text="${differentUser}"/> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <x id="0" equiv-text="&lt;b&gt;${user}&lt;/b&gt;"/></source>
<target><x id="0" equiv-text="&lt;b&gt;${differentUser}&lt;/b&gt;"/> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
6 changes: 6 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/goldens/xliff/zh_CN.xlf
View file
Expand Up @@ -47,6 +47,12 @@
<note>Description of 0</note>
<source>described 0</source>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <x id="0" equiv-text="${user}"/></source>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <x id="0" equiv-text="&lt;b&gt;${user}&lt;/b&gt;"/></source>
</trans-unit>
</body>
</file>
</xliff>
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/input/foo.ts
View file
Expand Up @@ -58,3 +58,7 @@ msg(html`<b>Hello</b>! Click <a href="${urlBase}/${urlPath}">here</a>!`);

// Escaped markup characters should remain escaped
msg(html`&lt;Hello<b>&lt;World &amp; Friends&gt;</b>!&gt;`);

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
8 changes: 8 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/input/xliff/es-419.xlf
View file
Expand Up @@ -59,6 +59,14 @@
<source>described 0</source>
<target>described 0</target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <x id="0" equiv-text="${user}"/></source>
<target><x id="0" equiv-text="${differentUser}"/> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <x id="0" equiv-text="&lt;b&gt;${user}&lt;/b&gt;"/></source>
<target><x id="0" equiv-text="&lt;b&gt;${differentUser}&lt;/b&gt;"/> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
6 changes: 6 additions & 0 deletions packages/localize-tools/testdata/build-runtime-xliff/input/xliff/zh_CN.xlf
View file
Expand Up @@ -47,6 +47,12 @@
<note>Description of 0</note>
<source>described 0</source>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <x id="0" equiv-text="${user}"/></source>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <x id="0" equiv-text="&lt;b&gt;${user}&lt;/b&gt;"/></source>
</trans-unit>
</body>
</file>
</xliff>
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/goldens/foo.ts
View file
Expand Up @@ -86,3 +86,7 @@ html`Hello <b foo=${msg('World')}>World</b>`;
html`<b foo=${msg('Hello')}>Hello</b><b bar=${msg('World')}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
html`Hello <b .foo=${msg('World')}>World</b>`;

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
3 changes: 3 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/goldens/tsout/en/foo.js
View file
Expand Up @@ -60,3 +60,6 @@ html`Hello <b foo=${'World'}>World</b>`;
html`<b foo=${'Hello'}>Hello</b><b bar=${'World'}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
// Placeholder in translation has different expression
`Different ${user}`;
html`Different <b>${user}</b>`;
5 changes: 4 additions & 1 deletion packages/localize-tools/testdata/build-transform-xliff/goldens/tsout/es-419/foo.js
View file
Expand Up @@ -57,6 +57,9 @@ html`&lt;Hola<b>&lt;Mundo &amp; Amigos&gt;</b>!&gt;`;
// Expressions as attribute values should stay as expressions
html`Hello <b foo=${'World'}>World</b>`;
html`Hello <b foo=${`Mundo`}>World</b>`;
html`<b foo=${'Hello'}>Hello</b><b bar=${`Mundo`}>World</b>`;
html`<b foo=${`Hola`}>Hello</b><b bar=${`Mundo`}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
html`Hello <b .foo=${`Mundo`}>World</b>`;
// Placeholder in translation has different expression
`${user} diferente`;
html`<b>${user}</b> diferente`;
3 changes: 3 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/goldens/tsout/zh_CN/foo.js
View file
Expand Up @@ -60,3 +60,6 @@ html`Hello <b foo=${'World'}>World</b>`;
html`<b foo=${'Hello'}>Hello</b><b bar=${'World'}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
html`Hello <b .foo=${'World'}>World</b>`;
// Placeholder in translation has different expression
`Different ${user}`;
html`Different <b>${user}</b>`;
12 changes: 12 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/goldens/xliff/es-419.xlf
View file
Expand Up @@ -46,6 +46,18 @@
<source>&lt;Hello<ph id="0">&lt;b></ph>&lt;World &amp; Friends><ph id="1">&lt;/b></ph>!></source>
<target>&lt;Hola<ph id="0">&lt;b></ph>&lt;Mundo &amp; Amigos><ph id="1">&lt;/b></ph>!></target>
</trans-unit>
<trans-unit id="s63f0bfacf2c00f6b">
<source>Hello</source>
<target>Hola</target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <ph id="0">${user}</ph></source>
<target><ph id="0">${differentUser}</ph> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <ph id="0">&lt;b&gt;${user}&lt;/b&gt;</ph></source>
<target><ph id="0">&lt;b&gt;${differentUser}&lt;/b&gt;</ph> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
4 changes: 4 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/input/foo.ts
View file
Expand Up @@ -86,3 +86,7 @@ html`Hello <b foo=${msg("World")}>World</b>`;
html`<b foo=${msg("Hello")}>Hello</b><b bar=${msg("World")}>World</b>`;
html`Hello <b .foo=${"World"}>World</b>`;
html`Hello <b .foo=${msg("World")}>World</b>`;

// Placeholder in translation has different expression
msg(str`Different ${user}`);
msg(html`Different <b>${user}</b>`);
12 changes: 12 additions & 0 deletions packages/localize-tools/testdata/build-transform-xliff/input/xliff/es-419.xlf
View file
Expand Up @@ -46,6 +46,18 @@
<source>&lt;Hello<ph id="0">&lt;b></ph>&lt;World &amp; Friends><ph id="1">&lt;/b></ph>!></source>
<target>&lt;Hola<ph id="0">&lt;b></ph>&lt;Mundo &amp; Amigos><ph id="1">&lt;/b></ph>!></target>
</trans-unit>
<trans-unit id="s63f0bfacf2c00f6b">
<source>Hello</source>
<target>Hola</target>
</trans-unit>
<trans-unit id="s372f95c2b25986c8">
<source>Different <ph id="0">${user}</ph></source>
<target><ph id="0">${differentUser}</ph> diferente</target>
</trans-unit>
<trans-unit id="h5e2d21ff71e6c8b5">
<source>Different <ph id="0">&lt;b&gt;${user}&lt;/b&gt;</ph></source>
<target><ph id="0">&lt;b&gt;${differentUser}&lt;/b&gt;</ph> diferente</target>
</trans-unit>
</body>
</file>
</xliff>
1 change: 0 additions & 1 deletion packages/localize-tools/testdata/placeholder-errors/goldens/foo.ts
View file
Expand Up @@ -11,6 +11,5 @@ const name = 'Friend';

msg(`Hello World`, {id: 'extra-expression'});
msg(str`Hello ${name}`, {id: 'missing-expression'});
msg(str`Hello ${name}`, {id: 'changed-expression'});
msg(html`<b>Hello World</b>`, {id: 'missing-html'});
msg(html`<b>Hello World</b>`, {id: 'changed-html'});
4 changes: 0 additions & 4 deletions packages/localize-tools/testdata/placeholder-errors/goldens/xliff/es-419.xlf
View file
Expand Up @@ -10,10 +10,6 @@
<source>Hello <ph id="0">${name}</ph></source>
<target>Hola Mundo</target>
</trans-unit>
<trans-unit id="changed-expression">
<source>Hello <ph id="0">${name}</ph></source>
<target>Hola <ph id="0">${alert("evil") || name}</ph></target>
</trans-unit>
<trans-unit id="missing-html">
<source><ph id="0">&lt;b></ph>Hello World<ph id="1">&lt;/b></ph></source>
<target>Hola Mundo</target>
Expand Down
1 change: 0 additions & 1 deletion packages/localize-tools/testdata/placeholder-errors/input/foo.ts
View file
Expand Up @@ -11,6 +11,5 @@ const name = 'Friend';

msg(`Hello World`, {id: 'extra-expression'});
msg(str`Hello ${name}`, {id: 'missing-expression'});
msg(str`Hello ${name}`, {id: 'changed-expression'});
msg(html`<b>Hello World</b>`, {id: 'missing-html'});
msg(html`<b>Hello World</b>`, {id: 'changed-html'});
4 changes: 0 additions & 4 deletions packages/localize-tools/testdata/placeholder-errors/input/xliff/es-419.xlf
View file
Expand Up @@ -10,10 +10,6 @@
<source>Hello <ph id="0">${name}</ph></source>
<target>Hola Mundo</target>
</trans-unit>
<trans-unit id="changed-expression">
<source>Hello <ph id="0">${name}</ph></source>
<target>Hola <ph id="0">${alert("evil") || name}</ph></target>
</trans-unit>
<trans-unit id="missing-html">
<source><ph id="0">&lt;b></ph>Hello World<ph id="1">&lt;/b></ph></source>
<target>Hola Mundo</target>
Expand Down

0 comments on commit 258142d

Please sign in to comment.