Not signed in (Sign In)

Issue information

  • 2
  • Dinoboff

    Dinoboff

    Bug Tracker

  • Resolved
  • Low
  • Bug

Vanilla 1.1.5 is a product of Lussumo. More Information: Documentation, Community Support.

    •  
      CommentAuthorWallPhone
    • CommentTimeAug 21st 2007
     
    http://lussumo.com/community/?CommentID=68817

    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.

    Should probably also check to make sure that the script exits normally after sending this header, instead of sending the page content, as detailed here: http://lussumo.com/community/discussion/2700/browsing-closed-vanilla-forums-without-signing-in/
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 21st 2007
     
    Done, Vanilla r588, People r74, Framework r114.
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 22nd 2007 edited
     
    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.
    •  
      CommentAuthorWallPhone
    • CommentTimeAug 22nd 2007
     
    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 (array_key_exists($Code, $CodeList)) {
    header( 'HTTP/1.1 ' . $Code . ' ' . $CodeList[$Code] );
    }
    header('Location: ' . $Location);

    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();
    }
    }
    •  
      CommentAuthorWallPhone
    • CommentTimeAug 22nd 2007 edited
     
    Another implementation, from PEAR: (BSD licensed)

    http://cvs.php.net/viewvc.cgi/pear/HTTP/HTTP.php?revision=1.50&view=markup
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 22nd 2007 edited
     
    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'
    );

    if ($Code && array_key_exists($Code, $CodeList)) {
    Header( 'HTTP/1.1 ' . $Code . ' ' . $CodeList[$Code] );
    }

    //$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);
    •  
      CommentAuthorMark
    • CommentTimeAug 22nd 2007
     
    Hi Guys,

    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...
    •  
      CommentAuthorJazzman
    • CommentTimeAug 23rd 2007
     
    Mark avoids the global keyword... I've seen it only once in some Control...

    Anyway, I suggest adding this function to Framework.Functions.php

    Don't forget the &-sign:
    function Redirect(&$Context, ...
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 23rd 2007
     
    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.
    •  
      CommentAuthorWallPhone
    • CommentTimeAug 23rd 2007 edited
     
    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 &amp;s:include('Framework.Functions.php');

    $Configuration['BaseUrl'] = 'http://your.base.url/to/vanilla/';

    $Location = 'http://wallphone.com/test.html?boo=1&amp;far=2
    set-cookie: Header-poison'; // Test absolute URL with header injection

    // $Location = 'test.html'; // test relative URL

    // Strip CRLFs and replace &amp; with & (case insensitive)
    $Location = preg_replace(array('/\r\n/', '/&amp;/i'), array('', '&'), $Location);

    // Make sure the URL is absolute TODO: Must global $Configuration first when in the core!
    $Location = ConcatenatePath($Configuration['BaseUrl'], $Location);
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 23rd 2007
     
    I don't think we should correct relative url. Although, the "Location" parameter should be an absolute url, browsers accept relatives url.
    •  
      CommentAuthorWallPhone
    • CommentTimeAug 24th 2007 edited
     
    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.
    •  
      CommentAuthorDinoboff
    • CommentTimeAug 24th 2007 edited
     
    Fixed: Vanilla r595-597, Framework r118-119, People r75-76.

    nb: for: /trunk/People.Control.RoleForm.php
    /trunk/People.Control.SignInForm.php
    /trunk/library/Vanilla/Vanilla.Control.IdentityForm.php
    /trunk/library/Vanilla/Vanilla.Control.CategoryForm.php
    /trunk/library/Vanilla/Vanilla.Control.PasswordForm.php
    /trunk/categories.php

    The process doesn't die after redirect as they didn't in the original scripts.