Item13795: Prevent redundant query parameters

If web= or topic= are used when building the URL path, then delete them from the query parameters. If the macro is used with parameters tu build the rest or jsonrpc script, display an error. This limitation will be resolved in Foswiki 2.2.
foswiki · Mar 22, 2016 · dd522e5 · dd522e5
1 parent cbcf4b7
commit dd522e5
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 10 deletions.
diff --git a/UnitTestContrib/test/unit/Fn_SCRIPTURL.pm b/UnitTestContrib/test/unit/Fn_SCRIPTURL.pm
@@ -30,6 +30,45 @@ sub test_SCRIPTURL {
     $this->assert_str_equals( "$Foswiki::cfg{ScriptUrlPath}/view.dot",
         $result );
 
+    # Topic is converted from query param to path web/topic.
+    $result =
+      $this->{test_topicObject}
+      ->expandMacros("%SCRIPTURLPATH{\"view\" topic=\"Main.WebHome\"}%");
+    $this->assert_str_equals(
+        "$Foswiki::cfg{ScriptUrlPath}/view.dot/Main/WebHome", $result );
+
+# Web is ignored in processing topic query param. so preserve in the query string.
+    $result =
+      $this->{test_topicObject}->expandMacros(
+        "%SCRIPTURLPATH{\"view\" topic=\"Main.WebHome\" web=\"System\"}%");
+    $this->assert_str_equals(
+        "$Foswiki::cfg{ScriptUrlPath}/view.dot/Main/WebHome?web=System",
+        $result );
+
+# SMELL: rest and jsonrpc cannot built the path automatically. Ignore all params.
+    $result =
+      $this->{test_topicObject}->expandMacros(
+        "%SCRIPTURLPATH{\"rest\" topic=\"Main.WebHome\" web=\"System\"}%");
+    $this->assert_str_equals(
+"<div class='foswikiAlert'>Paramaters are not supported when generating rest or jsonrpc URLs.</div>",
+        $result
+    );
+
+    $result =
+      $this->{test_topicObject}->expandMacros(
+        "%SCRIPTURLPATH{\"jsonrpc\" topic=\"Main.WebHome\" web=\"System\"}%");
+    $this->assert_str_equals(
+"<div class='foswikiAlert'>Paramaters are not supported when generating rest or jsonrpc URLs.</div>",
+        $result
+    );
+
+    # Web is used if topic contains no web, remove from query string.
+    $result =
+      $this->{test_topicObject}->expandMacros(
+        "%SCRIPTURLPATH{\"view\" topic=\"WebHome\" web=\"System\"}%");
+    $this->assert_str_equals(
+        "$Foswiki::cfg{ScriptUrlPath}/view.dot/System/WebHome", $result );
+
     $result = $this->{test_topicObject}->expandMacros("%SCRIPTURLPATH{snarf}%");
     $this->assert_str_equals( "sausages", $result );
 }

diff --git a/core/data/System/VarSCRIPTURL.txt b/core/data/System/VarSCRIPTURL.txt
@@ -1,4 +1,4 @@
-%META:TOPICINFO{author="ProjectContributor" date="1434650530" format="1.1" version="1"}%
+%META:TOPICINFO{author="ProjectContributor" date="1458616800" format="1.1" version="1"}%
 %META:TOPICPARENT{name="Macros"}%
 ---+ SCRIPTURL -- URL of script(s)
 Expands to the URL of a script, or the base URL of all scripts
@@ -21,6 +21,8 @@ Expands to the URL of a script, or the base URL of all scripts
 %H% The =edit= script should always be used in conjunction a =t="%<nop>GMTIME{"$epoch"}%"= parameter to ensure pages about to be edited are not cached in the browser
 
 %X% The 'old' way of building URLs using =SCRIPTURL= involved concatenating the web and topic names to the =SCRIPTURL= e.g. =%<nop>SCRIPTURL{"script"}%/Cartoons/EvilMonkey=. This practice is *strongly* discouraged, as it does not correctly handle encoding of the parts of the URL. At the first opportunity you should replace all such URLs with the equivalent =%<nop>SCRIPTURL%{"script" topic="Cartoons.EvilMonkey"}%=, which will handle URL encoding for you.
+
+%X% The SCRIPTURL macro does NOT support building =jsonrpc= or =rest= requests with parameters. They should still use the "contatenation" method.  This is expected to be fixed in Foswiki 2.2.
 </div>
 %STOPINCLUDE%
 ---++ Related

