Merge pull request #2966 from benjiwheeler/search-escape-fix

make search urls use consistent encoding

src/components/navigation/www/navigation.jsx

@@ -78,7 +78,11 @@ class Navigation extends React.Component {
         return `/users/${this.props.user.username}/`;
     }
     handleSearchSubmit (formData) {
-        window.location.href = `/search/projects?q=${encodeURIComponent(formData.q)}`;
+        let targetUrl = '/search/projects';
+        if (formData.q) {
+            targetUrl += `?q=${encodeURIComponent(formData.q)}`;
+        }
+        window.location.href = targetUrl;
     }
     render () {
         const createLink = this.props.user ? '/projects/editor/' : '/projects/editor/?tutorial=getStarted';

src/views/search/search.jsx

@@ -57,7 +57,9 @@ class Search extends React.Component {
 
     }
     componentDidMount () {
-        const query = decodeURIComponent(window.location.search);
+        // just in case there's a URL in the wild with pluses to indicate spaces,
+        // convert pluses to url-encoded spaces before decoding.
+        const query = decodeURIComponent(window.location.search.split('+').join('%20'));
         let term = query;
 
         const stripQueryValue = function (queryTerm) {
@@ -91,6 +93,13 @@ class Search extends React.Component {
             this.handleGetSearchMore();
         }
     }
+    encodeSearchTerm () {
+        let termText = '';
+        if (this.props.searchTerm) {
+            termText = encodeURIComponent(this.props.searchTerm);
+        }
+        return termText;
+    }
     getSearchState () {
         let pathname = window.location.pathname.toLowerCase();
         if (pathname[pathname.length - 1] === '/') {
@@ -105,21 +114,24 @@ class Search extends React.Component {
     }
     handleChangeSortMode (name, value) {
         if (ACCEPTABLE_MODES.indexOf(value) !== -1) {
-            const term = this.props.searchTerm.split(' ').join('+');
-            window.location =
-                `${window.location.origin}/search/${this.state.tab}?q=${term}&mode=${value}`;
+            const termText = this.encodeSearchTerm();
+            let newLocation = `${window.location.origin}/search/${this.state.tab}?mode=${value}`;
+            if (termText) {
+                newLocation += `&q=${termText}`;
+            }
+            window.location = newLocation;
         }
     }
     handleGetSearchMore () {
-        let termText = '';
-        if (this.props.searchTerm !== '') {
-            termText = `&q=${encodeURIComponent(this.props.searchTerm.split(' ').join('+'))}`;
-        }
+        const termText = this.encodeSearchTerm();
         const locale = this.props.intl.locale;
         const loadNumber = this.state.loadNumber;
         const offset = this.state.offset;
         const mode = this.state.mode;
-        const queryString = `limit=${loadNumber}&offset=${offset}&language=${locale}&mode=${mode}${termText}`;
+        let queryString = `limit=${loadNumber}&offset=${offset}&language=${locale}&mode=${mode}`;
+        if (termText) {
+            queryString += `&q=${termText}`;
+        }
 
         api({
             uri: `/search/${this.state.tab}?${queryString}`
@@ -137,9 +149,13 @@ class Search extends React.Component {
         });
     }
     getTab (type) {
-        const term = this.props.searchTerm.split(' ').join('+');
+        const termText = this.encodeSearchTerm();
+        let targetUrl = `/search/${type}`;
+        if (termText) {
+            targetUrl += `?q=${termText}`;
+        }
         let allTab = (
-            <a href={`/search/${type}?q=${term}/`}>
+            <a href={targetUrl}>
                 <li>
                     <img
                         className={`tab-icon ${type}`}
@@ -151,7 +167,7 @@ class Search extends React.Component {
         );
         if (this.state.tab === type) {
             allTab = (
-                <a href={`/search/${type}?q=${term}/`}>
+                <a href={targetUrl}>
                     <li className="active">
                         <img
                             className={`tab-icon ${type}`}

test/integration/selenium-helpers.js

@@ -7,13 +7,14 @@ const remote = process.env.SMOKE_REMOTE || false;
 const ci = process.env.CI || false;
 const buildID = process.env.TRAVIS_BUILD_NUMBER;
 const {SAUCE_USERNAME, SAUCE_ACCESS_KEY} = process.env;
-const {By, until} = webdriver;
+const {By, Key, until} = webdriver;
 
 class SeleniumHelper {
     constructor () {
         bindAll(this, [
             'getDriver',
             'getSauceDriver',
+            'getKey',
             'buildDriver',
             'clickXpath',
             'findByXpath',
@@ -22,6 +23,7 @@ class SeleniumHelper {
             'clickButton',
             'findByCss',
             'clickCss',
+            'urlMatches',
             'getLogs'
         ]);
     }
@@ -79,6 +81,10 @@ class SeleniumHelper {
         return driver;
     }
 
+    getKey (keyName) {
+        return Key[keyName];
+    }
+
     findByXpath (xpath) {
         return this.driver.wait(until.elementLocated(By.xpath(xpath), 5 * 1000));
     }
@@ -107,6 +113,10 @@ class SeleniumHelper {
         return this.findByCss(css).then(el => el.click());
     }
 
+    urlMatches (regex) {
+        return this.driver.wait(until.urlMatches(regex), 1000 * 5);
+    }
+
     getLogs (whitelist) {
         return this.driver.manage()
             .logs()

test/integration/smoke-testing/test_search.js

@@ -0,0 +1,58 @@
+/*
+ * Checks the behavior of the search interface
+ */
+require('chromedriver');
+const seleniumWebdriver = require('selenium-webdriver');
+const SeleniumHelper = require('../selenium-helpers.js');
+const helper = new SeleniumHelper();
+const {
+    urlMatches
+} = helper;
+
+const tap = require('tap');
+const test = tap.test;
+
+// Set test url through environment variable
+const rootUrl = process.env.ROOT_URL || 'http://localhost:8333';
+const searchBaseUrl = `${rootUrl}/search/`;
+
+// chrome driver
+const driver = helper.buildDriver('www-search test_search');
+
+tap.plan(3);
+
+tap.tearDown(function () {
+    driver.quit();
+});
+
+tap.beforeEach(function () {
+    return driver.get(searchBaseUrl);
+});
+
+test('Search escapes spaces', function (t) {
+    const searchInput = driver.findElement(seleniumWebdriver.By.name('q'));
+    searchInput.sendKeys('Test search string', helper.getKey('ENTER')).then(function () {
+        urlMatches(/^.*\?q=Test%20search%20string$/)
+            .then(() => t.end());
+    });
+});
+
+test('Search escapes symbols', function (t) {
+    const searchInput = driver.findElement(seleniumWebdriver.By.name('q'));
+    searchInput.sendKeys('100% pen', helper.getKey('ENTER')).then(function () {
+        urlMatches(/^.*\?q=100%25%20pen$/)
+            .then(() => t.end());
+    });
+});
+
+test('Switching to studios maintains search string', function (t) {
+    const searchInput = driver.findElement(seleniumWebdriver.By.name('q'));
+    searchInput.sendKeys('100% pen', helper.getKey('ENTER')).then(function () {
+        const studiosTab = driver.findElement(seleniumWebdriver.By.xpath(
+            '//a/li/span[contains(text(),"Studios")]'));
+        studiosTab.click().then(function () {
+            urlMatches(/^.*\?q=100%25%20pen$/)
+                .then(() => t.end());
+        });
+    });
+});