Item6006: redirectCgiQuery doesn't handle fragments properly & needs minor tweak

TWiki::Func::redirectCgiQuery does not handle URL fragments properly. (Fragment is the string after # in a URL that sends to a specific location on a page. TWiki sometimes calls them "Anchors".)

Specifically, redirectCgiQuery inserts its path (?twiki_redirect_cache=) string at the end of the URL instead of before the #. This si wrong. See RFC2396 section 5.2 Oddly enough, the code goes out of it's way to do it the way it does, but it breaks every browser I've tried.

It's a 1-line fix in Client.pm line 507 in TWiki-4.1.2, Sat, 03 Mar 2007, build 13046, Plugin API version 1.11

# Rewrite a URL inserting the session id
sub _rewriteURL {
... 
        # strip off the anchor
        my $anchor = '';
        if( $url =~ s/(#.*)// ) {
            $anchor = $1;
        }
        # rebuild the URL
        %RED%$url .= $anchor.$params;%ENDCOLOR%
        %GREEN%$url .= $params.$anchor;%ENDCOLOR%
    } # otherwise leave it untouched

} # otherwise leave it untouched
 

While in there, it would be nice if:

• One could get the URL without the print I use this for debugging to print what would have happened, diagnostic data, and provide a clickable link for manually redirecting.
• One could add a hash of additional parameters for CGI::redirect (e.g. -method=>'GET', -p3p=>[qw( xx, yy, zz)])

You can do this with a couple of lines of code.

Here's a scenario where it's useful (This follows the work-around given below):

if( $debug )  {
    Dump application state
     $redirect =~ s/^(Location: *)(.*)$/$1.$q->a({href=>$2}, $2)/mse;
     print $q->p, $q->pre( "Redirect header:\n", $redirect );
} else {
    print $redirect;
}
exit();

For anyone who has to live with this in the meantime, here's a work-around that I developed - obviously, the right fix is in redirectCgiQuery.

    my $vurl = TWiki::Func::getViewUrl($wikiWeb, $wikiTopic);  # Base URL of calendar
    my $tag = "#Create_new_entry";
    # Hack to add fragment tag to redirect, which TWiki doesn't handle right.
    # (Right would be passing "$vurl.$tag" to redirect, which should put it's
    # ?param string BEFORE the tag.  Instead, it puts it after the tag, which
    # doesn't work.)
    # This gets the user back to the form's position on the page.
    my ($tmp);
    open( TMP, '+>', \$tmp ) or die "Can't open tempfile $tmp";
    select TMP;
    $| = 1;
    TWiki::Func::redirectCgiQuery( $q, $vurl, 1 );
    select STDOUT;
    my $redirect = '';
    seek TMP, 0, SEEK_SET;


    while( <TMP> ) {
        $_ =~ s/\r//g;
        chomp $_;
        $_ =~ s/^(Location:.*)$/$1$tag/;
        $redirect .= "$_\r\n";
    }
    close TMP;


...


    print $redirect;

-- TWiki:Main/TimotheLitt - 20 Sep 2008

After review I consider this to be urgent. i may have already fixed it - I remember something similar.

• CrawfordCurrie - 24 Nov 2008

After review, I think the proposed fix is dubious. here is the current code:

    # If the URL has no colon in it, or it matches the local script
    # URL, it must be an internal URL and therefore needs the session.
    if ( $url !~ /:/ || $url =~ /^$s/ ) {

        # strip off existing params
        my $params = "?$Foswiki::LoginManager::Session::NAME=$sessionId";
        if ( $url =~ s/\?(.*)$// ) {
            $params .= ';' . $1;
        }

        # strip off the anchor
        my $anchor = '';
        if ( $url =~ s/(#.*)// ) {
            $anchor = $1;
        }

        # rebuild the URL
        $url .=  $anchor . $params;
    }   # otherwise leave it untouched

If I do the fix, I'm afraid the $params will already contain the anchor. Thus either we swap the 2 stripping blocks, and swap the concatenation:

        # strip off the anchor
        my $anchor = '';
        if ( $url =~ s/(#.*)$// ) {
            $anchor = $1;
        }
        # strip off existing params
        my $params = "?$Foswiki::LoginManager::Session::NAME=$sessionId";
        if ( $url =~ s/\?(.*)$// ) {
            $params .= ';' . $1;
        }

        # rebuild the URL
        $url .= $params . $anchor;
    }   # otherwise leave it untouched

So I think we have 4 test cases to consider (Note for self: write them as unit tests):

Case	Old code			New code
	$params	$anchor	$url	$params	$anchor	$url	$params
$url = /Web/Page	?session=$sessionid	null	/Web/Page?session=$sessionid	?session=$sessionid	null	/Web/Page?session=$sessionid
$url = /Web/Page?foo=bar	?session=$sessionid;foo=bar	null	/Web/Page	?session=$sessionid;foo=bar	null	/Web/Page?session=$sessionid;foo=bar
$url = /Web/Page#anchor	?session=$sessionid	#anchor	/Web/Page#anchor?session=$sessionid	?session=$sessionid	#anchor	/Web/Page?session=$sessionid#anchor
$url = /Web/Page?foo=bar#anchor	?session=$sessionid;foo=bar#anchor	null	/Web/Page?session=$sessionid;foo=bar#anchor	?session=$sessionid;foo=bar	#anchor	/Web/Page?session=$sessionid;foo=bar#anchor

So in fact, there is one case where the old code will fail. Committing the fix...

-- OlivierRaginel - 28 Nov 2008

Now that the unit tests are working, I can "safely" close this one.

-- OlivierRaginel - 03 Dec 2008

you should consider adding test cases with a trailing / eg, /Web/Page/, /Web/Page/?foo=bar, etc.

• WillNorris - 04 Dec 2008

If you can show me a way to test this piece of code, I'll gladly do it. I had enough troubles finding scenario when it is used that (for Foswikibugs:Item6008) I'm not sure I can reproduce it for a unit test.

I agree we should write tests for these, but I disagree this requires to be Urgent and block a release. It was a bug fix.

-- OlivierRaginel - 04 Dec 2008

wbniv: Babar: sorry, i didn't realize there weren't any existing unit tests
[3:08pm] wbniv: i brought up those examples because those are exactly the kinds of bugs that were already bugs in other parts of the code
[3:08pm] wbniv: "trailing" /
[3:08pm] wbniv: and, if there had been unit tests, would have been trivial to add those test cases
[3:08pm] wbniv: but since there aren't, i'll withdraw the request and update the bu

-- WillNorris - 04 Dec 2008