Skip to content

‏‏‎ #22041

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

Closed
wants to merge 1 commit into from
Closed

‏‏‎ #22041

wants to merge 1 commit into from

Conversation

quinndiggitypolymath
Copy link

@quinndiggitypolymath quinndiggitypolymath commented Dec 5, 2022

‏‏‎

@zeripath
Copy link
Contributor

zeripath commented Dec 5, 2022

Thanks for the PR but you really need to give it a MUCH BETTER title and describe the PR in a bit more detail.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 5, 2022
@quinndiggitypolymath quinndiggitypolymath changed the title #19852 #19852 (Valid email addresses are rejected with The email address is invalid, entirely preventing the use of gitea) Dec 5, 2022
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

I don't think we should allow an email like 我的$#[email protected].

@silverwind
Copy link
Member

I don't agree with completely removing validation, but I would agree to using golang stdlib email validation which I suppose should support everything you want.

@quinndiggitypolymath quinndiggitypolymath deleted the quinn/RFC_5322_3696 branch December 7, 2022 08:42
@eeyrjmr
Copy link
Contributor

eeyrjmr commented Dec 7, 2022

...

Except the one thing that you haven't provided a rationale for is what it originally aimed to mitigate.

https://github.com/go-gitea/gitea/blame/main/models/user/email_address.go -> #17688

A concern was raised with just a simple RFC_5322 check ( that ParseAddress does). The goal was to restrict the valid emails and an aim to refine in the future.
This PR undoes it with a simple "its too restrictive" stance with no thought into maybe it needs to be restrictive so either the premise in #17688 is flawed and it should be fully reverted or the concerns are valid and this needs to be bolstered .

One compromise is to load the regex string from config and thus if people do not want to restrict the email they have the flexibility

@lunny
Copy link
Member

lunny commented Dec 7, 2022

#22041 (comment)

I have no interest in continuing with Gitea in any form (this is not a healthy open source project), but the obvious answer to concerns about specific sets of unicode characters is to limit to the highly restrictive set:

Rather than simply blocking valid email addresses that have been happily used across all widely deployed MTA for ~20 years now, or otherwise just saying Chinese characters (or valid ascii characters) should be discriminated against simply for the hell of it:

image

I'm sorry if I made a mistake. I mean no unicode in the email address and I just post an example there not against any language. And since the issue has been mentioned many times I think you should read more previous issues or PRs before you post comments or PRs so that you can know the context.

@quinndiggitypolymath quinndiggitypolymath changed the title #19852 (Valid email addresses are rejected with The email address is invalid, entirely preventing the use of gitea) ‏‏‎ Dec 7, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 7, 2022

I think we should be careful here to note that git will allow these extremely weird email addresses and Gitea will just use them.

So by having this super-restrictive pattern we're not preventing weird and ambiguous email addresses from appearing in Gitea - just preventing the user from saying that one belongs to them.

Further, the potential problem of email addresses being ambiguous/confusable with another user isn't really an issue as the Gitea will not show the email address and will show the user that they map to instead. Thus unicode ambiguity of email addresses should only affect the user who the ambiguous email address belongs to.

Next we should consider if there are potential sec issues by allowing arbitrary email addresses.

  1. Sendmail - preventing an initial - seems to be all that is needed.
  2. We don't appear to display the email in any email templates - so I don't think there's a problem there.
  3. Otherwise the only places that the email is shown is on the user's own settings page or when they try to login with this.

As far as I can see the only person affected by Gitea allowing users to register their own weird email address is the user itself.

And thus apart from blocking the initial - I can't see a good reason for further restriction beyond RFC5322

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants