Refactor locale&string&template related code by wxiaoguang · Pull Request #29165 · go-gitea/gitea

wxiaoguang · 2024-02-14T07:13:56Z

Clarify when "string" should be used (and be escaped), and when "template.HTML" should be used (no need to escape)

And help PRs like #29059 , to render the error messages correctly.

The only possible regression bug is that something on the UI might get double-escaped, which is pretty easy to fix (and I will fix them in first time, as always). Update: if such regression happens, it should be the old code's bug, which needs to be fixed accordingly.

modules/context/context.go

KN4CK3R · 2024-02-14T14:18:19Z

Wouldn't it be easier to add a TrHtml method? Then you don't need most changes and we have no potential problem with current PRs which would use the wrong method.

wxiaoguang · 2024-02-14T14:20:31Z

Wouldn't it be easier to add a TrHtml method? Then you don't need most changes and we have no potential problem with current PRs which would use the wrong method.

No, the real problem is that Tr should return template.HTML in templates, otherwise you need to change much more template code ....

routers/web/repo/view.go

modules/auth/password/password.go

delvh · 2024-02-14T16:25:54Z

No, the real problem is that Tr should return template.HTML in templates, otherwise you need to change much more template code ....

Yes, but doesn't this enable XSS attacks?
If I understand this correctly, we are now responsible ourselves for ensuring that any parameter is safe to use…

delvh · 2024-02-14T16:26:35Z

I don't understand how this makes things safer, for me it seems less safe…

wxiaoguang · 2024-02-14T16:40:52Z

@delvh

Yes, but doesn't this enable XSS attacks?
I don't understand how this makes things safer, for me it seems less safe…

Why and how? Do you have an example to break it?

If I understand this correctly, we are now responsible ourselves for ensuring that any parameter is safe to use…

No, every argument is under control now. Every string gets "escaped".

delvh · 2024-02-14T16:43:06Z

Yep, I've noticed what you mean by looking further through your PR.
Now, we simply penalize having safe parameters by potentially double escaping them.
I don't like this write overhead for safe parameters, but it is much better than a security vulnerability.

wxiaoguang · 2024-02-14T16:46:14Z

I don't like this write overhead for safe parameters

There is no much difference for the "|Safe" modifier (comparing to before, I think the new approach is indeed better).

delvh · 2024-02-14T21:57:08Z

Backport?

wxiaoguang · 2024-02-14T22:00:24Z

Backport?

No idea (maybe I slightly prefer no), it doesn't seems easy to backport and usually we don't backport unnecessary refactoring PRs. If most people would like to, I could try.

Clarify when "string" should be used (and be escaped), and when "template.HTML" should be used (no need to escape) And help PRs like go-gitea#29059 , to render the error messages correctly.

