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

Setting the mail sender's name, fix issue #448 #633

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@filewalkwithme

filewalkwithme commented Jun 5, 2014

Hi!

Gmail appears to be not accepting characters like "<" and ">" into the 'From' field (at the Send method).

However, gmail accepts the syntax From: SOME NAME <SOME@MAIL.COM> at message body.

So the fix is just send only the email(without alias and special chars) at Send method.

@filewalkwithme filewalkwithme changed the title from Sanitize 'From' field, fix issue #448 to Setting the mail sender's name, fix issue #448 Jun 5, 2014

@brendensoares brendensoares added this to the v0.11 milestone Jun 5, 2014

@brendensoares brendensoares added the bug label Jun 5, 2014

@brendensoares

This comment has been minimized.

Member

brendensoares commented Jun 5, 2014

Thanks. Have you confirmed this works? Any reference that explains why gmail does not accept that format? Looks like it's a common address format.

@pushrax

This comment has been minimized.

Contributor

pushrax commented Jun 5, 2014

The current version has been working fine for me for the past months with Gmail.

@filewalkwithme

This comment has been minimized.

filewalkwithme commented Jun 6, 2014

Hi fellows!!

With current version, the test code below returns syntax error:

func (self App) MailTest() revel.Result {
    mailer := &mail.Mailer{Server:"smtp.gmail.com", Port:587, UserName:"USER", Password:"PASS"}
    mailer.Sender = &mail.Sender{From:"FROM_NAME <FROM_EMAIL>", ReplyTo:"REPLYTO_NAME <REPLYTO_EMAIL>"}
    message := mail.NewTextMessage([]string{"TO_EMAIL"}, "Test Subject", "Test Body")
    if err := mailer.SendMessage(message); err != nil {
    // Error
    return self.RenderText(fmt.Sprintf("mail failed: %s", err))
    }
    return self.RenderText("mail sent")
}
mail failed: 555 5.5.2 Syntax error. p1smxyzvdp.17 - gsmtp

But, if we remove the FROM_NAME...

mailer.Sender = &mail.Sender{From:"FROM_EMAIL", ReplyTo:"REPLYTO_NAME <REPLYTO_EMAIL>"}    

The email will be sent with no returning errors.

I checked the RFC 5321, which specify the Simple Mail Transfer Protocol and found this
http://tools.ietf.org/html/rfc5321#section-3.3

The <reverse-path> portion of the first or
only argument contains the source mailbox (between "<" and ">"
brackets), which can be used to report errors (see Section 4.2 for a
discussion of error reporting). 

(....) Normally, failures produce 550 or 553 replies. (....) 

Historically, the <reverse-path> was permitted to contain more than
just a mailbox; however, contemporary systems SHOULD NOT use source
routing (see Appendix C).

That occurs at 'MAIL command'.

Note, that the message header "From:" accepts the format 'FROM_NAME <FROM_EMAIL>', as described by http://tools.ietf.org/html/rfc5322#section-3.4
This message header is set in writeRecipient().

So, my code proposes that we continue using &mail.Sender.From, but now we can set the Sender Name without messing with the 'MAIL command'.

Hope that it helps. =]

@pushrax

This comment has been minimized.

Contributor

pushrax commented Jun 6, 2014

Cool, makes sense. Your original post was a bit misleading since you had used <pre> instead of backticks (fixed it for you now), causing it to render as From: SOME NAME SOME@MAIL.COM.

Shouldn't we do the same for the c.Rcpt calls as well? I think the spec for forward-path is the same as reverse-path.

@pushrax

View changes

mail/connection.go Outdated
@@ -96,3 +97,27 @@ func addRcpt(c *smtp.Client, address []string) error {
}
return nil
}
// convert 'Sender <sender@abc.com>' into 'sender@abc.com'. fix issue #448
func sanitizeFrom(s string) string {

This comment has been minimized.

@pushrax

pushrax Jun 6, 2014

Contributor

I'd call this method extractAddress or something like that to be more specific. I'd call the argument from.

This comment has been minimized.

@filewalkwithme
@pushrax

View changes

mail/connection.go Outdated
@@ -96,3 +97,27 @@ func addRcpt(c *smtp.Client, address []string) error {
}
return nil
}
// convert 'Sender <sender@abc.com>' into 'sender@abc.com'. fix issue #448

This comment has been minimized.

@pushrax

pushrax Jun 6, 2014

Contributor

