fix(ui): hardcode sort options in search syntax hint, improve look #11381

0ko · 2026-02-20T19:22:01+01:00

0ko commented

2026-02-20 19:22:01 +01:00

Followup to #9109

Fix issue reported by @mahlzahn that the string was confusing translators and they translated the part that wasn't meant to be translated
https://translate.codeberg.org/translate/forgejo/forgejo-next/en/?checksum=54d85b848166f618#translations
(this link is now broken because the string was replaced and weblate forgot it)

Part of this fix was to replace custom IterWithTr with simple dict iteration to allow for placeholders in strings.

Fix issue where <script>alert('🐸')</script> in locales would have been rendered. Similar to #7740. Locales are considered UGC as there are too many for developers to make sure they're not causing bugs, but in this case they are guarded by linter lint-locale.go. #11381 (comment)

Release notes

User Interface bug fixes
- PR: fix(ui): hardcode sort options in search syntax hint, improve look

Followup to https://codeberg.org/forgejo/forgejo/pulls/9109 Fix issue reported by @mahlzahn that the string was confusing translators and they translated the part that wasn't meant to be translated https://translate.codeberg.org/translate/forgejo/forgejo-next/en/?checksum=54d85b848166f618#translations (this link is now broken because the string was replaced and weblate forgot it) Part of this fix was to replace custom IterWithTr with simple dict iteration to allow for placeholders in strings. ~~Fix issue where `<script>alert('🐸')</script>` in locales would have been rendered. Similar to https://codeberg.org/forgejo/forgejo/pulls/7740. Locales are considered UGC as there are too many for developers to make sure they're not causing bugs, but in this case they are guarded by linter `lint-locale.go`.~~ https://codeberg.org/forgejo/forgejo/pulls/11381#issuecomment-10866701  ## Release notes  - User Interface bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/11381): fix(ui): hardcode sort options in search syntax hint, improve look

0ko added 4 commits

2026-02-20 19:22:01 +01:00

firstly, SafeHTML is not for safety f6caa1a635

it is for rendering raw trusted text as actual html, which translations are not

in both cases, adding `<script>alert('0ko')</script>` to the locale string causes script to execute, so SafeHTML does nothing here

it may need to be removed because it already caught at least two people thinking that it's safe to use it

deimplement IterWithTr, fix script execution c9769542fa

secondly, hard-code possible values in the hint via a placeholder 3447315675

to fix the issue where many translators got confused and thought that these were translatable

the key was changed to avoid either mass-updating the string in all locales or mass-breaking them

as i am here, replacing the button with newer equivalent

testing / semgrep/ci (pull_request) Successful in 16s

Details

testing / frontend-checks (pull_request) Successful in 1m4s

Details

testing / backend-checks (pull_request) Successful in 2m49s

Details

issue-labels / backporting (pull_request_target) Has been skipped

Details

issue-labels / cascade (pull_request_target) Has been skipped

Details

requirements / merge-conditions (pull_request) Successful in 3s

Details

issue-labels / release-notes (pull_request_target) Has been cancelled

Details

testing / test-mysql (pull_request) Has been cancelled

Details

testing / test-unit (pull_request) Has been cancelled

Details

testing / test-pgsql (pull_request) Has been cancelled

Details

testing / test-e2e (pull_request) Has been cancelled

Details

testing / test-sqlite (pull_request) Has been cancelled

Details

testing / test-remote-cacher (redis) (pull_request) Has been cancelled

Details

testing / test-remote-cacher (valkey) (pull_request) Has been cancelled

Details

testing / test-remote-cacher (garnet) (pull_request) Has been cancelled

Details

testing / test-remote-cacher (redict) (pull_request) Has been cancelled

Details

testing / security-check (pull_request) Has been cancelled

Details

85839973c5

also class icon was not needed as there was no icon in the button

0ko

2026-02-20 19:22:02 +01:00

added the
forgejo/i18n

forgejo/ui

bug
confirmed

test
not-needed

worth a release-note

backport/v14.0/forgejo
labels
requested reviews from Beowulf, Gusted, fogti

0ko added 1 commit

2026-02-20 19:29:02 +01:00

oops i submitted it broken

testing / semgrep/ci (pull_request) Successful in 14s

Details

testing / frontend-checks (pull_request) Successful in 1m19s

Details

testing / backend-checks (pull_request) Successful in 3m26s

Details

requirements / merge-conditions (pull_request) Successful in 4s

Details

issue-labels / release-notes (pull_request_target) Successful in 29s

Details

testing / test-unit (pull_request) Successful in 7m6s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m12s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m14s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m15s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m14s

Details

testing / test-e2e (pull_request) Successful in 20m21s

Details

testing / test-mysql (pull_request) Successful in 22m46s

Details

testing / test-sqlite (pull_request) Successful in 27m48s

Details

testing / test-pgsql (pull_request) Successful in 30m51s

Details

testing / security-check (pull_request) Successful in 53s

Details

9183ffb1da

0ko requested review from snematoda

2026-02-20 19:30:47 +01:00

mahlzahn reviewed

2026-02-20 19:42:43 +01:00

templates/shared/search/issue/syntax.tmpl

					
				@ -19,0 +14,4 @@

											"assignee:<username>" (ctx.Locale.TrString "repo.issues.filter_assignee.hint")

											"review:<username>" (ctx.Locale.TrString "repo.issues.filter_reviewers.hint")

											"mentions:<username>" (ctx.Locale.TrString "repo.issues.filter_mention.hint")

											"sort:<by>:[asc|desc]" (ctx.Locale.TrString "repo.issues.filter_sort.hint_with_placeholder" "created/comments/updated/deadline")

Maybe put the options in <code> element? And for consistency with pipes instead of slashes?

Maybe put the options in `<code>` element? And for consistency with pipes instead of slashes?

👍 1

Regarding <code>, it is close to impossible to implement it in the current way while retaining the script escaping. For that we would need some new version of TrString that HTMLEscapes the string itself and HTMLFormats placeholder values.

I now understand why we resorted to simply guarding the strings we have in the codebase by linting. It's much simpler and only needs to be time prior to runtime.

The issue with inserting scripts is real, but me trying to guard one string is useless, because every string rendered by Tr is affected in the same way.

So I'll drop that part of this PR.

Regarding `<code>`, it is close to impossible to implement it in the current way while retaining the script escaping. For that we would need some new version of `TrString` that HTMLEscapes the string itself and HTMLFormats placeholder values. I now understand why we resorted to simply guarding the strings we have in the codebase by linting. It's much simpler and only needs to be time prior to runtime. The issue with inserting scripts is real, but me trying to guard one string is useless, because every string rendered by `Tr` is affected in the same way. So I'll drop that part of this PR.

OK

~~I'll wait for someone to re-approve before merging.~~

~~I'll wait for someone to re-approve before merging.~~

Can we create a new issue for the string escaping thing?

@0ko wrote in #11381/files (comment):

Regarding <code>, it is close to impossible to implement it in the current way while retaining the script escaping. For that we would need some new version of TrString that HTMLEscapes the string itself and HTMLFormats placeholder values.

Considering that sort is just one out of nine... wouldn't it be simpler to just add that outside the loop :)

@0ko wrote in https://codeberg.org/forgejo/forgejo/pulls/11381/files#issuecomment-10866701: > Regarding `<code>`, it is close to impossible to implement it in the current way while retaining the script escaping. For that we would need some new version of `TrString` that HTMLEscapes the string itself and HTMLFormats placeholder values. Considering that sort is just one out of nine... wouldn't it be simpler to just add that outside the loop :)

One way to do it might be to handle the last case completely outside of the loop (this would also massively cut down on the size of this change).

One can't really handle it "to perfect satisfaction" in the place easily either, e.g. one can't use string parsing on the already substituted translation string because it would assume too much of the sentence structure of arbitrary languages, afaik (e.g. when just appending to the translated string, which would "get around" that problem in a meh way).

One way to do it might be to handle the last case completely outside of the loop (this would also massively cut down on the size of this change). One can't really handle it "to perfect satisfaction" in the place easily either, e.g. one can't use string parsing on the already substituted translation string because it would assume too much of the sentence structure of arbitrary languages, afaik (e.g. when just appending to the translated string, which would "get around" that problem in a meh way).

I don't see this as a significant issue worty solving because

