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

Refactor locale&string&template related code #29165

Merged
merged 7 commits into from
Feb 14, 2024
Merged

Refactor locale&string&template related code #29165

merged 7 commits into from
Feb 14, 2024

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Feb 14, 2024 •
edited
Loading

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.

@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Feb 14, 2024
@GiteaBot 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 pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 14, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Feb 14, 2024
@wxiaoguang wxiaoguang mentioned this pull request Feb 14, 2024
Closed
@wxiaoguang wxiaoguang force-pushed the refactor-web-locale branch 2 times, most recently from dee6c5d to 4ae600f Compare February 14, 2024 07:35
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 14, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 14, 2024

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
Copy link
Contributor Author

wxiaoguang commented Feb 14, 2024 •
edited
Loading

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 ....

lunny
lunny reviewed Feb 14, 2024
View reviewed changes
routers/web/repo/view.go Outdated Show resolved Hide resolved
@GiteaBot 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
Copy link
Member

delvh commented Feb 14, 2024

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
Copy link
Member

delvh commented Feb 14, 2024 •
edited
Loading

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

@wxiaoguang
Copy link
Contributor Author

@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
Copy link
Member

delvh commented Feb 14, 2024

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
Copy link
Contributor Author

wxiaoguang commented Feb 14, 2024 •
edited
Loading

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 delvh changed the title Refactor locale&string&template related code Remove possibility for XSS attacks Feb 14, 2024
@wxiaoguang wxiaoguang merged commit f3eb835 into go-gitea:main Feb 14, 2024
26 checks passed
@wxiaoguang wxiaoguang deleted the refactor-web-locale branch February 14, 2024 21:49
@delvh
Copy link
Member

delvh commented Feb 14, 2024

Backport?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 14, 2024 •
edited
Loading

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.

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 15, 2024
@wxiaoguang @silverwind
Refactor locale&string&template related code (go-gitea#29165)
21c428f 
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.
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 16, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
1688a0c 
* 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)
  ...
This was referenced Feb 17, 2024
Merged
Merged
lunny pushed a commit that referenced this pull request Feb 18, 2024
@wxiaoguang
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 wxiaoguang mentioned this pull request Feb 18, 2024
Merged
KN4CK3R pushed a commit that referenced this pull request Feb 18, 2024
@wxiaoguang
Refactor more code in templates (#29236)
31bb9f3 
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
wxiaoguang added a commit that referenced this pull request Feb 18, 2024
@wxiaoguang @KN4CK3R
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
@wxiaoguang @silverwind
Refactor locale&string&template related code (go-gitea#29165)
2bbd9ac 
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.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@wxiaoguang @silverwind
Use "Safe" modifier for manually constructed safe HTML strings in tem…
15278e4 
…plates (go-gitea#29227)

Follow go-gitea#29165. These HTML strings are safe to be rendered directly, to
avoid double-escaping.
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@wxiaoguang @silverwind
Refactor more code in templates (go-gitea#29236)
6069337 
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
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@wxiaoguang @KN4CK3R @silverwind
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
Merged
Merged
wxiaoguang added a commit that referenced this pull request Feb 22, 2024
@wxiaoguang
Remove unnecessary "Safe" modifier from templates (#29318)
7a1557d 
Follow #29165
wxiaoguang added a commit that referenced this pull request Feb 22, 2024
@wxiaoguang
Remove unnecessary "Str2html" modifier from templates (#29319)
c9d0e63 
Follow #29165
@6543 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
Merged
Merged
lunny pushed a commit that referenced this pull request Feb 25, 2024
@wxiaoguang
Remove incorrect and unnecessary Escape from templates (#29394)
d2f6588 
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
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@wxiaoguang @earl-warren
Refactor locale&string&template related code (go-gitea#29165)
6524894 
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
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@wxiaoguang @earl-warren
Use "Safe" modifier for manually constructed safe HTML strings in tem…
bdf4707 
…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)
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@wxiaoguang @earl-warren
Refactor more code in templates (go-gitea#29236)
81925eb 
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)
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@wxiaoguang @earl-warren
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 GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wxiaoguang @KN4CK3R @delvh @lunny @GiteaBot @6543