Wikimedia Bugzilla is closed!

Is that really a problem?  If you need a particular set of parameters for
Special:Recentchanges, wouldn't it make more sense to just bookmark it?  And if
you have a bookmark, you can just use that.

However, if someone really wants to "fix" this, I should point out that the same
concern applies to many other Special pages, including Special:Contributions:
after logging in, I get redirected to [[Special:Contributions]] which says "You
have not specified a target page or user to perform this function on."

MERGE OF SOURCEFORGE BUG #897952
http://sourceforge.net/tracker/index.php?func=detail&aid=897952&group_id=34373&atid=411192

Submitted By:
noririty - noririty
Date Submitted:
2004-02-16 13:20

 Reproducible: Always
Steps to Reproduce:
1. Logout
2. browse to User contributions page (ex.
http://en.wikipedia.org/w/wiki.phtml?title=Special:Contributions&target=RickK)
3. Login
Actual Results:
"No target" page [[Special:Contributions]] come out

Expected Results:
It should hold link to target=USERID

*** Bug 1397 has been marked as a duplicate of this bug. ***

*** Bug 4814 has been marked as a duplicate of this bug. ***

In response to Timwi's response, that's the main problem with "return to's."
Short of copying the entire URL from the referer, ensuring that EVERY page that
links to the page in question passes its entire URL to it, or defining a compact
syntax that has the ability to describe every non-idempotent page on the site,
this isn't getting fixed.

*** Bug 7453 has been marked as a duplicate of this bug. ***

Surely the redirect to the login page is handled by a generic block of code -
why can't that just pass the full request string rather than just the requested
page name?

I agree that passing the parameters would be useful - without them, my TableEdit extension has problems returning to the right place when users have to be logged in to edit.

Tangentially related to bug 7129, which I'm poking at now.

This bug hasn't been touched for 6 months so I would like to propose a solution I would be willing to code. Since it would be undesirable to allow the user to be redirected anywhere other than inside the current wiki this bug could be solved by urlencoding the query string and putting it in a returnquery= part of the login url. If a returnquery= value is found then the user is redirected to $wgScript . '?' . urldecode( querystring ); otherwise the original behaviour of returnto= is used. This would ensure backwards compatibility and allow for a solution to this bug.

I would do it a little more loosely, and also allow redirects to $wgServer as well, if possible.  That is, it would be good if pretty URLs could be kept.

Anyway, I'm not going to work on this anytime soon.  Feel free to take it if you want.

I also discussed this, but going down all the way to the server root could cause possible redirection security issues. Given that some hosts give you a directory rather than a subdomain. Also some sites run multiple wikis off the same domain and malicious users might decide to try to confuse them by setting up login links that redirect to a different wiki. Due to the fact that returnto= could also be kept the functions of them could be slightly different: returnto redirects back to a wiki page and is invoked if returnquery isn't found; returnquery is invoked by default and redirects to a querystring appended to $wgScript.

Of course these are all minor details and due to the nature of any patch this could be simply changed providing it hadn't gone in to common usage yet.

Created attachment 4587 [details]
Retain contextual parameters for login and logout links

This patch provides the necessery modifications to maintain contextual paramters, if present, for both the login and logout pages. If no query string is present then former behaviour is employed.

Fully tested on my development wiki.

Er, I didn't mean $wgServer, of course.  I meant $wgArticlePath.  So you could have returnquery and returnarticle, say, with the latter appending to $wgArticlePath instead of $wgScriptPath.  returnto could be aliased to returnarticle (or just keep the name "returnto", but that's inconsistent in terms of naming).  In this case, you'd want to allow both to be set, since URLs like /wiki/Article_name?action=purge are valid.  Or, you could pass the whole URL and check it for validity, which might be less complicated on the caller's side but also maybe less safe, and maybe harder .

As for the patch, here are some brief comments, without having tested it.  First of all, did you test with pretty URLs?  There are a variety of ways to achieve those, which I'm not sure would all work identically.  I might be wrong, but the patch seems to just assume ugly URLs.

More specific comments:

