Wrong folder when using imaps://

Wrong folder when using imaps://

[Konstantin Ryabitsev]

Hello:

I found an interesting problem using lei with imaps:// folders. I'm trying
things out with migadu, and the folder paths use '/' separators, so a full
IMAPS folder path for a folder "lore/mentions" is
imaps://imap.migadu.com/lore/mentions. However, if I configure lei-q to use
that remote path, everything actually ends up in the folder
imap.migadu.com/lore (not the "mentions" subfolder).

-K

[PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

[Eric Wong]

On Tue, 14 Sep 2021 13:50:25 -0400, Konstantin Ryabitsev wrote:
> Hello:
> 
> I found an interesting problem using lei with imaps:// folders. I'm trying
> things out with migadu, and the folder paths use '/' separators, so a full
> IMAPS folder path for a folder "lore/mentions" is
> imaps://imap.migadu.com/lore/mentions. However, if I configure lei-q to use
> that remote path, everything actually ends up in the folder
> imap.migadu.com/lore (not the "mentions" subfolder).

Oops, I think the patch below should fix it.

Btw, if you encounter more IMAP problems, I've found adding
"-c imap.debug -c imap.compress=0" to the command-line useful
(Mail::IMAPClient dumps the raw compressed traffic, so I need to
disable compression).

-----------8<-----------
Subject: [PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

Untested at the moment(*), but we were inadvertantly truncating
mailbox names with '/' due to our work-in-progress handling of
"/;UID=$NUM" parameter.

(*) strangely, my dovecot instance doesn't allow '/' by default,
    so the change to xt/net_writer-imap.t is untested.

Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Link: https://public-inbox.org/meta/20210914175025.eq7s2shkc323itaf@meerkat.local/
---
 lib/PublicInbox/URIimap.pm | 2 +-
 t/uri_imap.t               | 2 ++
 xt/net_writer-imap.t       | 3 ++-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/URIimap.pm b/lib/PublicInbox/URIimap.pm
index a309fde0..6907e32a 100644
--- a/lib/PublicInbox/URIimap.pm
+++ b/lib/PublicInbox/URIimap.pm
@@ -63,7 +63,7 @@ sub path {
 	my ($self) = @_;
 	my (undef, undef, $path) = uri_split($$self);
 	$path =~ s!\A/+!!;
-	$path =~ s![/;].*\z!!; # [;UIDVALIDITY=nz-number]/;UID=nz-number
+	$path =~ s!/?;.*\z!!; # [;UIDVALIDITY=nz-number]/;UID=nz-number
 	$path eq '' ? undef : $path;
 }
 
diff --git a/t/uri_imap.t b/t/uri_imap.t
index 14f0f346..b9e4583d 100644
--- a/t/uri_imap.t
+++ b/t/uri_imap.t
@@ -130,5 +130,7 @@ $uri = PublicInbox::URIimap->new('imap://[::1]:36281/');
 my $cred = bless { username => $uri->user, password => $uri->password };
 is($cred->{username}, undef, 'user is undef in array context');
 is($cred->{password}, undef, 'password is undef in array context');
+$uri = PublicInbox::URIimap->new('imap://u@example.com/slash/separator');
+is($uri->mailbox, 'slash/separator', "`/' separator accepted");
 
 done_testing;
diff --git a/xt/net_writer-imap.t b/xt/net_writer-imap.t
index 0a4cea68..f1228090 100644
--- a/xt/net_writer-imap.t
+++ b/xt/net_writer-imap.t
@@ -19,7 +19,8 @@ require_mods('Mail::IMAPClient');
 require_ok 'PublicInbox::NetWriter';
 my $host = (split(/\./, hostname))[0];
 my ($base) = ($0 =~ m!\b([^/]+)\.[^\.]+\z!);
-my $folder = "INBOX.$base-$host-".strftime('%Y%m%d%H%M%S', gmtime(time)).
+my $SEP = $ENV{IMAP_SEPARATOR} || '.';
+my $folder = "INBOX$SEP$base-$host-".strftime('%Y%m%d%H%M%S', gmtime(time)).
 		"-$$-".sprintf('%x', int(rand(0xffffffff)));
 my $nwr = PublicInbox::NetWriter->new;
 chop($imap_url) if substr($imap_url, -1) eq '/';

Re: [PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

[Konstantin Ryabitsev]

On Tue, Sep 14, 2021 at 07:35:28PM +0000, Eric Wong wrote:
> > I found an interesting problem using lei with imaps:// folders. I'm trying
> > things out with migadu, and the folder paths use '/' separators, so a full
> > IMAPS folder path for a folder "lore/mentions" is
> > imaps://imap.migadu.com/lore/mentions. However, if I configure lei-q to use
> > that remote path, everything actually ends up in the folder
> > imap.migadu.com/lore (not the "mentions" subfolder).
> 
> Oops, I think the patch below should fix it.

Yep, that worked. Thanks!

> Btw, if you encounter more IMAP problems, I've found adding
> "-c imap.debug -c imap.compress=0" to the command-line useful
> (Mail::IMAPClient dumps the raw compressed traffic, so I need to
> disable compression).

Good to know, thanks. Quick follow-up -- documentation says that .netrc should
work, but I've found that even though I have the following entries in
~/.netrc, I still get prompted for credentials:

    machine imap.migadu.com
      login konstantin.ryabitsev@linux.dev
      password [...]

The credential helper works after the initial "lei up" but I'm curious why
.netrc isn't happy. Not a huge deal, seeing as that requires storing passwords
in plaintext.

-K

Re: [PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Tue, Sep 14, 2021 at 07:35:28PM +0000, Eric Wong wrote:
> > Oops, I think the patch below should fix it.
> 
> Yep, that worked. Thanks!

Cool.

> Good to know, thanks. Quick follow-up -- documentation says that .netrc should
> work, but I've found that even though I have the following entries in
> ~/.netrc, I still get prompted for credentials:
> 
>     machine imap.migadu.com
>       login konstantin.ryabitsev@linux.dev
>       password [...]
> 
> The credential helper works after the initial "lei up" but I'm curious why
> .netrc isn't happy. Not a huge deal, seeing as that requires storing passwords
> in plaintext.

Ah, I forgot to update the docs again :x

My main concern with .netrc was actually inadvertantly sending
FTP auth info to an IMAP server just because they share the same
host.

Not sure if plaintext is a real problem on encrypted block
devices/filesystems.  Ordinary users can't mlock(2) to prevent
in-memory passwords from hitting swap (thus I always use
encrypted swap).

-----------8<---------
Subject: [PATCH] doc: update authentication notes for lei

~/.netrc isn't used by default any more, and I'm not sure it's
worthwhile to document the --netrc switch since it's rare for
non-FTP clients to support.

Followup-to: 9d11ed460ce113dd ("lei: do not read ~/.netrc by default")
Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
---
 Documentation/lei-convert.pod        | 2 +-
 Documentation/lei-import.pod         | 2 +-
 Documentation/lei-q.pod              | 2 +-
 Documentation/public-inbox-watch.pod | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/lei-convert.pod b/Documentation/lei-convert.pod
index 7f372327..750ba54f 100644
--- a/Documentation/lei-convert.pod
+++ b/Documentation/lei-convert.pod
@@ -13,7 +13,7 @@ lei convert [OPTIONS] (--stdin|-)
 Convert messages to another format.  C<LOCATION> is a source of
 messages: a directory (Maildir), a file, or a URL (C<imap://>,
 C<imaps://>, C<nntp://>, or C<nntps://>).  URLs requiring
-authentication must use L<netrc(5)> and/or L<git-credential(1)> to
+authentication use L<git-credential(1)> to
 fill in the username and password.
 
 For a regular file, the location must have a C<E<lt>formatE<gt>:>
diff --git a/Documentation/lei-import.pod b/Documentation/lei-import.pod
index c29a085b..9322dfa8 100644
--- a/Documentation/lei-import.pod
+++ b/Documentation/lei-import.pod
@@ -13,7 +13,7 @@ lei import [OPTIONS] (--stdin|-)
 Import messages into the local storage of L<lei(1)>.  C<LOCATION> is a
 source of messages: a directory (Maildir), a file, or a URL
 (C<imap://>, C<imaps://>, C<nntp://>, or C<nntps://>).  URLs requiring
-authentication must use L<netrc(5)> and/or L<git-credential(1)> to
+authentication use L<git-credential(1)> to
 fill in the username and password.
 
 For a regular file, the location must have a C<E<lt>formatE<gt>:>
diff --git a/Documentation/lei-q.pod b/Documentation/lei-q.pod
index 1d9e66cd..2823ced8 100644
--- a/Documentation/lei-q.pod
+++ b/Documentation/lei-q.pod
@@ -43,7 +43,7 @@ Destination for results (e.g., C</tmp/results-Maildir>,
 C<imaps://user@mail.example.com/INBOX.test>, or
 C<mboxcl2:/tmp/results-mboxcl2>).  The prefix may be a supported protocol:
 C<imap://> or C<imaps://>.  URLs requiring
-authentication must use L<netrc(5)> and/or L<git-credential(1)> to
+authentication use L<git-credential(1)> to
 fill in the username and password.
 
 A prefix can specify the format of the output: C<maildir>,
diff --git a/Documentation/public-inbox-watch.pod b/Documentation/public-inbox-watch.pod
index 5fa966be..e8f97c80 100644
--- a/Documentation/public-inbox-watch.pod
+++ b/Documentation/public-inbox-watch.pod
@@ -82,7 +82,7 @@ C<imap://> and C<imaps://> URLs:
 
 This may be specified multiple times to combine several mailboxes
 into a single public-inbox.  URLs requiring authentication
-will require L<netrc(5)> and/or L<git-credential(1)> to fill
+will require L<netrc(5)> and/or L<git-credential(1)> (preferred) to fill
 in the username and password.
 
 Default: none

Re: [PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

[Konstantin Ryabitsev]

On Tue, Sep 14, 2021 at 08:12:16PM +0000, Eric Wong wrote:
> Ah, I forgot to update the docs again :x
> 
> My main concern with .netrc was actually inadvertantly sending
> FTP auth info to an IMAP server just because they share the same
> host.

No big deal -- folks can always just use the "store" credential helper to
pretty much the same effect.

> Not sure if plaintext is a real problem on encrypted block
> devices/filesystems.  Ordinary users can't mlock(2) to prevent
> in-memory passwords from hitting swap (thus I always use
> encrypted swap).

Right, plus most of them probably have their .gitconfig with
sendemail.smtppass configured anyway. :)

-K

Re: [PATCH] uri_imap: handle '/' as an IMAP hierarchy separator

[Konstantin Ryabitsev]

On Tue, Sep 14, 2021 at 03:55:10PM -0400, Konstantin Ryabitsev wrote:
> On Tue, Sep 14, 2021 at 07:35:28PM +0000, Eric Wong wrote:
> > > I found an interesting problem using lei with imaps:// folders. I'm trying
> > > things out with migadu, and the folder paths use '/' separators, so a full
> > > IMAPS folder path for a folder "lore/mentions" is
> > > imaps://imap.migadu.com/lore/mentions. However, if I configure lei-q to use
> > > that remote path, everything actually ends up in the folder
> > > imap.migadu.com/lore (not the "mentions" subfolder).
> > 
> > Oops, I think the patch below should fix it.
> 
> Yep, that worked. Thanks!

I think I found a couple of other bugs while testing this with migadu. E.g.:

	$ export MFOLDER=imaps://imap.migadu.com/lore/b4
	$ lei q -o $MFOLDER -I https://lore.kernel.org/all/ '(s:b4 OR nq:b4 OR dfn:b4) AND rt:1.week.ago..'
	# /usr/bin/curl -Sf -s -d '' https://lore.kernel.org/all/?q=(s%3Ab4+OR+nq%3Ab4+OR+dfn%3Ab4)+AND+rt%3A1631066349..&x=m
	# /home/user/.local/share/lei/store 54/54

So far so good, but then:

	2021-09-14T20:59:12Z 20428 20428 die: BUG: imaps://imap.migadu.com/lore/b4;UIDVALIDITY=1621977334 has no UIDVALIDITY at /usr/local/share/perl/5.32.1/PublicInbox/LeiStore.pm line 313.
	 (from nowait set_sync_info)
	# https://lore.kernel.org/all/ 19/?
	# https://lore.kernel.org/all/ 25/?
	# https://lore.kernel.org/all/ 51/?
	# https://lore.kernel.org/all/ 54/54
	# 54 written to imaps://imap.migadu.com/lore/b4 (108 matches)

However, it doesn't show up in ls-search:

	$ lei ls-search
	/home/user/work/temp/lei/lockdown
	/home/user/work/temp/lei/mentions

That would appear to be due to them being saved in the lore/ subdir:

	$ find .local/share/lei/saved-searches/ -type d
	.local/share/lei/saved-searches/
	.local/share/lei/saved-searches/lockdown-1804cfad691a409f55598a8528566d5f1539b2632e1db7e206cb147396582631
	.local/share/lei/saved-searches/mentions-f467d0a01dfdc3e42523b5d0d090773269e199a6a109b0713dc48142f0e30526
	.local/share/lei/saved-searches/lore
	.local/share/lei/saved-searches/lore/mentions-e9ca065affe84b4e4637620c72b64b09970a02b83171ba75c86afff95489d392
	.local/share/lei/saved-searches/lore/b4-4811ca1722c2c2817e8cdc6a8d390f63a3b723c3c991f0267425d380aa1c8add

Cheers,
-K

[PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> 	2021-09-14T20:59:12Z 20428 20428 die: BUG: imaps://imap.migadu.com/lore/b4;UIDVALIDITY=1621977334 has no UIDVALIDITY at /usr/local/share/perl/5.32.1/PublicInbox/LeiStore.pm line 313.
> 	 (from nowait set_sync_info)

Same bug, different place :x
--------8<------
Subject: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator

Again, we were failing to account for '/' use in mailbox names :x

Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Link: https://public-inbox.org/meta/20210914210547.akdp4cqmwaheayp5@meerkat.local/
---
 lib/PublicInbox/URIimap.pm |  7 ++++---
 t/uri_imap.t               | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/URIimap.pm b/lib/PublicInbox/URIimap.pm
index 6907e32a..81644914 100644
--- a/lib/PublicInbox/URIimap.pm
+++ b/lib/PublicInbox/URIimap.pm
@@ -86,7 +86,8 @@ sub uidvalidity { # read/write
 		$$self = uri_join($scheme, $auth, $path, $query, $frag);
 	}
 	$path =~ s!\A/+!!;
-	$path =~ m!\A[^;/]+;UIDVALIDITY=([1-9][0-9]*)\b!i ? ($1 + 0) : undef;
+	$path =~ m!\A[^;]+;UIDVALIDITY=([1-9][0-9]*)\b!i ?
+		($1 + 0) : undef;
 }
 
 sub uid {
@@ -97,11 +98,11 @@ sub uid {
 			$path =~ s!/;UID=[^;/]*\b!!i;
 		} else {
 			$path =~ s!/;UID=[^;/]*\b!/;UID=$val!i or
-				$path .= ";UID=$val";
+				$path .= "/;UID=$val";
 		}
 		$$self = uri_join($scheme, $auth, $path, $query);
 	}
-	$path =~ m!\A/[^/;]+(?:;UIDVALIDITY=[^;/]+)?/;UID=([1-9][0-9]*)\b!i ?
+	$path =~ m!\A/[^;]+(?:;UIDVALIDITY=[^;/]+)?/;UID=([1-9][0-9]*)\b!i ?
 		($1 + 0) : undef;
 }
 
diff --git a/t/uri_imap.t b/t/uri_imap.t
index b9e4583d..7a97f875 100644
--- a/t/uri_imap.t
+++ b/t/uri_imap.t
@@ -132,5 +132,22 @@ is($cred->{username}, undef, 'user is undef in array context');
 is($cred->{password}, undef, 'password is undef in array context');
 $uri = PublicInbox::URIimap->new('imap://u@example.com/slash/separator');
 is($uri->mailbox, 'slash/separator', "`/' separator accepted");
+is($uri->uidvalidity(6), 6, "UIDVALIDITY set with `/' separator");
+is($$uri, 'imap://u@example.com/slash/separator;UIDVALIDITY=6',
+	"URI correct after adding UIDVALIDITY w/ `/' separator");
+
+$uri = PublicInbox::URIimap->new('imap://u@example.com/a/b;UIDVALIDITY=3');
+is($uri->uidvalidity, 3, "UIDVALIDITY w/ `/' separator");
+is($uri->mailbox, 'a/b', "mailbox w/ `/' separator + UIDVALIDITY");
+is($uri->uidvalidity(4), 4, "UIDVALIDITY set w/ `/' separator");
+is($$uri, 'imap://u@example.com/a/b;UIDVALIDITY=4',
+	"URI correct after replacing UIDVALIDITY w/ `/' separator");
+is($uri->uid(5), 5, "set /;UID= w/ `/' separator");
+
+$uri = PublicInbox::URIimap->new('imap://u@example.com/a/b/;UID=9');
+is($uri->uid, 9, "UID read with `/' separator w/o UIDVALIDITY");
+is($uri->uid(8), 8, "UID set with `/' separator w/o UIDVALIDITY");
+is($$uri, 'imap://u@example.com/a/b/;UID=8',
+	"URI correct after replacing UID w/ `/' separator");
 
 done_testing;

Re: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator

[Konstantin Ryabitsev]

On Tue, Sep 14, 2021 at 10:10:36PM +0000, Eric Wong wrote:
> Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> > 	2021-09-14T20:59:12Z 20428 20428 die: BUG: imaps://imap.migadu.com/lore/b4;UIDVALIDITY=1621977334 has no UIDVALIDITY at /usr/local/share/perl/5.32.1/PublicInbox/LeiStore.pm line 313.
> > 	 (from nowait set_sync_info)
> 
> Same bug, different place :x
> --------8<------
> Subject: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator
> 
> Again, we were failing to account for '/' use in mailbox names :x

The error is gone, but the saved search still doesn't show up when I run
"lei ls-search". Looking in .local/share/lei/saved-searches, I see that they
still get created as lore/foldername-${checksum}, which is probably why
ls-search doesn't find them?

-K

Re: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator

[Eric Wong]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> On Tue, Sep 14, 2021 at 10:10:36PM +0000, Eric Wong wrote:
> > Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> > > 	2021-09-14T20:59:12Z 20428 20428 die: BUG: imaps://imap.migadu.com/lore/b4;UIDVALIDITY=1621977334 has no UIDVALIDITY at /usr/local/share/perl/5.32.1/PublicInbox/LeiStore.pm line 313.
> > > 	 (from nowait set_sync_info)
> > 
> > Same bug, different place :x
> > --------8<------
> > Subject: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator
> > 
> > Again, we were failing to account for '/' use in mailbox names :x
> 
> The error is gone, but the saved search still doesn't show up when I run
> "lei ls-search". Looking in .local/share/lei/saved-searches, I see that they
> still get created as lore/foldername-${checksum}, which is probably why
> ls-search doesn't find them?

Ah, it's because the "foldername" part was incorrectly parsed
and it should be .local/share/lei/saved-searches/foldername-$checksum
without the "lore/" parent.

Once patched with the below, you should be able to move the
folder up a level and remove the now-empty "lore" dir inside
saved-searches.

Anyways, I think this fixes it:
---------8<---------
Subject: [PATCH] lei_saved_search: fix prefix for IMAP folders w/ slash

We failed to account for IMAP mailboxes containing `/'
characters when creating saved search files for them.

Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Link: https://public-inbox.org/meta/20210915123347.knr4qpaei73tjc5q@meerkat.local/
---
 lib/PublicInbox/LeiSavedSearch.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/LeiSavedSearch.pm b/lib/PublicInbox/LeiSavedSearch.pm
index 8ceaaf01..96ff816e 100644
--- a/lib/PublicInbox/LeiSavedSearch.pm
+++ b/lib/PublicInbox/LeiSavedSearch.pm
@@ -39,21 +39,21 @@ sub cfg_dump ($$) {
 
 sub lss_dir_for ($$;$) {
 	my ($lei, $dstref, $on_fs) = @_;
-	my @n;
+	my $pfx;
 	if ($$dstref =~ m,\Aimaps?://,i) { # already canonicalized
 		require PublicInbox::URIimap;
 		my $uri = PublicInbox::URIimap->new($$dstref)->canonical;
 		$$dstref = $$uri;
-		@n = ($uri->mailbox);
+		$pfx = $uri->mailbox;
 	} else {
 		# can't use Cwd::abs_path since dirname($$dstref) may not exist
 		$$dstref = $lei->rel2abs($$dstref);
 		$$dstref =~ tr!/!/!s;
-		@n = ($$dstref =~ m{([^/]+)/*\z}); # basename
+		$pfx = $$dstref;
 	}
-	push @n, sha256_hex($$dstref);
+	($pfx) = ($pfx =~ m{([^/]+)/*\z}); # basename
 	my $lss_dir = $lei->share_path . '/saved-searches/';
-	my $d = $lss_dir . join('-', @n);
+	my $d = "$lss_dir$pfx-".sha256_hex($$dstref);
 
 	# fall-back to looking up by st_ino + st_dev in case we're in
 	# a symlinked or bind-mounted path
@@ -61,7 +61,7 @@ sub lss_dir_for ($$;$) {
 		my @cur = stat(_);
 		my $want = pack('dd', @cur[1,0]); # st_ino + st_dev
 		my ($c, $o, @st);
-		for my $g ("$n[0]-*", '*') {
+		for my $g ("$pfx-*", '*') {
 			my @maybe = glob("$lss_dir$g/lei.saved-search");
 			for my $f (@maybe) {
 				$c = cfg_dump($lei, $f) // next;

Re: [PATCH] uri_imap: fix ->uidvalidity and ->uid w/ `/' separator

[Konstantin Ryabitsev]

On Wed, Sep 15, 2021 at 05:43:41PM +0000, Eric Wong wrote:
> > The error is gone, but the saved search still doesn't show up when I run
> > "lei ls-search". Looking in .local/share/lei/saved-searches, I see that they
> > still get created as lore/foldername-${checksum}, which is probably why
> > ls-search doesn't find them?
> 
> Ah, it's because the "foldername" part was incorrectly parsed
> and it should be .local/share/lei/saved-searches/foldername-$checksum
> without the "lore/" parent.
> 
> Once patched with the below, you should be able to move the
> folder up a level and remove the now-empty "lore" dir inside
> saved-searches.
> 
> Anyways, I think this fixes it:

Yep, confirmed. And lei up --all now picks up remote imap folders.

Thanks!

-K