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

📌 go-guerrilla cannot properly handle some valid addresses. #199

Closed
issuefiler opened this issue Nov 23, 2019 · 3 comments
Closed

📌 go-guerrilla cannot properly handle some valid addresses. #199

issuefiler opened this issue Nov 23, 2019 · 3 comments
Labels

Comments

@issuefiler
Copy link

issuefiler commented Nov 23, 2019

// Address encodes an email address of the form `<user@host>`
type Address struct {
// User is local part
User string
// Host is the domain
Host string
// ADL is at-domain list if matched
ADL []string
// PathParams contains any ESTMP parameters that were matched
PathParams [][]string
// NullPath is true if <> was received
NullPath bool
}
func (ep *Address) String() string {
return fmt.Sprintf("%s@%s", ep.User, ep.Host)
}

What this little structure can do is rather limited. Address.String() is and may be used in many places throughout the code (e.g. populating headers, storing into a database.), yet it sometimes screws things up; it is not reliable.

Test cases

Quoted Local-parts VALID INPUTS

MAIL FROM: <"  yo-- man wazz'''up? surprise surprise, this is [email protected] "@example.com>
250 2.1.0 OK
  • (return_path).User: [SPACE][SPACE]yo--[SPACE]man[SPACE]wazz'''up?[SPACE]surprise[SPACE]surprise,[SPACE]this[SPACE]is[SPACE][email protected][SPACE]. INVALID Local-part
  • (return_path).Host: example.com.
  • (return_path).String(): [SPACE][SPACE]yo--[SPACE]man[SPACE]wazz'''up?[SPACE]surprise[SPACE]surprise,[SPACE]this[SPACE]is[SPACE][email protected][SPACE]@example.com. INVALID Mailbox

address-literal mailboxes VALID INPUTS

MAIL FROM: <hi@[1.1.1.1]>
250 2.1.0 OK
  • (return_path).User: hi.
  • (return_path).Host: 1.1.1.1. INVALID address-literal
  • (return_path).String(): [email protected]. INVALID Mailbox
MAIL FROM: <hi@[IPv6:2001:db8::8a2e:370:7334]>
250 2.1.0 OK
  • (return_path).User: hi.
  • (return_path).Host: 2001:db8::8a2e:370:7334. INVALID address-literal; NEEDS TO BE NORMALIZED FOR CORRECT COMPARISONS.
  • (return_path).String(): hi@2001:db8::8a2e:370:7334. INVALID Mailbox
MAIL FROM: <hi@[IPv6:2001:DB8::8A2E:370:7334]>
250 2.1.0 OK
  • (return_path).User: hi.
  • (return_path).Host: 2001:DB8::8A2E:370:7334. INVALID address-literal; NEEDS TO BE NORMALIZED FOR CORRECT COMPARISONS.
  • (return_path).String(): hi@2001:DB8::8A2E:370:7334. INVALID Mailbox

Null return-path and Postmaster recipient VALID INPUTS

MAIL FROM: <>
250 2.1.0 OK
RCPT TO: <Postmaster>
250 2.1.5 OK
  • (return_path).IsEmpty(): true.
  • (return_path).String(): @. INVALID Mailbox; and p_sql.go doesn’t care about it. Would be better to make it return an empty string instead.
  • (recipient).String(): Postmaster@ SHOULD JUST BE Postmaster.

Null return-path (November 24, 2019.)


Notes

For the User part

  1. Minimize quoting, but preserve quotes if needed. This normalization can be done by getting the unquoted & unescaped form and quoting & escaping it if needed.

From RFC 5321:

For any purposes that require generating or comparing
   Local-parts (e.g., to specific mailbox names), all quoted forms MUST
   be treated as equivalent, and the sending system SHOULD transmit the
   form that uses the minimum quoting possible.
  1. The Postmaster mailbox is case-insensitive (by RFC). go-guerrilla may safely normalize it into Postmaster.

if i := bytes.Index(bytes.ToLower(s.buf[s.pos+1:]), []byte(postmasterPath)); i == 0 {
s.LocalPart = postmasterLocalPart
return nil
}

That’s the code doing that, but what’s inconsistent there is that it only does that when it has no host part: <posTMAstEr><Postmaster> but <[email protected]><[email protected]>. Postmaster is case-insensitive regardless of having a host. So why don’t we normalize it for the both cases? Hold on, what should we do for <"P\O\STMASTER"@example.com>?

For the Host part

If an address-literal is given, it should normalize it into bytes, and give them some appropriate brackets, [IPv6:……] or [……]. Such normalization also helps to resolve #197.

