fix!: Only sanitize the result string when replacing variables #648
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #648 +/- ##
==========================================
+ Coverage 84.57% 86.17% +1.59%
==========================================
Files 6 6
Lines 188 188
Branches 67 67
==========================================
+ Hits 159 162 +3
+ Misses 28 23 -5
- Partials 1 3 +2
☔ View full report in Codecov by Sentry. |
susnux
changed the title
ChristophWurst
requested review from
nickvergessen
and removed request for
ChristophWurst
|
this is a review job for our sanitization connoisseur @nickvergessen |
| * 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) |
There was a problem hiding this comment.
I agree with the escape on all the returns, that should be fine (and could have some tests if it makes a difference).
| return optSanitize(optEscape(r)) | ||
| const replacement = vars[key] | ||
| if (typeof replacement === 'string' || typeof replacement === 'number') { | ||
| return optEscape(`${replacement}`) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
nickvergessen
changed the title
|
Merge or not to merge @susnux ? :) |
There is no need to sanitize the replacement values as it is sufficient to sanitize the result.
1. This will improve the performance if multiple placeholders are used.
2. This allows this: `See {linkstart}documentation{linkend}` with `{ linkstart: '<a ...>', linkend: '</a>' }` while the string is still sanitized.
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
skjnldsv
force-pushed
the
fix/only-sanitize-full-string
branch
from
f112d14
to
586c3b2
Compare
@skjnldsv merge, but that would mean the next release is a major version bump. |
Not sure if this is a bug fix or a new feature but:
There is no need to sanitize the replacement values as it is sufficient to sanitize the result.
See {linkstart}documentation{linkend}with{ linkstart: '<a ...>', linkend: '</a>' }while the string is still sanitized.