From f99ce6e3076a3bc59ba692d54dc32e55877e0abb Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Fri, 23 Sep 2016 15:14:59 -0700
Subject: [PATCH] Properly encode URL for next/back links

Adds gr-url-encoding-behavior, which abstracts and centralizes the URL
encoding and prettifying responsibility for the app.
Also adds a testing suite for gr-change-list-view.

Bug: Issue 4602
Change-Id: I8c8605e1b79dffbbabf92b57ef0cb1bb6260e4f9
---
 .../behaviors/gr-url-encoding-behavior.html   | 42 +++++++++++++++
 .../gr-change-list-item.html                  |  3 +-
 .../gr-change-list-item.js                    |  6 +--
 .../gr-change-list-view.html                  |  1 +
 .../gr-change-list-view.js                    |  4 +-
 .../gr-change-list-view_test.html             | 51 +++++++++++++++++++
 .../change/gr-file-list/gr-file-list.html     |  3 +-
 .../change/gr-file-list/gr-file-list.js       | 18 ++-----
 .../app/elements/core/gr-router/gr-router.js  |  2 +
 .../core/gr-search-bar/gr-search-bar.html     |  3 +-
 .../core/gr-search-bar/gr-search-bar.js       |  5 +-
 polygerrit-ui/app/test/index.html             |  1 +
 12 files changed, 116 insertions(+), 23 deletions(-)
 create mode 100644 polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html
 create mode 100644 polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html

diff --git a/polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html b/polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html
new file mode 100644
index 0000000000..b7d71fc723
--- /dev/null
+++ b/polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html
@@ -0,0 +1,42 @@
+<!--
+Copyright (C) 2016 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+<script>
+(function(window) {
+  'use strict';
+
+  /** @polymerBehavior Gerrit.URLEncodingBehavior */
+  var URLEncodingBehavior = {
+    /**
+     * Pretty-encodes a URL. Double-encodes the string, and then replaces
+     *   benevolent characters for legibility.
+     */
+    encodeURL: function(url, replaceSlashes) {
+      // @see Issue 4255 regarding double-encoding.
+      var output = encodeURIComponent(encodeURIComponent(url));
+      // @see Issue 4577 regarding more readable URLs.
+      output = output.replace(/%253A/g, ':');
+      output = output.replace(/%2520/g, '+');
+      if (replaceSlashes) {
+        output = output.replace(/%252F/g, '/');
+      }
+      return output;
+    },
+  };
+
+  window.Gerrit = window.Gerrit || {};
+  window.Gerrit.URLEncodingBehavior = URLEncodingBehavior;
+})(window);
+</script>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
index 91267852be..df1ade3250 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->
 
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
+<link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../styles/gr-change-list-styles.html">
-<link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
 <link rel="import" href="../../shared/gr-change-star/gr-change-star.html">
 <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
index a43f564483..275a8cddee 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
@@ -44,6 +44,7 @@
 
     behaviors: [
       Gerrit.RESTClientBehavior,
+      Gerrit.URLEncodingBehavior,
     ],
 
     _computeChangeURL: function(changeNum) {
@@ -108,15 +109,14 @@
     },
 
     _computeProjectURL: function(project) {
-      // @see Issue 4255.
       return '/q/status:open+project:' +
-          encodeURIComponent(encodeURIComponent(project));
+          this.encodeURL(project, false);
     },
 
     _computeProjectBranchURL: function(project, branch) {
       // @see Issue 4255.
       return this._computeProjectURL(project) +
-          '+branch:' + encodeURIComponent(encodeURIComponent(branch));
+          '+branch:' + this.encodeURL(branch, false);
     },
   });
 })();
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
index 1f06dff4c7..91b2f075dd 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->
 
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
 <link rel="import" href="../gr-change-list/gr-change-list.html">
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
index 7fbe455207..45d9a57fe5 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
@@ -23,6 +23,7 @@
      * @event title-change
      */
 
