Skip to content

use proper subaddress#36639

Merged
techknowlogick merged 3 commits intogo-gitea:mainfrom
TheFox0x7:use-proper-subaddressing
Feb 16, 2026
Merged

use proper subaddress#36639
techknowlogick merged 3 commits intogo-gitea:mainfrom
TheFox0x7:use-proper-subaddressing

Conversation

@TheFox0x7
Copy link
Copy Markdown
Contributor

followup to #36550

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 15, 2026
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Feb 15, 2026
@TheFox0x7
Copy link
Copy Markdown
Contributor Author

FYI @Enzime

@silverwind
Copy link
Copy Markdown
Member

This comment was written by an AI assistant (Claude Code).

Review of PR #36639 — "use proper subaddress"

This PR swaps the noreply email format from user+id@noreply to id+user@noreply, aligning with GitHub's format (ID+USERNAME@users.noreply.github.com). Overall, the change is correct and well-scoped.

Code correctness

The else branch addition in parseLocalPartToNameID is necessary and correct. After swapping the strings.Cut return values, when there is no + separator, the entire local-part ends up in idstr (previously it ended up in name). Without the else { name = idstr } fix, a plain username@noreply.host address would incorrectly return an empty name. Good catch.

Backward compatibility

The intermediate format introduced by #36550 (user+id@noreply) will no longer parse correctly — with the new code, user+id would attempt ParseInt("user") which fails, then return name="id" and id=0. Since #36550 hasn't shipped in a release yet, this is acceptable. Worth noting for anyone running main in production though.

The oldest format (username@noreply.host, no +) continues to work correctly.

Comparison with GitHub's implementation

Aspect GitHub Gitea (after this PR)
Format ID+USERNAME@users.noreply.github.com ID+username@<NoReplyAddress>
Legacy format USERNAME@users.noreply.github.com (still recognized) USERNAME@<NoReplyAddress> (still recognized)
Lookup priority ID first, then username ID first, then username (same)

One behavioral difference from GitHub worth being aware of: GitHub explicitly handles username renames — commits with the old USERNAME@users.noreply.github.com format continue to be attributed to the account even after a rename (since the old username stays reserved for a period). In Gitea, the plain-username lookup (GetUserByName) will resolve to whoever currently holds that username. This is pre-existing behavior not introduced by this PR, but it's one of the reasons GitHub moved to the ID+USERNAME format in the first place — the ID provides stable attribution across renames.

LGTM — the change is minimal, tests are updated to match, and the format now matches GitHub's convention.

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Feb 15, 2026

Might be worth implementing the user rename logic, e.g. keep original username in email after rename, if it's not too complex to implement.

Copy link
Copy Markdown
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

lgtm in any case

@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 16, 2026
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Feb 16, 2026

Might be worth implementing the user rename logic, e.g. keep original username in email after rename, if it's not too complex to implement.

Actually I'm not sure the value of this. With ID present, there is no actual risk of impersonification attacks because a user ID can not be changed on any exposed API, to my knowledge.

@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 16, 2026
@techknowlogick techknowlogick enabled auto-merge (squash) February 16, 2026 00:25
@techknowlogick techknowlogick added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Feb 16, 2026
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 16, 2026
@techknowlogick techknowlogick merged commit 08d9845 into go-gitea:main Feb 16, 2026
24 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Feb 16, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 16, 2026
silverwind pushed a commit to Excellencedev/gitea that referenced this pull request Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants