In oauth_commons.pages.inc on line 134, you'll find

  else {
    $query = $_GET;
    unset($query['q']); // why are there so few q's?
    drupal_goto('user/login', array(
      'destination' => url('oauth/authorize', array(
        'query' => $query,
        'absolute' => FALSE,
      )),
    ));
  }

A problem is introduce by the combination of using drupal_goto() and url() with in both cases $query as an array.

In url() this happens with $query as an array:

  if (is_array($options['query'])) {
    $options['query'] = drupal_query_string_encode($options['query']);
  }

The drupal_query_string_encode() is called twice which causes a double encoding.

The fix:

  else {
    $query = $_GET;
    unset($query['q']); // why are there so few q's?
    drupal_goto('user/login', 'destination=' . url('oauth/authorize', 
      array(
        'query' => $query,
        'absolute' => FALSE,
      )
    ));
  }

Find patch attached. (applies by both 6.x-2.x and 6.x-1.x

CommentFileSizeAuthor
oauth_commons.pages-double-encode.patch567 bytestoemaz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

toemaz’s picture

toemaz CreditAttribution: toemaz commented
Status: Active » Needs review

Changing to right status

toemaz’s picture

toemaz CreditAttribution: toemaz commented
Project: OAuth Common (deprecated) » OAuth 1.0
Version: 6.x-2.x-dev » 6.x-3.x-dev

Moving from oauth_common to oauth as advised by voxpelli. Patch still applies.

toemaz’s picture

toemaz CreditAttribution: toemaz commented

Just to make sure that the problem of the double encoding is understood, this is how double encoded destination url looks like:

http://yourdomain.com/user/login?destination=%2Foauth%2Fauthorize%3Foaut...

voxpelli’s picture

voxpelli CreditAttribution: voxpelli commented

This seems to somehow be related to #922110: user/login destination parameter incorrectly including base directory on authorize page - right? Should the patches be merged or does one replace the other?

miqmago’s picture

miqmago CreditAttribution: miqmago commented
Version: 6.x-3.x-dev » 7.x-3.x-dev

I'm having same problem on drupal 7. Patch doesn't solve.

Getting the following error after the patch here: Recoverable fatal error: Argument 2 passed to drupal_goto() must be an array, string given, called in C:\Users\Miquel\Documents\00\03-www\public_html\sites\all\modules\oauth\oauth_common.pages.inc on line 236 and defined en drupal_goto() (línea 668 de C:\Users\Miquel\Documents\00\03-www\public_html\includes\common.inc).

#922110 Also not solving the problem.

Candidate solution:

Seems that changing from:

    drupal_goto('user/login', array('query' => array(
      'destination' => url('oauth/authorize', array(
        'query' => $query,
      )),
    )));

to:

    drupal_goto('user/login', array('query' => array(
      'destination' => 'oauth/authorize?' . drupal_http_build_query($query),
      ),
    ));

solves the problem. If it is a good solution I can post the patch.
I'm not sure if 'oauth/authorize?' . drupal_http_build_query() could affect anywherelse.

chustedde’s picture

chustedde CreditAttribution: chustedde commented

I was having the issue in #922110: user/login destination parameter incorrectly including base directory on authorize page (but in D7), and I was able to fix it using your solution, so I posted a patch there.