From 6ad61a984f5e65422cff81d7aaf14ea337cbc5d7 Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Thu, 8 Aug 2019 15:13:19 -0700 Subject: [PATCH] Remove unneeded req params related to the URL --- modules/createServer.js | 28 +++---- modules/middleware/findEntry.js | 4 +- modules/middleware/validateFilename.js | 7 +- modules/middleware/validatePackagePathname.js | 19 +++++ modules/middleware/validatePackageURL.js | 23 ------ ...teVersion.js => validatePackageVersion.js} | 3 +- .../__tests__/parsePackagePathname-test.js | 59 ++++++++++++++ .../utils/__tests__/parsePackageURL-test.js | 80 ------------------- modules/utils/createPackageURL.js | 14 ++-- modules/utils/parsePackagePathname.js | 26 ++++++ modules/utils/parsePackageURL.js | 33 -------- 11 files changed, 131 insertions(+), 165 deletions(-) create mode 100644 modules/middleware/validatePackagePathname.js delete mode 100644 modules/middleware/validatePackageURL.js rename modules/middleware/{validateVersion.js => validatePackageVersion.js} (97%) create mode 100644 modules/utils/__tests__/parsePackagePathname-test.js delete mode 100644 modules/utils/__tests__/parsePackageURL-test.js create mode 100644 modules/utils/parsePackagePathname.js delete mode 100644 modules/utils/parsePackageURL.js diff --git a/modules/createServer.js b/modules/createServer.js index ebb96e5..4a5f727 100644 --- a/modules/createServer.js +++ b/modules/createServer.js @@ -17,9 +17,9 @@ import noQuery from './middleware/noQuery.js'; import redirectLegacyURLs from './middleware/redirectLegacyURLs.js'; import requestLog from './middleware/requestLog.js'; import validateFilename from './middleware/validateFilename.js'; -import validatePackageURL from './middleware/validatePackageURL.js'; +import validatePackagePathname from './middleware/validatePackagePathname.js'; import validatePackageName from './middleware/validatePackageName.js'; -import validateVersion from './middleware/validateVersion.js'; +import validatePackageVersion from './middleware/validatePackageVersion.js'; function createApp(callback) { const app = express(); @@ -61,18 +61,18 @@ export default function createServer() { app.get( '*/', noQuery(), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, serveDirectoryBrowser ); app.get( '*', noQuery(), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, serveFileBrowser ); }) @@ -86,9 +86,9 @@ export default function createServer() { app.get( '*/', allowQuery('meta'), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, validateFilename, serveDirectoryMetadata ); @@ -96,9 +96,9 @@ export default function createServer() { app.get( '*', allowQuery('meta'), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, validateFilename, serveFileMetadata ); @@ -120,9 +120,9 @@ export default function createServer() { app.get( '*', allowQuery('module'), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, validateFilename, findEntry, serveModule @@ -145,9 +145,9 @@ export default function createServer() { app.get( '*', noQuery(), - validatePackageURL, + validatePackagePathname, validatePackageName, - validateVersion, + validatePackageVersion, validateFilename, findEntry, serveFile diff --git a/modules/middleware/findEntry.js b/modules/middleware/findEntry.js index 86ca535..4fa1b47 100644 --- a/modules/middleware/findEntry.js +++ b/modules/middleware/findEntry.js @@ -23,7 +23,7 @@ function fileRedirect(req, res, entry) { req.packageName, req.packageVersion, entry.path, - createSearch(req.query) + req.query ) ); } @@ -42,7 +42,7 @@ function indexRedirect(req, res, entry) { req.packageName, req.packageVersion, entry.path, - createSearch(req.query) + req.query ) ); } diff --git a/modules/middleware/validateFilename.js b/modules/middleware/validateFilename.js index 4b08384..4f0c77f 100644 --- a/modules/middleware/validateFilename.js +++ b/modules/middleware/validateFilename.js @@ -1,7 +1,4 @@ import createPackageURL from '../utils/createPackageURL.js'; -import createSearch from '../utils/createSearch.js'; - -const leadingSlashes = /^\/*/; function filenameRedirect(req, res) { let filename; @@ -63,8 +60,8 @@ function filenameRedirect(req, res) { createPackageURL( req.packageName, req.packageVersion, - filename.replace(leadingSlashes, '/'), - createSearch(req.query) + filename.replace(/^\/*/, '/'), + req.query ) ); } diff --git a/modules/middleware/validatePackagePathname.js b/modules/middleware/validatePackagePathname.js new file mode 100644 index 0000000..b76e1dd --- /dev/null +++ b/modules/middleware/validatePackagePathname.js @@ -0,0 +1,19 @@ +import parsePackagePathname from '../utils/parsePackagePathname.js'; + +/** + * Parse the pathname in the URL. Reject invalid URLs. + */ +export default function validatePackagePathname(req, res, next) { + const parsed = parsePackagePathname(req.path); + + if (parsed == null) { + return res.status(403).send({ error: `Invalid URL: ${req.path}` }); + } + + req.packageName = parsed.packageName; + req.packageVersion = parsed.packageVersion; + req.packageSpec = parsed.packageSpec; + req.filename = parsed.filename; + + next(); +} diff --git a/modules/middleware/validatePackageURL.js b/modules/middleware/validatePackageURL.js deleted file mode 100644 index 153adf7..0000000 --- a/modules/middleware/validatePackageURL.js +++ /dev/null @@ -1,23 +0,0 @@ -import parsePackageURL from '../utils/parsePackageURL.js'; - -/** - * Parse the URL and add various properties to the request object to - * do with the package/file being requested. Reject invalid URLs. - */ -export default function validatePackageURL(req, res, next) { - const url = parsePackageURL(req.url); - - if (url == null) { - return res.status(403).send({ error: `Invalid URL: ${req.url}` }); - } - - req.packageName = url.packageName; - req.packageVersion = url.packageVersion; - req.packageSpec = `${url.packageName}@${url.packageVersion}`; - req.pathname = url.pathname; // TODO: remove - req.filename = url.filename; - req.search = url.search; - req.query = url.query; - - next(); -} diff --git a/modules/middleware/validateVersion.js b/modules/middleware/validatePackageVersion.js similarity index 97% rename from modules/middleware/validateVersion.js rename to modules/middleware/validatePackageVersion.js index 1a29d91..507a3c4 100644 --- a/modules/middleware/validateVersion.js +++ b/modules/middleware/validatePackageVersion.js @@ -13,7 +13,7 @@ function semverRedirect(req, res, newVersion) { .redirect( 302, req.baseUrl + - createPackageURL(req.packageName, newVersion, req.filename, req.search) + createPackageURL(req.packageName, newVersion, req.filename, req.query) ); } @@ -65,7 +65,6 @@ async function validateVersion(req, res, next) { ); if (!req.packageConfig) { - // TODO: Log why. return res .status(500) .type('text') diff --git a/modules/utils/__tests__/parsePackagePathname-test.js b/modules/utils/__tests__/parsePackagePathname-test.js new file mode 100644 index 0000000..816f02c --- /dev/null +++ b/modules/utils/__tests__/parsePackagePathname-test.js @@ -0,0 +1,59 @@ +import parsePackagePathname from '../parsePackagePathname.js'; + +describe('parsePackagePathname', () => { + it('parses plain packages', () => { + expect(parsePackagePathname('/history@1.0.0/umd/history.min.js')).toEqual({ + packageName: 'history', + packageVersion: '1.0.0', + packageSpec: 'history@1.0.0', + filename: '/umd/history.min.js' + }); + }); + + it('parses plain packages with a hyphen in the name', () => { + expect(parsePackagePathname('/query-string@5.0.0/index.js')).toEqual({ + packageName: 'query-string', + packageVersion: '5.0.0', + packageSpec: 'query-string@5.0.0', + filename: '/index.js' + }); + }); + + it('parses plain packages with no version specified', () => { + expect(parsePackagePathname('/query-string/index.js')).toEqual({ + packageName: 'query-string', + packageVersion: 'latest', + packageSpec: 'query-string@latest', + filename: '/index.js' + }); + }); + + it('parses plain packages with version spec', () => { + expect(parsePackagePathname('/query-string@>=4.0.0/index.js')).toEqual({ + packageName: 'query-string', + packageVersion: '>=4.0.0', + packageSpec: 'query-string@>=4.0.0', + filename: '/index.js' + }); + }); + + it('parses scoped packages', () => { + expect( + parsePackagePathname('/@angular/router@4.3.3/src/index.d.ts') + ).toEqual({ + packageName: '@angular/router', + packageVersion: '4.3.3', + packageSpec: '@angular/router@4.3.3', + filename: '/src/index.d.ts' + }); + }); + + it('parses package names with a period in them', () => { + expect(parsePackagePathname('/index.js')).toEqual({ + packageName: 'index.js', + packageVersion: 'latest', + packageSpec: 'index.js@latest', + filename: '' + }); + }); +}); diff --git a/modules/utils/__tests__/parsePackageURL-test.js b/modules/utils/__tests__/parsePackageURL-test.js deleted file mode 100644 index 3ead80a..0000000 --- a/modules/utils/__tests__/parsePackageURL-test.js +++ /dev/null @@ -1,80 +0,0 @@ -import parsePackageURL from '../parsePackageURL.js'; - -describe('parsePackageURL', () => { - it('parses plain packages', () => { - expect(parsePackageURL('/history@1.0.0/umd/history.min.js')).toEqual({ - pathname: '/history@1.0.0/umd/history.min.js', - search: '', - query: {}, - packageName: 'history', - packageVersion: '1.0.0', - filename: '/umd/history.min.js' - }); - }); - - it('parses plain packages with a hyphen in the name', () => { - expect(parsePackageURL('/query-string@5.0.0/index.js')).toEqual({ - pathname: '/query-string@5.0.0/index.js', - search: '', - query: {}, - packageName: 'query-string', - packageVersion: '5.0.0', - filename: '/index.js' - }); - }); - - it('parses plain packages with no version specified', () => { - expect(parsePackageURL('/query-string/index.js')).toEqual({ - pathname: '/query-string/index.js', - search: '', - query: {}, - packageName: 'query-string', - packageVersion: 'latest', - filename: '/index.js' - }); - }); - - it('parses plain packages with version spec', () => { - expect(parsePackageURL('/query-string@>=4.0.0/index.js')).toEqual({ - pathname: '/query-string@>=4.0.0/index.js', - search: '', - query: {}, - packageName: 'query-string', - packageVersion: '>=4.0.0', - filename: '/index.js' - }); - }); - - it('parses scoped packages', () => { - expect(parsePackageURL('/@angular/router@4.3.3/src/index.d.ts')).toEqual({ - pathname: '/@angular/router@4.3.3/src/index.d.ts', - search: '', - query: {}, - packageName: '@angular/router', - packageVersion: '4.3.3', - filename: '/src/index.d.ts' - }); - }); - - it('parses package names with a period in them', () => { - expect(parsePackageURL('/index.js')).toEqual({ - pathname: '/index.js', - search: '', - query: {}, - packageName: 'index.js', - packageVersion: 'latest', - filename: '' - }); - }); - - it('parses valid query parameters', () => { - expect(parsePackageURL('/history?main=browser')).toEqual({ - pathname: '/history', - search: '?main=browser', - query: { main: 'browser' }, - packageName: 'history', - packageVersion: 'latest', - filename: '' - }); - }); -}); diff --git a/modules/utils/createPackageURL.js b/modules/utils/createPackageURL.js index 8718dc8..0b2626d 100644 --- a/modules/utils/createPackageURL.js +++ b/modules/utils/createPackageURL.js @@ -1,14 +1,16 @@ +import createSearch from './createSearch.js'; + export default function createPackageURL( packageName, - version, - pathname, - search + packageVersion, + filename, + query ) { let url = `/${packageName}`; - if (version != null) url += `@${version}`; - if (pathname) url += pathname; - if (search) url += search; + if (packageVersion) url += `@${packageVersion}`; + if (filename) url += filename; + if (query) url += createSearch(query); return url; } diff --git a/modules/utils/parsePackagePathname.js b/modules/utils/parsePackagePathname.js new file mode 100644 index 0000000..cfd391e --- /dev/null +++ b/modules/utils/parsePackagePathname.js @@ -0,0 +1,26 @@ +const packagePathnameFormat = /^\/((?:@[^/@]+\/)?[^/@]+)(?:@([^/]+))?(\/.*)?$/; + +export default function parsePackagePathname(pathname) { + try { + pathname = decodeURIComponent(pathname); + } catch (error) { + return null; + } + + const match = packagePathnameFormat.exec(pathname); + + // Disallow invalid pathnames. + if (match == null) return null; + + const packageName = match[1]; + const packageVersion = match[2] || 'latest'; + const filename = (match[3] || '').replace(/\/\/+/g, '/'); + + return { + // If the pathname is /@scope/name@version/file.js: + packageName, // @scope/name + packageVersion, // version + packageSpec: `${packageName}@${packageVersion}`, // @scope/name@version + filename // /file.js + }; +} diff --git a/modules/utils/parsePackageURL.js b/modules/utils/parsePackageURL.js deleted file mode 100644 index 72c3628..0000000 --- a/modules/utils/parsePackageURL.js +++ /dev/null @@ -1,33 +0,0 @@ -import url from 'url'; - -const packageURLFormat = /^\/((?:@[^/@]+\/)?[^/@]+)(?:@([^/]+))?(\/.*)?$/; - -export default function parsePackageURL(originalURL) { - let { pathname, search, query } = url.parse(originalURL, true); - try { - pathname = decodeURIComponent(pathname); - } catch (error) { - return null; - } - - const match = packageURLFormat.exec(pathname); - - // Disallow invalid URL formats. - if (match == null) { - return null; - } - - const packageName = match[1]; - const packageVersion = match[2] || 'latest'; - const filename = (match[3] || '').replace(/\/\/+/g, '/'); - - return { - // If the URL is /@scope/name@version/file.js?main=browser: - pathname, // /@scope/name@version/path.js - search: search || '', // ?main=browser - query, // { main: 'browser' } - packageName, // @scope/name - packageVersion, // version - filename // /file.js - }; -}