Re: pgarchives: strip angle brackets when checking for msgid search

On Sat, Oct 3, 2015 at 7:42 PM, Amir Rohan <amir(dot)rohan(at)zoho(dot)com> wrote:

> Hi Magnus,
>
> On 10/03/2015 07:31 PM, Magnus Hagander wrote:
> > On Fri, Oct 2, 2015 at 7:29 PM, Amir Rohan <amir(dot)rohan(at)zoho(dot)com
> > <mailto:amir(dot)rohan(at)zoho(dot)com>> wrote:
> >
> > In some email-clients, when you copy the Message-ID to the clipboard
> > it surrounds in (or perhaps, doesn't strip) angle brackets.
> > When pasting this into the mail archive search, search fails to find
> > the message
> > and this happens often enough so as to be annoying. The fix is
> trivial.
> >
> > Discussion of similar problem in the commitfest app:
> >
> > <CABUevExRGr+Q5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A(at)mail(dot)gmail(dot)com>
> > <mailto:
> CABUevExRGr+Q5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A(at)mail(dot)gmail(dot)com>
> >
> > patch attached.
> >
> >
> > Interesting - we also had a similar fix in 9b7e9b59 for the archives,
> > but that one only deals with the URLs and not the search box. Makes much
> > sense to do it in the search box as well, so applied. Thanks!
> >
> > Actually, before I sent this.
> >It's also broken because it redirects to bad URLs if there are spaces
> in it.
>
> What is "It"?
>
> > Or if there is just one of the two, > e.g. it has < but not >.
>
> I'm sorry, I lost you. Let's try and sync...
>
> Prior to this patch:
>
> "msgid" = works
> " msgid " = fails
> " msgid" = fails
> "msgid " = fails
> <msgid>" = fails
> <msgid" = fails
> "msgid>" = fails
> " msgid>" = fails
> etc'
>
> With this patch -- they should all work.
>
> If there's a search query that currently works and is broken by this
> patch, can you give an example?
>

The search works. Meaning the API returns "this messageid exists", which
causes the search client to send a redirect to an URL that still has the
non-stripped version of the URL (including the < or space).

The regexp used for parsing URLs only allows either "msgid" or "<msgid>".
Any other of those combinations will result in a 404 at the receiving end
of that redirect.

> All those now generate 404s, which is not good.
> >
>
> "Now" meaning before applying this patch?
>

No, after. Today they render a 200 OK with "search returned no hits".

>
> > This patch needs to be in sync with the callers of that API as well as
> > the code that actually views entries in the archives before it can be
> > safely applied.
>
> There is no API change. afaict nothing that currently works and uses
> this endpoint should be affected. The only effect should be that
> queries that now fail but should succeed, succeed.
>

That is exactly what I'm saying it doesn't. I deployed it and tested and it
broke.

So either the API needs to be updated to return the actual URL, and the
consumers (which at this point I believe are just the main website) need to
be updated to actually use that URL, or the consumer also needs to be
updated with the same "cleaning" rules, or the URL parsing in the archives
code need to also deal with all those cases.

I think either the first, or a combination of first and third, of those are
the way to go. The second one (doing the same changes in the pgweb code)
seems to just put us in a position where we'll make the same mistake again
next time we try to fix these rules.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/