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

On 10/05/2015 11:57 AM, Magnus Hagander wrote:
>
>
> On Sat, Oct 3, 2015 at 7:42 PM, Amir Rohan <amir(dot)rohan(at)zoho(dot)com
> <mailto: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>
> > <mailto: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%2BQ5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A(at)mail(dot)gmail(dot)com>>
> >
> <mailto:CABUevExRGr+Q5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A(at)mail(dot)gmail(dot)com
> <mailto:CABUevExRGr%2BQ5gzp-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.
>

Ah, thanks for clearing that up, I understand now and I didn't think to
test that. I'll send an updated patch.

Amir