Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
openid.module
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
15 Feb 2008 at 09:35 UTC
Updated:
3 Oct 2009 at 11:20 UTC
Jump to comment: Most recent file
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | openid-realm-undefined-222042.patch | 597 bytes | walkah |
Comments
Comment #1
cburschkaChanging title.
Comment #2
gábor hojtsyHm, 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?
Comment #3
cburschkaFine, 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
$realmwithNULLmakes 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.Comment #4
walkah commentedSimple programming error... one line patch to fix attached.
Comment #5
cburschkaThis appears to make sense, and it gets rid of the notice.
Comment #6
keturn commentedOn myopenid.com we're receiving some requests from drupal installations that are sending openid.trust_root as the empty string. This would explain it.
Comment #7
htalvitie commentedApplying 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.
Comment #8
willmoy commentedAlready fixed in HEAD:
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