832993 - HTML bugmail links to bugs and attachments in comments use relative URLs

Status: NEW

(Bugzilla :: Email Notifications, defect)

Description

[Ed Morley [:emorley]]

Example:
{
    <div id="comments">
        <div>
          <pre>Created <span class=""><a href="attachment.cgi?id=704555" name="attach_704555" title="Patch">attachment 704555 [details] [diff] [review]</a> <a href="attachment.cgi?id=704555&amp;action=edit" title="Patch">[details]</a> <a href="attachment.cgi?id=704555&amp;action=diff" title="Patch">[diff]</a></span> <a href='page.cgi?id=splinter.html&amp;bug=832989&amp;attachment=704555'>[review]</a>
Patch

The mentioned test is causing orange for ASan try-builds. According to dbaron,
the test expects children to segfault which makes it incompatible when running
under ASan. The attached patch disables it for ASan builds.</pre>
        </div>
    </div>
}

Expected:
[details] [diff] [review] are clickable

Actual:
No clicky! :'(

Comment 1

[:glob ✱]

looks like your email client doesn't support <base href="https://bugzilla.mozilla.org/">

what be it?

Comment 2

[Ed Morley [:emorley]]

Occurs in Zimbra webmail on desktop firefox beta and also android k-9 mail app

Comment 3

[:glob ✱]

for the attachment.cgi urls, this is an upstream bug; moving.

for the splinter review link, all this requires is a configuration change, which i'll do once bug 850675 is fixed.

Comment 4

[:glob ✱]

Attachment: patch v1

Comment 5

[Frédéric Buclin]

Comment on attachment 724431 [details] [diff] [review]
patch v1

>-        my $linkval = "attachment.cgi?id=$attachid";
>+        my $linkval = Bugzilla::Util::correct_urlbase() . "attachment.cgi?id=$attachid";

There is no need to prepend Bugzilla::Util::, correct_urlbase() is already exported by Util.pm. So remove the class name for simplicity.

Now, I'm not sure it's a good idea to always force the full URL to be returned. I don't want to reproduce bug 765189.

Comment 6

[:glob ✱]

(In reply to Frédéric Buclin from comment #5)
> There is no need to prepend Bugzilla::Util::, correct_urlbase() is already
> exported by Util.pm. So remove the class name for simplicity.

ok, i'll fix the other occurrence in that file too.

> Now, I'm not sure it's a good idea to always force the full URL to be
> returned. I don't want to reproduce bug 765189.

good point.

i could add a 'mode' to the template object (or similar) to indicate that it's being used to generate html for email instead of for web.  we'll then be able to always return absolute urls in bugmail, and not include stuff which doesn't belong in bugmail.  do you have thoughts on that?

Comment 7

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #6)
> i could add a 'mode' to the template object (or similar) to indicate that
> it's being used to generate html for email instead of for web.  we'll then
> be able to always return absolute urls in bugmail, and not include stuff
> which doesn't belong in bugmail.  do you have thoughts on that?

get_bug_link() already accepts a full_url option to decide if bug links should prepend urlbase(). For consistency, we should do the same for get_attachment_link() via quoteUrls().

Comment 8

[:glob ✱]

(In reply to Frédéric Buclin from comment #7)
> get_bug_link() already accepts a full_url option to decide if bug links
> should prepend urlbase(). For consistency, we should do the same for
> get_attachment_link() via quoteUrls().

unfortunately we don't pass anything in to quoteUrls to tell it to use the full url -- it's only used where the bug_link filter is explicitly used in the email template.

Comment 9

[:glob ✱]

Attachment: 832993_2.patch

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 780252 [details] [diff] [review]
832993_2.patch

Review of attachment 780252 [details] [diff] [review]:
-----------------------------------------------------------------

Otherwise looks and works fine.

::: Bugzilla/BugMail.pm
@@ +426,4 @@
>          new_comments => \@send_comments,
>          threadingmarker => build_thread_marker($bug->id, $user->id, !$bug->lastdiffed),
>          referenced_bugs => $referenced_bugs,
> +        full_url => 1,

this part of patch is rejected for bit rot reasons. simple fix.

::: Bugzilla/Template.pm
@@ +210,5 @@
>      $text =~ s~\b(${urlbase_re}\Qshow_bug.cgi?id=\E([0-9]+)(\#c([0-9]+))?)\b
> +              ~($things[$count++] = get_bug_link($3, $1, {
> +                                        comment_num => $5,
> +                                        user => $user,
> +                                        full_url => $full_url,

nit: line up =>

@@ +242,4 @@
>      # attachment links
>      # BMO: Bug 652332 [e-mail] 2011-07-20
>      $text =~ s~\b(attachment\s*\#?\s*(\d+)(?:\s+\[diff\])?(?:\s+\[details\])?)
> +              ~($things[$count++] = get_attachment_link($2, $1, $user, $full_url)) &&

Convert get_attachment_link to take an options hash similar to get_bug_link for consistency.

Comment 11

[Frédéric Buclin]

If this patch lands in 4.4 (and even trunk, IMO), could you please leave unrelated code changes alone? This generates conflict with third-party patches for no reasons. Half of your patch is about fixing indentation of code you don't edit.

Comment 12

[rctgamer3]

Bump. Attachment links and bugs referenced in comments are still broken. [review] seems to be working, though.

https://show_bug.cgi/?id=1000000
https://attachment.cgi/?id=8411169
https://attachment.cgi/?id=8411169&action=edit
https://attachment.cgi/?id=8411169&action=diff

Comment 13

[:glob ✱]

Attachment: 832993_3.patch

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 8520692 [details] [diff] [review]
832993_3.patch

Review of attachment 8520692 [details] [diff] [review]:
-----------------------------------------------------------------

Two issues.

1) Linking in markdown comments is displayed improperly. Example:

David Lawrence admin 2015-03-04 21:39:24 GMT

new comment for title="RESOLVED FIXED - Dependency loops are possible."
href="show_bug.cgi?id=36#c3">bug 36 comment 3

2) Attachment links are still using relative URLs in HTML email notifications. bugmail.html.tmpl is calling 

<pre>[% comment.body_full({ wrap => 1 }) FILTER markdown(bug, comment, to_user) %]</pre>

which doesn't pass through the full_url => 1 defined by Bugzilla/BugMail.pm. Bugzilla::Template::get_attachment_link() is looking for $options->{full_url} which is not there.

dkl

Comment 15

[:glob ✱]

> 1) Linking in markdown comments is displayed improperly.

that's bug 1093868, unrelated to these changes.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

I'm also seeing this in eM Client, https://forum.emclient.com/emclient/topics/-base-href-in-html-mail-ignored-images-do-not-display

Comment 17

[Ed Morley [:emorley]]

This issue no longer affects GMail, so I was going to close as incomplete, though there's comment 16, so I'll leave it upto someone else to decide :-)

Comment 18

[Watilin]

I can reproduce with ProtonMail web client (mail.protonmail.com). Here is an extract of an email I received last week:

<pre style="font-size: initial">*** <a href="/show_bug.cgi?id=1515324" title="RESOLVED DUPLICATE - U2F is disabled by default">Bug 1515324</a> has been marked as a duplicate of this bug. ***</pre>

Comment 19

[Chris Morgan]

It’s very common for email clients not to support <base>. FastMail (which company I work for) also doesn’t. To the best of my knowledge, Bugzilla emails are literally the only emails I’ve ever encountered that use <base>.

Webmail clients can in theory support <base> by applying it to all the links manually, but when most of the clients out there don’t support it (though judging by comment 17 Gmail now does) and when it’s so very rare, there’s no particularly compelling cause to go to the trouble of implementing it.

On the severity of this issue: this has gotten a lot worse on BMO recently; my vague guess is that this is connected to the plaintext → Markdown transition. I don’t get bugmail all the time, but the regression window looks to be between an email from 2018-12-21 containing preformatted text and absolute links, and 2018-12-28 with proportional text content containing relative links for other bugs, attachments added, &c.

Comment 20

[Denis Müller [:Webworkr]]

See also: → bug 1840262