From 269b756aebc437b179bbc055c55727843b1840d9 Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Sat, 19 May 2018 08:34:36 -0700 Subject: [PATCH] Break up parseURL middleware --- server/__tests__/server-test.js | 26 ++++---- server/createRouter.js | 14 ++-- server/createServer.js | 2 +- server/middleware/parseURL.js | 85 ------------------------ server/middleware/redirectLegacyURLs.js | 23 +++++++ server/middleware/validatePackageName.js | 20 ++++++ server/middleware/validatePackageURL.js | 6 +- server/middleware/validateQuery.js | 34 ++++++++++ 8 files changed, 103 insertions(+), 107 deletions(-) delete mode 100644 server/middleware/parseURL.js create mode 100644 server/middleware/redirectLegacyURLs.js create mode 100644 server/middleware/validatePackageName.js create mode 100644 server/middleware/validateQuery.js diff --git a/server/__tests__/server-test.js b/server/__tests__/server-test.js index abcf27e..e79b631 100644 --- a/server/__tests__/server-test.js +++ b/server/__tests__/server-test.js @@ -1,11 +1,13 @@ const request = require("supertest"); + const createServer = require("../createServer"); + const clearBlacklist = require("./utils/clearBlacklist"); const withBlacklist = require("./utils/withBlacklist"); const withRevokedToken = require("./utils/withRevokedToken"); const withToken = require("./utils/withToken"); -describe("The server", () => { +describe("The production server", () => { let server; beforeEach(() => { server = createServer(); @@ -61,6 +63,17 @@ describe("The server", () => { }); }); + describe("GET /_publicKey", () => { + it("echoes the public key", done => { + request(server) + .get("/_publicKey") + .end((err, res) => { + expect(res.text).toMatch(/PUBLIC KEY/); + done(); + }); + }); + }); + describe("POST /_auth", () => { it("creates a new auth token", done => { request(server) @@ -114,17 +127,6 @@ describe("The server", () => { }); }); - describe("GET /_publicKey", () => { - it("echoes the public key", done => { - request(server) - .get("/_publicKey") - .end((err, res) => { - expect(res.text).toMatch(/PUBLIC KEY/); - done(); - }); - }); - }); - describe("POST /_blacklist", () => { afterEach(clearBlacklist); diff --git a/server/createRouter.js b/server/createRouter.js index fcaa9ac..2e7225e 100644 --- a/server/createRouter.js +++ b/server/createRouter.js @@ -2,9 +2,8 @@ const express = require("express"); const bodyParser = require("body-parser"); const cors = require("cors"); -const renderPage = require("./utils/renderPage"); -const requireAuth = require("./middleware/requireAuth"); const MainPage = require("./components/MainPage"); +const renderPage = require("./utils/renderPage"); function route(setup) { const app = express.Router(); @@ -43,17 +42,17 @@ function createRouter() { route(app => { app.post( "/", - requireAuth("blacklist.add"), + require("./middleware/requireAuth")("blacklist.add"), require("./actions/addToBlacklist") ); app.get( "/", - requireAuth("blacklist.read"), + require("./middleware/requireAuth")("blacklist.read"), require("./actions/showBlacklist") ); app.delete( "*", - requireAuth("blacklist.remove"), + require("./middleware/requireAuth")("blacklist.remove"), require("./middleware/validatePackageURL"), require("./actions/removeFromBlacklist") ); @@ -66,7 +65,10 @@ function createRouter() { app.get( "*", - require("./middleware/parseURL"), + require("./middleware/redirectLegacyURLs"), + require("./middleware/validatePackageURL"), + require("./middleware/validatePackageName"), + require("./middleware/validateQuery"), require("./middleware/checkBlacklist"), require("./middleware/fetchPackage"), require("./middleware/findFile"), diff --git a/server/createServer.js b/server/createServer.js index 775adf3..bea5b66 100644 --- a/server/createServer.js +++ b/server/createServer.js @@ -28,7 +28,7 @@ function createServer(publicDir, statsFile) { if (process.env.NODE_ENV !== "test") { app.use( morgan( - // Modified version of the Heroku router's log format + // Modified version of Heroku's log format // https://devcenter.heroku.com/articles/http-routing#heroku-router-log-format 'method=:method path=":url" host=:req[host] request_id=:req[x-request-id] cf_ray=:req[cf-ray] fwd=:fwd status=:status bytes=:res[content-length]' ) diff --git a/server/middleware/parseURL.js b/server/middleware/parseURL.js deleted file mode 100644 index babc593..0000000 --- a/server/middleware/parseURL.js +++ /dev/null @@ -1,85 +0,0 @@ -const validateNpmPackageName = require("validate-npm-package-name"); -const parsePackageURL = require("../utils/parsePackageURL"); -const createSearch = require("./utils/createSearch"); - -const knownQueryParams = { - main: true, // Deprecated, see #63 - meta: true, - module: true -}; - -function isKnownQueryParam(param) { - return !!knownQueryParams[param]; -} - -function queryIsKnown(query) { - return Object.keys(query).every(isKnownQueryParam); -} - -function sanitizeQuery(query) { - const saneQuery = {}; - - Object.keys(query).forEach(param => { - if (isKnownQueryParam(param)) saneQuery[param] = query[param]; - }); - - return saneQuery; -} - -/** - * Parse and validate the URL. - */ -function parseURL(req, res, next) { - // Permanently redirect /_meta/path to /path?meta. - if (req.path.match(/^\/_meta\//)) { - req.query.meta = ""; - return res.redirect(301, req.path.substr(6) + createSearch(req.query)); - } - - // Permanently redirect /path?json => /path?meta - if (req.query.json != null) { - delete req.query.json; - req.query.meta = ""; - return res.redirect(301, req.path + createSearch(req.query)); - } - - // Redirect requests with unknown query params to their equivalents - // with only known params so they can be served from the cache. This - // prevents people using random query params designed to bust the cache. - if (!queryIsKnown(req.query)) { - return res.redirect(302, req.path + createSearch(sanitizeQuery(req.query))); - } - - const url = parsePackageURL(req.url); - - // Disallow invalid URLs. - if (url == null) { - return res - .status(403) - .type("text") - .send(`Invalid URL: ${req.url}`); - } - - const nameErrors = validateNpmPackageName(url.packageName).errors; - - // Disallow invalid package names. - if (nameErrors) { - const reason = nameErrors.join(", "); - return res - .status(403) - .type("text") - .send(`Invalid package name "${url.packageName}" (${reason})`); - } - - req.packageName = url.packageName; - req.packageVersion = url.packageVersion; - req.packageSpec = `${url.packageName}@${url.packageVersion}`; - req.pathname = url.pathname; - req.filename = url.filename; - req.search = url.search; - req.query = url.query; - - next(); -} - -module.exports = parseURL; diff --git a/server/middleware/redirectLegacyURLs.js b/server/middleware/redirectLegacyURLs.js new file mode 100644 index 0000000..b8d9d67 --- /dev/null +++ b/server/middleware/redirectLegacyURLs.js @@ -0,0 +1,23 @@ +const createSearch = require("./utils/createSearch"); + +/** + * Redirect old URLs that we no longer support. + */ +function redirectLegacyURLs(req, res, next) { + // Permanently redirect /_meta/path to /path?meta. + if (req.path.match(/^\/_meta\//)) { + req.query.meta = ""; + return res.redirect(301, req.path.substr(6) + createSearch(req.query)); + } + + // Permanently redirect /path?json => /path?meta + if (req.query.json != null) { + delete req.query.json; + req.query.meta = ""; + return res.redirect(301, req.path + createSearch(req.query)); + } + + next(); +} + +module.exports = redirectLegacyURLs; diff --git a/server/middleware/validatePackageName.js b/server/middleware/validatePackageName.js new file mode 100644 index 0000000..ca89025 --- /dev/null +++ b/server/middleware/validatePackageName.js @@ -0,0 +1,20 @@ +const validateNpmPackageName = require("validate-npm-package-name"); + +/** + * Reject requests for invalid npm package names. + */ +function validatePackageName(req, res, next) { + const nameErrors = validateNpmPackageName(req.packageName).errors; + + if (nameErrors) { + const reason = nameErrors.join(", "); + return res + .status(403) + .type("text") + .send(`Invalid package name "${req.packageName}" (${reason})`); + } + + next(); +} + +module.exports = validatePackageName; diff --git a/server/middleware/validatePackageURL.js b/server/middleware/validatePackageURL.js index 8ed670e..9d8d6a0 100644 --- a/server/middleware/validatePackageURL.js +++ b/server/middleware/validatePackageURL.js @@ -1,8 +1,8 @@ const parsePackageURL = require("../utils/parsePackageURL"); /** - * Adds various properties to the request object to do with the - * package/file being requested. + * Parse the URL and add various properties to the request object to + * do with the package/file being requested. Reject invalid URLs. */ function validatePackageURL(req, res, next) { const url = parsePackageURL(req.url); @@ -14,7 +14,7 @@ function validatePackageURL(req, res, next) { req.packageName = url.packageName; req.packageVersion = url.packageVersion; req.packageSpec = `${url.packageName}@${url.packageVersion}`; - req.pathname = url.pathname; + req.pathname = url.pathname; // TODO: remove req.filename = url.filename; req.search = url.search; req.query = url.query; diff --git a/server/middleware/validateQuery.js b/server/middleware/validateQuery.js new file mode 100644 index 0000000..21e9ad2 --- /dev/null +++ b/server/middleware/validateQuery.js @@ -0,0 +1,34 @@ +const createSearch = require("./utils/createSearch"); + +const knownQueryParams = { + main: true, // Deprecated, see #63 + meta: true, + module: true +}; + +function isKnownQueryParam(param) { + return !!knownQueryParams[param]; +} + +function sanitizeQuery(originalQuery) { + const query = {}; + + Object.keys(originalQuery).forEach(param => { + if (isKnownQueryParam(param)) query[param] = originalQuery[param]; + }); + + return query; +} + +/** + * Reject URLs with invalid query parameters to increase cache hit rates. + */ +function validateQuery(req, res, next) { + if (!Object.keys(req.query).every(isKnownQueryParam)) { + return res.redirect(302, req.path + createSearch(sanitizeQuery(req.query))); + } + + next(); +} + +module.exports = validateQuery;