A major problem with this line of code

  // Translate empty callback url to oob (out of band).
  if (empty($values['callback_url'])) {
    $values['callback_url'] = 'oob';
  }
  // Add scheme if missing, and if the callback_url isn't out of band.
  else if ($values['callback_url'] != 'oob' && preg_match('/^http:\/\/|https:\/\//', $values['callback_url']) === 0) {
    //TODO: What about custom callback url:s used by eg iphone-apps? We should allow them - right?
    $values['callback_url'] = 'http://' . $values['callback_url'];
  }

  // Remove trailing slash
  $values['callback_url'] = rtrim($values['callback_url'], '/');

Is that I cannot set a custom callback, just like the comment states.

The added patch removes the code where it automatically adds http if it doesnt exist. Ive also added a description so it states this to the user.

The only way around this is to export the Context to code and change the callback in that code, which is wonky, especially if I want it in the database.

Ive also updated the trailing slash code to only do this in cases where its not // so that I can have a callback of just iphoneappname:// Which is all iOS requires to get the notification that my request tokens are authorized.

Patch inc.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kylebrowning’s picture

kylebrowning’s picture

Status: Active » Needs review
MrMaksimize’s picture

Status: Needs review » Needs work

Hi Kyle,
Was testing this out today, and looks like that patch unfortunately is not the end of the story.... Or it may be.

In oauth_common.pages.inc inside the oauth_common_form_authorize_submit

if (!empty($callback) && $callback !== 'oob') {
    // Pick the callback url apart and add the token parameter
    $callback = parse_url($callback);
    $query = array();
    parse_str($callback['query'], $query);
    $query['oauth_token'] = $token->key;
    // TODO: Verify store oauth_verifier with expires
    // Doc: http://tools.ietf.org/html/rfc5849#section-2.2
    $query['oauth_verifier'] = md5(microtime() . mt_rand());
    $callback['query'] = http_build_query($query, 'idx_', '&');

    // Return to the consumer site
    header('Location: ' . _oauth_common_glue_url($callback), TRUE, 302);
    exit;
  }

The parse_url function returns FALSE if you have entered iphoneapp:// as the callback url. It also happens in the same file in oauth_common_form_authorize():

// Pick the callback url apart and add the token parameter
      $callback = parse_url($consumer->callback_url);
      $query = array();
      parse_str($callback['query'], $query);
      $query['oauth_token'] = $token->key;
      $callback['query'] = http_build_query($query, 'idx_', '&');

However it seems to work fine with iphoneapp://some-path because it's able to parse the url.

I'm not sure if this is really desired functionality - I could see it being a best practice in the app to have the oauth callback at a specific path rather than the app protocol root.

In that case - I would vote for updating the ['description'] on the callback form element to be updated.

However, if the oauth should be allowed to call back to a global app namespace, I think some regex is required to account for that and not fire parse_url in those scenarios.

Setting this back to needs work - let me know what you think :)

MrMaksimize’s picture

Also - saw that this patch was agains the d7 version of the module - and testing with drupal_parse_url (introduced in d7 - http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_p...) it seems to work :)

kylebrowning’s picture

Oauth should be allowed to set whatever URL it wants as a callback, theres no requirement in the spec that states it needs to be http://

So I suppose I will work on some regex unless you are fine with switching it to use drupal_parse_url.

Its strange that in my testing this code works flawlessly and my callbacks are called even those parse_url returns false.

What version of PHP are you using?

MrMaksimize’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Generated a patch that validates the form submission to have a proper url

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

Just tested this and it looks good to me.

juampynr’s picture

Status: Reviewed & tested by the community » Fixed

Removed trailing white space, wrapped a string with t() and committed. Thanks!

http://drupalcode.org/project/oauth.git/commitdiff/5cb2e79

kylebrowning’s picture

Title: Cannot set custom callback for iOS application » Cannot leave callback formfield blank even though it is not required.
Status: Fixed » Needs work

Callback is not a required field but it appears that if you hit submit without filling in it, it complains its not a valid http schema.

:/

Deciphered’s picture

Status: Needs work » Needs review
FileSize
692 bytes

Yeah... that's just silly. If it's not required, don't validate it when it's empty.

Patch attached.

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community
juampynr’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.