OAuthRequester unexpectedly attaches php://input parameters

[GoogleCodeExporter]

What steps will reproduce the problem?

1. User submits a form to a PHP script on server A with form variables x, y and 
z. It's a POST request with variables x, y, z in the request body.

2. While processing this request, server A uses oauth-php to make a two-legged 
call to server B with parameters p, q, r:

$params = array('p'=>1, 'q'=>1, 'r'=>1);
$request = new OAuthRequester($url, $method, $params);
$result = $request->doRequest();

What is the expected output? What do you see instead?
Server B should receive variables p, q and r, but in fact receives p, q, r, x, 
y and z. OAuthRequest::getRequestBody attaches the parameters from the original 
request body before signing and transmitting the whole thing.

What version of the product are you using? On what operating system?
r155, CentOS 5, PHP 5.2

Please provide any additional information below.
Since I'm only using the library as an OAuth consumer, I was able to work 
around the problem by putting return ''; on the first line of 
OAuthRequest::getRequestBody. This workaround probably breaks the server 
functionality but that's not actually a problem for me.

At a glance, OAuthRequest::getRequestBody should only be called when we are 
verifying a request, and not when we are in the consumer role. Since both 
OAuthRequester and OAuthVerifier extend OAuthRequest, a real solution might 
involve letting OAuthRequest know if it should try to read php://input or if it 
should strictly rely on the $body argument of its constructor.

Original issue reported on code.google.com by kris0...@gmail.com on 1 Oct 2010 at 8:46

[GoogleCodeExporter]

Ok, this is a bug (and a wonderful bug report, thanks). But I think it's being 
caused by something else. Could you please post the $method variable? I think 
you may be using lowercase 'post' instead of 'POST' and this may be triggering 
the bug.

Original comment by brunobg%...@gtempaccount.com on 5 Oct 2010 at 9:37

• Changed state: Accepted

[GoogleCodeExporter]

Thank you for looking into it.

I'm using an uppercase 'POST' for $method. You can see my consumer code here: 
http://github.com/sugestio/sugestio-php/blob/master/SugestioClient.php#L78

addConsumption defines $method and $params before calling the execute method at 
line 193.

Original comment by kris0...@gmail.com on 6 Oct 2010 at 2:19

[GoogleCodeExporter]

I've studied the code and I don't understand why this is happening to you. 
OAuthRequester inherits from OAuthRequestSigner, and the constructor for 
OAuthRequestSigner overrides the $body variable. I just sent a patch for 
lowercase 'post' (bug corrected, thanks, unfortunately not yours) in r160.

The only possible place for this bug seems to be OAuthRequest.php:122:

$parameters .= $this->getRequestBody();

Would you be kind and make a test for me? Please comment this line (and remove 
that return ''; from getRequestBody) and see if this fixes the problem. This 
part of the code is not mine and I don't understand why it is there. I 
contacted the original developer, let's see if he can enlighten me...

Original comment by brunobg%...@gtempaccount.com on 6 Oct 2010 at 8:44

[GoogleCodeExporter]

Yes, commenting out line 122 solves the problem in r155, thanks.

I thought that code was there so that OAuthRequestVerifier could get the POST 
parameters directly from the input stream rather than through $_REQUEST or 
$_POST. I'm not in a position right now to test if the server still works with 
line 122 removed...

Original comment by kris0...@gmail.com on 6 Oct 2010 at 9:10

[GoogleCodeExporter]

Ok, I got a reply from the original developer and this is being handled 
incorrectly. He suggested a possible patch and I'll work on it ASAP.

Original comment by brunobg%...@gtempaccount.com on 7 Oct 2010 at 3:22

[GoogleCodeExporter]

I did not forget about it, just haven't had time lately. It's not a trivial 
fix, so it will take some time. Sorry. Here's what Marc, the original 
developer, said to me, if anybody is interested in tackling this:

The code indeed doesn't check correctly if it should construct from the 
parameters or from the arguments.

Maybe the best is to rewrite the function:

    if ($method === null)
    {
        ... fetch all arguments from the current request
    }
    else
    {
        ... fetch from the parameters, set sensible defaults
    }

Right now all assignments are interwoven, and incorrectly so.
For example there is an assignment to $headers, which overwrites the $headers 
from the parameters.

I think the above split in the logic should help solve the problem.

Original comment by brunobg%...@gtempaccount.com on 28 Oct 2010 at 4:32

[GoogleCodeExporter]

I just ran into this issue myself when using the consumer as a proxy for AJAX 
calls. I concur that it is necessary to differentiate whether OAuthRequest is a 
parent of OAuthRequestSigner (as a consumer) or OAuthRequestVerifier (as a 
server). My workaround has been to add another variable to 
OAuthRequest::__construct() rather than use "$method". And then skip the 
POST/PUT checks appropriately.

