I'm marking this as critical, because although it's only a notice there is an underlying logic error and the error message is very visible to new users on a site.

This code produces the error:

function openid_authentication_request($claimed_id, $identity, $return_to = '', $assoc_handle = '', $version = 2) {
  module_load_include('inc', 'openid');

  $ns = ($version == 2) ? OPENID_NS_2_0 : OPENID_NS_1_0;
  $request =  array(
    'openid.ns' => $ns,
    'openid.mode' => 'checkid_setup',
    'openid.identity' => $identity,
    'openid.claimed_id' => $claimed_id,
    'openid.assoc_handle' => $assoc_handle,
    'openid.return_to' => $return_to,
  );

  if ($version == 2) {
    $request['openid.realm'] = url('', array('absolute' => TRUE));
  }
  else {
    $request['openid.trust_root'] = $realm; // NOTE: $realm is NOT DEFINED anywhere in this scope, under ANY conditions.
  }

  // Simple Registration
  $request['openid.sreg.required'] = 'nickname,email';
  $request['openid.ns.sreg'] = "http://openid.net/extensions/sreg/1.1";

  $request = array_merge($request, module_invoke_all('openid', 'request', $request));

  return $request;
}

I don't know what variable the coder meant to use instead of $realm, or what value it is supposed to contain. But a brief scan over the function code shows that unless openid.inc defines $realm in its global code (and it doesn't), the variable scope of this function does not ever contain a $realm variable.

CommentFileSizeAuthor
#4 openid-realm-undefined-222042.patch597 byteswalkah

Comments

cburschka’s picture

Title: Undefined variable » Undefined variable $realm in openid_authentication_request

Changing title.

gábor hojtsy’s picture

Hm, the notice will not be visible in a production environment, since Drupal 6 does not ship with notices turned on. Apart from that whether this is critical or not depends on what an empty trust root leads to. Is it a security problem?

cburschka’s picture

Priority: Critical » Normal

Fine, the priority is determined by the nature of the result, not the nature of the cause. I don't know if its a security problem.

But basically, it's a programming error. There should be no circumstances under which replacing $realm with NULL makes any difference, so the else block does not actually do anything. If it's not meant to do anything, then that's just redundant code - but until that's established, it should be examined a bit more carefully.

walkah’s picture

Assigned: Unassigned » walkah
Status: Active » Needs review
StatusFileSize
new597 bytes

Simple programming error... one line patch to fix attached.

cburschka’s picture

This appears to make sense, and it gets rid of the notice.

keturn’s picture

On myopenid.com we're receiving some requests from drupal installations that are sending openid.trust_root as the empty string. This would explain it.

htalvitie’s picture

Applying the patch fixed two cases where an empty realm name got passed to the authenticator, which produced strange output on the confirmation dialog on the remote site.

The problem seems to be isolated to cases, where the protocol used is OpenID 1.0.

willmoy’s picture

Status: Needs review » Fixed

Already fixed in HEAD:

  if ($version == 2) {
    $request['openid.ns'] = OPENID_NS_2_0;
    $request['openid.claimed_id'] = $claimed_id;
    $request['openid.realm'] = url('', array('absolute' => TRUE));
  }
  else {
    $request['openid.trust_root'] = url('', array('absolute' => TRUE));
  }

which is correct as per

http://openid.net/specs/openid-authentication-1_1.html#anchor22
http://openid.net/specs/openid-authentication-2_0.html#realms

Status: Fixed » Closed (fixed)

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