Location: headers are sometimes misinterpeted when passing through IIS with IE. Recommended to make them all start with an uppercase L, and have a space after the colon.
With this problem and also to be sure that the process will die after setting the Location header, maybe we should use a redirect function instead like: /** * Redirect to an other page * * @todo Should $Location be encoded? * @param string $Location Absolute URL * @param string $Code Status code * @param string $Name Name of the page * @param bool $Die Should the script terminate * @return void */ function Redirect($Location, $Code = '302', $Name = null, $Die = true) { // Set status $CodeList = array( '301' => 'Moved Permanently', '302' => 'Found', '303' => 'See Other' ); if ($Code) { if (!array_key_exists($Code, $CodeList)) { $Code = '302'; } Header( 'HTTP/1.1 ' . $Code . ' ' . $CodeList[$Code] ); }
//$Location should be well encoded (should it be done here?) header('Location: ' . $Location);
if ($Die) { @ob_end_clean(); if (!$Name) { $Name = $Location; } // display a lick in case the redirect fails echo '<a href="' . $Location . '">' . FormatStringForDisplay($Name) . '</a>'; die(); } }
The function doesn't doesn't try to prevent HTTP Response Injection. It assume the url is already encoded.
I rewrote that a bit to make it a little simpler, and to follow some conventions in Vanilla like using 1 and 0 for true and false. PHP also sends the HTTP/1.1 302 header by default if you just do a Location() by itself.
I think this might count as adding a feature, so lets let Mark review it before making the change. He may also want to put it in a class, i.e. $Context->Redirect().
I agree that this function is the best place to make sure the URL is well-formed. Maybe add a $Location = str_replace('&', '&', $Location), and append the BASE_URL if the URL isin't absolute.
function Redirect($Context, $Location, $Code = '302', $Name = '', $Die = 1) { // Set status $CodeList = array( '301' => 'Moved Permanently', '303' => 'See Other' );
if ($Die) { @ob_end_clean(); if (!$Name) { $Name = $Location; } // display a link in case the redirect fails echo '<a href="' . $Location . '">' . FormatStringForDisplay($Name) . '</a>'; $Context->Unload(); die(); } }
Good points. I also look at PEAR function added a test for the HEAD method case.
For the Context argument, what about using the global keyword? function Redirect($Location, $Code = '302', $Name = '', $Die = 1) { // Set status $CodeList = array( '301' => 'Moved Permanently', '303' => 'See Other' );
//$Location should be escape header('Location: ' . $Location);
if ($Die) { @ob_end_clean(); if (isset($_SERVER['REQUEST_METHOD']) && $_SERVER['REQUEST_METHOD'] != 'HEAD') { if (!$Name) { $Name = $Location; } // display a link in case the redirect fails echo '<a href="' . $Location . '">' . FormatStringForDisplay($Name) . '</a>'; } global $Context; $Context->Unload(); die(); } }
I think the function should just remove any carriage return and line feed from the url. $Location= preg_replace(array("/\n/", "/\r/"), '', $Location);
I had come across some problems with this before on my windows machines, and I could never figure it out. It's nice to see you found the problem. I'd say it's fine to go ahead and add the Redirect function to the core...
Do we really need " $Context->Unload();"? php is suppose to clean-up everything by itself. Adding $Context as argument just for unloading of the context objects seems unnecessary.
In retrospect, unload does seem unnecessary. It might even slow down exiting of the script.
As far as passing &$Context vs. global--the function would break if there is a call to redirect when $Context isin't avaliable. Global is used to get $Configuration in FormatStringForDisplay(), and I think redirects are fundamental enough to be used in cases where you might not need or have $Context.
Here are some working cleanup functions (with test code!) on the URL that would remove CRLFs, ensure an absolute URL, and remove all &s:include('Framework.Functions.php');
$Location = 'http://wallphone.com/test.html?boo=1&far=2 set-cookie: Header-poison'; // Test absolute URL with header injection
// $Location = 'test.html'; // test relative URL
// Strip CRLFs and replace & with & (case insensitive) $Location = preg_replace(array('/\r\n/', '/&/i'), array('', '&'), $Location);
// Make sure the URL is absolute TODO: Must global $Configuration first when in the core! $Location = ConcatenatePath($Configuration['BaseUrl'], $Location);
I was nervous about something somewhere breaking because it might not be absolute... but the RFC says "SHOULD" and defines should as something that is recommended, but not required.
99% of the URLs passed to redirect are already absolute anyway. So let's just forget this global/absolute/config nonsense.