pass-through query string on redirect (GoogleChrome#3090)

* expand redirect-handler

* express .query has weird parsing rules

* comment about why we don't use query

* add tests

redirect-handler.js

@@ -18,32 +18,34 @@ const yaml = require('js-yaml');
 const fs = require('fs');
 const escapeStringRegexp = require('escape-string-regexp');
 
+const baseUrlPrefix = 'https://web.dev/';
+
 /**
- * Normalizes the passed URL to ensure that it ends with a simple trailing
- * slash. Removes "index.html" if found.
+ * Normalizes the passed path (not a whole URL) to ensure that it ends with a
+ * simple trailing slash. Removes "index.html" if found.
  *
- * @param {string} url to normalize
+ * @param {string} path to normalize
  * @return {string}
  */
-function ensureTrailingSlashOnly(url) {
-  if (url.endsWith('/index.html')) {
-    return url.slice(0, -'index.html'.length);
-  } else if (!url.endsWith('/')) {
-    return `${url}/`;
+function ensureTrailingSlashOnly(path) {
+  if (path.endsWith('/index.html')) {
+    return path.slice(0, -'index.html'.length);
+  } else if (!path.endsWith('/')) {
+    return `${path}/`;
   }
-  return url;
+  return path;
 }
 
 /**
- * Builds HTTP middleware that serves redirects for web.dev's _redirects.yaml
- * configuration file, originally from DevSite.
+ * Builds a method which returns a URL to redirect to.
  *
- * @param {string} filename to load configuration from
- * @param {number=} code to use (DevSite uses 301)
- * @return {!Function}
+ * @param {string} yamlSource to parse redirects from
+ * @param {string} baseUrlPrefix domain prefix to use, e.g. "https://web.dev/"
+ * @return {function(string): ?string}
  */
-module.exports = function buildRedirectHandler(filename, code = 301) {
-  const doc = yaml.safeLoad(fs.readFileSync(filename, 'utf8'));
+function prepareHandler(yamlSource, baseUrlPrefix) {
+  const doc = yaml.safeLoad(yamlSource);
+  baseUrlPrefix = ensureTrailingSlashOnly(baseUrlPrefix);
 
   const groupRedirect = {};
   const singleRedirect = {};
@@ -55,31 +57,89 @@ module.exports = function buildRedirectHandler(filename, code = 301) {
       continue;
     }
 
-    // "Group" redirects' from and to must end with "/...", i.e., match the last
+    // "Group" redirects' from must end with "/...", i.e., match the last
     // whole path component.
-    if (!from.endsWith('/...') || !to.endsWith('/...')) {
+    if (!from.endsWith('/...')) {
       throw new TypeError(`got redirect with invalid ...: ${from} => ${to}`);
     }
-    groupRedirect[from.slice(0, -3)] = to.slice(0, -3); // but only slice "..."
+    groupRedirect[from.slice(0, -3)] = to;
   }
 
   // Build a single RegExp for group matches, for speed of matching.
   const escaped = Object.keys(groupRedirect).map(escapeStringRegexp);
   const groupMatcher = new RegExp(`^(${escaped.join('|')})`);
 
-  return (req, res, next) => {
-    const url = ensureTrailingSlashOnly(req.path);
-    if (url in singleRedirect) {
-      return res.redirect(code, singleRedirect[url]);
-    }
+  return (raw) => {
+    let target;
+
+    // Split the raw URL (expected to be "/foo?bar") into path and query parts.
+    const parts = raw.split('?');
+    const path = parts[0];
+    const query = parts[1] || '';
 
-    const m = groupMatcher.exec(url);
-    if (m && m[1] in groupRedirect) {
+    const url = ensureTrailingSlashOnly(path);
+    if (url in singleRedirect) {
+      target = new URL(singleRedirect[url], baseUrlPrefix);
+    } else {
+      const m = groupMatcher.exec(url);
+      if (!(m && m[1] in groupRedirect)) {
+        return null;
+      }
       const base = groupRedirect[m[1]];
       const rest = url.slice(m[1].length);
-      return res.redirect(code, base + rest);
+
+      if (base.endsWith('/...')) {
+        target = new URL(base.slice(0, -3) + rest, baseUrlPrefix);
+      } else {
+        target = new URL(base, baseUrlPrefix);
+      }
     }
 
-    next();
+    // Merge the original request's params into the target. We don't use
+    // Express' req.query here as it expands the query too much, e.g.,
+    // 'foo[bar]=123' ends up like "{foo: {bar: 123}}".
+    const requestParams = new URLSearchParams(query);
+    for (const [key, value] of requestParams) {
+      target.searchParams.append(key, value);
+    }
+
+    // If the result URL starts with "https://web.dev/" (with trailing slash),
+    // then strip it and treat as local. This allows redirects to work in dev,
+    // where the domain is localhost or a staging URL.
+    let s = target.toString();
+    if (s.startsWith(baseUrlPrefix)) {
+      s = '/' + s.substr(baseUrlPrefix.length);
+    }
+    return s;
   };
+}
+
+/**
+ * Builds HTTP middleware that serves redirects for web.dev's _redirects.yaml
+ * configuration file, originally from DevSite.
+ *
+ * @param {string} filename to load configuration from
+ * @param {number=} code to use (DevSite uses 301)
+ * @return {!Function}
+ */
+function build(filename, code = 301) {
+  const handler = prepareHandler(
+    fs.readFileSync(filename, 'utf8'),
+    baseUrlPrefix,
+  );
+
+  return (req, res, next) => {
+    const target = handler(req.url);
+    if (target !== null) {
+      return res.redirect(code, target);
+    }
+    return next();
+  };
+}
+
+module.exports = {
+  build,
+  prepareHandler,
+  baseUrlPrefix,
+  ensureTrailingSlashOnly,
 };

server.js

@@ -20,7 +20,7 @@ const compression = require('compression');
 const express = require('express');
 const cookieParser = require('cookie-parser');
 const localeHandler = require('./locale-handler.js');
-const buildRedirectHandler = require('./redirect-handler.js');
+const {build: buildRedirectHandler} = require('./redirect-handler.js');
 
 // If true, we'll aggressively nuke the prod Service Worker. For emergencies.
 const serviceWorkerKill = false;

test/unit/index.js

@@ -1,3 +1,4 @@
 describe('Unit tests', function () {
   require('./src');
+  require('./redirect-handler');
 });

test/unit/redirect-handler.js

@@ -0,0 +1,95 @@
+/**
+ * @fileoverview Tests for the redirect-handler used by the server.
+ */
+
+const assert = require('assert');
+const {
+  baseUrlPrefix,
+  ensureTrailingSlashOnly,
+  prepareHandler,
+} = require('../../redirect-handler');
+
+const sharedYamlSource = `
+redirects:
+- from: /subscribe
+  to: /newsletter
+
+- from: /subscribe/all/...
+  to: /newsletter
+
+- from: /foo/...
+  to: /bar/...
+
+- from: /external/...
+  to: https://google.com/...
+`;
+
+describe('redirect-handler', function () {
+  it('normalizes paths', function () {
+    assert.equal(ensureTrailingSlashOnly('/foo/index.html'), '/foo/');
+    assert.equal(
+      ensureTrailingSlashOnly('/foo/index.HTML'),
+      '/foo/index.HTML/',
+      'must only match lower-case index.html',
+    );
+    assert.strictEqual(ensureTrailingSlashOnly(''), '/');
+  });
+
+  it('handles simple redirects', function () {
+    const h = prepareHandler(sharedYamlSource, 'https://web.dev');
+
+    assert.strictEqual(h('/subscribe'), '/newsletter');
+    assert.strictEqual(
+      h('/subscribe/index.html'),
+      '/newsletter',
+      'trailing index.html is ignored',
+    );
+    assert.strictEqual(
+      h('/subscribe/other.html'),
+      null,
+      'unhandled URL returns null',
+    );
+  });
+
+  it('handles group redirects', function () {
+    const h = prepareHandler(sharedYamlSource, 'https://web.dev');
+
+    assert.strictEqual(
+      h('/subscribe/all/foo'),
+      '/newsletter',
+      'group => non-group redirect',
+    );
+    assert.strictEqual(
+      h('/foo/x'),
+      '/bar/x/',
+      'group redirect functions, trailing slash added',
+    );
+    assert.strictEqual(
+      h('/foo/x/index.html'),
+      '/bar/x/',
+      'index.html is stripped',
+    );
+    assert.strictEqual(
+      h('/external/hello'),
+      'https://google.com/hello/',
+      'external redirect is also normalized',
+    );
+    assert.strictEqual(h('/external/'), 'https://google.com/');
+    assert.strictEqual(
+      h('/external'),
+      'https://google.com/',
+      'matches without trailing slash',
+    );
+  });
+
+  it('includes query strings', function () {
+    const h = prepareHandler(sharedYamlSource, 'https://web.dev');
+
+    assert.strictEqual(h('/subscribe/?foo'), '/newsletter?foo=');
+    assert.strictEqual(h('/subscribe/?foo=1&foo=2'), '/newsletter?foo=1&foo=2');
+  });
+
+  it('has the expected baseUrl', function () {
+    assert.strictEqual(baseUrlPrefix, 'https://web.dev/');
+  });
+});