Go convention calls for title case sentences in comments. You can remove the fix issue #448 bit, it should be easy to find from the commit log.

This comment has been minimized.

@filewalkwithme
@pushrax

View changes

mail/connection.go Outdated
}
//if using html entities...
i = strings.Index(s, "&lt;")

This comment has been minimized.

@pushrax

pushrax Jun 6, 2014

Contributor

Does this ever happen? I think we can probably remove this case.

This comment has been minimized.

@filewalkwithme

filewalkwithme Jun 8, 2014

Hmmm, maybe I'm just a little bit paranoid, but I'm thinking about the cases where the email addresses come from a non sanitized database (eg.: the address is stored as '& lt;sender@abc.com& gt;')

If we let "& lt;" and "& gt;" the mail will return syntax error...

Or we should just let the user handle this?

This comment has been minimized.

@pushrax

pushrax Jun 10, 2014

Contributor

IMO this shouldn't be the concern of the framework.

This comment has been minimized.

@filewalkwithme

filewalkwithme Jun 10, 2014

Allright! I'll gonna change this, thanks!

@pushrax

View changes

mail/connection.go Outdated
func sanitizeFrom(s string) string {
i := strings.Index(s, "<")
if i > -1 {
sAlias := s[i:]

This comment has been minimized.

@pushrax

pushrax Jun 6, 2014

Contributor

addr might be a better name for sAlias

@brendensoares

This comment has been minimized.

Member

brendensoares commented Jun 19, 2014

Curious, why not just separate the sender's name and email address in the Sender struct (https://github.com/revel/revel/blob/master/mail/mailer.go#L21)?

PS - @maiconio we need you to target the develop branch per our contribution guidelines. Thanks :]

@brendensoares

This comment has been minimized.

Member

brendensoares commented Sep 10, 2014

@revel/core Why do we even have our own mailer? Why aren't using/supporting an existing package? Seems we're duplicating effort needlessly here. I see no benefit to having a Revel-specific mailer package.

I'm considering removing this altogether in favor of @jordan-wright's https://github.com/jordan-wright/email

@brendensoares brendensoares modified the milestones: v0.12, v0.11 Sep 10, 2014

@brendensoares brendensoares self-assigned this Sep 10, 2014

@jordan-wright

This comment has been minimized.

jordan-wright commented Sep 10, 2014

+1 for using the email package (though, I may be a little biased 😉)

@brendensoares brendensoares changed the title from Setting the mail sender's name, fix issue #448 to CANCELLED: Setting the mail sender's name, fix issue #448 Sep 10, 2014

@brendensoares

This comment has been minimized.

Member

brendensoares commented Oct 27, 2014

@pushrax @AnonX Do you have any feedback on removing the mail code from Revel and standardizing on the above mentioned email package?

@brendensoares brendensoares changed the title from CANCELLED: Setting the mail sender's name, fix issue #448 to Setting the mail sender's name, fix issue #448 Oct 27, 2014

@ghost

This comment has been minimized.

ghost commented Oct 28, 2014

@brendensoares, I totally agree with your arguments:

Why aren't using/supporting an existing package? Seems we're duplicating effort needlessly here. I see no benefit to having a Revel-specific mailer package.

I'm 👍 to use existing third party components for Revel as long as they satisfy the following conditions:

  1. The code is high quality
  2. It is tested, CI is used
  3. The project is active
    I haven't used jordan-wright/email yet but it looks good to me, the code is gofmted, linted, and tested. The only thing I'm a bit concerned about is PRs which are dated the beginning of August/September and still having no any answer at all.
@brendensoares

This comment has been minimized.

Member

brendensoares commented Oct 28, 2014

@AnonX thanks for the validation.

As far as the aged PR's, maybe we should setup the Revel Github organization as a sort of Apache Foundation and offer to support under-staffed open source Go code ;)

@jordan-wright could you use our help?

@jordan-wright

This comment has been minimized.

jordan-wright commented Oct 28, 2014

Let me take a look at those PR's. I've been swamped the past few months with other projects but I'll address them soon.

Regardless, the core API should be stable. At this point, I won't introduce any breaking changes - and each of these PR's just seem to be convenience functions to add to the API.

@pushrax

This comment has been minimized.

Contributor

pushrax commented Oct 28, 2014

@brendensoares killing the mail code in Revel is fine with me. (if you ask me about deleting code 99% of the time I will say it's a good idea :P)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment