From 1930b3e80feeaf4d8df1d1d81a19e3071b760402 Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Mon, 9 Aug 2021 12:56:38 -0400
Subject: [PATCH] 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.
---
 Civi/Api4/Query/Api4SelectQuery.php        |  5 +++-
 tests/phpunit/api/v4/Action/FkJoinTest.php | 34 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php
index 7232d02b3f97..c3e6e3c09af7 100644
--- a/Civi/Api4/Query/Api4SelectQuery.php
+++ b/Civi/Api4/Query/Api4SelectQuery.php
@@ -680,7 +680,10 @@ private function addExplicitJoins() {
         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 d0ed5c87b1b0..91aff6ea3b1b 100644
--- a/tests/phpunit/api/v4/Action/FkJoinTest.php
+++ b/tests/phpunit/api/v4/Action/FkJoinTest.php
@@ -121,6 +121,40 @@ public function testExcludeJoin() {
     $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')