From 3ff5ca789a6027cda0f0a03f71fff5e5fd905369 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 12 Oct 2015 17:02:42 -0700
Subject: [PATCH] Fix `/tag/aa%20bb` project URIs

Summary:
Ref T9551. To set things up:

  - Name a project `aa bb`. This will have the tag `aa_bb`.
  - Try to visit `/tag/aa%20bb`.

Here's what happens now:

  - You get an Aphront redirect error as it tries to add the trailing `/`. Add `phutil_escape_uri()` so that works again.
  - Then, you 404, even though this tag is reasonably equivalent to the real project tag and could be redirected. Add a fallback to lookup, resolve, and redirect if we can find a hit for the tag.

This also fixes stuff like `/tag/AA_BB/`.

Test Plan: Visited URIs like `/tag/aa%20bb`, `/tag/aa%20bb/`, `/tag/Aa_bB/`, etc. None of them worked before and now they all do.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9551

Differential Revision: https://secure.phabricator.com/D14260
---
 .../AphrontApplicationConfiguration.php       |  7 ++++
 .../PhabricatorProjectViewController.php      | 36 ++++++++++++++++++-
 .../PhabricatorProjectTransactionEditor.php   |  4 +--
 src/infrastructure/util/PhabricatorSlug.php   |  6 ++++
 4 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 50b8c123fd..b48ea3be13 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -372,6 +372,13 @@ abstract class AphrontApplicationConfiguration extends Phobject {
       $result = $this->routePath($maps, $path.'/');
       if ($result) {
         $slash_uri = $request->getRequestURI()->setPath($path.'/');
+
+        // We need to restore URI encoding because the webserver has
+        // interpreted it. For example, this allows us to redirect a path
+        // like `/tag/aa%20bb` to `/tag/aa%20bb/`, which may eventually be
+        // resolved meaningfully by an application.
+        $slash_uri = phutil_escape_uri($slash_uri);
+
         $external = strlen($request->getRequestURI()->getDomain());
         return $this->buildRedirectController($slash_uri, $external);
       }
diff --git a/src/applications/project/controller/PhabricatorProjectViewController.php b/src/applications/project/controller/PhabricatorProjectViewController.php
index 45329285df..bfc9827ab5 100644
--- a/src/applications/project/controller/PhabricatorProjectViewController.php
+++ b/src/applications/project/controller/PhabricatorProjectViewController.php
@@ -26,10 +26,17 @@ final class PhabricatorProjectViewController
     }
     $project = $query->executeOne();
     if (!$project) {
+
+      // If this request corresponds to a project but just doesn't have the
+      // slug quite right, redirect to the proper URI.
+      $uri = $this->getNormalizedURI($slug);
+      if ($uri !== null) {
+        return id(new AphrontRedirectResponse())->setURI($uri);
+      }
+
       return new Aphront404Response();
     }
 
-
     $columns = id(new PhabricatorProjectColumnQuery())
       ->setViewer($viewer)
       ->withProjectPHIDs(array($project->getPHID()))
@@ -53,4 +60,31 @@ final class PhabricatorProjectViewController
     return $this->delegateToController($controller_object);
   }
 
+  private function getNormalizedURI($slug) {
+    if (!strlen($slug)) {
+      return null;
+    }
+
+    $normal = PhabricatorSlug::normalizeProjectSlug($slug);
+    if ($normal === $slug) {
+      return null;
+    }
+
+    $viewer = $this->getViewer();
+
+    // Do execute() instead of executeOne() here so we canonicalize before
+    // raising a policy exception. This is a little more polished than letting
+    // the user hit the error on any variant of the slug.
+
+    $projects = id(new PhabricatorProjectQuery())
+      ->setViewer($viewer)
+      ->withSlugs(array($normal))
+      ->execute();
+    if (!$projects) {
+      return null;
+    }
+
+    return "/tag/{$normal}/";
+  }
+
 }
diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
index c5253da956..ecd2d5119f 100644
--- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
+++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php
@@ -498,9 +498,7 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorLiskDAO $object,
     $name) {
 
-    $object = (clone $object);
-    $object->setPhrictionSlug($name);
-    $slug = $object->getPrimarySlug();
+    $slug = PhabricatorSlug::normalizeProjectSlug($name);
 
     $slug_object = id(new PhabricatorProjectSlug())->loadOneWhere(
       'slug = %s',
diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php
index 53330391b1..fdac30a094 100644
--- a/src/infrastructure/util/PhabricatorSlug.php
+++ b/src/infrastructure/util/PhabricatorSlug.php
@@ -2,6 +2,12 @@
 
 final class PhabricatorSlug extends Phobject {
 
+  public static function normalizeProjectSlug($slug) {
+    $slug = str_replace('/', ' ', $slug);
+    $slug = self::normalize($slug);
+    return rtrim($slug, '/');
+  }
+
   public static function normalize($slug) {
     $slug = preg_replace('@/+@', '/', $slug);
     $slug = trim($slug, '/');