we have 4500 usages of Locale.Tr in templates, we want them to retain ability for basic html formatting (a, bold, italic) while not allowing undesired formatting (img, script, style), and we already have protection in place for that via lint-locale.
while a lot of surface area can be mitigated by using Locale.TrString, currently it's almost never used in templates, and we won't be able to convert all usages of Locale.Tr to Locale.TrString because (1), so it's not even worth trying
if we have to use Locale.Tr, we can't just have some html tags escaped and some preserved, it will have to be a policy similar to what we have applied to markdown in UGC. But bisecting between good and bad formatting in runtime can't be implemented without degrading performance, and it's not needed because of (1)

The mitigations don't have to be implemented as either new Locale funcs or amended Locale funcs, the way of implementation simply doesn't matter, because it can't be done in a nice way because of (3).

I don't see this as a significant issue worty solving because 1. we have 4500 usages of `Locale.Tr` in templates, we want them to retain ability for basic html formatting (a, bold, italic) while not allowing undesired formatting (img, script, style), and we already have protection in place for that via lint-locale. 2. while a lot of surface area can be mitigated by using `Locale.TrString`, currently it's almost never used in templates, and we won't be able to convert all usages of `Locale.Tr` to `Locale.TrString` because (1), so it's not even worth trying 3. if we have to use `Locale.Tr`, we can't just have some html tags escaped and some preserved, it will have to be a policy similar to what we have applied to markdown in UGC. But bisecting between good and bad formatting in runtime can't be implemented without degrading performance, and it's not needed because of (1) The mitigations don't have to be implemented as either new Locale funcs or amended Locale funcs, the way of implementation simply doesn't matter, because it can't be done in a nice way because of (3).

snematoda approved these changes

2026-02-21 13:33:13 +01:00

snematoda left a comment

🫣 my bad... thank you for cleaning this up! ...and mahlzahn for reporting this!

fogti approved these changes

2026-02-21 13:57:45 +01:00

0ko added 1 commit

2026-02-22 11:22:28 +01:00

use pipes, add <code>

testing / semgrep/ci (pull_request) Successful in 12s

Details

testing / frontend-checks (pull_request) Successful in 59s

Details

testing / backend-checks (pull_request) Successful in 3m12s

Details

Integration tests for the release process / release-simulation (pull_request) Successful in 5m38s

Details

testing / test-unit (pull_request) Successful in 6m0s

Details

testing / test-remote-cacher (redict) (pull_request) Successful in 2m9s

Details

testing / test-remote-cacher (garnet) (pull_request) Successful in 2m9s

Details

testing / test-remote-cacher (redis) (pull_request) Successful in 2m10s

Details

testing / test-remote-cacher (valkey) (pull_request) Successful in 2m10s

Details

testing / test-e2e (pull_request) Successful in 18m23s

Details

testing / test-mysql (pull_request) Successful in 20m15s

Details

testing / test-sqlite (pull_request) Successful in 26m19s

Details

testing / test-pgsql (pull_request) Successful in 29m33s

Details

testing / security-check (pull_request) Successful in 58s

Details

milestone / set (pull_request_target) Successful in 5s

Details

requirements / merge-conditions (pull_request) Successful in 2s

Details

issue-labels / backporting (pull_request_target) Successful in 33s

Details

issue-labels / release-notes (pull_request_target) Successful in 23s

Details

62fb5d6275

0ko changed title from ~~fix(ui): hardcode sort options in search syntax hint, fix html~~ to fix(ui): hardcode sort options in search syntax hint, improve look

2026-02-22 11:23:44 +01:00

0ko merged commit f9a22b335e into forgejo

2026-02-23 06:03:25 +01:00

0ko referenced this pull request from a commit

2026-02-23 06:03:27 +01:00

fix(ui): hardcode sort options in search syntax hint, improve look (#11381)

forgejo-release-notes-assistant added this to the Forgejo v15.0.0 milestone

2026-02-23 06:03:54 +01:00

forgejo-backport-action referenced this pull request

2026-02-23 06:04:18 +01:00

[v14.0/forgejo] fix(ui): hardcode sort options in search syntax hint, improve look #11413

forgejo-backport-action referenced this pull request from a commit