Closed Bug 998323 Opened 6 years ago Closed 6 years ago

URLs pasted in comments are no longer displayed

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

4.4.3
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: idl0r, Assigned: dkl)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140321194345

Steps to reproduce:

File a bug and post some URIs, e.g. http://www.google.com


Actual results:

The URIs will be replaced in Bugzilla/Template.pm by \0\0<ID>\0\0.
The control chars are stripped because of the change in Bugzilla/Util.pm:
$var =~ s/(?![\t\r\n])[[:cntrl:]]//g;

https://git.mozilla.org/?p=bugzilla/bugzilla.git;a=commitdiff;h=58b92d3b0245f6565a7ff34e78fce1e9ec56b355 [github]

The final URI will be just the <ID> instead of the quoted URI (<a href ...>)

XML, printing and XMLRPC etc. are not affected it seems.


Expected results:

URIs should be quoted properly.
A proposed patch. Should do the trick.
Version: unspecified → 4.4.3
Hm, dunno but it might be a "major" issue as it will break some comments, at least when hitting the "reply" link. The quoted text will contain and keep the ID instead of the actual URI.
Confirmed. All branches are affected due to bug 968576.
Assignee: general → create-and-change
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Bugzilla-General → Creating/Changing Bugs
Depends on: 968576
Ever confirmed: true
Flags: blocking4.4.4+
Flags: blocking4.2.9+
Flags: blocking4.0.13+
Keywords: regression
Summary: URIs not quoted in Bugzilla/Template.pm:quoteUrls() → URLs pasted in comments are no longer displayed
Target Milestone: --- → Bugzilla 4.0
Attached patch 998323_1.patch (obsolete) — Splinter Review
This fixes the problem without modifying html_quote() by using a random string for token replacements instead of the \0 control characters.

dkl
Attachment #8409068 - Flags: review?(LpSolit)
Comment on attachment 8409068 [details] [diff] [review]
998323_1.patch

>=== modified file 'Bugzilla/Template.pm'

>+    my $token = generate_random_password();

I think that instead of a token (which calls our cryptographic code every time we call quoteUrls()), we should use one of the reserved characters that Unicode offers, as we did to fix bug 405011, see bug 405011 comment 58. As we already use \x{FDD0} and \x{FDD1}, we could use e.g. \x{FDD2}.
Attachment #8409068 - Flags: review?(LpSolit) → review-
Attached patch 998323_2.patch (obsolete) — Splinter Review
User \x{FDD2}$count\x{FDD3} instead if \0\0$count\0\0

dkl
Assignee: create-and-change → dkl
Attachment #8409068 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8409099 - Flags: review?(LpSolit)
Attachment #8409099 - Attachment description: 998323_2.patch → Patch for 4.0
Attachment #8409099 - Attachment filename: 998323_2.patch → 998323_40_2.patch
Attachment #8409112 - Flags: review?(LpSolit)
Comment on attachment 8409099 [details] [diff] [review]
998323_2.patch

Need to change comment from discussion in IRC
Attachment #8409099 - Attachment is obsolete: true
Attachment #8409099 - Flags: review?(LpSolit)
Attached patch Patch for 4.0Splinter Review
Attachment #8409114 - Flags: review?(LpSolit)
Comment on attachment 8409099 [details] [diff] [review]
998323_2.patch

>+    # \x{FDDx} is used because it's unlikely to occur in the text,

I would say: # \x{FDDx} is a reserved character in Unicode and so is safe to use.

Or you could even drop the comment entirely. :)


>+    no warnings 'utf8';

Add a comment explaining that this is required for Perl 5.13.8 and older.


Otherwise looks good and works fine. r=LpSolit
Attachment #8409099 - Attachment description: Patch for 4.0 → 998323_2.patch
Attachment #8409099 - Attachment filename: 998323_40_2.patch → 998323_2.patch
Attachment #8409099 - Attachment is obsolete: false
Attachment #8409099 - Attachment is obsolete: true
Comment on attachment 8409114 [details] [diff] [review]
Patch for 4.0

>+    # and are reserved unicode characters. We disable warnings for now
>+    # until we require Perl 5.13.8 or newer.

5.13.8 is still affected. 5.13.9 is the first version to no longer complain. r=LpSolit with this fix on checkin.
Attachment #8409114 - Flags: review?(LpSolit) → review+
Comment on attachment 8408965 [details] [diff] [review]
0001-html_quote-Don-t-purge-0-s-because-of-quoteUrls.patch

\0 is evil and must be removed.
Attachment #8408965 - Attachment is obsolete: true
Attachment #8408965 - Flags: review-
Comment on attachment 8409112 [details] [diff] [review]
Patch for 4.2, 4.4, trunk

>+    # and are reserved unicode characters. We disable warnings for now
>+    # until we require Perl 5.13.8 or newer.

5.13.9 or newer. r=LpSolit
Attachment #8409112 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval4.4?
Flags: approval4.2?
Flags: approval4.0?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval4.2?
Flags: approval4.2+
Flags: approval4.0?
Flags: approval4.0+
Flags: approval+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   406dcbb..f409dc5  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   3f79ec4..af76027  4.4 -> 4.4

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   b909fa7..12f4d73  4.2 -> 4.2

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   aa71c23..1f44ef2  4.0 -> 4.0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.