From 4fef0a6128244b90d0ba60f4758214a6518d4508 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 9 Feb 2018 14:23:39 -0800
Subject: [PATCH] Allow a wider range of characters in macro names, including
 emoji
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Summary:
Fixes T6121. See PHI357.

  - Allow emoji and other unicode (like Chinese characters) as long as you have at least three of them.
  - Disallow macros with only latin symbols. These were previously allowed.

Test Plan: Created a macro for "🐶🐶🐶", then used it in a comment.

Maniphest Tasks: T6121

Differential Revision: https://secure.phabricator.com/D19051
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorImageMacroRemarkupRule.php     |  2 +-
 .../PhabricatorMacroNameTransaction.php       | 47 ++++++++++++++++---
 .../__tests__/PhabricatorMacroTestCase.php    | 46 ++++++++++++++++++
 4 files changed, 90 insertions(+), 7 deletions(-)
 create mode 100644 src/applications/macro/xaction/__tests__/PhabricatorMacroTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 6c3514a2c9..13dd7374d2 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3203,6 +3203,7 @@ phutil_register_library_map(array(
     'PhabricatorMacroQuery' => 'applications/macro/query/PhabricatorMacroQuery.php',
     'PhabricatorMacroReplyHandler' => 'applications/macro/mail/PhabricatorMacroReplyHandler.php',
     'PhabricatorMacroSearchEngine' => 'applications/macro/query/PhabricatorMacroSearchEngine.php',
+    'PhabricatorMacroTestCase' => 'applications/macro/xaction/__tests__/PhabricatorMacroTestCase.php',
     'PhabricatorMacroTransaction' => 'applications/macro/storage/PhabricatorMacroTransaction.php',
     'PhabricatorMacroTransactionComment' => 'applications/macro/storage/PhabricatorMacroTransactionComment.php',
     'PhabricatorMacroTransactionQuery' => 'applications/macro/query/PhabricatorMacroTransactionQuery.php',
@@ -8751,6 +8752,7 @@ phutil_register_library_map(array(
     'PhabricatorMacroQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorMacroReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler',
     'PhabricatorMacroSearchEngine' => 'PhabricatorApplicationSearchEngine',
+    'PhabricatorMacroTestCase' => 'PhabricatorTestCase',
     'PhabricatorMacroTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorMacroTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'PhabricatorMacroTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
diff --git a/src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php b/src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php
index 8911181a77..4c50322daa 100644
--- a/src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php
+++ b/src/applications/macro/markup/PhabricatorImageMacroRemarkupRule.php
@@ -8,7 +8,7 @@ final class PhabricatorImageMacroRemarkupRule extends PhutilRemarkupRule {
 
   public function apply($text) {
     return preg_replace_callback(
-      '@^\s*([a-zA-Z0-9:_\-]+)$@m',
+      '@^\s*([a-zA-Z0-9:_\x7f-\xff-]+)$@m',
       array($this, 'markupImageMacro'),
       $text);
   }
diff --git a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php
index 68710605aa..f55de443bf 100644
--- a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php
+++ b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php
@@ -52,12 +52,16 @@ final class PhabricatorMacroNameTransaction
           new PhutilNumber($max_length)));
       }
 
-      if (!preg_match('/^[a-z0-9:_-]{3,}\z/', $new_value)) {
-      $errors[] = $this->newInvalidError(
-        pht('Macro name "%s" be at least three characters long and contain '.
-            'only lowercase letters, digits, hyphens, colons and '.
-            'underscores.',
-            $new_value));
+      if (!self::isValidMacroName($new_value)) {
+        // This says "emoji", but the actual rule we implement is "all other
+        // unicode characters are also fine".
+        $errors[] = $this->newInvalidError(
+          pht(
+            'Macro name "%s" be: at least three characters long; and contain '.
+            'only lowercase letters, digits, hyphens, colons, underscores, '.
+            'and emoji; and not be composed entirely of latin symbols.',
+            $new_value),
+          $xaction);
       }
 
       // Check name is unique when updating / creating
@@ -78,4 +82,35 @@ final class PhabricatorMacroNameTransaction
     return $errors;
   }
 
+  public static function isValidMacroName($name) {
+    if (preg_match('/^[:_-]+\z/', $name)) {
+      return false;
+    }
+
+    // Accept trivial macro names.
+    if (preg_match('/^[a-z0-9:_-]{3,}\z/', $name)) {
+      return true;
+    }
+
+    // Reject names with fewer than 3 glyphs.
+    $length = phutil_utf8v_combined($name);
+    if (count($length) < 3) {
+      return false;
+    }
+
+    // Check character-by-character for any symbols that we don't want.
+    $characters = phutil_utf8v($name);
+    foreach ($characters as $character) {
+      if (ord($character[0]) > 0x7F) {
+        continue;
+      }
+
+      if (preg_match('/^[^a-z0-9:_-]/', $character)) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
 }
diff --git a/src/applications/macro/xaction/__tests__/PhabricatorMacroTestCase.php b/src/applications/macro/xaction/__tests__/PhabricatorMacroTestCase.php
new file mode 100644
index 0000000000..aa13ec0ac1
--- /dev/null
+++ b/src/applications/macro/xaction/__tests__/PhabricatorMacroTestCase.php
@@ -0,0 +1,46 @@
+<?php
+
+final class PhabricatorMacroTestCase
+  extends PhabricatorTestCase {
+
+  public function testMacroNames() {
+    $lit = "\xF0\x9F\x94\xA5";
+    $combining_diaeresis = "\xCC\x88";
+
+    $cases = array(
+      // Only 2 glyphs long.
+      "u{$combining_diaeresis}n" => false,
+      "{$lit}{$lit}" => false,
+
+      // Too short.
+      'a' => false,
+      '' => false,
+
+      // Bad characters.
+      'yes!' => false,
+      "{$lit} {$lit} {$lit}" => false,
+      "aaa\nbbb" => false,
+      'aaa~' => false,
+      'aaa`' => false,
+
+      // Special rejections for only latin symbols.
+      '---' => false,
+      '___' => false,
+      '-_-' => false,
+      ':::' => false,
+      '-_:' => false,
+
+      "{$lit}{$lit}{$lit}" => true,
+      'bwahahaha' => true,
+      "u{$combining_diaeresis}nt" => true,
+      'a-a-a-a' => true,
+    );
+
+    foreach ($cases as $input => $expect) {
+      $this->assertEqual(
+        $expect,
+        PhabricatorMacroNameTransaction::isValidMacroName($input),
+        pht('Validity of macro "%s"', $input));
+    }
+  }
+}