APIv4 - Throw exception instead of munging illegal join aliases

The problem with "fixing" an illegal join alias is that it's then mysterious
what the correct alias will be, leading to bugs when the incorrect alias
gets used throughout the rest of the api params.

Throwing an exception seems like a better way to ensure developers
are alerted to the error.

diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php
index 7232d02b3f978c973dc2ad9826a62f3f60aed1e9..c3e6e3c09af78d86123d65413f881b3a1bff1e5c 100644 (file)
--- a/Civi/Api4/Query/Api4SelectQuery.php
+++ b/Civi/Api4/Query/Api4SelectQuery.php
@@ -680,7 +680,10 @@ class Api4SelectQuery {
         continue;
       }
       // Ensure alias is a safe string, and supply default if not given
-      $alias = $alias ? \CRM_Utils_String::munge($alias, '_', 256) : strtolower($entity);
+      $alias = $alias ?: strtolower($entity);
+      if ($alias === self::MAIN_TABLE_ALIAS || !preg_match('/^[-\w]{1,256}$/', $alias)) {
+        throw new \API_Exception('Illegal join alias: "' . $alias . '"');
+      }
       // First item in the array is a boolean indicating if the join is required (aka INNER or LEFT).
       // The rest are join conditions.
       $side = array_shift($join);

diff --git a/tests/phpunit/api/v4/Action/FkJoinTest.php b/tests/phpunit/api/v4/Action/FkJoinTest.php
index d0ed5c87b1b064d37bde1ff25ddc1689e4f2e22c..91aff6ea3b1b06bfc2c31c1daad9631313e6d4e0 100644 (file)
--- a/tests/phpunit/api/v4/Action/FkJoinTest.php
+++ b/tests/phpunit/api/v4/Action/FkJoinTest.php
@@ -121,6 +121,40 @@ class FkJoinTest extends UnitTestCase {
     $this->assertNotContains($this->getReference('test_contact_1')['id'], $contacts);
   }
 
+  public function testInvalidJoinAlias() {
+    // Not allowed to use same alias as the base table
+    try {
+      Contact::get(FALSE)->addJoin('Address AS a')->execute();
+    }
+    catch (\API_Exception $e) {
+      $message = $e->getMessage();
+    }
+    $this->assertEquals('Illegal join alias: "a"', $message);
+
+    // Not allowed to use dots in the alias
+    try {
+      Contact::get(FALSE)->addJoin('Address AS add.ress')->execute();
+    }
+    catch (\API_Exception $e) {
+      $message = $e->getMessage();
+    }
+    $this->assertEquals('Illegal join alias: "add.ress"', $message);
+
+    // Not allowed to use an alias > 256 characters
+    try {
+      $longAlias = str_repeat('z', 257);
+      Contact::get(FALSE)->addJoin("Address AS $longAlias")->execute();
+    }
+    catch (\API_Exception $e) {
+      $message = $e->getMessage();
+    }
+    $this->assertEquals("Illegal join alias: \"$longAlias\"", $message);
+
+    // Alpha-numeric with dashes 256 characters long - weird but allowed
+    $okAlias = str_repeat('-0_a-9Z_', 32);
+    Contact::get(FALSE)->addJoin("Address AS $okAlias")->execute();
+  }
+
   public function testJoinToTheSameTableTwice() {
     $cid1 = Contact::create(FALSE)
       ->addValue('first_name', 'Aaa')