Considerations

  • How to get the unquoted & unescaped form of the Local-part: as shown above, the current go-guerrilla does unquote it, but does it unescape it, too? ⸺ No, it doesn’t……. "moto\y\a\s\u" becomes just moto\y\a\s\u. Could we modify the parser not to accumulate the backslash, so that we can use it for the normalization steps?
    if ch == '\\' {
    state = 1
    s.accept.WriteByte(ch)
    continue
  • When to quote: when the unquoted & unescaped form of the Local-part has a character that is neither atext nor ..
   atext           =   ALPHA / DIGIT /    ; Printable US-ASCII
                       "!" / "#" /        ;  characters not including
                       "$" / "%" /        ;  specials.  Used for atoms.
                       "&" / "'" /
                       "*" / "+" /
                       "-" / "/" /
                       "=" / "?" /
                       "^" / "_" /
                       "`" / "{" /
                       "|" / "}" /
                       "~"
  • What to escape when quoting the unquoted & unescaped: \ and ".
   qtextSMTP      = %d32-33 / %d35-91 / %d93-126
                  ; i.e., within a quoted string, any
                  ; ASCII graphic or space is permitted
                  ; without blackslash-quoting except
                  ; double-quote and the backslash itself.
@issuefiler issuefiler changed the title The “Address” structure cannot handle some valid addresses. go-guerrilla cannot properly handle some valid addresses. Nov 23, 2019
@issuefiler
Copy link
Author

issuefiler commented Nov 24, 2019

Personally this is the number one priority for me. I can tackle everything else as I have my own set of processors, but this……, this, unfortunately, is a job of the core, which I have zero knowledge of. I need your insight and help on this if you don’t mind.

Unlike internationalized emails (SMTPUTF8) (#152), of which go-guerrilla lacks support to begin with, go-guerrilla doesn’t decline those addresses above (as they are valid), and it may cause some processors to malfunction.

@issuefiler issuefiler changed the title go-guerrilla cannot properly handle some valid addresses. 📌 go-guerrilla cannot properly handle some valid addresses. Nov 24, 2019
@flashmob
Copy link
Owner

flashmob commented Nov 27, 2019

Thanks!

The evaluation of quoted/escaped strings in the todo list #201 , so that one should be done first.

TODO

  • issue go-guerrilla doesn’t support “RCPT TO: <Postmaster>”. #201
  • add support for address-literals to mail.Address. Add a new property, AddressType, with 0 = none, 1 = ipv4, 2 = ipv6). The literal will be stored in the Host, but when String() is used, the literal will be output inside square brackets []
  • Normalize ipv6. It looks like we are already normalizing it, but throwing away the result! See parse.go ipv6AddressLiteral. Instead of using ipstr, we should use v.String() and save that result to our s.accept buffer
  • Fix the null return path so that String() returns an empty string
  • Fix String() return if the local-part is just postmaster. (The string should be just postmaster

@flashmob flashmob added the todo label Nov 27, 2019
flashmob added a commit that referenced this issue Dec 12, 2019
… empty string of address is empty, or just "postmaster" it it's a postmaster address
@flashmob
Copy link
Owner

Changes made in PR #202

  • Quoted Local-parts now should be quoted when returned from String()
  • A new Quoted bool property added to Address, to flag if local part's quoted
  • A new IP net.IPproperty added added to Address to store the IP in binary form. (It's nil if no valid address matched)
  • address-literals should now format with braces when returned from String()
  • ipv6 address-literals are now normalized
  • if local part is just postmaster, String() returns postmaster
  • Null return path returns empty string

@flashmob flashmob added testing and removed todo labels Dec 13, 2019
flashmob added a commit that referenced this issue Dec 26, 2019
also fixes a bugs:
- emails such as "user"@test.com do not need quoting
- Address.String() now outputs the IP around in braces
@flashmob flashmob added lgtm and removed testing labels Dec 26, 2019
flashmob added a commit that referenced this issue Dec 27, 2019
- Parser captures quoted local-parts without the escape characters
- mail.Address.String() know when to quote local-part, 
- server's `allowsHost` function is ipv6 address aware (addresses specified in the config will get normalized to their ipv6 simplest form, addresses parsed from RCPT and MAIL commands will have ipv6 normalized)
- if `<postmaster>` is used in the RCPT TO (without a host), then new functionality was added to assume that the host is assumed to be the Hostname setting for the Server
- HELO/EHLO argument validation. #200
- The “header” processor populates “Received:” headers wrongly. #198
-  tiny bug in “p_redis.go”. #196
- “MimeHeaderDecode” (envelope.go) returns an incorrectly-spaced string. #195
- go-guerrilla cannot properly handle some valid addresses. #199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants