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

Include username in email headers #28981

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

gwymor
Copy link
Contributor

@gwymor gwymor commented Jan 29, 2024

Emails from Gitea comments do not contain the username of the commenter
anywhere, only their display name, so it is not possible to verify who
made a comment from the email itself:

From: "Alice" <email@gitea>
X-Gitea-Sender: Alice
X-Gitea-Recipient: Bob
X-GitHub-Sender: Alice
X-GitHub-Recipient: Bob

This comment looks like it's from @alice.

The X-Gitea/X-GitHub headers also use display names, which is not very
reliable for filtering, and inconsistent with GitHub's behavior:

X-GitHub-Sender: lunny
X-GitHub-Recipient: gwymor

This change includes both the display name and username in the From
header, and switches the other headers from display name to username:

From: "Alice (@fakealice)" <email@gitea>
X-Gitea-Sender: fakealice
X-Gitea-Recipient: bob
X-GitHub-Sender: fakealice
X-GitHub-Recipient: bob

This comment looks like it's from @alice.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 29, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 29, 2024
msg := NewMessageFrom(recipient.Email, ctx.Doer.DisplayName(), setting.MailService.FromEmail, subject, mailBody.String())
msg := NewMessageFrom(
recipient.Email,
fmt.Sprintf("%s (@%s)", ctx.Doer.DisplayName(), ctx.Doer.Name),
Copy link
Member

Choose a reason for hiding this comment

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

I think this value should be offered as a method in /models/user/user.go instead.
Perhaps something like GetCompleteName?
That way, it can be used by other parts as well.

@delvh delvh added the type/enhancement An improvement of existing functionality label Jan 31, 2024
Emails from Gitea comments do not contain the username of the commenter
anywhere, only their display name, so it is not possible to verify who
made a comment from the email itself:

	From: "Alice" <email@gitea>
	X-Gitea-Sender: Alice
	X-Gitea-Recipient: Bob
	X-GitHub-Sender: Alice
	X-GitHub-Recipient: Bob

	This comment looks like it's from @alice.

The X-Gitea/X-GitHub headers also use display names, which is not very
reliable for filtering, and inconsistent with GitHub's behavior:

	X-GitHub-Sender: lunny
	X-GitHub-Recipient: gwymor

This change includes both the display name and username in the From
header, and switches the other headers from display name to username:

	From: "Alice (@fakeAlice)" <email@gitea>
	X-Gitea-Sender: fakealice
	X-Gitea-Recipient: bob
	X-GitHub-Sender: fakealice
	X-GitHub-Recipient: bob

	This comment looks like it's from @alice.
@gwymor gwymor force-pushed the username-in-email-headers branch from 0faf27f to 6d8f2e1 Compare January 31, 2024 21:06
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 31, 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 2, 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 2, 2024
@lafriks lafriks enabled auto-merge (squash) February 3, 2024 00:17
@lafriks lafriks merged commit 360b3fd into go-gitea:main Feb 3, 2024
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Feb 3, 2024
// GetCompleteName returns the the full name and username in the form of
// "Full Name (@username)" if full name is not empty, otherwise it returns
// "@username".
func (u *User) GetCompleteName() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is quite confusing to introduce this "common" function into the User model.

For example:

  • There is already GetDisplayName and DisplayName
  • Why the GetCompleteName has an @ , is it a general approch .....

(I know the old code is not good, but introducing a new GetCompleteName here makes things more complex ....)

zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 4, 2024
* giteaofficial/main:
  Add `must-change-password` cli parameter (go-gitea#27626)
  Include username in email headers (go-gitea#28981)
  Update tool dependencies (go-gitea#29030)
  Add artifacts v4 jwt to job message and accept it (go-gitea#28885)
  Pass es2020 to esbuild-loader as well (go-gitea#29027)
  Fix default avatar image size in PR diff page (go-gitea#28971)
  Update JS and PY dependencies, build for `es2020` browsers (go-gitea#28977)
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 8, 2024

And maybe it should be reverted (or fixed):

image

@gwymor
Copy link
Contributor Author

gwymor commented Feb 8, 2024

@wxiaoguang What displays that message? Are you aware of email providers/stacks that will take that as a sign of spam/phishing and flag the message? I would hope nothing would make that mistake as there's no user-part before the @.

If email providers do, we should probably drop the @, and display the sender as Display Name (username).

When a user has no display name the sender currently falls back to @username. If we change that to just username, I'm afraid that doesn't make it clear enough that this is a username that cannot be spoofed (aside from inspecting the X-Gitea/GitHub-Sender headers, which I think is too much to require a user to do in a spoofing attempt) -- you can't easily tell the difference between a post-1.22 Gitea sending a username, and a pre-1.22 Gitea sending a displayname that looks like a username. We could remove the fallback code, and always show Display Name (username), which will fall back to username (username). That would be more consistent and easy to verify at a glance, but perhaps noisy.

@wxiaoguang
Copy link
Contributor

@wxiaoguang What displays that message? Are you aware of email providers/stacks that will take that as a sign of spam/phishing and flag the message? I would hope nothing would make that mistake as there's no user-part before the @.

I copied this screenshot from a user's feedback.

I think the root problem is that using @ in some places isn't a good security practice .....

(ps: I just shared the information here, but I haven't really looked into the problem nor I need this feature 🤣 )

@techknowlogick
Copy link
Member

I don't want this to get lost, so I'm gonna open a new issue about it, and lock this thread. I'll ping everyone here, over there.

@go-gitea go-gitea locked and limited conversation to collaborators Feb 9, 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants