Skip to content

FIX: URL sanitization to handle schemeless credentials#37440

Merged
wxiaoguang merged 18 commits intogo-gitea:mainfrom
bircni:fix/sanitization
Apr 28, 2026
Merged

FIX: URL sanitization to handle schemeless credentials#37440
wxiaoguang merged 18 commits intogo-gitea:mainfrom
bircni:fix/sanitization

Conversation

@bircni
Copy link
Copy Markdown
Member

@bircni bircni commented Apr 26, 2026

Fixes #37435

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2026
@bircni bircni changed the title Enhance URL sanitization to handle schemeless credentials FIX: URL sanitization to handle schemeless credentials Apr 26, 2026
@bircni bircni requested a review from wxiaoguang April 26, 2026 17:52
@bircni bircni marked this pull request as ready for review April 26, 2026 17:56
@bircni bircni requested a review from lunny April 26, 2026 17:58
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 26, 2026

Theoretically speaking, if you have introduced the new regexp, then you don't need to keep the old algorithm

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 26, 2026

Theoretically speaking, if you have introduced the new regexp, then you don't need to keep the old algorithm

yep just had a look at it again and that also makes it easier

bircni and others added 2 commits April 26, 2026 21:01
@bircni
fixes
1d3bafb
@silverwind @claude
remove duplicate % in userinfo regex char classes
7fecb15 
Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@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 Apr 26, 2026
@wxiaoguang wxiaoguang marked this pull request as draft April 26, 2026 20:54
@wxiaoguang wxiaoguang self-assigned this Apr 26, 2026
@bircni
Copy link
Copy Markdown
Member Author

bircni commented Apr 26, 2026

@silverwind why you remove the % ??

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Apr 26, 2026 •
edited
Loading

Because it was duplicate in the [] character set and therefor no-op. [%%] matches the same things as [%], e.g. a single % character.

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Apr 26, 2026

Found some edge cases, I think regexp is not easy to be right. Will try to improve.

@wxiaoguang wxiaoguang requested review from Copilot and removed request for wxiaoguang April 26, 2026 21:30
@wxiaoguang wxiaoguang marked this pull request as ready for review April 26, 2026 21:33
Copilot AI reviewed Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses credential exposure risks by improving URL/userinfo sanitization so that credential-like substrings (including schemeless user:token@host forms seen in git stderr) are masked before being surfaced in logs/UI errors.

Changes:

  • Sanitize repo.CloneURL before logging during repository migration.
  • Rework util.SanitizeCredentialURLs to mask schemeless credential patterns and update the masking placeholder.
  • Update/add unit tests and expand git command log sanitization coverage for schemeless a:b@c patterns.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
services/migrations/migrate.go Sanitizes the clone URL before trace logging during migration.
modules/util/sanitize.go Updates sanitization logic to catch schemeless credential patterns and standardizes the placeholder.
modules/util/sanitize_test.go Adjusts/adds test cases for the updated sanitization behavior.
modules/git/gitcmd/command.go Ensures arguments are passed through the sanitization routine when building debug log strings.
modules/git/gitcmd/command_test.go Updates expectations for the new placeholder and adds coverage for schemeless a:b@c/.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread modules/util/sanitize.go
Comment thread services/migrations/migrate.go
Comment thread modules/util/sanitize.go
Comment thread modules/util/sanitize.go Outdated
@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 Apr 27, 2026
@lunny lunny added the type/bug label Apr 27, 2026
@silverwind silverwind self-requested a review April 27, 2026 21:54
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Apr 27, 2026
@silverwind @claude
sanitize: fix IPv6 host parsing and fast-path SSH-URL leak
69003ad 
- Consume "[...]" IP-literals as a single host token. The previous switch
  arms for `[`/`]` were dead: at `sepAtPos+1` the `[` fell through, and
  the next `:` broke the host loop. Output looked correct only because
  the unparsed tail was appended verbatim.
- Replace the fast path's `sepColPos > sepAtPos` check (which used only
  the first `@` and `:`) with an existence test on both. Lines containing
  a `git@host:path` SSH URL followed by a credential URL were skipped.
- Add tests for `[2001:db8::1]:8080`, multi-URL with `[::1]`, unmatched
  `[`, and SSH-URL-then-HTTPS.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@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 Apr 27, 2026
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Apr 28, 2026

@wxiaoguang check 69003ad please.

@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 28, 2026
@silverwind silverwind enabled auto-merge (squash) April 28, 2026 04:20
@wxiaoguang wxiaoguang disabled auto-merge April 28, 2026 04:55
@wxiaoguang wxiaoguang merged commit fedc9dc into go-gitea:main Apr 28, 2026
26 checks passed
@GiteaBot GiteaBot added this to the 1.27.0 milestone Apr 28, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 28, 2026
@GiteaBot GiteaBot mentioned this pull request Apr 28, 2026
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 28, 2026
lunny pushed a commit that referenced this pull request Apr 28, 2026
@GiteaBot @bircni
@silverwind @claude @wxiaoguang
FIX: URL sanitization to handle schemeless credentials (#37440) (#37471)
4ee74d7 
Backport #37440 by @bircni

Fixes #37435

Co-authored-by: Nicolas <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.7) <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 29, 2026
@zjjhot
Merge branch 'main' into mainzjj
277bb5d 
* main:
  Add DEFAULT_TITLE_SOURCE setting for pull request title default behavior (go-gitea#37465)
  Fix compare dropdown for branches without common history (go-gitea#37470)
  FIX: URL sanitization to handle schemeless credentials (go-gitea#37440)
  Refactor pull request view (4) (go-gitea#37451)
  Fix scheduled action panic with null event payload (go-gitea#37459)
  Fix attachment Content-Security-Policy (go-gitea#37455)
  [skip ci] Updated translations via Crowdin
  Rename CurrentRefPath to CurrentRefSubURL (go-gitea#37453)
  Clean up org pages layout (go-gitea#37445)
  Fix script error alert (go-gitea#37458)
  Fix inconsistent disabled styling on logged-out repo header buttons (go-gitea#37406)
  Add API endpoint to reply to pull request review comments (go-gitea#36683)
  Add CurrentURL template variable back (go-gitea#37444)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@lunny lunny lunny approved these changes

@silverwind silverwind silverwind approved these changes

Assignees

@wxiaoguang wxiaoguang

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

Gitea Credential Exposure via git:// URL vulnerability

6 participants

@bircni @wxiaoguang @silverwind @lunny @GiteaBot