If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

be more permissive on attachment ID in GET request

NEW
Unassigned

Status

()

Bugzilla
Attachments & Requests
--
enhancement
8 years ago
7 years ago

People

(Reporter: vlad, Unassigned)

Tracking

Details

Attachments

(2 attachments)

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.
What mail client do you use that linkifies ending ')'s?
zimbra.
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...
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

8 years ago
I don't really want to change the validation of attachment IDs. Better is to remove parens in that case.
Assignee: attach-and-request → email-notifications
Severity: normal → enhancement
Component: Attachments & Requests → Email Notifications
... 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.
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.
(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.
(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?
Assignee: email-notifications → attach-and-request
Component: Email Notifications → Attachments & Requests
Created attachment 440835 [details] [diff] [review]
patch - v1

Strip any non-digits from $attach_id.
Assignee: attach-and-request → reed
Status: NEW → ASSIGNED
Attachment #440835 - Flags: review?(LpSolit)

Comment 11

8 years ago
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.
Attachment #440835 - Flags: review?(LpSolit) → review-
Assignee: reed → attach-and-request
Status: ASSIGNED → NEW
(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 ).
Created attachment 452377 [details] [diff] [review]
strip only characters after digits

Updated, only strips non-numeric characters after digits
Attachment #452377 - Flags: review?(LpSolit)

Comment 14

7 years ago
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.
Attachment #452377 - Flags: review?(LpSolit) → review-
You need to log in before you can comment on or make changes to this bug.