* giteaofficial/main: (23 commits) Remove jQuery from SSH key form parser (go-gitea#29193) Refactor request function (go-gitea#29187) Docker Tag Information in Docs (go-gitea#29047) Fix gitea-action user avatar broken on edited menu (go-gitea#29190) Disable parallel Make execution (go-gitea#29186) Auto-update the system status in admin dashboard (go-gitea#29163) Avoid vue warning in dev mode (go-gitea#29188) Update JS and PY dependencies (go-gitea#29184) [skip ci] Updated translations via Crowdin Implement contributors graph (go-gitea#27882) Add support for action artifact serve direct (go-gitea#29120) Advertise WebAuthn support (go-gitea#29176) Tweak repo header (go-gitea#29134) Change webhook-type in create-view (go-gitea#29114) Remove jQuery from the comment task list (go-gitea#29170) Fix can not select team reviewers when reviewers is empty (go-gitea#29174) move sign in labels to be above inputs (go-gitea#28753) Refactor locale&string&template related code (go-gitea#29165) Extract linguist code to method (go-gitea#29168) bump to use go 1.22 (go-gitea#29119) ...

…plates (#29227) Follow #29165. These HTML strings are safe to be rendered directly, to avoid double-escaping.

Follow #29165. * Introduce JSONTemplate to help to render JSON templates * Introduce JSEscapeSafe for templates. Now only use `{{ ... | JSEscape}}` instead of `{{ ... | JSEscape | Safe}}` * Simplify "UserLocationMapURL" useage

Follow #29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

Clarify when "string" should be used (and be escaped), and when "template.HTML" should be used (no need to escape) And help PRs like go-gitea#29059 , to render the error messages correctly.

…plates (go-gitea#29227) Follow go-gitea#29165. These HTML strings are safe to be rendered directly, to avoid double-escaping.

Follow go-gitea#29165. * Introduce JSONTemplate to help to render JSON templates * Introduce JSEscapeSafe for templates. Now only use `{{ ... | JSEscape}}` instead of `{{ ... | JSEscape | Safe}}` * Simplify "UserLocationMapURL" useage

Follow go-gitea#29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

Follow #29165

Follow #29165 * some of them are incorrect, which would lead to double escaping (eg: `(print (Escape $.RepoLink)`) * other of them are not necessary, because `Tr` handles strings&HTML automatically Suggest to review by "unified view": https://github.com/go-gitea/gitea/pull/29394/files?diff=unified&w=0

Clarify when "string" should be used (and be escaped), and when "template.HTML" should be used (no need to escape) And help PRs like go-gitea#29059 , to render the error messages correctly. (cherry picked from commit f3eb835) Conflicts: modules/web/middleware/binding.go routers/web/feed/convert.go tests/integration/branches_test.go tests/integration/repo_branch_test.go trivial context conflicts

…plates (go-gitea#29227) Follow go-gitea#29165. These HTML strings are safe to be rendered directly, to avoid double-escaping. (cherry picked from commit a784ed3)

Follow go-gitea#29165. * Introduce JSONTemplate to help to render JSON templates * Introduce JSEscapeSafe for templates. Now only use `{{ ... | JSEscape}}` instead of `{{ ... | JSEscape | Safe}}` * Simplify "UserLocationMapURL" useage (cherry picked from commit 31bb9f3)

Follow go-gitea#29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me> (cherry picked from commit 4345cac)

github-actions · 2024-03-01T00:03:59Z

Automatically locked because of our CONTRIBUTING guidelines

wxiaoguang added this to the 1.22.0 milestone Feb 14, 2024

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 14, 2024

pull-request-size bot added the size/XL label Feb 14, 2024

github-actions bot added the modifies/api This PR adds API routes or modifies them label Feb 14, 2024

wxiaoguang mentioned this pull request Feb 14, 2024

Fix bug to go to 404 when creating pull request #29059

Closed

wxiaoguang force-pushed the refactor-web-locale branch 2 times, most recently from dee6c5d to 4ae600f Compare February 14, 2024 07:35

wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 14, 2024

wxiaoguang force-pushed the refactor-web-locale branch from 4ae600f to 74a09e6 Compare February 14, 2024 08:05

lunny reviewed Feb 14, 2024

View reviewed changes

modules/context/context.go Show resolved Hide resolved

refactor

33c6f23

wxiaoguang force-pushed the refactor-web-locale branch from 74a09e6 to 33c6f23 Compare February 14, 2024 13:47

wxiaoguang added 3 commits February 14, 2024 21:48

fix JSONError

3a5c9a7

make Flash message more strict

4150d82

revert unnecessary changes

83c28c0

lunny reviewed Feb 14, 2024

View reviewed changes

routers/web/repo/view.go Outdated Show resolved Hide resolved

wxiaoguang added 2 commits February 14, 2024 22:33

fix test

e84954b

revert to old code for ctx.NotFound

32f0cd3

lunny reviewed Feb 14, 2024

View reviewed changes

modules/auth/password/password.go Show resolved Hide resolved

lunny approved these changes Feb 14, 2024

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 14, 2024

delvh changed the title ~~Refactor locale&string&template related code~~ Remove possibility for XSS attacks Feb 14, 2024

wxiaoguang merged commit f3eb835 into go-gitea:main Feb 14, 2024

wxiaoguang deleted the refactor-web-locale branch February 14, 2024 21:49

This was referenced Feb 17, 2024

Use "Safe" modifier for manually constructed safe HTML strings in templates #29227

Merged

Improve TrHTML and add more tests #29228

Merged

lunny pushed a commit that referenced this pull request Feb 18, 2024

Use "Safe" modifier for manually constructed safe HTML strings in tem…

a784ed3

…plates (#29227) Follow #29165. These HTML strings are safe to be rendered directly, to avoid double-escaping.

wxiaoguang mentioned this pull request Feb 18, 2024

Refactor more code in templates #29236

Merged

wxiaoguang added a commit that referenced this pull request Feb 18, 2024

Improve TrHTML and add more tests (#29228)

4345cac

Follow #29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024

Improve TrHTML and add more tests (go-gitea#29228)

b6b52ed

Follow go-gitea#29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

This was referenced Feb 22, 2024

Remove unnecessary "Safe" modifier from templates #29318

Merged

Remove unnecessary "Str2html" modifier from templates #29319

Merged

wxiaoguang added a commit that referenced this pull request Feb 22, 2024

Remove unnecessary "Safe" modifier from templates (#29318)

7a1557d

Follow #29165

wxiaoguang added a commit that referenced this pull request Feb 22, 2024

Remove unnecessary "Str2html" modifier from templates (#29319)

c9d0e63

Follow #29165

6543 added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Feb 22, 2024

This was referenced Feb 25, 2024

Refactor Safe modifier #29392

Merged

Remove incorrect and unnecessary Escape from templates #29394

Merged

6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024

Improve TrHTML and add more tests (go-gitea#29228)

d611fe8

Follow go-gitea#29165. Co-authored-by: KN4CK3R <admin@oldschoolhack.me> (cherry picked from commit 4345cac)

github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor locale&string&template related code#29165

Refactor locale&string&template related code#29165
wxiaoguang merged 7 commits intogo-gitea:mainfrom
wxiaoguang:refactor-web-locale

wxiaoguang commented Feb 14, 2024 •

edited

Loading

Uh oh!

Uh oh!

KN4CK3R commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

delvh commented Feb 14, 2024 •

edited

Loading

Uh oh!

wxiaoguang commented Feb 14, 2024

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 •

edited

Loading

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

wxiaoguang commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KN4CK3R commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

delvh commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Feb 14, 2024

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delvh commented Feb 14, 2024

Uh oh!

wxiaoguang commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wxiaoguang commented Feb 14, 2024 •

edited

Loading

wxiaoguang commented Feb 14, 2024 •

edited

Loading

delvh commented Feb 14, 2024 •

edited

Loading

wxiaoguang commented Feb 14, 2024 •

edited

Loading

wxiaoguang commented Feb 14, 2024 •

edited

Loading