From 47ee68510c174c3286a11052f1278044548f973b Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Mon, 7 Oct 2019 10:24:16 +0800 Subject: [PATCH 1/4] Make trailing slash optional when requesting the url --- modules/createServer.js | 21 +++---- modules/middleware/validateFileType.js | 77 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 12 deletions(-) create mode 100644 modules/middleware/validateFileType.js diff --git a/modules/createServer.js b/modules/createServer.js index 4a5f727f..0a10e8f9 100644 --- a/modules/createServer.js +++ b/modules/createServer.js @@ -20,6 +20,7 @@ import validateFilename from './middleware/validateFilename.js'; import validatePackagePathname from './middleware/validatePackagePathname.js'; import validatePackageName from './middleware/validatePackageName.js'; import validatePackageVersion from './middleware/validatePackageVersion.js'; +import validateFileType from './middleware/validateFileType.js'; function createApp(callback) { const app = express(); @@ -56,24 +57,20 @@ export default function createServer() { app.use( '/browse', createApp(app => { - app.enable('strict routing'); - - app.get( - '*/', - noQuery(), - validatePackagePathname, - validatePackageName, - validatePackageVersion, - serveDirectoryBrowser - ); - app.get( '*', noQuery(), validatePackagePathname, validatePackageName, validatePackageVersion, - serveFileBrowser + validateFileType, + (req, res) => { + if (req.type === 'directory') { + return serveDirectoryBrowser(req, res); + } + + return serveFileBrowser(req, res); + } ); }) ); diff --git a/modules/middleware/validateFileType.js b/modules/middleware/validateFileType.js new file mode 100644 index 00000000..a15f676a --- /dev/null +++ b/modules/middleware/validateFileType.js @@ -0,0 +1,77 @@ +import tar from 'tar-stream'; + +import asyncHandler from '../utils/asyncHandler.js'; +import { getPackage } from '../utils/npm.js'; +import createPackageURL from '../utils/createPackageURL.js'; + +async function findEntryType(stream, filename) { + // filename = /some/file/name.js + return new Promise((accept, reject) => { + let hasFoundFile = false; + + stream + .pipe(tar.extract()) + .on('error', reject) + .on('entry', async (header, stream, next) => { + if (hasFoundFile) { + next(); + } + + const entry = { + // Most packages have header names that look like `package/index.js` + // so we shorten that to just `/index.js` here. A few packages use a + // prefix other than `package/`. e.g. the firebase package uses the + // `firebase_npm/` prefix. So we just strip the first dir name. + path: header.name.replace(/^[^/]+\/?/, '/'), + type: header.type + }; + + // Ignore files that don't match the name. + if (entry.type !== 'file' || entry.path !== filename) { + stream.resume(); + stream.on('end', next); + } else { + hasFoundFile = true; + + next(); + } + }) + .on('finish', () => { + accept(hasFoundFile ? 'file' : 'directory'); + }); + }); +} + +async function validateFileType(req, res, next) { + if (!req.filename) { + req.fileType = 'directory'; + } + + const stream = await getPackage(req.packageName, req.packageVersion, req.log); + + const entryType = await findEntryType(stream, req.filename); + + req.type = entryType; + + if (req.type === 'directory' && req.filename.slice(-1) !== '/') { + // If there is no trailing slash, redirect to the one that have + return res + .set({ + 'Cache-Control': 'public, max-age=31536000', // 1 year + 'Cache-Tag': 'redirect, filename-redirect' + }) + .redirect( + 301, + createPackageURL( + req.packageName, + req.packageVersion, + req.filename.replace(/^\/+/, '/') + '/', + req.query + ) + ); + } + + next(); +} + +export default asyncHandler(validateFileType); From 9f91123d4bc0ac310e0609736bf10b811ec374be Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Mon, 7 Oct 2019 15:56:17 +0800 Subject: [PATCH 2/4] Update to get rid of tailing slash instead --- modules/actions/serveDirectoryBrowser.js | 2 +- modules/client/browse/App.js | 18 +++++++++---- modules/client/browse/DirectoryViewer.js | 21 +++++++-------- modules/client/utils/path.js | 20 ++++++++++++++ modules/createServer.js | 2 +- modules/middleware/validateFileType.js | 33 ++++++++++-------------- 6 files changed, 59 insertions(+), 37 deletions(-) create mode 100644 modules/client/utils/path.js diff --git a/modules/actions/serveDirectoryBrowser.js b/modules/actions/serveDirectoryBrowser.js index 561fb2e6..651a7ab6 100644 --- a/modules/actions/serveDirectoryBrowser.js +++ b/modules/actions/serveDirectoryBrowser.js @@ -67,7 +67,7 @@ async function findMatchingEntries(stream, filename) { async function serveDirectoryBrowser(req, res) { const stream = await getPackage(req.packageName, req.packageVersion, req.log); - const filename = req.filename.slice(0, -1) || '/'; + const filename = req.filename || '/'; const entries = await findMatchingEntries(stream, filename); if (Object.keys(entries).length === 0) { diff --git a/modules/client/browse/App.js b/modules/client/browse/App.js index 828d9f97..ff3bbd2d 100644 --- a/modules/client/browse/App.js +++ b/modules/client/browse/App.js @@ -141,15 +141,19 @@ export default function App({ ); } + // We're on the package root directory when filename is an empty string + const isRoot = !filename; + const breadcrumbs = []; + const rootUrl = `/browse/${packageName}@${packageVersion}`; - if (filename === '/') { + if (isRoot) { breadcrumbs.push(packageName); } else { - let url = `/browse/${packageName}@${packageVersion}`; + let url = rootUrl; breadcrumbs.push( - + {packageName} ); @@ -164,7 +168,7 @@ export default function App({ segments.forEach(segment => { url += `/${segment}`; breadcrumbs.push( - + {segment} ); @@ -298,7 +302,11 @@ export default function App({ }} > {target.type === 'directory' ? ( - + ) : target.type === 'file' ? ( ) : null} diff --git a/modules/client/browse/DirectoryViewer.js b/modules/client/browse/DirectoryViewer.js index 93301ece..6c33a32e 100644 --- a/modules/client/browse/DirectoryViewer.js +++ b/modules/client/browse/DirectoryViewer.js @@ -5,6 +5,7 @@ import VisuallyHidden from '@reach/visually-hidden'; import sortBy from 'sort-by'; import { formatBytes } from '../utils/format.js'; +import { join, basename, dirname } from '../utils/path.js'; import { DirectoryIcon, CodeFileIcon } from './Icons.js'; @@ -44,19 +45,16 @@ const typeCellStyle = { } }; -function getRelName(path, base) { - return path.substr(base.length > 1 ? base.length + 1 : 1); -} - -export default function DirectoryViewer({ path, details: entries }) { +export default function DirectoryViewer({ path, isRoot, details: entries }) { + const parentPath = dirname(path); const rows = []; - if (path !== '/') { + if (!isRoot) { rows.push( - + .. @@ -83,8 +81,8 @@ export default function DirectoryViewer({ path, details: entries }) { ); subdirs.sort(sortBy('path')).forEach(({ path: dirname }) => { - const relName = getRelName(dirname, path); - const href = relName + '/'; + const relName = basename(dirname); + const href = join(path, relName); rows.push( @@ -105,8 +103,8 @@ export default function DirectoryViewer({ path, details: entries }) { files .sort(sortBy('path')) .forEach(({ path: filename, size, contentType }) => { - const relName = getRelName(filename, path); - const href = relName; + const relName = basename(filename); + const href = join(path, relName); rows.push( @@ -174,6 +172,7 @@ export default function DirectoryViewer({ path, details: entries }) { if (process.env.NODE_ENV !== 'production') { DirectoryViewer.propTypes = { path: PropTypes.string.isRequired, + isRoot: PropTypes.bool.isRequired, details: PropTypes.objectOf( PropTypes.shape({ path: PropTypes.string.isRequired, diff --git a/modules/client/utils/path.js b/modules/client/utils/path.js new file mode 100644 index 00000000..cd9c6423 --- /dev/null +++ b/modules/client/utils/path.js @@ -0,0 +1,20 @@ +export function join(...paths) { + return ( + paths + .join('/') + .replace(/\/+/g, '/') + .replace(/\/$/, '') || '/' + ); +} + +export function basename(path) { + return path.split('/').slice(-1); +} + +export function dirname(path) { + return path + .replace(/\/$/, '') + .split('/') + .slice(0, -1) + .join('/'); +} diff --git a/modules/createServer.js b/modules/createServer.js index 0a10e8f9..9df808a5 100644 --- a/modules/createServer.js +++ b/modules/createServer.js @@ -136,7 +136,7 @@ export default function createServer() { // Send old */ requests to the new /browse UI. app.get('*/', (req, res) => { - res.redirect(302, '/browse' + req.url); + res.redirect(302, '/browse' + req.url.slice(0, -1)); }); app.get( diff --git a/modules/middleware/validateFileType.js b/modules/middleware/validateFileType.js index a15f676a..de57d813 100644 --- a/modules/middleware/validateFileType.js +++ b/modules/middleware/validateFileType.js @@ -2,7 +2,6 @@ import tar from 'tar-stream'; import asyncHandler from '../utils/asyncHandler.js'; import { getPackage } from '../utils/npm.js'; -import createPackageURL from '../utils/createPackageURL.js'; async function findEntryType(stream, filename) { // filename = /some/file/name.js @@ -44,31 +43,27 @@ async function findEntryType(stream, filename) { async function validateFileType(req, res, next) { if (!req.filename) { - req.fileType = 'directory'; - } - - const stream = await getPackage(req.packageName, req.packageVersion, req.log); + req.type = 'directory'; + } else { + const stream = await getPackage( + req.packageName, + req.packageVersion, + req.log + ); - const entryType = await findEntryType(stream, req.filename); + const entryType = await findEntryType(stream, req.filename); - req.type = entryType; + req.type = entryType; + } - if (req.type === 'directory' && req.filename.slice(-1) !== '/') { - // If there is no trailing slash, redirect to the one that have + if (req.type === 'directory' && req.filename.slice(-1) === '/') { + // If there is a trailing slash, redirect to the url without it return res .set({ 'Cache-Control': 'public, max-age=31536000', // 1 year - 'Cache-Tag': 'redirect, filename-redirect' + 'Cache-Tag': 'redirect, file-type-redirect' }) - .redirect( - 301, - createPackageURL( - req.packageName, - req.packageVersion, - req.filename.replace(/^\/+/, '/') + '/', - req.query - ) - ); + .redirect(302, req.originalUrl.slice(0, -1)); } next(); From ab1e9b54117e47398f60fc80b2dcf37a9d5ed1e6 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Mon, 7 Oct 2019 16:07:25 +0800 Subject: [PATCH 3/4] Add default '.' for path utils --- modules/client/utils/path.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/modules/client/utils/path.js b/modules/client/utils/path.js index cd9c6423..ade242ea 100644 --- a/modules/client/utils/path.js +++ b/modules/client/utils/path.js @@ -3,7 +3,7 @@ export function join(...paths) { paths .join('/') .replace(/\/+/g, '/') - .replace(/\/$/, '') || '/' + .replace(/\/$/, '') || '.' ); } @@ -12,9 +12,11 @@ export function basename(path) { } export function dirname(path) { - return path - .replace(/\/$/, '') - .split('/') - .slice(0, -1) - .join('/'); + return ( + path + .replace(/\/$/, '') + .split('/') + .slice(0, -1) + .join('/') || '.' + ); } From 0b40a65a66f22d2a51e06c8dbdd8f66f9b0694e6 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 8 Oct 2019 14:51:04 +0800 Subject: [PATCH 4/4] Fix and add tests --- modules/__tests__/browseDirectory-test.js | 14 ++++++-------- modules/__tests__/browseRedirect-test.js | 16 ++++++++++++++-- modules/__tests__/legacyURLs-test.js | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/modules/__tests__/browseDirectory-test.js b/modules/__tests__/browseDirectory-test.js index 220ca74f..588194ad 100644 --- a/modules/__tests__/browseDirectory-test.js +++ b/modules/__tests__/browseDirectory-test.js @@ -11,12 +11,10 @@ describe('A request to browse a directory', () => { describe('with no version specified', () => { it('redirects to the latest version', done => { request(server) - .get('/browse/react/') + .get('/browse/react') .end((err, res) => { expect(res.statusCode).toBe(302); - expect(res.headers.location).toMatch( - /\/browse\/react@\d+\.\d+\.\d+\// - ); + expect(res.headers.location).toMatch(/\/browse\/react@\d+\.\d+\.\d+/); done(); }); }); @@ -25,7 +23,7 @@ describe('A request to browse a directory', () => { describe('when the directory exists', () => { it('returns an HTML page', done => { request(server) - .get('/browse/react@16.8.0/umd/') + .get('/browse/react@16.8.0/umd') .end((err, res) => { expect(res.statusCode).toBe(200); expect(res.headers['content-type']).toMatch(/\btext\/html\b/); @@ -37,7 +35,7 @@ describe('A request to browse a directory', () => { describe('when the directory does not exist', () => { it('returns a 404 HTML page', done => { request(server) - .get('/browse/react@16.8.0/not-here/') + .get('/browse/react@16.8.0/not-here') .end((err, res) => { expect(res.statusCode).toBe(404); expect(res.headers['content-type']).toMatch(/\btext\/html\b/); @@ -49,10 +47,10 @@ describe('A request to browse a directory', () => { describe('with invalid query params', () => { it('strips them from the query string', done => { request(server) - .get('/browse/react@16.8.0/umd/?invalid') + .get('/browse/react@16.8.0/umd?invalid') .end((err, res) => { expect(res.statusCode).toBe(302); - expect(res.headers.location).toEqual('/browse/react@16.8.0/umd/'); + expect(res.headers.location).toEqual('/browse/react@16.8.0/umd'); done(); }); }); diff --git a/modules/__tests__/browseRedirect-test.js b/modules/__tests__/browseRedirect-test.js index c88cf7ce..e7d67381 100644 --- a/modules/__tests__/browseRedirect-test.js +++ b/modules/__tests__/browseRedirect-test.js @@ -14,7 +14,7 @@ describe('A request with a trailing slash', () => { .get('/react/') .end((err, res) => { expect(res.statusCode).toBe(302); - expect(res.headers.location).toEqual('/browse/react/'); + expect(res.headers.location).toEqual('/browse/react'); done(); }); }); @@ -26,7 +26,19 @@ describe('A request with a trailing slash', () => { .get('/react@16.8.0/') .end((err, res) => { expect(res.statusCode).toBe(302); - expect(res.headers.location).toEqual('/browse/react@16.8.0/'); + expect(res.headers.location).toEqual('/browse/react@16.8.0'); + done(); + }); + }); + }); + + describe('in /browse', () => { + it('redirects to the same url without trailing slash', done => { + request(server) + .get('/browse/react@16.8.0/') + .end((err, res) => { + expect(res.statusCode).toBe(302); + expect(res.headers.location).toEqual('/browse/react@16.8.0'); done(); }); }); diff --git a/modules/__tests__/legacyURLs-test.js b/modules/__tests__/legacyURLs-test.js index 83428875..ceeb789e 100644 --- a/modules/__tests__/legacyURLs-test.js +++ b/modules/__tests__/legacyURLs-test.js @@ -33,7 +33,7 @@ describe('Legacy URLs', () => { .get('/react@16.8.0/umd/') .end((err, res) => { expect(res.statusCode).toBe(302); - expect(res.headers.location).toEqual('/browse/react@16.8.0/umd/'); + expect(res.headers.location).toEqual('/browse/react@16.8.0/umd'); done(); }); });