Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9908

Send "Moved Permanently" before stripping off session ids for Bots.

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.8-RC1
    • Fix Version/s: 3.0.9-RC1
    • Component/s: Sessions
    • Labels:
      None

      Description

      Hello,

      The way sid are removed for bot is wrong in session.php. The code will result into HTTP 302 redirecting the url with sid to its equivalent without.

      As stated in rfc2616 about the meaning of a 302 :http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
      "The requested resource resides temporarily under a different URI. Since the redirection might be altered on occasion, the client SHOULD continue to use the Request-URI for future requests."

      This mean that the code just fails its purpose as bot will be told to continue using the original url, with sid !

      Suitable HTTP header for this is 301 :
      "The requested resource has been assigned a new permanent URI and any future references to this resource SHOULD use one of the returned URIs."

      Simple fix in session.php find :

      redirect(build_url(array('sid')));

      Before add :

      send_status_line(301, 'Moved Permanently');

      Of course this does not fully solves the matter for so called PITA server since in that case, redirect will use meta refresh, but we can assume that no such server does run a search engine bot.

      Regards,

      dcz

        Issue Links

          Activity

          Hide
          bantu Andreas Fischer added a comment -

          We were already in RC phase which means that we will only fix regression bugs, no new non-severe fixes should be added between 3.0.8-RC1 and 3.0.8.

          Show
          bantu Andreas Fischer added a comment - We were already in RC phase which means that we will only fix regression bugs, no new non-severe fixes should be added between 3.0.8-RC1 and 3.0.8.
          Hide
          Dionisiy Dionisiy added a comment -

          The bug is also in incorrect behavior.
          As stated in the code this redirection should be applied to bots only, but logout of registered user is also redirected to /ucp.php?mode=logout first.

          This creates a problem for 3rd party phpBB integrations and creates additional page load per each logout.

          The correct code is:

          if (isset($_GET['sid']) && $bot)

          { redirect(build_url(array('sid'))); }
          Show
          Dionisiy Dionisiy added a comment - The bug is also in incorrect behavior. As stated in the code this redirection should be applied to bots only, but logout of registered user is also redirected to /ucp.php?mode=logout first. This creates a problem for 3rd party phpBB integrations and creates additional page load per each logout. The correct code is: if (isset($_GET ['sid'] ) && $bot) { redirect(build_url(array('sid'))); }
          Hide
          bantu Andreas Fischer added a comment - - edited

          I am wondering whether redirect() should always send 301, 'Moved Permanently'. Probably not.

          Show
          bantu Andreas Fischer added a comment - - edited I am wondering whether redirect() should always send 301, 'Moved Permanently'. Probably not.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          I wonder if instead of https://github.com/bantu/phpbb3/commit/642582dc6dd87675cd5d684ce5ad57430d343fee a better approach would be to have redirect() take an extra parameter indicating whether to redirect permanently or temporarily. Maybe add a redirect_permanent() function considering the parameter list of redirect().

          Show
          Oleg Oleg [X] (Inactive) added a comment - I wonder if instead of https://github.com/bantu/phpbb3/commit/642582dc6dd87675cd5d684ce5ad57430d343fee a better approach would be to have redirect() take an extra parameter indicating whether to redirect permanently or temporarily. Maybe add a redirect_permanent() function considering the parameter list of redirect().
          Hide
          bantu Andreas Fischer added a comment -

          Agree. I wouldn't have a problem with adding another paramter to redirect().

          Show
          bantu Andreas Fischer added a comment - Agree. I wouldn't have a problem with adding another paramter to redirect().

            People

            • Assignee:
              bantu Andreas Fischer
              Reporter:
              dcz dcz [X] (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development