-		if ( !empty( $this->mReturnTo ) ) {
+		if ( !empty( $this->mReturnQuery ) ) {
+			$wgOut->addReturnQuery( $this->mReturnQuery, $this->mReturnTo );
+		} elseif ( !empty( $this->mReturnTo ) ) {

This issue wasn't added by you, but surely it's erroneous to use empty() here and elsewhere?  The string "0" is a valid article name and a valid query string, but it would be excluded by this.  $var === '' should be used here, and in the other places.

+			$returnquery = '&returnquery=' . wfUrlencode( $this->mReturnQuery );
. . .
+			$attr[] = 'returnquery=' . $this->mReturnQuery;

You seem to be inconsistent here (as, it looks like, was the existing code).  Is $this->mReturnQuery already URL-encoded or not?  It might be a good idea to make it clear if it's already escaped, like calling it mEncReturnQuery if it is and mUnencReturnQuery or something if it's not.  Otherwise it can be pretty confusing.  I think it's not escaped, which would cause few problems with mReturnTo (mainly if the title contains &) but lots of problems with a multipart query string.

+	/**
+	 * Add a "return to" link pointing to the script with the
+	 * specified query string.
+	 * 
+	 * @param $string Query string to append (excluding ?)
+	 */
+	public function addReturnQuery( $query, $title ) {

The parameter list is inconsistent with the documentation.  The documentation should preferably state what sort of encoding the parameters are in.  You require $query to be entity-encoded here -- this is probably wrong, you want to htmlspecialchars() it in the function and accept it non-entity-encoded.  And you want a non-URL-encoded but, again, entity-encoded $title, which is a display title and not a DB-key title.  I would suggest the param descriptions be

@param string $query Unescaped query string to append, excluding the initial ?
@param string $title Unescaped display title string

and fix the code to correctly escape both of them.

+					$wgTitle->isSpecial( 'Preferences' ) ? '' : "returnto={$this->thisurl}" . '&returnquery=' . urlencode( $_SERVER[ 'QUERY_STRING' ] )
. . .
+					'href' => self::makeSpecialUrl( 'Userlogin', 'returnto=' . $this->thisurl . '&returnquery=' . urlencode( $_SERVER[ 'QUERY_STRING' ] ) ),

I'm guessing these will break with magic_quotes enabled.  There should be some way to use $wgRequest or other built-in functions here rather than directly accessing the superglobal.

Created attachment 4591 [details]
Patch providing full URL path redirection for login and logout

In this patch I have taken into account some of the things pointed out. Instead of just passing the query string forward this patch passes everything after the domain name to the "Return to __" bit, but with security checking to ensure that the path is part of the wiki.

After checking some of the stuff I decided not to rename the variables to Unenc and Enc as getVal() always returns unencoded values. I also changed some of the existing code to fix the bugs stated so far (for example the 0 page not being detected properly).

Comment on attachment 4591 [details]
Patch providing full URL path redirection for login and logout

This patch has a slight flaw in linking to non-existant pages, needs fixing.

+		global $wgAuth, $wgArticlePath, $wgScript;

Is $wgScript correct here, or do you want $wgScriptPath?  If you're going to be strict and only allow $wgScript, you exclude redirect.php, api.php, thumb.php, trackback.php, aliases like wiki.phtml and index.php5, . . .

 		# When switching accounts, it sucks to get automatically logged out
 		if( $this->mReturnTo == $wgLang->specialPage( 'Userlogout' ) ) {
-			$this->mReturnTo = '';
+			$this->mReturnTo = null;
+			$this->mReturnPath = null;
 		}

This test might no longer be sufficient.  It might be better to construct a Title object and compare it to SpecialPage::getTitleFor( 'Userlogout' ), or something.  As far as I can see, mReturnTo is actually not used for anything but display, if mReturnPath is set, so you can just send someone to returnto=Main_Page&returnpath=/wiki/Special:Userlogout.

+		if( strpos( str_replace( '$1', '', $wgArticlePath ), $this->mReturnPath ) > 0 || strpos( $wgScript, $this->mReturnPath ) > 0 ) {

Doesn't this return true if both strposes return false, i.e., if the return path doesn't contain either of the strings?  I would think something like this would be desired:

if( strpos( str_replace( '$1', '', $wgArticlePath ), $this->mReturnPath ) !== 0 && strpos( $wgScript, $this->mReturnPath ) !== 0 ) {

This way of doing things (i.e., passing the full URL and checking for validity) makes me sort of uneasy, though, as though we might miss a case.  If it makes Brion feel uneasy he'll revert it.  Maybe you should ask him before committing.  When I think about it, it still does seem best, though.

Perhaps you could dispense with the returnto parameter, and parse the URL to determine the article title to use for display?  It seems cleaner.

+		if ( is_null( $this->mReturnPath ) && !is_null( $this->mReturnTo ) ) {
 			$wgOut->returnToMain( $auto, $this->mReturnTo );
+		} elseif( !is_null( $this->mReturnPath ) ) {
+			$wgOut->addReturnToPath( $this->mReturnPath, $this->mReturnTo );

These comparisons aren't good either, are they?  What if one of them is set to the empty string?  You probably shouldn't try redirecting in that case.  Remember that WebRequest::getVal always returns a string, so you can take '' as the value meaning not to redirect.

You might even do more validation than that, if you like.  You could parse the title out and check validity with Title::newFromURL(), and not print redirects to invalid titles.

+	 * @param String $path Correctly escaped path to link

What does "escaped" mean?  urlencoded, entity-encoded, both?  Actually I think you mean it should be entirely unescaped -- it's the literal URL you're redirecting to.

+	if( strpos( str_replace( '$1', '', $wgArticlePath ), $returnPath ) > 0 || strpos( $wgScript, $returnPath ) > 0 ) {
+		// invalid returnpath, fall back to returnto
+		$returnPath = null;
+	}
+	if ( is_null( $returnPath ) && !is_null( $returnTo ) ) {
+		$wgOut->returnToMain( $auto, $returnTo );
+	} elseif( !is_null( $returnPath ) ) {
+		$wgOut->addReturnToPath( $returnPath, $returnTo );
+	} else {
+		$wgOut->returnToMain( $auto );
+	}

It's not good to duplicate logic like this.  Why don't you add this logic to OutputPage::returnToMain(), adding an extra $query parameter and doing validation there?

Assigning to Trevor -- didn't you guys set up something like this for usability opt-in? Is this generalized already or does that need to be done still?

Roan (catrope) resolved this, or something very darn similar in r53270 - for our own purposes, but the fix should fix a bug that was affecting everyone. Does this look like it solves the problem to anyone else? Can we get this pushed into the deployment branch? Looks like deployment branch only has the equivilant of r52290 with some tweaks.

Solved in r53270.