go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
43.43k stars 5.34k forks source link

Removal/Sanitization of HTML from translations #24402

Open lunny opened 1 year ago

lunny commented 1 year ago

Is it possible to inject <script>alert('xss')</script> via translation string or is there sanitization to prevent this? It's not a new issues but I suspect all translations may be vulnerable to attacks like this. Of course, given Crowdin's review process, something like this is unlikely to pass review, but it's good to have defense in depth.

Originally posted by @silverwind in https://github.com/go-gitea/gitea/issues/24397#issuecomment-1527159991

wxiaoguang commented 1 year ago

Also related to https://github.com/go-gitea/gitea/issues/23863

We need our own INI package first, then we can either pre-check or reject malicious translations.

Without our own INI package support, we can do the sanitizing when loading the locale assets

silverwind commented 1 year ago

I think we could eliminate all HTML from translations, after which this sanitization could just strip all HTML tags from the translated string.

wxiaoguang commented 1 year ago

It's also a big work.

Just like proposing "dropping jQuery", "dropping Fomantic UI", without a feasible plan and enough time spent on it, I do not think the good end would come.


At least, we can do more strict reviewing from now on, do the best to avoid more HTML appearing in translation

silverwind commented 1 year ago

HTML removal isn't so hard, see https://github.com/go-gitea/gitea/pull/24397. This can be started now. Only issue is there's quite a lot of them:


$ rg '<' options/locale/locale_en-US.ini | wc -l
202
lunny commented 1 year ago

I used a new key so that old system will not be broken.

silverwind commented 1 year ago

I used a new key so that old system will not be broken.

If it's a single-use key, we must use the same key, as the old key will no longer be used and remain as a "dead" translation entry.

wxiaoguang commented 1 year ago

I used a new key so that old system will not be broken.

If it's a single-use key, we must use the same key, as the old key will no longer be used and remain as a "dead" translation entry.

Sometimes we backport the locales from main to 1.19, old key could be removed safely, the removal doesn't affect backporting.

silverwind commented 1 year ago

Maybe it would also be useful to have a script to detect dead translation entries. Could even run it as a verification on CI.

wxiaoguang commented 1 year ago

Actually I also have such plan, the problem is that some keys are constructed dynamically, so we need some "hint" syntax to tell the linter that "the next line would use the following keys: ...."