~sircmpwn/aerc

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 4

[PATCH] worker/lib/parse: be more tolerant with parsing email addresses

Timmy Douglas <mail@timmydouglas.com>
Details
Message ID
<20200130071853.3394-1-mail@timmydouglas.com>
DKIM signature
missing
Download raw message
Patch: +8 -21
This is a hack I needed to do to get aerc to open up my email folder,
otherwise it would display [..] and I could not select or open
email. Rather than using less/mv to read my maildir, I thought I'd
make a small change to make aerc a little more tolerant.

I'm sort of creating this to have a discussion rather than to say this
would be the right way to actually fix it. It looked like the parsing
code was in another library and I didn't review it.

The email at fault had a CC line like:
"Cc: , emacs-devel@gnu.org"
---
 worker/lib/parse.go | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/worker/lib/parse.go b/worker/lib/parse.go
index 288dade..ff5bace 100644
--- a/worker/lib/parse.go
+++ b/worker/lib/parse.go
@@ -101,26 +101,11 @@ func parseEnvelope(h *mail.Header) (*models.Envelope, error) {
	if err != nil {
		return nil, fmt.Errorf("could not parse date header: %v", err)
	}
	from, err := parseAddressList(h, "from")
	if err != nil {
		return nil, fmt.Errorf("could not read from address: %v", err)
	}
	to, err := parseAddressList(h, "to")
	if err != nil {
		return nil, fmt.Errorf("could not read to address: %v", err)
	}
	cc, err := parseAddressList(h, "cc")
	if err != nil {
		return nil, fmt.Errorf("could not read cc address: %v", err)
	}
	bcc, err := parseAddressList(h, "bcc")
	if err != nil {
		return nil, fmt.Errorf("could not read bcc address: %v", err)
	}
	replyTo, err := parseAddressList(h, "reply-to")
	if err != nil {
		return nil, fmt.Errorf("could not read reply-to address: %v", err)
	}
	from, _ := parseAddressList(h, "from")
	to, _ := parseAddressList(h, "to")
	cc, _ := parseAddressList(h, "cc")
	bcc, _ := parseAddressList(h, "bcc")
	replyTo, _ := parseAddressList(h, "reply-to")
	subj, err := h.Subject()
	if err != nil {
		return nil, fmt.Errorf("could not read subject: %v", err)
@@ -150,7 +135,9 @@ func parseAddressList(h *mail.Header, key string) ([]*models.Address, error) {
				Name: hdr,
			}}, nil
		}
		return nil, err
		return []*models.Address{&models.Address{
			Name: err.Error(),
		}}, err
	}
	for _, addr := range addrs {
		parts := strings.Split(addr.Address, "@")
-- 
2.20.1
Details
Message ID
<0RoE0oNJrz9JkmBSy66o4Zqv5omWeO4dobjxWCa-ApXczLSh5GCoR__WTRvTSZMfxpzoIyih21Nt8FhN5eIeq_fxev6QSs526oIRy8JhOHo=@emersion.fr>
In-Reply-To
<20200130071853.3394-1-mail@timmydouglas.com> (view parent)
DKIM signature
missing
Download raw message
I think this is already supposed to be handled by parseAddressList.
The condition is inverted on line 148:

    if hdr, err := h.Text(key); err != nil && strings.Contains(hdr, "@") {

Should read like this:

    if hdr, err := h.Text(key); err == nil && strings.Contains(hdr, "@") {
Details
Message ID
<C099G7IAEYTC.235F60ZQW0MW5@jupiter.local>
In-Reply-To
<0RoE0oNJrz9JkmBSy66o4Zqv5omWeO4dobjxWCa-ApXczLSh5GCoR__WTRvTSZMfxpzoIyih21Nt8FhN5eIeq_fxev6QSs526oIRy8JhOHo=@emersion.fr> (view parent)
DKIM signature
missing
Download raw message
On Thu Jan 30, 2020 at 08:53, Simon Ser wrote:
> I think this is already supposed to be handled by parseAddressList.

That is correct.

> if hdr, err := h.Text(key); err == nil && strings.Contains(hdr, "@") {

The strings.Contains also was inverted in
baa70469c3b25e1b937c9c2a9e0b16762a227bca, it should be
!strings.Contains. Which won't actually solve the problem for the
message you're seeing; perhaps we can drop the strings.Contains
altogether.

In any case, parseAddressList is meant to be lenient.
Timmy Douglas <mail@timmydouglas.com>
Details
Message ID
<C09QRYKRVGF9.3LW1P10B7HU0Z@timmy-4570>
In-Reply-To
<C099G7IAEYTC.235F60ZQW0MW5@jupiter.local> (view parent)
DKIM signature
missing
Download raw message
On Thu Jan 30, 2020 at 11:19 AM, Ben Burwell wrote:
> The strings.Contains also was inverted in
> baa70469c3b25e1b937c9c2a9e0b16762a227bca, it should be
> !strings.Contains. Which won't actually solve the problem for the
> message you're seeing; perhaps we can drop the strings.Contains
> altogether.
>
> In any case, parseAddressList is meant to be lenient.

I don't remember string.Contains being an issue when I looked at
it... I think it's go's net/mail/message.go's parseAddressList(). It
expects an email address before the first comma, so it starts parsing
the comma as an email address, gives up, and returns an error. (I'm
not 100% sure about this, but just what I gathered from a quick
glance)

I don't think anyone would change go's parseAddressList to handle
this, but I would expect aerc to be able to show something...so that I
could delete it or pipe it to an editor if I want it to be reparsed
before replying, etc.
Details
Message ID
<C09ZRXES3DC5.34R34PP75IO7T@cirno>
In-Reply-To
<20200130071853.3394-1-mail@timmydouglas.com> (view parent)
DKIM signature
missing
Download raw message
We need to send these fixes to Go upstream imo.
Details
Message ID
<C0A2OQVABQK8.37W6G7TONX885@jupiter.local>
In-Reply-To
<C09QRYKRVGF9.3LW1P10B7HU0Z@timmy-4570> (view parent)
DKIM signature
missing
Download raw message
I was talking about aerc's parseAddressList function in
worker/lib/parse.go, which calls Header.AddressList from emersion's
go-message library, which in turn calls AddressParser.ParseList from the
standard library.

Both the standard library and emersion's go-message are pretty strict in
their adherence to the RFC 5322. Aerc's parseAddressList function is
meant to attempt parsing with go-message, but if an unparsable address
list is found for whatever reason, it should still return something
useful for the UI.

So I think the solution is 2-fold:

1) update aerc's parseAddressList to fix the err == nil check and remove
   strings.Contains condition, and
2) see if go would accept an upstream patch to its parseAddressList
   function which ignores an empty address.
Timmy Douglas <mail@timmydouglas.com>
Details
Message ID
<C0B1I1ZK54L1.1Q6JK21D2P1H0@timmy-4570>
In-Reply-To
<C0A2OQVABQK8.37W6G7TONX885@jupiter.local> (view parent)
DKIM signature
missing
Download raw message
On Fri Jan 31, 2020 at 10:13 AM, Ben Burwell wrote:
> So I think the solution is 2-fold:
>
> 1) update aerc's parseAddressList to fix the err == nil check and remove
> strings.Contains condition, and

Created a v2 patch for this.

> 2) see if go would accept an upstream patch to its parseAddressList
> function which ignores an empty address.

https://github.com/golang/go/pull/36966
Reply to thread Export thread (mbox)