diff --git a/core/data/System/VarSCRIPTURLPATH.txt b/core/data/System/VarSCRIPTURLPATH.txt
@@ -1,4 +1,4 @@
-%META:TOPICINFO{author="ProjectContributor" date="1434650530" format="1.1" version="1"}%
+%META:TOPICINFO{author="ProjectContributor" date="1458616800" format="1.1" version="1"}%
 %META:TOPICPARENT{name="Macros"}%
 ---+ SCRIPTURLPATH -- URL path of script(s)
 Expands to the base URL of scripts, without protocol or host
@@ -20,7 +20,9 @@ Expands to the base URL of scripts, without protocol or host
 
 %H% See =[[VarSCRIPTURL][SCRIPTURL]]= if you expect to need the protocol and host e.g. if you are saving the HTML of the page and using it on a different host.
 
-%X% The 'old' way of building URLs using =SCRIPTURLPATH= involved concatenating the web and topic names to the =SCRIPTURLPATH= e.g. =%<nop>SCRIPTURLPATH{"script"}%/Cartoons/EvilMonkey=. This practice is *strongly* discouraged, as it does not correctly handle encoding of the parts of the URL. At the first opportunity you should replace such URLs with the equivalent %<nop>SCRIPTURLPATH%{"script" topic="Cartoons.EvilMonkey"}%, which will handle URL encoding for you.</div>
+%X% The 'old' way of building URLs using =SCRIPTURLPATH= involved concatenating the web and topic names to the =SCRIPTURLPATH= e.g. =%<nop>SCRIPTURLPATH{"script"}%/Cartoons/EvilMonkey=. This practice is *strongly* discouraged, as it does not correctly handle encoding of the parts of the URL. At the first opportunity you should replace such URLs with the equivalent %<nop>SCRIPTURLPATH%{"script" topic="Cartoons.EvilMonkey"}%, which will handle URL encoding for you.
+
+%X% The SCRIPTURL macro does NOT support building =jsonrpc= or =rest= requests with parameters. They should still use the "contatenation" method.  This is expected to be fixed in Foswiki 2.2.</div>
 %STOPINCLUDE%
 ---++ Related
  [[VarPUBURLPATH][PUBURLPATH]], [[VarSCRIPTNAME][SCRIPTNAME]], [[VarSCRIPTSUFFIX][SCRIPTSUFFIX]], [[VarSCRIPTURL][SCRIPTURL]]
diff --git a/core/lib/Foswiki/Macros/SCRIPTURL.pm b/core/lib/Foswiki/Macros/SCRIPTURL.pm
@@ -15,26 +15,44 @@ BEGIN {
 
 sub SCRIPTURL {
     my ( $this, $params, $topicObject, $relative ) = @_;
-    my @p =
-      map { $_ => $params->{$_} }
-      grep { !/^(_.*|path)$/ }
-      keys %$params;
     my ( $web, $topic, $script );
     $script = $params->{_DEFAULT};
     $web    = $params->{web};
-    if ( defined $params->{topic} ) {
+
+    my $jsonrest = ( defined $script && $script =~ m/^jsonrpc|rest(:?auth)?$/ );
+
+    if ( !$jsonrest && defined $params->{topic} ) {
         my @path = split( /[\/.]+/, $params->{topic} );
         $topic = pop(@path) if scalar(@path);
-        $web = join( '/', @path ) if scalar(@path);    # web= is ignored
+        if ( scalar(@path) ) {
+            $web = join( '/', @path )
+              ;    # web= is ignored, so preserve it in the query string
+        }
+        else {
+            delete
+              $params->{web};    # web= used, so drop the duplicate query param.
+        }
+        delete $params->{topic};
+    }
+
+    my @p =
+      map { $_ => $params->{$_} }
+      grep { !/^(_.*|path)$/ }
+      keys %$params;
+
+    if ( $jsonrest && ( $params->{web} || $params->{topic} || scalar @p ) ) {
+        return
+"<div class='foswikiAlert'>Parameters are not supported when generating rest or jsonrpc URLs.</div>";
     }
+
     return $this->getScriptUrl( !$relative, $script, $web, $topic, @p );
 }
 
 1;
 __END__
 Foswiki - The Free and Open Source Wiki, http://foswiki.org/
 
-Copyright (C) 2015 Foswiki Contributors. Foswiki Contributors
+Copyright (C) 2015-2016 Foswiki Contributors. Foswiki Contributors
 are listed in the AUTHORS file in the root of this distribution.
 NOTE: Please extend that file, not this notice.