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.

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

Fix XSS vulnerabilities #29336

Merged
merged 7 commits into from Feb 22, 2024
Merged

Fix XSS vulnerabilities #29336

merged 7 commits into from Feb 22, 2024

Conversation

6543
Copy link
Member

@6543 6543 commented Feb 22, 2024

  • The Wiki page did not sanitize author name
  • the reviewer name on a "dismiss review" comment is also affected
  • the migration page has some spots

Gusted and others added 4 commits February 22, 2024 22:49
- On the wiki and revisions page, information is shown about the last
commit that modified that wiki page. This includes the time it was last
edited and by whom. That whole string is not being sanitized (passed
trough `Safe` in the templates), because the last edited bit is
formatted as an HTML element and thus shouldn't be sanitized. The
problem with this is that now `.Author.Name` is not being sanitized.
- This can be exploited, the names of authors and commiters on a Git
commit is user controlled, they can be any value and thus also include
HTML. It's not easy to actually exploit this, as you cannot use the
official git binary to do use, as they actually strip `<` and `>` from
user names (trivia: this behaviour was introduced in the initial commit
of Git). In the integration testing, go-git actually has to generate
this commit as they don't have such restrictions.
- Pass `.Author.Name` trough `Escape` in order to be sanitized.
- Adds integration testing.
- It's possible for reviews to not be assiocated with users, when they
were migrated from another forge instance. In the migration code,
there's no sanitization check for author names, so they could contain
HTML tags and thus needs to be properely escaped.
- Pass `$reviewerName` trough `Escape`.
- Adds integration testing.
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@6543 6543 added type/bug topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Feb 22, 2024
@6543 6543 added this to the 1.21.6 milestone Feb 22, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2024
@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 22, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 22, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 22, 2024
@6543 6543 merged commit 4435d8a into go-gitea:release/v1.21 Feb 22, 2024
26 of 27 checks passed
@6543 6543 deleted the xss_vuln branch February 22, 2024 22:37
LeSuisse added a commit to LeSuisse/nixpkgs that referenced this pull request Feb 23, 2024
Patch has been slightly adjusted to be applied on top of 1.20.6.

Upstream PR:
go-gitea/gitea#29336
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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants