diff --git a/includes/mollom.class.inc b/includes/mollom.class.inc index a5ed8d8..51a7b5b 100644 --- a/includes/mollom.class.inc +++ b/includes/mollom.class.inc @@ -36,13 +36,33 @@ abstract class Mollom { const RESPONSE_ERROR = 511; /** - * Server communication failure code: Authentication error. + * Client communication failure code: Bad request. + * + * Used in case of a too large system time offset from UTC. + * + * @see MollomBadRequestException + * @see Mollom::TIME_OFFSET_MAX + */ + const REQUEST_ERROR = 400; + + /** + * Client communication failure code: Authentication error. * * @see MollomAuthenticationException */ const AUTH_ERROR = 401; /** + * Maximum allowed time offset from UTC in seconds allowed for the client's local time. + * + * @see Mollom::handleRequest() + * @see MollomBadRequestException + * + * @var integer + */ + const TIME_OFFSET_MAX = 600; + + /** * The public Mollom API key to use for request authentication. * * @var string @@ -111,6 +131,7 @@ abstract class Mollom { * If not TRUE, then the value is either one of * - Mollom::NETWORK_ERROR * - Mollom::RESPONSE_ERROR + * - Mollom::REQUEST_ERROR * - Mollom::AUTH_ERROR * or the actual HTTP status code returned by the server. * @@ -278,6 +299,10 @@ abstract class Mollom { try { $result = $this->handleRequest($method, $server, $path, $data, $expected); } + catch (MollomBadRequestException $e) { + // This is an irrecoverable error, so don't try further. + break; + } catch (MollomAuthenticationException $e) { // This is an irrecoverable error, so don't try further. break; @@ -327,6 +352,11 @@ abstract class Mollom { if ($this->lastResponseCode === self::RESPONSE_ERROR) { return $this->lastResponseCode; } + // Return a request error, which always requires to take client-side + // measures to resolve the problem. + if ($this->lastResponseCode === self::REQUEST_ERROR) { + return $this->lastResponseCode; + } // Return an authentication error, which may require special client-side // processing. if ($this->lastResponseCode === self::AUTH_ERROR) { @@ -464,6 +494,19 @@ abstract class Mollom { throw new MollomNetworkException('Network error.', self::NETWORK_ERROR, NULL, $this, $request_info); } if ($response->code == self::AUTH_ERROR) { + // Check whether authentication failed due to a too large time offset. + if (isset($response->headers['date'])) { + // Parse the 'data' header in the response into a UNIX timestamp; + // strtotime() normally performs poorly on date operations, but in + // this case, there is a standardized date format and timezone + // conversion, so it is safe to use. + $offset = abs(time() - strtotime($response->headers['date'])); + // The abs() above turns a negative offset into an absolute integer + // value, so the difference is always positive. + if ($offset > self::TIME_OFFSET_MAX) { + throw new MollomBadRequestException(sprintf('Invalid client system time: Too large offset from UTC: %s seconds.', $offset), self::REQUEST_ERROR, NULL, $this, $request_info); + } + } throw new MollomAuthenticationException('Invalid authentication.', self::AUTH_ERROR, NULL, $this, $request_info); } if ($response->code == self::RESPONSE_ERROR || $response->code >= 500) { @@ -907,12 +950,14 @@ abstract class Mollom { * * @return mixed * TRUE on success. On failure, the error response code returned by the - * server; either Mollom::AUTH_ERROR or Mollom::NETWORK_ERROR. + * server; either Mollom::REQUEST_ERROR, Mollom::AUTH_ERROR or + * Mollom::NETWORK_ERROR. */ public function verifyKeys() { $data = $this->getClientInformation(); $result = $this->updateSite($data); - // lastResponseCode will either be TRUE, AUTH_ERROR, or NETWORK_ERROR. + // lastResponseCode will either be TRUE, REQUEST_ERROR, AUTH_ERROR, or + // NETWORK_ERROR. return $this->lastResponseCode === TRUE ? TRUE : $this->lastResponseCode; } @@ -1334,6 +1379,18 @@ class MollomAuthenticationException extends MollomException { } /** + * Mollom error due to bad client request exception. + * + * Thrown in case the local time diverges too much from UTC. + * + * @see Mollom::TIME_OFFSET_MAX + * @see Mollom::REQUEST_ERROR + * @see Mollom::handleRequest() + */ +class MollomBadRequestException extends MollomException { +} + +/** * Mollom server response exception. * * Thrown when a request to a Mollom server succeeds, but the response does not diff --git a/mollom.install b/mollom.install index ac98b1d..ec67f88 100644 --- a/mollom.install +++ b/mollom.install @@ -53,6 +53,13 @@ function mollom_requirements($phase = 'runtime', $check = TRUE) { '!admin-message' => $admin_message, )); } + // Bad request: Invalid client system time: Too large offset from UTC. + elseif ($status['keys valid'] === Mollom::REQUEST_ERROR) { + $requirements['mollom']['value'] = t('Client error'); + $requirements['mollom']['description'] = t('The server time of this site is incorrect. The time of the operating system is not synchronized with the Coordinated Universal Time (UTC), which prevents a successful authentication with Mollom. The maximum allowed offset is @minutes minutes. Please consult your hosting provider or server operator to correct the server time.', array( + '@minutes' => Mollom::TIME_OFFSET_MAX / 60, + )); + } // Invalid API keys. elseif ($status['keys valid'] === Mollom::AUTH_ERROR) { $requirements['mollom']['value'] = t('Invalid'); diff --git a/mollom.module b/mollom.module index e56b184..6199d2d 100644 --- a/mollom.module +++ b/mollom.module @@ -1417,12 +1417,23 @@ function _mollom_status($reset = FALSE) { 'message' => 'API keys are valid.', ), WATCHDOG_INFO); } - // If the response is neither positive nor a negative authentication error, - // the previous status was positive, and if the Mollom service might be + elseif ($status['keys valid'] === Mollom::REQUEST_ERROR) { + mollom_log(array( + 'message' => 'Invalid client configuration.', + ), WATCHDOG_ERROR); + } + elseif ($status['keys valid'] === Mollom::AUTH_ERROR) { + mollom_log(array( + 'message' => 'Invalid API keys.', + ), WATCHDOG_ERROR); + } + // If the response is neither positive nor a known negative error, and + // the previous status was positive, then the Mollom service might be // temporarily down. In this case, return an enforced RESPONSE_ERROR to - // indicate a server-side service failure. + // indicate a server-side service failure, but do not update the stored + // status. // @todo Test this. - elseif ($status['keys valid'] !== Mollom::AUTH_ERROR && $current_status['keys valid'] === TRUE) { + elseif ($current_status['keys valid'] === TRUE) { $status['keys valid'] = Mollom::RESPONSE_ERROR; // Do not update the stored configuration status with the bogus result // caused by the server-side failure. @@ -1431,11 +1442,6 @@ function _mollom_status($reset = FALSE) { 'message' => 'API keys could not be verified.', ), WATCHDOG_WARNING); } - else { - mollom_log(array( - 'message' => 'Invalid API keys.', - ), WATCHDOG_ERROR); - } } // Otherwise, if there are no keys, they cannot be valid. elseif (!$status['keys']) { diff --git a/tests/mollom_test_server.module b/tests/mollom_test_server.module index f55822a..f4bef2b 100644 --- a/tests/mollom_test_server.module +++ b/tests/mollom_test_server.module @@ -172,6 +172,14 @@ function mollom_test_server_rest_validate_auth() { $header = mollom_test_server_rest_get_auth_header(); $sites = variable_get('mollom_test_server_site', array()); + // Validate the timestamp. + $client_time = $header['oauth_timestamp']; + $time = REQUEST_TIME; + $offset = abs($time - $client_time); + if ($offset > Mollom::TIME_OFFSET_MAX) { + return FALSE; + } + $sent_signature = $header['oauth_signature']; unset($header['oauth_signature']); @@ -180,7 +188,6 @@ function mollom_test_server_rest_validate_auth() { Mollom::rawurlencode($GLOBALS['base_url'] . '/' . $_GET['q']), Mollom::rawurlencode(Mollom::httpBuildQuery($data + $header)), )); - $time = $header['oauth_timestamp']; $nonce = $header['oauth_nonce']; if (!isset($sites[$header['oauth_consumer_key']]['privateKey'])) {