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

net/smtp: possible typo in Client.Rcpt #17036

Closed
dtelyukh opened this issue Sep 9, 2016 · 8 comments
Closed

net/smtp: possible typo in Client.Rcpt #17036

dtelyukh opened this issue Sep 9, 2016 · 8 comments
Milestone

Comments

@dtelyukh
Copy link

dtelyukh commented Sep 9, 2016

https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L256
c.cmd(25, "RCPT TO:<%s>", to)

I guess it must be 250, but not 25. Am I right?

@bradfitz bradfitz changed the title Possible typo in net/smtp net/smtp: possible typo in Client.Rcpt Sep 9, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Yes, looks like it.

Why did nobody (or no tests) notice this before?

Does Rcpt return an error now, but nobody checks the error?

@bradfitz bradfitz added this to the Go1.8 milestone Sep 9, 2016
@mattn
Copy link
Member

mattn commented Sep 9, 2016

@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Paging Mr. @campoy for analysis! :)

@mattn
Copy link
Member

mattn commented Sep 9, 2016

if 1 <= expectCode && expectCode < 10 && code/100 != expectCode ||
10 <= expectCode && expectCode < 100 && code/10 != expectCode ||
100 <= expectCode && expectCode < 1000 && code != expectCode {
err = &Error{code, message}
}

https://play.golang.org/p/YTS3qTb_PE

@odeke-em
Copy link
Member

odeke-em commented Sep 9, 2016

Hmm, as alluded by @mattn, the signature of Client.cmd https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L103

func (c *Client) cmd(expectCode int, format string, args ...interface{}) (int, string, error) {

takes in an expectCode which is used in verifying the status of normalized message.

Perhaps we could improve the readability of that code by defining a table/small function of expect codes and then changing that to something like

+   _, _, err := c.cmd(expectCode(250), "RCPT TO:<%s>", to)
-   _, _, err := c.cmd(25, "RCPT TO:<%s>", to)

or set it to 250

+   _, _, err := c.cmd(250, "RCPT TO:<%s>", to)
-   _, _, err := c.cmd(25, "RCPT TO:<%s>", to)

following suit with other usages:
https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L155
https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L184
https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L220
https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L248

@mattn
Copy link
Member

mattn commented Sep 9, 2016

I guess the code is trying to handle codes like 221/251. But seems not good.

@odeke-em
Copy link
Member

odeke-em commented Sep 9, 2016

So I was curious about the acceptable status codes for RCPT and looked at a few resources on the 25X code intentions so found some hits off Google:

1. https://cr.yp.to/smtp/mail.html by D. J. Bernstein:

screen shot 2016-09-09 at 3 14 01 am

2. http://www.greenend.org.uk/rjk/tech/smtpreplies.html by Richard Kettlewell:

screen shot 2016-09-09 at 3 15 17 am

It seems like 250 and 251 are the acceptable success codes for RCPT responses. According to https://cr.yp.to/smtp/mail.html, 251 was deprecated, so using 250 is the way to go.

Apologies if my reverse engineering is causing a lot of noise here, but perhaps if we are still in touch with Evan Shaw who created the package with CL df74d8d, we could reach out to them or to someone familiar with the protocol?

@bradfitz
Copy link
Contributor

bradfitz commented Sep 9, 2016

Thanks, @mattn. I should've read the docs.

@dtelyukh, not a typo. See https://golang.org/pkg/net/textproto/#Reader.ReadResponse ...

For example, if expectCode is 31, an error will be returned if the status is not in the range [310,319].

@bradfitz bradfitz closed this as completed Sep 9, 2016
@golang golang locked and limited conversation to collaborators Sep 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants