[Rt-commit] rt branch, 4.2/clicky-escaping, created. rt-4.2.13-120-g3586878

[Alex Vandiver]

The branch, 4.2/clicky-escaping has been created
        at  35868782610198b0ea675e476cf5ff8cebcb2b65 (commit)

- Log -----------------------------------------------------------------
commit 35868782610198b0ea675e476cf5ff8cebcb2b65
Author: Alex Vandiver <alex at chmrr.net>
Date:   Mon Feb 20 16:13:11 2017 -0800

    Stop double-escaping HTML which is made into links
    
    The MakeClicky component does two things: escapes plain-text content,
    and makes inline references to URLs into <a href="..."> links.
    
    It does this by using a regex to find all URL-like things; content
    between them is left as-is (if it was originally HTML) or is
    HTML-escaped (if it was plaintext).
    
    The replacement, however, is _always_ run on matched content before it
    is output.  The result is that a link in HTML with an ampersand, found
    as the characters "http://foo/?bar=baz&troz=zort" is HTML-escaped
    once again, in both the link and the output, to create:
    
      <a href="http://foo/?bar=baz&amp;troz=zort">http://foo/?bar=baz&amp;troz=zort</a>
    
    ...which is entirely incorrect.
    
    Skip the additional HTML-escaping if the content passed in is already
    HTML.  It is not MakeClicky's domain to ensure that HTML is
    well-formed and safe -- that is performed by /Elements/ScrubHTML.  As
    such, there is no harm in passing through matched content as-is.
    
    Fixes: I#31169

diff --git a/share/html/Elements/MakeClicky b/share/html/Elements/MakeClicky
index c991c7b..252711b 100644
--- a/share/html/Elements/MakeClicky
+++ b/share/html/Elements/MakeClicky
@@ -63,7 +63,7 @@ my %actions = (
         my %args = @_;
         my $post = "";
         $post = ")" if $args{value} !~ /\(/ and $args{value} =~ s/\)$//;
-        $args{value} = $escaper->($args{value});
+        $args{value} = $escaper->($args{value}) unless $args{html};
         my $result = qq{[<a target="_blank" href="$args{value}">}. loc('Open URL') .qq{</a>]};
         return $args{value} . qq{ <span class="clickylink">$result</span>$post};
     },
@@ -71,7 +71,7 @@ my %actions = (
         my %args = @_;
         my $post = "";
         $post = ")" if $args{value} !~ /\(/ and $args{value} =~ s/\)$//;
-        $args{value} = $escaper->($args{value});
+        $args{value} = $escaper->($args{value}) unless $args{html};
         my $result = qq{<a target="_blank" href="$args{value}">$args{value}</a>};
         return qq{<span class="clickylink">$result</span>$post};
     },
@@ -169,7 +169,7 @@ while ( $$content =~ /($regexp)/gsio ) {
         $pos += length($plain);
     }
     my $plain = $handle->(
-        %ARGS, 
+        %ARGS,
         value => $match,
         all_matches => [ $1, $2, $3, $4, $5, $6, $7, $8, $9 ],
     );
diff --git a/t/data/plugins/MakeClicky/html/makeclicky b/t/data/plugins/MakeClicky/html/makeclicky
index aef660f..c329880 100644
--- a/t/data/plugins/MakeClicky/html/makeclicky
+++ b/t/data/plugins/MakeClicky/html/makeclicky
@@ -1,8 +1,9 @@
 <%args>
 $content => ""
+$html => 0
 </%args>
 <%init>
-$m->comp("/Elements/MakeClicky", content => \$content);
+$m->comp("/Elements/MakeClicky", content => \$content, html => $html);
 $m->out($content);
 $m->abort;
 </%init>
diff --git a/t/web/make-clicky.t b/t/web/make-clicky.t
index a5efe86..da99d11 100644
--- a/t/web/make-clicky.t
+++ b/t/web/make-clicky.t
@@ -26,21 +26,55 @@ diag "Trailing punctuation";
 
 diag "Punctuation as part of the url";
 {
-    my $url = 'http://bestpractical.com/rt/download.html?foo=bar,baz&bat=1.2';
-    my $escaped_url = $url;
-    RT::Interface::Web::EscapeHTML( \$escaped_url );
+    my $url = 'http://bestpractical.com/rt/download.html?foo=bar,baz.2';
     is_string(
         make_clicky($m, "Refer to $url.  A following sentence."),
-        qq[Refer to <span class="clickylink"><a target="_blank" href="$escaped_url">$escaped_url</a></span>.  A following sentence.],
+        qq[Refer to <span class="clickylink"><a target="_blank" href="$url">$url</a></span>.  A following sentence.],
         "Punctuation in middle of URL",
     );
 }
 
+diag "Anchor in URL";
+{
+    my $url = 'http://wiki.bestpractical.com/test#anchor';
+    is_string(
+        make_clicky($m, "Anchor $url here"),
+        qq[Anchor <span class="clickylink"><a target="_blank" href="$url">$url</a></span> here],
+        "Captured anchor in URL",
+    );
+}
+
+diag "Query parameters in URL";
+for my $html (0, 1) {
+    my $url = "https://wiki.bestpractical.com/?q=test&search=1";
+    my $escaped_url = $url;
+    RT::Interface::Web::EscapeHTML( \$escaped_url );
+    is_string(
+        make_clicky($m, $html ? $escaped_url : $url, $html),
+        qq[<span class="clickylink"><a target="_blank" href="$escaped_url">$escaped_url</a></span>],
+        "Single escaped @{[$html ? 'HTML' : 'text']} query parameters",
+    );
+}
+
+diag "Found in href";
+{
+    my $url = '<a href="http://bestpractical.com/rt">Best Practical</a>';
+    is_string( make_clicky($m, $url, 1), $url, "URL in existing href is a no-op" );
+}
+
+diag "Found in other attribute";
+{
+    my $url = '<img src="http://bestpractical.com/rt" alt="Some image" />';
+    is_string( make_clicky($m, $url, 1), $url, "URL in image src= is a no-op" );
+}
+
+
 sub make_clicky {
     my $m    = shift;
     my $text = shift;
+    my $html = shift || 0;
     RT::Interface::Web::EscapeURI(\$text);
-    $m->get_ok("/makeclicky?content=$text", "made clicky")
+    $m->get_ok("/makeclicky?content=$text&html=$html", "made clicky")
         or diag $m->status;
     return $m->success ? $m->content : "";
 }

-----------------------------------------------------------------------