+    behaviors: [Gerrit.URLEncodingBehavior],
     properties: {
       /**
        * URL params passed from the router.
@@ -116,7 +117,8 @@
       // Offset could be a string when passed from the router.
       offset = +(offset || 0);
       var newOffset = Math.max(0, offset + (changesPerPage * direction));
-      var href = '/q/' + query;
+      // Double encode URI component.
+      var href = '/q/' + this.encodeURL(query, false);
       if (newOffset > 0) {
         href += ',' + newOffset;
       }
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html
new file mode 100644
index 0000000000..af2359c254
--- /dev/null
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html
@@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<!--
+Copyright (C) 2016 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+<title>gr-change-list-view</title>
+
+<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+
+<link rel="import" href="gr-change-list-view.html">
+
+<test-fixture id="basic">
+  <template>
+    <gr-change-list-view></gr-change-list-view>
+  </template>
+</test-fixture>
+
+<script>
+  suite('gr-change-list-view tests', function() {
+    var element;
+
+    setup(function() {
+      element = fixture('basic');
+    });
+
+    test('url is properly encoded', function() {
+      assert.equal(element._computeNavLink(
+          'status:open project:platform/frameworks/base', 0, -1, 25),
+          '/q/status:open+project:platform%252Fframeworks%252Fbase'
+      );
+      assert.equal(element._computeNavLink(
+          'status:open project:platform/frameworks/base', 0, 1, 25),
+          '/q/status:open+project:platform%252Fframeworks%252Fbase,25'
+      );
+    });
+  });
+</script>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index d1cb7191be..9c4d7325ec 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -14,8 +14,9 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->
 
-<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../diff/gr-diff/gr-diff.html">
 <link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index aff635c24a..f5311bbc59 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -57,6 +57,7 @@
 
     behaviors: [
       Gerrit.KeyboardShortcutBehavior,
+      Gerrit.URLEncodingBehavior,
     ],
 
     reload: function() {
@@ -128,8 +129,8 @@
     _handlePatchChange: function(e) {
       var patchRange = Object.assign({}, this.patchRange);
       patchRange.basePatchNum = Polymer.dom(e).rootTarget.value;
-      page.show('/c/' + encodeURIComponent(this.changeNum) + '/' +
-          encodeURIComponent(this._patchRangeStr(patchRange)));
+      page.show(this.encodeURL('/c/' + this.changeNum + '/' +
+          this._patchRangeStr(patchRange), true));
     },
 
     _forEachDiff: function(fn) {
@@ -383,17 +384,8 @@
     },
 
     _computeDiffURL: function(changeNum, patchRange, path) {
-      // @see Issue 4255 regarding double-encoding.
-      path = encodeURIComponent(encodeURIComponent(path));
-      // @see Issue 4577 regarding more readable URLs.
-      path = path.replace(/%252F/g, '/');
-      path = path.replace(/%2520/g, '+');
-      return '/c/' +
-          encodeURIComponent(changeNum) +
-          '/' +
-          encodeURIComponent(this._patchRangeStr(patchRange)) +
-          '/' +
-          path;
+      return this.encodeURL('/c/' + changeNum + '/' +
+          this._patchRangeStr(patchRange) + '/' + path, true);
     },
 
     _patchRangeStr: function(patchRange) {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
index a182f8a45b..f185977371 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@@ -134,6 +134,8 @@
       };
       // Don't allow diffing the same patch number against itself.
       if (params.basePatchNum === params.patchNum) {
+        // TODO(kaspern): Utilize gr-url-encoding-behavior.html when the router
+        // is replaced with a Polymer counterpart.
         // @see Issue 4255 regarding double-encoding.
         var path = encodeURIComponent(encodeURIComponent(path));
         // @see Issue 4577 regarding more readable URLs.
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
index 957b2ceebf..a79e8305f0 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
@@ -14,8 +14,9 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->
 
-<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../shared/gr-autocomplete/gr-autocomplete.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
index c33f889ba8..2818121f0f 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@@ -87,6 +87,7 @@
 
     behaviors: [
       Gerrit.KeyboardShortcutBehavior,
+      Gerrit.URLEncodingBehavior,
     ],
 
     listeners: {
@@ -140,9 +141,7 @@
         target.blur();
       }
       if (this._inputVal) {
-        // @see Issue 4255.
-        page.show('/q/' +
-            encodeURIComponent(encodeURIComponent(this._inputVal)));
+        page.show('/q/' + this.encodeURL(this._inputVal, false));
       }
     },
 
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index 156c873ef6..042a497bf8 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -43,6 +43,7 @@ limitations under the License.
     'change/gr-reviewer-list/gr-reviewer-list_test.html',
     'change-list/gr-change-list/gr-change-list_test.html',
     'change-list/gr-change-list-item/gr-change-list-item_test.html',
+    'change-list/gr-change-list-view/gr-change-list-view_test.html',
     'core/gr-account-dropdown/gr-account-dropdown_test.html',
     'core/gr-error-manager/gr-error-manager_test.html',
     'core/gr-main-header/gr-main-header_test.html',