Skip to content

Commit

Permalink
Merge pull request #19727 from totten/master-authx-rest
Browse files Browse the repository at this point in the history
(dev/core#2077) Make 'civicrm/ajax/rest' interoperable with 'extern/rest.php' parameters
  • Loading branch information
colemanw authored Feb 5, 2022
2 parents 87d25a7 + 8d5feab commit e859e64
Show file tree
Hide file tree
Showing 9 changed files with 210 additions and 38 deletions.
5 changes: 1 addition & 4 deletions CRM/Api4/Page/AJAX.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@ class CRM_Api4_Page_AJAX extends CRM_Core_Page {
*/
public function run() {
$config = CRM_Core_Config::singleton();
if (!$config->debug && (!array_key_exists('HTTP_X_REQUESTED_WITH', $_SERVER) ||
$_SERVER['HTTP_X_REQUESTED_WITH'] != "XMLHttpRequest"
)
) {
if (!$config->debug && !CRM_Utils_REST::isWebServiceRequest()) {
$response = [
'error_code' => 401,
'error_message' => "SECURITY ALERT: Ajax requests can only be issued by javascript clients, eg. CRM.api4().",
Expand Down
4 changes: 3 additions & 1 deletion CRM/Core/Page/AJAX.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ public static function returnJsonResponse($response) {
$output = json_encode($response);

// CRM-11831 @see http://www.malsup.com/jquery/form/#file-upload
if (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest') {
// COMMENT: Wouldn't the `Accept:` header be more appropriate? Only use `X-Requested-With:` as a
// fallback where `Accept:` is missing?
if (CRM_Utils_REST::isWebServiceRequest()) {
CRM_Utils_System::setHttpHeader('Content-Type', 'application/json');
}
else {
Expand Down
62 changes: 50 additions & 12 deletions CRM/Utils/REST.php
Original file line number Diff line number Diff line change
Expand Up @@ -399,9 +399,7 @@ public static function loadTemplate() {
unset($param['q']);
$smarty->assign_by_ref("request", $param);

if (!array_key_exists('HTTP_X_REQUESTED_WITH', $_SERVER) ||
$_SERVER['HTTP_X_REQUESTED_WITH'] != "XMLHttpRequest"
) {
if (!self::isWebServiceRequest()) {

$smarty->assign('tplFile', $tpl);
$config = CRM_Core_Config::singleton();
Expand Down Expand Up @@ -434,10 +432,7 @@ public static function ajaxJson() {

require_once 'api/v3/utils.php';
$config = CRM_Core_Config::singleton();
if (!$config->debug && (!array_key_exists('HTTP_X_REQUESTED_WITH', $_SERVER) ||
$_SERVER['HTTP_X_REQUESTED_WITH'] != "XMLHttpRequest"
)
) {
if (!$config->debug && !self::isWebServiceRequest()) {
$error = civicrm_api3_create_error("SECURITY ALERT: Ajax requests can only be issued by javascript clients, eg. CRM.api3().",
[
'IP' => $_SERVER['REMOTE_ADDR'],
Expand Down Expand Up @@ -499,11 +494,7 @@ public static function ajax() {
// restrict calls to this etc
// the request has to be sent by an ajax call. First line of protection against csrf
$config = CRM_Core_Config::singleton();
if (!$config->debug &&
(!array_key_exists('HTTP_X_REQUESTED_WITH', $_SERVER) ||
$_SERVER['HTTP_X_REQUESTED_WITH'] != "XMLHttpRequest"
)
) {
if (!$config->debug && !self::isWebServiceRequest()) {
require_once 'api/v3/utils.php';
$error = civicrm_api3_create_error("SECURITY ALERT: Ajax requests can only be issued by javascript clients, eg. CRM.api3().",
[
Expand Down Expand Up @@ -636,4 +627,51 @@ public function loadCMSBootstrap() {
}
}

/**
* Does this request appear to be a web-service request?
*
* It is important to distinguish regular browser-page-loads from web-service-requests. Regular
* page-loads can be CSRF vectors, and we don't web-services to run via CSRF.
*
* @return bool
* TRUE if the current request appears to either XMLHttpRequest or non-browser-based.
* Indicated by either (a) custom headers like `X-Request-With`/`X-Civi-Auth`
* or (b) strong-secret-params that could theoretically appear in URL bar but which
* cannot be meaningfully forged for CSRF purposes (like `?api_key=SECRET` or `?_authx=SECRET`).
* FALSE if the current request looks like a standard browser request. This request may be generated by
* <A HREF>, <IFRAME>, <IMG>, `Location:`, or similar CSRF vector.
*/
public static function isWebServiceRequest(): bool {
if (($_SERVER['HTTP_X_REQUESTED_WITH'] ?? NULL) === 'XMLHttpRequest') {
return TRUE;
}

// If authx is enabled, and if the user gives a credential, it will store metadata.
$authx = \CRM_Core_Session::singleton()->get('authx');
$allowFlows = [
// Some flows are resistant to CSRF. Allow these:

// <legacyrest> Current request has valid `?api_key=SECRET&key=SECRET` ==> Strong-secret params
'legacyrest',

// <param> Current request has valid `?_authx=SECRET` ==> Strong-secret param
'param',

// <xheader> Current request has valid `X-Civi-Auth:` ==> Custom header AND strong-secret param
'xheader',

// Other flows are not resistant to CSRF on their own (need combo w/`X-Requested-With:`).
// Ignore these:
// <login> Relies on a session `Cookie:` (which browsers re-send automatically).
// <auto> First request might be resistant, but all others use session `Cookie:`.
// <header> Browsers often retain list of credentials and re-send automatically.
];

if (!empty($authx) && in_array($authx['flow'], $allowFlows)) {
return TRUE;
}

return FALSE;
}

}
42 changes: 42 additions & 0 deletions ext/authx/Civi/Authx/LegacyRestAuthenticator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php
/*
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC. All rights reserved. |
| |
| This work is published under the GNU AGPLv3 license with some |
| permitted exceptions and without any warranty. For full license |
| and copyright information, see https://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/

namespace Civi\Authx;

use GuzzleHttp\Psr7\Response;

/**
* Historically, 'extern/rest.php' and 'civicrm/ajax/rest' were similar interfaces
* based on the same controller, but they used different authentication styles.
*
* This authenticator is activated if one requests 'civicrm/ajax/rest' using the
* authentication style of 'extern/rest.php'.
*
* @package Civi\Authx
*/
class LegacyRestAuthenticator extends Authenticator {

protected function reject($message = 'Authentication failed') {
$data = ["error_message" => "FATAL: $message", "is_error" => 1];
$r = new Response(200, ['Content-Type' => 'text/javascript'], json_encode($data));
\CRM_Utils_System::sendResponse($r);
}

protected function login(AuthenticatorTarget $tgt) {
parent::login($tgt);
\Civi::dispatcher()->addListener('hook_civicrm_permission_check', function ($e) {
if ($e->permission === 'access AJAX API') {
$e->granted = TRUE;
}
});
}

}
8 changes: 8 additions & 0 deletions ext/authx/authx.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@
_authx_redact(['_authx']);
}
}

// Accept legacy auth (?key=...&api_key=...) for 'civicrm/ajax/rest' and 'civicrm/ajax/api4/*'.
// The use of `?key=` could clash on some endpoints. Only accept on a small list of endpoints that are compatible with it.
if (count($e->args) > 2 && $e->args[1] === 'ajax' && in_array($e->args[2], ['rest', 'api4'])) {
if ((!empty($_REQUEST['api_key']) || !empty($_REQUEST['key']))) {
return (new \Civi\Authx\LegacyRestAuthenticator())->auth($e, ['flow' => 'legacyrest', 'cred' => 'Bearer ' . $_REQUEST['api_key'] ?? '', 'siteKey' => $_REQUEST['key'] ?? NULL]);
}
}
});

/**
Expand Down
5 changes: 4 additions & 1 deletion ext/authx/settings/authx.setting.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* @copyright CiviCRM LLC https://civicrm.org/licensing
*/
$_authx_settings = function() {
$flows = ['param', 'header', 'xheader', 'login', 'auto', 'script', 'pipe'];
$flows = ['param', 'header', 'xheader', 'login', 'auto', 'script', 'pipe', 'legacyrest'];
$basic = [
'group_name' => 'CiviCRM Preferences',
'group' => 'authx',
Expand Down Expand Up @@ -77,6 +77,9 @@
];
}

// Override defaults for a few specific elements
$s['authx_legacyrest_cred']['default'] = ['jwt', 'api_key'];
$s['authx_legacyrest_user']['default'] = 'require';
$s['authx_param_cred']['default'] = ['jwt', 'api_key'];
$s['authx_header_cred']['default'] = ['jwt', 'api_key'];
$s['authx_xheader_cred']['default'] = ['jwt', 'api_key'];
Expand Down
45 changes: 45 additions & 0 deletions tests/phpunit/E2E/Extern/AuthxRestTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php
/*
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC. All rights reserved. |
| |
| This work is published under the GNU AGPLv3 license with some |
| permitted exceptions and without any warranty. For full license |
| and copyright information, see https://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/

/**
* Verify that the REST API bindings correctly parse and authenticate requests.
*
* @group e2e
*/
class E2E_Extern_AuthxRestTest extends E2E_Extern_BaseRestTest {

public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();
\Civi\Test::e2e()
->install(['authx'])
->callback(
function() {
\CRM_Utils_System::synchronizeUsers();
},
'synchronizeUsers'
)
->apply();
}

protected function getRestUrl() {
return CRM_Utils_System::url('civicrm/ajax/rest', NULL, TRUE, NULL, FALSE, TRUE);
}

public function apiTestCases() {
$r = parent::apiTestCases();
$r = array_filter($r, function($case) {
// The 'civicrm/ajax/rest' end-point does not support '?q' inputs.
return !isset($case[0]['q']);
});
return $r;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,17 @@
+--------------------------------------------------------------------+
*/

use Civi\Test\HttpTestTrait;

/**
* Verify that the REST API bindings correctly parse and authenticate requests.
*
* @group e2e
*/
class E2E_Extern_RestTest extends CiviEndToEndTestCase {
abstract class E2E_Extern_BaseRestTest extends CiviEndToEndTestCase {

use HttpTestTrait;

protected $url;
protected static $api_key;
protected $session_id;
Expand Down Expand Up @@ -46,10 +51,7 @@ protected function setUp(): void {
$this->old_api_keys = [];
}

protected function getRestUrl() {
return CRM_Core_Resources::singleton()
->getUrl('civicrm', 'extern/rest.php');
}
abstract protected function getRestUrl();

protected function tearDown(): void {
if (!empty($this->old_api_keys)) {
Expand Down Expand Up @@ -211,9 +213,11 @@ public function apiTestCases() {
public function testAPICalls($query, $is_error) {
$this->updateAdminApiKey();

$client = CRM_Utils_HttpClient::singleton();
list($status, $data) = $client->post($this->getRestUrl(), $query);
$this->assertEquals(CRM_Utils_HttpClient::STATUS_OK, $status);
$http = $this->createGuzzle(['http_errors' => FALSE]);
$response = $http->post($this->getRestUrl(), ['form_params' => $query]);
$this->assertStatusCode(200, $response);
$data = (string) $response->getBody();

$result = json_decode($data, TRUE);
if ($result === NULL) {
$msg = print_r(array(
Expand All @@ -231,7 +235,7 @@ public function testAPICalls($query, $is_error) {
* a real user. Submit in "?entity=X&action=X" notation
*/
public function testNotCMSUser_entityAction() {
$client = CRM_Utils_HttpClient::singleton();
$http = $this->createGuzzle(['http_errors' => FALSE]);

//Create contact with api_key
$test_key = "testing1234";
Expand All @@ -251,9 +255,10 @@ public function testNotCMSUser_entityAction() {
"json" => "1",
"api_key" => $test_key,
);
list($status, $data) = $client->post($this->getRestUrl(), $params);
$this->assertEquals(CRM_Utils_HttpClient::STATUS_OK, $status);
$result = json_decode($data, TRUE);

$response = $http->post($this->getRestUrl(), ['form_params' => $params]);
$this->assertStatusCode(200, $response);
$result = json_decode((string) $response->getBody(), TRUE);
$this->assertNotNull($result);
$this->assertAPIErrorCode($result, 1);
}
Expand All @@ -264,7 +269,7 @@ public function testNotCMSUser_entityAction() {
*/
public function testGetCorrectUserBack() {
$this->updateAdminApiKey();
$client = CRM_Utils_HttpClient::singleton();
$http = $this->createGuzzle(['http_errors' => FALSE]);

//Create contact with api_key
// The key associates with a real contact but not a real user
Expand All @@ -276,9 +281,9 @@ public function testGetCorrectUserBack() {
"api_key" => self::getApiKey(),
"id" => "user_contact_id",
);
list($status, $data) = $client->post($this->getRestUrl(), $params);
$this->assertEquals(CRM_Utils_HttpClient::STATUS_OK, $status);
$result = json_decode($data, TRUE);
$response = $http->post($this->getRestUrl(), ['form_params' => $params]);
$this->assertStatusCode(200, $response);
$result = json_decode((string) $response->getBody(), TRUE);
$this->assertNotNull($result);
$this->assertEquals($result['id'], $this->adminContactId);
}
Expand All @@ -288,7 +293,7 @@ public function testGetCorrectUserBack() {
* a real user. Submit in "?q=civicrm/$entity/$action" notation
*/
public function testNotCMSUser_q() {
$client = CRM_Utils_HttpClient::singleton();
$http = $this->createGuzzle(['http_errors' => FALSE]);

//Create contact with api_key
$test_key = "testing1234";
Expand All @@ -307,9 +312,10 @@ public function testNotCMSUser_q() {
"json" => "1",
"api_key" => $test_key,
);
list($status, $data) = $client->post($this->getRestUrl(), $params);
$this->assertEquals(CRM_Utils_HttpClient::STATUS_OK, $status);
$result = json_decode($data, TRUE);
$response = $http->post($this->getRestUrl(), ['form_params' => $params]);

$this->assertStatusCode(200, $response);
$result = json_decode((string) $response->getBody(), TRUE);
$this->assertNotNull($result);
$this->assertAPIErrorCode($result, 1);
}
Expand Down
31 changes: 31 additions & 0 deletions tests/phpunit/E2E/Extern/LegacyRestTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/*
+--------------------------------------------------------------------+
| Copyright CiviCRM LLC. All rights reserved. |
| |
| This work is published under the GNU AGPLv3 license with some |
| permitted exceptions and without any warranty. For full license |
| and copyright information, see https://civicrm.org/licensing |
+--------------------------------------------------------------------+
*/

/**
* Verify that the REST API bindings correctly parse and authenticate requests.
*
* @group e2e
*/
class E2E_Extern_LegacyRestTest extends E2E_Extern_BaseRestTest {

protected function setUp(): void {
if (CIVICRM_UF === 'Drupal8') {
$this->markTestSkipped('Legacy extern/rest.php does not apply to Drupal 8+');
}
parent::setUp();
}

protected function getRestUrl() {
return CRM_Core_Resources::singleton()
->getUrl('civicrm', 'extern/rest.php');
}

}

0 comments on commit e859e64

Please sign in to comment.