OAuthRequest
  function __construct ($signer, $uri = null, $method = null, $parameters = '', $headers = array(), $body = null)
  {
    ...
  // Skip the POST/PUT if signing/making a request
  if (! $signer) {
    // If this is a post then also check the posted variables
    if (strcasecmp($method, 'POST') == 0)
    {
      ...
    else if (strcasecmp($method, 'PUT') == 0)
    {
      ...
    }
  }

From OauthRequestVerifier:
  parent::__construct(FALSE, $uri, $method ...

From OauthRequestSigner:
  parent::__construct(TRUE, $uri, $method ...

Original comment by archul...@seqcentral.com on 2 Mar 2011 at 9:14

[GoogleCodeExporter]

This may have consequences I don't know about but it has worked so far for me. 
If you make the constructor in OAuth Request Read as 
    function __construct ( $uri = null, $method = null, $parameters = '', $headers = array(), $body = null )
    {

        $this->method  = strtoupper($method);
        $this->headers = $headers;
        // Store the values, prepare for oauth
        $this->uri     = $uri;
        $this->body    = $body;
        $this->parseUri($parameters);
        $this->parseHeaders();
        $this->transcodeParams();
    }

and the constructor in OAuthRequestVerifier read as 

function __construct ( $uri = null, $method = null, $params = null )
    {
        if ($params) {
            $encodedParams = array();
            foreach ($params as $key => $value) {
                if (preg_match("/^oauth_/", $key)) {  
                    continue;
                }
                $encodedParams[rawurlencode($key)] = rawurlencode($value);
            }
            $this->param = array_merge($this->param, $encodedParams);
        }

        $this->store = OAuthStore::instance();

        if (is_object($_SERVER))
        {
            // Tainted arrays - the normal stuff in anyMeta
            if (!$method) {
                $method = $_SERVER->REQUEST_METHOD->getRawUnsafe();
            }
            if (empty($uri)) {
                $uri    = $_SERVER->REQUEST_URI->getRawUnsafe();
                $proto = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') ? 'https' : 'http';
                if (strpos($uri, "://") === false) {
                    $uri = sprintf('%s://%s%s', $proto, $_SERVER->HTTP_HOST->getRawUnsafe(), $uri);
                }
            }
        }
        else
        {
            // non anyMeta systems
            if (!$method) {
                if (isset($_SERVER['REQUEST_METHOD'])) {
                    $method = $_SERVER['REQUEST_METHOD'];
                }
                else {
                    $method = 'GET';
                }
            }
            $proto = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') ? 'https' : 'http';
            if (empty($uri)) {
                if (strpos($_SERVER['REQUEST_URI'], "://") !== false) {
                    $uri = $_SERVER['REQUEST_URI'];
                }
                else {
                    $uri = sprintf('%s://%s%s', $proto, $_SERVER['HTTP_HOST'], $_SERVER['REQUEST_URI']);
                }
            }
        }
        $headers      = OAuthRequestLogger::getAllHeaders();
        $this->method = strtoupper($method);

        $parameters = '';

        if (strcasecmp($method, 'POST') == 0)
        {
            // TODO: what to do with 'multipart/form-data'?
            if ($this->getRequestContentType() == 'multipart/form-data')
            {
                // Get the posted body (when available)
                if (!isset($headers['X-OAuth-Test']))
                {
                    $parameters .= $this->getRequestBodyOfMultipart();
                }
            }
            if ($this->getRequestContentType() == 'application/x-www-form-urlencoded')
            {
                // Get the posted body (when available)
                if (!isset($headers['X-OAuth-Test']))
                {
                    $parameters .= $this->getRequestBody();
                }
            }
            else
            {
                $body = $this->getRequestBody();
            }
        }
        else if (strcasecmp($method, 'PUT') == 0)
        {
            $body = $this->getRequestBody();
        }

        parent::__construct($uri, $method,$parameters);

        OAuthRequestLogger::start($this);
    }

Then in all the cases I've tried the problem is fixed. 

Original comment by glademil...@gmail.com on 23 Jan 2013 at 6:02

[GoogleCodeExporter]

For those, who are looking for a workaround, I found the following:

If you set the $_SERVER['CONTENT_TYPE'] to null, or something that isn't 
multipart/form-data or application/x-www-form-urlencoded then the problem does 
not occur anymore.

Not sure, if the project is still maintained, but here's my proposed fix: I've 
overwritten the getRequestBody() and getRequestBodyOfMultipart() in the 
OAuthRequester class, and they both return null. This way the original logic 
can be kept in the base class, but the OAuthRequester will stop reading the 
post input and so it won't add it to the query parameters.

Original comment by Nagy.Att...@gmail.com on 6 May 2014 at 4:05

Attachments:

• oauth-php_request_body_as_parameters_fix.patch