561006 - be more permissive on attachment ID in GET request

Status: NEW

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The emails that bugzilla sends out with attachments have the attachment URL in parens, e.g.:

 --> (https://bugzilla.mozilla.org/attachment.cgi?id=440671)

Auto-linkification often includes the trailing ) in the URL, leading to an invalid attachment ID.  Can the ID parsing be changed such that it ignores trailing )'s?  Better yet, in addition to this, can the email template be changed so that itd oesn't place the ()s around the attachment URL?  Just the former would be enough to fix this issue, though.

Comment 1

[Reed Loden [:reed]]

What mail client do you use that linkifies ending ')'s?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

zimbra.

Comment 3

[Reed Loden [:reed]]

Honestly, sounds more like a client problem, as most clients are smart about such things. ')' is clearly ending punctuation and generally should not be linkified. However, I would have no problem right trimming 'id' of anything but digits if it helps...

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I agree, it is a client problem, but fixing it in bugzilla would be pretty easy and is much easier than fixing it in zimbra.  Trimming everything after [0-9]+ would be great for the id value.

Comment 5

[Frédéric Buclin]

I don't really want to change the validation of attachment IDs. Better is to remove parens in that case.

Comment 6

[Reed Loden [:reed]]

... or just WONTFIX, considering Zimbra has already fixed this bug upstream (https://bugzilla.zimbra.com/show_bug.cgi?id=42270). Looks like the fix will be in ZCS 6.0.7, due out at the end of May.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

it's a trivial fix on the bugzilla side that works around a common bug in UAs; would really prefer it to go in the bugzilla side (and to do it on the attachment validation side, otherwise it could come up in different contexts -- e.g. irc links to attachments, etc.; plus older mails will still have the paren).  Seems like it's a big usability win with no downside.

Comment 8

[Reed Loden [:reed]]

(In reply to comment #7)
> it's a trivial fix on the bugzilla side that works around a common bug in UAs;

Besides Zimbra, do you know of another product that has this problem?

> e.g. irc links to attachments, etc.; 

My IRC client parses URLs with ending ')'s just fine. Does yours not?

> plus older mails will still have the paren).

What do you mean? In the case of Zimbra, once the fixed version is installed, all old bugmail will have proper linkification and not be affected by this issue.

> Seems like it's a big usability win with no downside.

It's possible it could break existing clients/bots/scripts that parse bugmail, as they might be expecting the parentheses to always be there.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> (In reply to comment #7)
> > it's a trivial fix on the bugzilla side that works around a common bug in UAs;
> 
> Besides Zimbra, do you know of another product that has this problem?

Ok, it's a trivial fix on the bugzilla side that works around a potential bug in UAs.

> > e.g. irc links to attachments, etc.; 
> 
> My IRC client parses URLs with ending ')'s just fine. Does yours not?

My IRC client doesn't parse URLs, but my terminal does, and it often gets it wrong.

> > plus older mails will still have the paren).
> 
> What do you mean? In the case of Zimbra, once the fixed version is installed,
> all old bugmail will have proper linkification and not be affected by this
> issue.

Assuming I have that particular Zimbra version, yes.

> > Seems like it's a big usability win with no downside.
> 
> It's possible it could break existing clients/bots/scripts that parse bugmail,
> as they might be expecting the parentheses to always be there.

I was referring to just ignoring trailing chars (or even just trailing parens) in attachment IDs.

I don't really understand the pushback; what's the downside to ignoring trailing junk after numerics in attachment IDs?

Comment 10

[Reed Loden [:reed]]

Attachment: patch - v1

Strip any non-digits from $attach_id.

Comment 11

[Frédéric Buclin]

Comment on attachment 440835 [details] [diff] [review]
patch - v1

No, I don't want to change the way we check attachment IDs. And this regexp would convert q23p45 into 2345, which is evil.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #11)
> (From update of attachment 440835 [details] [diff] [review])
> No, I don't want to change the way we check attachment IDs.

What's the harm, given the utility?

> And this regexp would convert q23p45 into 2345, which is evil.

This I agree is evil.  Should just strip trailing chars when it hits the first non-digit, and perhaps even just trailing ).

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: strip only characters after digits

Updated, only strips non-numeric characters after digits

Comment 14

[Frédéric Buclin]

Comment on attachment 452377 [details] [diff] [review]
strip only characters after digits

>+    # Trim any non-digits from $attach_id.
>+    $attach_id =~ s/^([0-9]+).*/\1/;

Perl doesn't like \1, see http://perldoc.perl.org/perlre.html#Warning-on-\1-Instead-of-$1. It throws:

  \1 better written as $1

Also, you can write \d+ instead of [0-9]+. Otherwise looks good.