From 617787b2f71547e53a56d0fccbd2f1ca6253d621 Mon Sep 17 00:00:00 2001 From: Crawford Currie Date: Tue, 30 Jun 2015 08:36:20 +0100 Subject: [PATCH] Item13478: after sleeping on it, concluded that it is *not* appropriate to decode either path_info or query_string. Both of these strings should always be byte strings, url-encoded if necessary, and it's not the engine's business to decode them. Callers may decode when - for example - extracting path components such as web and topic names from the path_info, or parameter values from the query_string. Note also that when composing a URL, it is essential to urlEncode - however this is not necessary with the query_string, as it is never urlDecoded --- .../lib/Foswiki/Plugins/EmptyPlugin.pm | 4 +-- .../lib/Foswiki/Plugins/HomePagePlugin.pm | 3 +- .../lib/Foswiki/Plugins/TablePlugin/Core.pm | 6 ++-- core/lib/Foswiki.pm | 33 +++++++++++++------ core/lib/Foswiki/LoginManager.pm | 4 ++- core/lib/Foswiki/Request.pm | 10 ++++++ core/lib/Foswiki/Store.pm | 3 ++ core/lib/Foswiki/UI/Rest.pm | 2 +- core/lib/Foswiki/UI/Viewfile.pm | 2 +- 9 files changed, 47 insertions(+), 20 deletions(-) diff --git a/EmptyPlugin/lib/Foswiki/Plugins/EmptyPlugin.pm b/EmptyPlugin/lib/Foswiki/Plugins/EmptyPlugin.pm index 15777da91c..af253ebbdd 100644 --- a/EmptyPlugin/lib/Foswiki/Plugins/EmptyPlugin.pm +++ b/EmptyPlugin/lib/Foswiki/Plugins/EmptyPlugin.pm @@ -263,7 +263,7 @@ exceptions handled by =Foswiki::UI= (for example, =Foswiki::OopsException=). ---++ initializeUserHandler( $loginName, $url, $pathInfo ) * =$loginName= - login name recovered from $ENV{REMOTE_USER} * =$url= - request url - * =$pathInfo= - pathinfo from the CGI query + * =$path_info= - path_info from the Foswiki::Request Allows a plugin to set the username. Normally Foswiki gets the username from the login manager. This handler gives you a chance to override the login manager. @@ -277,7 +277,7 @@ This handler is called very early, immediately after =earlyInitPlugin=. =cut #sub initializeUserHandler { -# my ( $loginName, $url, $pathInfo ) = @_; +# my ( $loginName, $url, $path_info ) = @_; #} =begin TML diff --git a/HomePagePlugin/lib/Foswiki/Plugins/HomePagePlugin.pm b/HomePagePlugin/lib/Foswiki/Plugins/HomePagePlugin.pm index 3cddf08be1..1556a46470 100644 --- a/HomePagePlugin/lib/Foswiki/Plugins/HomePagePlugin.pm +++ b/HomePagePlugin/lib/Foswiki/Plugins/HomePagePlugin.pm @@ -56,7 +56,8 @@ sub initializeUserHandler { # we don't know the user at this point so can only set up the # site wide default - my $path_info = $Foswiki::Plugins::SESSION->{request}->path_info(); + my $path_info = + Foswiki::urlDecode( $Foswiki::Plugins::SESSION->{request}->path_info() ); return unless ( ( $path_info eq '' or $path_info eq '/' ) diff --git a/TablePlugin/lib/Foswiki/Plugins/TablePlugin/Core.pm b/TablePlugin/lib/Foswiki/Plugins/TablePlugin/Core.pm index d0e47aa1b9..ba8a96689d 100644 --- a/TablePlugin/lib/Foswiki/Plugins/TablePlugin/Core.pm +++ b/TablePlugin/lib/Foswiki/Plugins/TablePlugin/Core.pm @@ -673,8 +673,8 @@ sub _processTableRow { my $nRows = scalar(@curTable); my $rspan = $rowspan[$col] + 1; if ( $rspan > 1 ) { - $curTable[ $nRows - $rspan ][$col]->{attrs}->{rowspan} - = $rspan; + $curTable[ $nRows - $rspan ][$col]->{attrs}->{rowspan} = + $rspan; } undef( $rowspan[$col] ); } @@ -1895,8 +1895,6 @@ sub handler { $url = $cgi->url( -absolute => 1, -path => 1 ) . '?'; my $queryString = $cgi->query_string(); if ($queryString) { - $queryString = Foswiki::decode_utf8($queryString) - if $Foswiki::UNICODE; $url .= $queryString . ';'; } diff --git a/core/lib/Foswiki.pm b/core/lib/Foswiki.pm index 4a50e54a9e..d27ea9ce1a 100644 --- a/core/lib/Foswiki.pm +++ b/core/lib/Foswiki.pm @@ -1485,7 +1485,11 @@ sub getSkin { Returns the URL to a Foswiki script, providing the web and topic as "path info" parameters. The result looks something like this: "http://host/foswiki/bin/$script/$web/$topic". - * =...= - an arbitrary number of name,value parameter pairs that will be url-encoded and added to the url. The special parameter name '#' is reserved for specifying an anchor. e.g. getScriptUrl('x','y','view','#'=>'XXX',a=>1,b=>2) will give .../view/x/y?a=1&b=2#XXX + * =...= - an arbitrary number of name,value parameter pairs that will +be url-encoded and added to the url. The special parameter name '#' is +reserved for specifying an anchor. e.g. +=getScriptUrl('x','y','view','#'=>'XXX',a=>1,b=>2)= will give +=.../view/x/y?a=1&b=2#XXX= If $absolute is set, generates an absolute URL. $absolute is advisory only; Foswiki can decide to generate absolute URLs (for example when run from the @@ -1501,6 +1505,9 @@ $script is not given, absolute URLs will always be generated. If either the web or the topic is defined, will generate a full url (including web and topic). Otherwise will generate only up to the script name. An undefined web will default to the main web name. +As required by RFC3986, the returned URL will only contain the +allowed characters -A-Za-z0-9_.~!*\'();:@&=+$,/?%#[] + =cut sub getScriptUrl { @@ -1554,8 +1561,8 @@ sub getScriptUrl { =begin TML ---++ StaticMethod make_params(...) -Generate a URL parameters string from parameters given. A parameter named '#' will -generate an anchor. +Generate a URL parameters string from parameters given. A parameter +named '#' will generate a fragment identifier. =cut @@ -1611,6 +1618,9 @@ Note: for compatibility with older plugins, which use %PUBURL*% with a constructed URL path, do not use =*= unless =web=, =topic=, and =attachment= are all specified. +As required by RFC3986, the returned URL will only contain the +allowed characters -A-Za-z0-9_.~!*\'();:@&=+$,/?%#[] + =cut sub getPubURL { @@ -2164,17 +2174,20 @@ sub new { $this->{scriptUrlPath} = $1; } -# The web/topic can be provided by either the query path_info, or by URL Parameters -# urlparams topic: Specifies web.topic or topic. Overrides the path given in the URL -# defaultweb: Overrides the default web, for use when topic= does not provide a web. -# path_info web/topic Always provdes a web, if topic not provided, defaults to the Home topic + # The web/topic can be provided by either the query path_info, + # or by URL Parameters: + # topic: Specifies web.topic or topic. + # Overrides the path given in the URL + # defaultweb: Overrides the default web, for use when topic= + # does not provide a web. + # path_info Defaults to the Users web Home topic # Set the default for web # Development.AddWebParamToAllCgiScripts: enables # bin/script?topic=WebPreferences;defaultweb=Sandbox my $defaultweb = $query->param('defaultweb') || $Foswiki::cfg{UsersWebName}; - my $webtopic = $query->path_info() || ''; + my $webtopic = urlDecode( $query->path_info() || '' ); my $topicOverride = ''; my $topic = $query->param('topic'); if ( defined $topic ) { @@ -2191,8 +2204,8 @@ sub new { } } -# SMELL Scripts like rest, jsonrpc, don't use web/topic path. so this ends up all bogus -# but doesn't do any harm. + # SMELL Scripts like rest, jsonrpc, don't use web/topic path. + # So this ends up all bogus, but doesn't do any harm. ( my $web, $topic ) = $this->_parsePath( $webtopic, $defaultweb, $topicOverride ); diff --git a/core/lib/Foswiki/LoginManager.pm b/core/lib/Foswiki/LoginManager.pm index 9042ed5426..fe1fa2e084 100644 --- a/core/lib/Foswiki/LoginManager.pm +++ b/core/lib/Foswiki/LoginManager.pm @@ -632,7 +632,7 @@ sub redirectToLoggedOutUrl { ); #TODO: consider if we should risk passing on the urlparams on logout - my $path_info = $session->{request}->path_info(); + my $path_info = Foswiki::urlDecode( $session->{request}->path_info() ); if ( my $topic = $session->{request}->param('topic') ) { #we should at least respect the ?topic= request my $topicRequest = Foswiki::Sandbox::untaintUnchecked($topic); @@ -645,6 +645,8 @@ sub redirectToLoggedOutUrl { $path_info = substr( $path_info, 0, ( ( pos $path_info ) - 1 ) ); } + $path_info = Foswiki::urlEncode($path_info); + my $redirectUrl; if ( $Foswiki::cfg{ForceDefaultUrlHost} ) { diff --git a/core/lib/Foswiki/Request.pm b/core/lib/Foswiki/Request.pm index 58bab6b0fa..e30fa7799a 100644 --- a/core/lib/Foswiki/Request.pm +++ b/core/lib/Foswiki/Request.pm @@ -171,6 +171,11 @@ Called without parameters returns current pathInfo. There is a =path_info()= alias for compatibility with CGI. +Note that the string returned is a *URL encoded byte string* +i.e. it will only contain characters -A-Za-z0-9_.~!*\'();:@&=+$,/?%#[] +If you intend to analyse it, you will probably have to +Foswiki::urlDecode it first. + =cut *path_info = \&pathInfo; @@ -212,6 +217,11 @@ Returns query_string part of request uri, if any. =query_string()= alias provided for compatibility with CGI. +Note that the string returned is a *URL encoded byte string* +i.e. it will only contain characters -A-Za-z0-9_.~!*\'();:@&=+$,/?%#[] +If you intend to analyse it, you will probably have to +Foswiki::urlDecode it first. + =cut *query_string = \&queryString; diff --git a/core/lib/Foswiki/Store.pm b/core/lib/Foswiki/Store.pm index 917e760a1c..c86fc7d3b3 100644 --- a/core/lib/Foswiki/Store.pm +++ b/core/lib/Foswiki/Store.pm @@ -194,6 +194,9 @@ fallback for distributed topics (such as those in System) when content is not held in the store itself (e.g. if the store doesn't recognise the web it can call SUPER::getAttachmentURL) +As required by RFC3986, the returned URL may only contain the +allowed characters -A-Za-z0-9_.~!*\'();:@&=+$,/?%#[] + =cut sub getAttachmentURL { diff --git a/core/lib/Foswiki/UI/Rest.pm b/core/lib/Foswiki/UI/Rest.pm index f1a020275d..6a6ac11072 100644 --- a/core/lib/Foswiki/UI/Rest.pm +++ b/core/lib/Foswiki/UI/Rest.pm @@ -126,7 +126,7 @@ sub rest { "computing REST for $session->{webName}.$session->{topicName}") if Foswiki::PageCache::TRACE(); - my $pathInfo = $req->path_info(); + my $pathInfo = Foswiki::urlDecode( $req->path_info() ); # Foswiki rest invocations are defined as having a subject (pluginName) # and verb (restHandler in that plugin). Make sure the path_info is diff --git a/core/lib/Foswiki/UI/Viewfile.pm b/core/lib/Foswiki/UI/Viewfile.pm index 91e0e2784c..ebf2ceebf9 100644 --- a/core/lib/Foswiki/UI/Viewfile.pm +++ b/core/lib/Foswiki/UI/Viewfile.pm @@ -76,7 +76,7 @@ sub viewfile { # This is a standard path extended by the attachment name e.g. # /Web/Topic/Attachment.gif - $pathInfo = $query->path_info(); + $pathInfo = Foswiki::urlDecode( $query->path_info() ); } # If we have path_info but no ?filename=