From 9838f1124604d492bb0c214f7364143318fe2282 Mon Sep 17 00:00:00 2001 From: George Clark Date: Wed, 18 Mar 2015 17:07:24 -0400 Subject: [PATCH] Item13185: Rework the URL path parser Do better validations of the web path components and topic names. Preserve the invalid names, so that they can be reported in oops messages. Add oops messages to Preview, Rename, Save and View when they are called with an invalid web or topic name. Previously they were often sanitized to a valid default web or topic name. Add an error message for invalid characters in a topic name. Revise the bad_web error to show the incorrect value. Client tests were a test error. It was setting a query parameter into the query_path. ViewScriptTests have 3 changes: - /Sandbox/WebIndex/?topic=WebChanges Previously the WebIndex would be preserved in the web path, Now it's detected as a valid topic name and overridden by the topic= parameter. - /Someweb/Subweb?topic=WebChanges Prevously the Subweb was ignored because of no trailing slash, and overridden by the topic name, returning /Someweb/WebChanges. If the Subweb is a valid webname, it now returns /Someweb/Subweb/WebChanges. - /A:B/WebHome used to return an empty webName. It now sets the default UsersWeb webname, but also sets invalidWeb, so that an error will be thrown. --- UnitTestContrib/test/unit/ClientTests.pm | 3 +- UnitTestContrib/test/unit/ViewScriptTests.pm | 11 +- core/lib/Foswiki.pm | 290 ++++++++++++++----- core/lib/Foswiki/UI.pm | 21 ++ core/lib/Foswiki/UI/Preview.pm | 11 + core/lib/Foswiki/UI/Rename.pm | 11 + core/lib/Foswiki/UI/Save.pm | 11 + core/lib/Foswiki/UI/View.pm | 10 + core/templates/messages.tmpl | 10 + 9 files changed, 296 insertions(+), 82 deletions(-) diff --git a/UnitTestContrib/test/unit/ClientTests.pm b/UnitTestContrib/test/unit/ClientTests.pm index e74927c7c5..71ba62675e 100644 --- a/UnitTestContrib/test/unit/ClientTests.pm +++ b/UnitTestContrib/test/unit/ClientTests.pm @@ -150,7 +150,8 @@ sub verify_edit { }; $query = Unit::Request->new(); - $query->path_info("/$this->{test_web}/$this->{test_topic}?breaklock=1"); + $query->path_info("/$this->{test_web}/$this->{test_topic}"); + $query->param( '-breaklock', 1 ); $this->createNewFoswikiSession( undef, $query ); diff --git a/UnitTestContrib/test/unit/ViewScriptTests.pm b/UnitTestContrib/test/unit/ViewScriptTests.pm index 1276a328d9..9c02c08b99 100644 --- a/UnitTestContrib/test/unit/ViewScriptTests.pm +++ b/UnitTestContrib/test/unit/ViewScriptTests.pm @@ -491,9 +491,9 @@ sub test_urlparsing { $this->urltest( '/Sandbox//WebIndex?topic=WebChanges', 'Sandbox', 'WebChanges' ); $this->urltest( '/Sandbox/WebIndex/?topic=WebChanges', - 'Sandbox/WebIndex', 'WebChanges' ); + 'Sandbox', 'WebChanges' ); $this->urltest( '/Sandbox/WebIndex//?topic=WebChanges', - 'Sandbox/WebIndex', 'WebChanges' ); + 'Sandbox', 'WebChanges' ); $this->urltest( '/Sandbox?topic=WebChanges', 'Sandbox', 'WebChanges' ); $this->urltest( '/Sandbox/?topic=WebChanges', 'Sandbox', 'WebChanges' ); @@ -568,7 +568,7 @@ sub test_urlparsing { #you may read 'subweb' but as its the last string in the url, foswiki treats it as a topicname $this->urltest( '/' . $this->{test_subweb} . '?topic=WebChanges', - $this->{test_web}, 'WebChanges' ); + $this->{test_subweb}, 'WebChanges' ); #trailing slashes make it a web again $this->urltest( '/' . $this->{test_subweb} . '/?topic=WebChanges', @@ -709,7 +709,10 @@ sub test_urlparsing { #invalid.. # - Invalid web name - Tasks.Item8713 - $this->urltest( '/A:B/WebPreferences', '', 'WebPreferences' ); + $this->urltest( '/A:B/WebPreferences', $Foswiki::cfg{UsersWebName}, + 'WebPreferences' ); + $this->urltest( '/A\'":B/WebPreferences', $Foswiki::cfg{UsersWebName}, + 'WebPreferences' ); #invalid topic name $this->urltest( '/System/WebPre@ferences', 'System', 'WebHome' ); diff --git a/core/lib/Foswiki.pm b/core/lib/Foswiki.pm index 54760a619e..0bd5fd7ab3 100644 --- a/core/lib/Foswiki.pm +++ b/core/lib/Foswiki.pm @@ -1754,6 +1754,187 @@ sub load_package { =begin TML +---++ Private _parsePath( $this, $webtopic, $defaultweb, $topicOverride ) + +Parses the Web/Topic path parameters to safely establish a valid web and topic, +or assign safe defaults. + + * $webtopic - A "web/topic" path. It might have originated from the query path_info, + or from the topic= URL parameter. (only when the topic param contained a web component) + * $defaultweb - The default web to use if the web part of webtopic is missing or invalid. + This can be from the default UsersWebName, or from the url parameter defaultweb. + * $topicOverride - A topic name to use instead of any topic provided in the pathinfo. + +Note if $webtopic ends with a trailing slash, it provides a hint that the last component should be +considered web. This allows disambiguation between a topic and subweb with the same name. +Trailing slash forces it to be recognized as a webname, otherwise the topic is shown. +Note. If the web doesn't exist, the force will be ignored. It's not possible to create a missing web +by referencing it in a URL. + +This routine sets two variables when encountering invalid input: + * $this->{invalidWeb} contains original invalid web / pathinfo content when validation fails. + * $this->{invalidTopic} Same function but for topic name +When invalid / illegal characters are encountered, the session {webName} and {topicName} will be +defaulted to safe defaults. Scripts using those fields should also test if the corresponding +invalid* versions are defined, and should throw an oops exception rathern than allowing execution +to proceed with defaulted values. + +The topic name will always have the first character converted to upper case, to prevent creation of +or access to invalid topics. + +=cut + +sub _parsePath { + my $this = shift; + my $webtopic = shift; + my $defaultweb = shift; + my $topicOverride = shift; + + #print STDERR "_parsePath called WT ($webtopic) DEF ($defaultweb)\n"; + + my $trailingSlash = ( $webtopic =~ s/\/$// ); + + #print STDERR "TRAILING = $trailingSlash\n"; + + # Remove any leading slashes or dots. + $webtopic =~ s/^[\/.]+//; + + my @parts = split /[\/.]+/, $webtopic; + my $cur = 0; + my @webs; # Collect valid webs from path + my @badpath; # Collect all webs, including illegal + my $temptopic; # Candidate topicname extracted from path, defaults. + + foreach (@parts) { + + # Lax check on name to elimnate evil characters. + my $p = Foswiki::Sandbox::untaint( $_, + \&Foswiki::Sandbox::validateTopicName ); + unless ($p) { + push @badpath, $_; + next; + } + + if ( \$_ == \$parts[-1] ) { # This is the last part of path + + if ( $this->topicExists( join( '/', @webs ) || $defaultweb, $p ) + && !$trailingSlash ) + { + + #print STDERR "Exists and no trailing slash\n"; + + # It exists in Store as a topic and there is no trailing slash + $temptopic = $p || ''; + } + elsif ( $this->webExists( join( '/', @webs, $p ) ) ) { + + #print STDERR "Web Exists " . join( '/', @webs, $p ) . "\n"; + + # It exists in Store as a web + push @badpath, $p; + push @webs, $p; + } + elsif ($trailingSlash) { + + #print STDERR "Web forced ...\n"; + if ( !$this->webExists( join( '/', @webs, $p ) ) + && $this->topicExists( join( '/', @webs ) || $defaultweb, + $p ) ) + { + + #print STDERR "Forced, but no such web, and topic exists"; + $temptopic = $p; + } + else { + + #print STDERR "Append it to the webs\n"; + $p = Foswiki::Sandbox::untaint( $p, + \&Foswiki::Sandbox::validateWebName ); + + unless ($p) { + push @badpath, $_; + next; + } + else { + push @badpath, $p; + push @webs, $p; + } + } + } + else { + #print STDERR "Just a topic. " . scalar @webs . "\n"; + $temptopic = $p; + } + } + else { + $p = Foswiki::Sandbox::untaint( $p, + \&Foswiki::Sandbox::validateWebName ); + unless ($p) { + push @badpath, $_; + next; + } + else { + push @badpath, $p; + push @webs, $p; + } + } + } + + my $web = join( '/', @webs ); + my $badweb = join( '/', @badpath ); + + # Set the requestedWebName before applying defaults - used by statistics + # generation. Note: This is validated using Topic name rules to permit + # names beginning with lower case. + $this->{requestedWebName} = + Foswiki::Sandbox::untaint( $badweb, + \&Foswiki::Sandbox::validateTopicName ); + + #print STDERR "Set requestedWebName to $this->{requestedWebName} \n" + # if $this->{requestedWebName}; + + if ( length($web) != length($badweb) ) { + + #print STDERR "RESULTS:\nPATH: $web\nBAD: $badweb\n"; + $this->{invalidWeb} = $badweb; + } + + unless ($web) { + $web = Foswiki::Sandbox::untaint( $defaultweb, + \&Foswiki::Sandbox::validateWebName ); + unless ($web) { + $this->{invalidWeb} = $defaultweb; + $web = $Foswiki::cfg{UsersWebName}; + } + } + + # Override topicname if urlparam $topic is provided. + $temptopic = $topicOverride if ($topicOverride); + + # Provide a default topic if none specified + $temptopic = $Foswiki::cfg{HomeTopicName} unless defined($temptopic); + + # Item3270 - here's the appropriate place to enforce spec + # http://develop.twiki.org/~twiki4/cgi-bin/view/Bugs/Item3270 + my $topic = + Foswiki::Sandbox::untaint( ucfirst($temptopic), + \&Foswiki::Sandbox::validateTopicName ); + + unless ($topic) { + $this->{invalidTopic} = $temptopic; + $topic = $Foswiki::cfg{HomeTopicName}; + + #print STDERR "RESULTS:\nTOPIC $topic\nBAD: $temptopic\n"; + $this->{invalidTopic} = $temptopic; + } + + #print STDERR "PARSE returns web $web topic $topic\n"; + + return ( $web, $topic ); +} + +=begin TML + ---++ ClassMethod new( $defaultUser, $query, \%initialContext ) Constructs a new Foswiki session object. A unique session object exists for @@ -1996,105 +2177,54 @@ sub new { $this->{scriptUrlPath} = $1; } - my $web = ''; - my $topic = ''; +# 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 - # Set the default for web if specified + # Set the default for web # Development.AddWebParamToAllCgiScripts: enables # bin/script?topic=WebPreferences;defaultweb=Sandbox - $web = $query->param('defaultweb') || ''; + my $defaultweb = $query->param('defaultweb') || $Foswiki::cfg{UsersWebName}; -#if the user has only asked for the web portion, then we can make a better guess (view/One/Two/Three/ is just a web (trailing /)) - my $topicSpecified = 0; - - #look at the url path - my $pathInfo = $query->path_info(); + my $webtopic = $query->path_info() || ''; + my $topicOverride = ''; + if ( $query->param('topic') ) { + if ( $query->param('topic') =~ m/[\/.]+/ ) { + $webtopic = $query->param('topic'); - # Truncate the path_info at the first quote - if ( $pathInfo =~ m/['"]/g ) { - $pathInfo = substr( $pathInfo, 0, ( ( pos $pathInfo ) - 1 ) ); - } - if ( $pathInfo && $pathInfo =~ m|^/+(.+)| ) { - $pathInfo = $1; - if ( $pathInfo =~ m|[./]| ) { - ( $web, $topic ) = $this->normalizeWebTopicName( $web, $pathInfo ); - $topicSpecified = ( $pathInfo =~ m/$topic$/ ); + #print STDERR "candidate webtopic set to $webtopic by query param\n"; } else { - $web = $pathInfo; - } - } + $topicOverride = $query->param('topic'); - # Set the requestedWebName before applying defaults - used by statistics - # generation. Note: This is validated using Topic name rules to permit - # names beginning with lower case. - $this->{requestedWebName} = - Foswiki::Sandbox::untaint( $web, \&Foswiki::Sandbox::validateTopicName ); - - #non-'' ?topic - if ( $query->param('topic') ) { - $topic = $query->param('topic'); - $topicSpecified = 1; + #print STDERR + # "candidate topic set to $topicOverride by query param\n"; + } } - ( $web, $topic ) = $this->normalizeWebTopicName( $web, $topic ); - $web =~ s|/*$||; +# SMELL Scripts like rest, jsonrpc, don't use web/topic path. so this ends up all bogus +# but doesn't do any harm. - my $topicNameTemp = $this->UTF82SiteCharSet($topic); - if ($topicNameTemp) { - $topic = $topicNameTemp; - } - - # Item3270 - here's the appropriate place to enforce spec - # http://develop.twiki.org/~twiki4/cgi-bin/view/Bugs/Item3270 - $topic = ucfirst($topic); - - # Validate and untaint topic name from path info - $topic = - Foswiki::Sandbox::untaint( $topic, \&Foswiki::Sandbox::validateTopicName ) - || $Foswiki::cfg{HomeTopicName}; - -# Set the default for web -# -# SMELL: This appears to be useless. normalizeWebTopicName will always assign a default -# By now $web is always set. -# - $web ||= $Foswiki::cfg{UsersWebName}; - - # Validate web name from path info or default - $web = - Foswiki::Sandbox::untaint( $web, \&Foswiki::Sandbox::validateWebName ) - || ''; + my ( $web, $topic ) = + $this->_parsePath( $webtopic, $defaultweb, $topicOverride ); # Convert UTF-8 web and topic name from URL into site charset if # necessary # SMELL: merge these two cases, browsers just don't mix two encodings # in one URL - can also simplify into 2 lines by making function # return unprocessed text if no conversion + # + my $topicNameTemp = $this->UTF82SiteCharSet($topic); + if ($topicNameTemp) { + $topic = $topicNameTemp; + } + my $webNameTemp = $this->UTF82SiteCharSet($web); if ($webNameTemp) { $web = $webNameTemp; } - #if the parsed per spec web&topic don't exist, try to help the user - if ($topicSpecified) { - if ( !$this->topicExists( $web, $topic ) - && $this->webExists( $web . '/' . $topic ) ) - { - -#requested view/Web/Sub - when there is no Web.Sub topic, but there is such a web - $web = $web . '/' . $topic; - $topic = $Foswiki::cfg{HomeTopicName}; - } - } - else { - if ( !$this->webExists($web) ) { - -#requested view/Web/Sub/ - when there is no Web.Sub web, but there is such a topic - my ( $w, $t ) = $this->normalizeWebTopicName( '', $web ); - ( $web, $topic ) = ( $w, $t ) if ( $this->topicExists( $w, $t ) ); - } - } $this->{topicName} = $topic; $this->{webName} = $web; @@ -2109,6 +2239,10 @@ sub new { # SMELL: what happens if we move this into the Foswiki::Users::new? $this->{user} = $this->{users}->initialiseUser( $this->{remoteUser} ); + # SMELL: Initializing the user overrides the web/topic for some reason + $this->{topicName} = $topic; + $this->{webName} = $web; + #Monitor::MARK("Initialised user"); # Static session variables that can be expanded in topics when they @@ -2384,6 +2518,8 @@ sub finish { undef $this->{topic}; undef $this->{webName}; undef $this->{topicName}; + undef $this->{invalidWeb}; + undef $this->{invalidTopic}; undef $this->{_ICONSPACE}; undef $this->{_EXT2ICON}; undef $this->{_KNOWNICON}; diff --git a/core/lib/Foswiki/UI.pm b/core/lib/Foswiki/UI.pm index 8c6099b543..41a57905de 100644 --- a/core/lib/Foswiki/UI.pm +++ b/core/lib/Foswiki/UI.pm @@ -538,6 +538,16 @@ sub checkWebExists { my ( $session, $webName, $op ) = @_; ASSERT( $session->isa('Foswiki') ) if DEBUG; + if ( $session->{invalidWeb} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'bad_web_name', + web => $webName, + topic => $Foswiki::cfg{WebPrefsTopicName}, + params => [ $op, $session->{invalidWeb} ] + ); + } unless ($webName) { throw Foswiki::OopsException( 'accessdenied', @@ -574,6 +584,17 @@ sub checkTopicExists { my ( $session, $web, $topic, $op ) = @_; ASSERT( $session->isa('Foswiki') ) if DEBUG; + if ( $session->{invalidTopic} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'invalid_topic_name', + web => $web, + topic => $topic, + params => [ $op, $session->{invalidTopic} ] + ); + } + unless ( $session->topicExists( $web, $topic ) ) { throw Foswiki::OopsException( 'accessdenied', diff --git a/core/lib/Foswiki/UI/Preview.pm b/core/lib/Foswiki/UI/Preview.pm index 017afc8f44..2058f40829 100644 --- a/core/lib/Foswiki/UI/Preview.pm +++ b/core/lib/Foswiki/UI/Preview.pm @@ -27,6 +27,17 @@ sub preview { my $topic = $session->{topicName}; my $user = $session->{user}; + if ( $session->{invalidTopic} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'invalid_topic_name', + web => $web, + topic => $topic, + params => [ $session->{invalidTopic} ] + ); + } + # SMELL: it's probably not good to do this here, because a preview may # give enough time for a new topic with the same name to be created. # It would be better to do it only on an actual save. diff --git a/core/lib/Foswiki/UI/Rename.pm b/core/lib/Foswiki/UI/Rename.pm index 114180b9db..d0571d11f1 100644 --- a/core/lib/Foswiki/UI/Rename.pm +++ b/core/lib/Foswiki/UI/Rename.pm @@ -66,6 +66,17 @@ sub rename { Foswiki::UI::checkWebExists( $session, $oldWeb, 'rename' ); + if ( $session->{invalidTopic} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'invalid_topic_name', + web => $oldWeb, + topic => $oldTopic, + params => [ $session->{invalidTopic} ] + ); + } + my $new_url; if ( $action eq 'renameweb' ) { $new_url = _renameWeb( $session, $oldWeb ); diff --git a/core/lib/Foswiki/UI/Save.pm b/core/lib/Foswiki/UI/Save.pm index 5502f94a4d..9a6bb5a7e7 100644 --- a/core/lib/Foswiki/UI/Save.pm +++ b/core/lib/Foswiki/UI/Save.pm @@ -475,6 +475,17 @@ WARN $session->normalizeWebTopicName( $session->{webName}, $session->{topicName} ); + if ( $session->{invalidTopic} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'invalid_topic_name', + web => $web, + topic => $topic, + params => [ $session->{invalidTopic} ] + ); + } + $topic = expandAUTOINC( $session, $web, $topic ); my $topicObject = Foswiki::Meta->new( $session, $web, $topic ); diff --git a/core/lib/Foswiki/UI/View.pm b/core/lib/Foswiki/UI/View.pm index de4e2291f6..08295aee6f 100644 --- a/core/lib/Foswiki/UI/View.pm +++ b/core/lib/Foswiki/UI/View.pm @@ -59,6 +59,16 @@ sub view { my $user = $session->{user}; my $users = $session->{users}; + if ( $session->{invalidTopic} ) { + throw Foswiki::OopsException( + 'accessdenied', + status => 404, + def => 'invalid_topic_name', + web => $web, + topic => $topic, + params => [ $session->{invalidTopic} ] + ); + } if ( defined $query->param('release_lock') && $query->param('release_lock') ne '' ) { diff --git a/core/templates/messages.tmpl b/core/templates/messages.tmpl index f1b485c2eb..051c6777e0 100644 --- a/core/templates/messages.tmpl +++ b/core/templates/messages.tmpl @@ -89,6 +89,13 @@ registermessages.tmpl ---++ %MAKETEXT{"You must pass a topic name."}% %MAKETEXT{"The name of the topic must not be empty." args="%PARAM1%"}% +%MAKETEXT{"Please go back in your browser and try again."}% +%TMPL:END% +%{==============================================================================}% +%TMPL:DEF{"invalid_topic_name"}% +---++ %MAKETEXT{"Invalid topic name."}% +%MAKETEXT{"The name of the topic ([_1]) contained invalid characters." args="%PARAM1%"}% + %MAKETEXT{"Please go back in your browser and try again."}% %TMPL:END% %{==============================================================================}% @@ -400,6 +407,9 @@ registermessages.tmpl %{==============================================================================}% %TMPL:DEF{"bad_web_name"}% ---++ %MAKETEXT{"Missing or illegal web name"}% + +%MAKETEXT{"The Web/Topic path: '[_1]' has missing components, or contains invalid characters." args="%PARAM2%"}% + %MAKETEXT{"A Foswiki site is divided into webs; each one represents one subject, one area of collaboration. You are trying to [_1] in a web that does not exist." args="'%PARAM1%'"}% ---+++ %MAKETEXT{"If you came here by clicking on a question mark link"}%