Skip to content
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

Jump to bottom

fix!: Only sanitize the result string when replacing variables #648

Merged
merged 1 commit into from
Sep 24, 2023
Merged

fix!: Only sanitize the result string when replacing variables #648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 9 additions & 5 deletions lib/translation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,18 @@ export function translate(
const _build = (text: string, vars?: Record<string, string | number>, number?: number) => {
return text.replace(/%n/g, '' + number).replace(/{([^{}]*)}/g, (match, key) => {
if (vars === undefined || !(key in vars)) {
return optSanitize(match)
return optEscape(match)
}

const r = vars[key]
if (typeof r === 'string' || typeof r === 'number') {
return optSanitize(optEscape(r))
const replacement = vars[key]
if (typeof replacement === 'string' || typeof replacement === 'number') {
return optEscape(`${replacement}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see how the removal of the sanitize fixes the test that you added. We've been working around this for a long time with the following trick:
https://github.com/nextcloud/server/blob/8633f13e0af8c803cd419af2a611c41708e9c7c1/apps/theming/src/UserThemes.vue#L134-L139

I can see how it can be useful. But there could be a problem if the parameters are user input etc, but in most such cases we would use escape anyway and that should prevent most issues.
However, I would see this as a breaking change (just to raise awareness with in the changelog).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but in most such cases we would use escape anyway and that should prevent most issues.

Yes and the result is still sanitized, just not every variable but only the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickvergessen So even if escaping is deactivated user input would still be sanitized, which we already test here:
https://github.com/nextcloud/nextcloud-l10n/blob/f112d1401214675bf26f2a52efa8e91e78bf6d76/tests/translation.test.ts#L63

} else {
return optSanitize(match)
/* This should not happen,
* but the variables are used defined so not allowed types could still be given,
* in this case ignore the replacement and use the placeholder
*/
return optEscape(match)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the escape on all the returns, that should be fine (and could have some tests if it makes a difference).

}
})
}
Expand Down
6 changes: 6 additions & 0 deletions tests/translation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ describe('translate', () => {
expect(translation).toBe('Hallo <del>Name</del>')
})

it('without placeholder HTML escaping on links', () => {
const text = 'Hello {start}Nextcloud{end}'
const translation = translate('core', text, { start: '<a href="https://nextcloud.com">', end: '</a>' }, undefined, { escape: false })
expect(translation).toBe('Hello <a href="https://nextcloud.com">Nextcloud</a>')
})

it('with placeholder HTML escaping', () => {
const text = 'Hello {name}'
const translation = translate('core', text, { name: '<del>Name</del>' })
Expand Down
Loading