More granular control over query handling

modules/__tests__/invalidQueryParams-test.js

@@ -1,20 +0,0 @@
-import request from 'supertest';
-
-import createServer from '../createServer';
-
-describe('Invalid query params', () => {
-  let server;
-  beforeEach(() => {
-    server = createServer();
-  });
-
-  it('redirect to the same path w/out those params', done => {
-    request(server)
-      .get('/d3?module&invalid-param')
-      .end((err, res) => {
-        expect(res.statusCode).toBe(302);
-        expect(res.headers.location).toBe('/d3?module');
-        done();
-      });
-  });
-});

modules/__tests__/meta-test.js

@@ -29,4 +29,16 @@ describe('A request for metadata', () => {
         });
     });
   });
+
+  describe('when the URL includes invalid query parameters', () => {
+    it('removes them from the URL', done => {
+      request(server)
+        .get('/react@16.8.0/?meta&invalid')
+        .end((err, res) => {
+          expect(res.statusCode).toBe(302);
+          expect(res.headers.location).toBe('/react@16.8.0/?meta');
+          done();
+        });
+    });
+  });
 });

modules/__tests__/module-test.js

@@ -0,0 +1,24 @@
+import request from 'supertest';
+
+import createServer from '../createServer.js';
+
+describe('A request for a module', () => {
+  let server;
+  beforeEach(() => {
+    server = createServer();
+  });
+
+  // TODO: More tests here
+
+  describe('when the URL includes invalid query parameters', () => {
+    it('removes them from the URL', done => {
+      request(server)
+        .get('/react@16.8.0/?module&invalid')
+        .end((err, res) => {
+          expect(res.statusCode).toBe(302);
+          expect(res.headers.location).toBe('/react@16.8.0/?module');
+          done();
+        });
+    });
+  });
+});

modules/__tests__/regularFile-test.js

@@ -35,4 +35,16 @@ describe('A request for a JavaScript file', () => {
         });
     });
   });
+
+  describe('when the URL has invalid query params', () => {
+    it('removes them from the URL', done => {
+      request(server)
+        .get('/react@16.8.0/index.js?invalid')
+        .end((err, res) => {
+          expect(res.statusCode).toBe(302);
+          expect(res.headers.location).toBe('/react@16.8.0/index.js');
+          done();
+        });
+    });
+  });
 });

modules/createServer.js

@@ -11,6 +11,7 @@ import serveMainPage from './actions/serveMainPage.js';
 import serveModule from './actions/serveModule.js';
 import serveStats from './actions/serveStats.js';
 
+import allowQuery from './middleware/allowQuery.js';
 import findEntry from './middleware/findEntry.js';
 import noQuery from './middleware/noQuery.js';
 import redirectLegacyURLs from './middleware/redirectLegacyURLs.js';
@@ -19,7 +20,6 @@ import staticFiles from './middleware/staticFiles.js';
 import validateFilename from './middleware/validateFilename.js';
 import validatePackageURL from './middleware/validatePackageURL.js';
 import validatePackageName from './middleware/validatePackageName.js';
-import validateQuery from './middleware/validateQuery.js';
 import validateVersion from './middleware/validateVersion.js';
 
 function createApp(callback) {
@@ -86,9 +86,9 @@ export default function createServer() {
 
       app.get(
         '*/',
+        allowQuery('meta'),
         validatePackageURL,
         validatePackageName,
-        validateQuery,
         validateVersion,
         validateFilename,
         serveDirectoryMetadata
@@ -96,9 +96,9 @@ export default function createServer() {
 
       app.get(
         '*',
+        allowQuery('meta'),
         validatePackageURL,
         validatePackageName,
-        validateQuery,
         validateVersion,
         validateFilename,
         serveFileMetadata
@@ -113,14 +113,16 @@ export default function createServer() {
       }
     });
 
+    // We need to route in this weird way because Express
+    // doesn't have a way to route based on query params.
     const moduleApp = createApp(app => {
       app.enable('strict routing');
 
       app.get(
         '*',
+        allowQuery('module'),
         validatePackageURL,
         validatePackageName,
-        validateQuery,
         validateVersion,
         validateFilename,
         findEntry,
@@ -143,9 +145,9 @@ export default function createServer() {
 
     app.get(
       '*',
+      noQuery(),
       validatePackageURL,
       validatePackageName,
-      validateQuery,
       validateVersion,
       validateFilename,
       findEntry,

modules/middleware/allowQuery.js

@@ -0,0 +1,27 @@
+import createSearch from '../utils/createSearch.js';
+
+/**
+ * Reject URLs with invalid query parameters to increase cache hit rates.
+ */
+export default function allowQuery(validKeys = []) {
+  if (!Array.isArray(validKeys)) {
+    validKeys = [validKeys];
+  }
+
+  return (req, res, next) => {
+    const keys = Object.keys(req.query);
+
+    if (!keys.every(key => validKeys.includes(key))) {
+      const newQuery = keys
+        .filter(key => validKeys.includes(key))
+        .reduce((query, key) => {
+          query[key] = req.query[key];
+          return query;
+        }, {});
+
+      return res.redirect(302, req.baseUrl + req.path + createSearch(newQuery));
+    }
+
+    next();
+  };
+}

modules/middleware/validateQuery.js

@@ -1,29 +0,0 @@
-import createSearch from '../utils/createSearch.js';
-
-const knownQueryParams = {
-  main: true, // Deprecated, see #63
-  meta: true,
-  module: true
-};
-
-function isKnownQueryParam(param) {
-  return !!knownQueryParams[param];
-}
-
-/**
- * Reject URLs with invalid query parameters to increase cache hit rates.
- */
-export default function validateQuery(req, res, next) {
-  const keys = Object.keys(req.query);
-
-  if (!keys.every(isKnownQueryParam)) {
-    const newQuery = keys.filter(isKnownQueryParam).reduce((query, key) => {
-      query[key] = req.query[key];
-      return query;
-    }, {});
-
-    return res.redirect(302, req.path + createSearch(newQuery));
-  }
-
-  next();
-}