From 06061d5d6c40b60d0f53cd4301950cb9de4a5ef9 Mon Sep 17 00:00:00 2001 From: Aileen Nowak Date: Mon, 14 Nov 2016 21:38:55 +0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=84=20=20Improve=20URL=20consistency,?= =?UTF-8?q?=20Part=201:=20urlJoin=20(#7668)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #7666 Use urlJoin for more consistency instead of concatenating url strings. --- core/server/api/authentication.js | 4 +--- core/server/api/index.js | 14 ++++++++------ core/server/api/invites.js | 2 +- core/server/api/settings.js | 3 ++- core/server/api/themes.js | 2 +- core/server/blog/routes.js | 8 ++++---- core/server/controllers/frontend/channel-config.js | 5 +++-- core/server/controllers/frontend/channels.js | 8 ++++---- core/server/controllers/frontend/index.js | 3 +-- core/server/data/importer/handlers/image.js | 5 ++--- core/server/data/meta/amp_url.js | 3 +-- core/server/data/meta/asset_url.js | 6 +++--- core/server/data/meta/paginated_url.js | 10 +++++----- core/server/data/migration/backup.js | 3 ++- core/server/data/xml/rss/index.js | 10 +++++----- core/server/storage/local-file-store.js | 7 +++---- core/server/utils/url.js | 8 ++++++++ core/test/functional/routes/frontend_spec.js | 2 +- 18 files changed, 55 insertions(+), 48 deletions(-) diff --git a/core/server/api/authentication.js b/core/server/api/authentication.js index b894e88506..0a123a54f5 100644 --- a/core/server/api/authentication.js +++ b/core/server/api/authentication.js @@ -217,9 +217,7 @@ authentication = { function sendResetNotification(data) { var baseUrl = config.get('forceAdminSSL') ? (config.get('urlSSL') || config.get('url')) : config.get('url'), - resetUrl = baseUrl.replace(/\/$/, '') + - '/ghost/reset/' + - globalUtils.encodeBase64URLsafe(data.resetToken) + '/'; + resetUrl = globalUtils.url.urlJoin(baseUrl, 'ghost/reset', globalUtils.encodeBase64URLsafe(data.resetToken), '/'); return mail.utils.generateContent({ data: { diff --git a/core/server/api/index.js b/core/server/api/index.js index 4b5717f315..3adb1e2580 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -93,7 +93,7 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) { if (hasStatusChanged || wasPublishedUpdated) { return INVALIDATE_ALL; } else { - return utils.url.urlFor({relativeUrl: '/' + config.get('routeKeywords').preview + '/' + post.uuid + '/'}); + return utils.url.urlFor({relativeUrl: utils.url.urlJoin('/', config.get('routeKeywords').preview, post.uuid, '/')}); } } } @@ -113,21 +113,23 @@ cacheInvalidationHeader = function cacheInvalidationHeader(req, result) { locationHeader = function locationHeader(req, result) { var apiRoot = utils.url.urlFor('api'), location, - newObject; + newObject, + statusQuery; if (req.method === 'POST') { if (result.hasOwnProperty('posts')) { newObject = result.posts[0]; - location = apiRoot + '/posts/' + newObject.id + '/?status=' + newObject.status; + statusQuery = '/?status=' + newObject.status; + location = utils.url.urlJoin(apiRoot, 'posts', newObject.id, statusQuery); } else if (result.hasOwnProperty('notifications')) { newObject = result.notifications[0]; - location = apiRoot + '/notifications/' + newObject.id + '/'; + location = utils.url.urlJoin(apiRoot, 'notifications', newObject.id, '/'); } else if (result.hasOwnProperty('users')) { newObject = result.users[0]; - location = apiRoot + '/users/' + newObject.id + '/'; + location = utils.url.urlJoin(apiRoot, 'users', newObject.id, '/'); } else if (result.hasOwnProperty('tags')) { newObject = result.tags[0]; - location = apiRoot + '/tags/' + newObject.id + '/'; + location = utils.url.urlJoin(apiRoot, 'tags', newObject.id, '/'); } } diff --git a/core/server/api/invites.js b/core/server/api/invites.js index acf5769f06..c678e5b125 100644 --- a/core/server/api/invites.js +++ b/core/server/api/invites.js @@ -113,7 +113,7 @@ invites = { invitedByName: loggedInUser.get('name'), invitedByEmail: loggedInUser.get('email'), // @TODO: resetLink sounds weird - resetLink: baseUrl.replace(/\/$/, '') + '/ghost/signup/' + globalUtils.encodeBase64URLsafe(invite.get('token')) + '/' + resetLink: globalUtils.url.urlJoin(baseUrl, 'ghost/signup', globalUtils.encodeBase64URLsafe(invite.get('token')), '/') }; return mail.utils.generateContent({data: emailData, template: 'invite-user'}); diff --git a/core/server/api/settings.js b/core/server/api/settings.js index b518e38aa7..97132a3bfe 100644 --- a/core/server/api/settings.js +++ b/core/server/api/settings.js @@ -9,6 +9,7 @@ var _ = require('lodash'), logging = require('../logging'), utils = require('./utils'), i18n = require('../i18n'), + generalUtils = require('../utils'), docName = 'settings', settings, @@ -64,7 +65,7 @@ updateConfigCache = function () { config.set('theme:twitter', (settingsCache.twitter && settingsCache.twitter.value) || ''); config.set('theme:facebook', (settingsCache.facebook && settingsCache.facebook.value) || ''); config.set('theme:timezone', (settingsCache.activeTimezone && settingsCache.activeTimezone.value) || config.get('theme').timezone); - config.set('theme:url', config.get('url') ? config.get('url').replace(/\/$/, '') : ''); + config.set('theme:url', config.get('url') ? generalUtils.url.urlJoin(config.get('url'), '/') : ''); _.each(labsValue, function (value, key) { config.set('labs:' + key, value); diff --git a/core/server/api/themes.js b/core/server/api/themes.js index 404427d80e..ff9381b724 100644 --- a/core/server/api/themes.js +++ b/core/server/api/themes.js @@ -64,7 +64,7 @@ themes = { }); }) .then(function () { - return storageAdapter.exists(config.getContentPath('themes') + '/' + zip.shortName); + return storageAdapter.exists(utils.url.urlJoin(config.getContentPath('themes'), zip.shortName)); }) .then(function (themeExists) { // delete existing theme diff --git a/core/server/blog/routes.js b/core/server/blog/routes.js index 02dd356b02..091122b30b 100644 --- a/core/server/blog/routes.js +++ b/core/server/blog/routes.js @@ -14,19 +14,19 @@ frontendRoutes = function frontendRoutes() { // ### Admin routes router.get(/^\/(logout|signout)\/$/, function redirectToSignout(req, res) { - utils.redirect301(res, subdir + '/ghost/signout/'); + utils.redirect301(res, utils.url.urlJoin(subdir, '/ghost/signout/')); }); router.get(/^\/signup\/$/, function redirectToSignup(req, res) { - utils.redirect301(res, subdir + '/ghost/signup/'); + utils.redirect301(res, utils.url.urlJoin(subdir, '/ghost/signup/')); }); // redirect to /ghost and let that do the authentication to prevent redirects to /ghost//admin etc. router.get(/^\/((ghost-admin|admin|wp-admin|dashboard|signin|login)\/?)$/, function redirectToAdmin(req, res) { - utils.redirect301(res, subdir + '/ghost/'); + utils.redirect301(res, utils.url.urlJoin(subdir, '/ghost/')); }); // Post Live Preview - router.get('/' + routeKeywords.preview + '/:uuid', frontend.preview); + router.get(utils.url.urlJoin('/', routeKeywords.preview, ':uuid'), frontend.preview); // Channels router.use(channels.router()); diff --git a/core/server/controllers/frontend/channel-config.js b/core/server/controllers/frontend/channel-config.js index 766e0ce988..3386611168 100644 --- a/core/server/controllers/frontend/channel-config.js +++ b/core/server/controllers/frontend/channel-config.js @@ -1,5 +1,6 @@ var _ = require('lodash'), config = require('../../config'), + utils = require('../../utils'), channelConfig; channelConfig = function channelConfig() { @@ -11,7 +12,7 @@ channelConfig = function channelConfig() { }, tag: { name: 'tag', - route: '/' + config.get('routeKeywords').tag + '/:slug/', + route: utils.url.urlJoin('/', config.get('routeKeywords').tag, ':slug/'), postOptions: { filter: 'tags:\'%s\'+tags.visibility:\'public\'' }, @@ -27,7 +28,7 @@ channelConfig = function channelConfig() { }, author: { name: 'author', - route: '/' + config.get('routeKeywords').author + '/:slug/', + route: utils.url.urlJoin('/', config.get('routeKeywords').author, ':slug/'), postOptions: { filter: 'author:\'%s\'' }, diff --git a/core/server/controllers/frontend/channels.js b/core/server/controllers/frontend/channels.js index 94d1368bf2..84edc38627 100644 --- a/core/server/controllers/frontend/channels.js +++ b/core/server/controllers/frontend/channels.js @@ -45,9 +45,9 @@ rssRouter = function rssRouter(channelConfig) { baseRoute = '/rss/'; router.get(baseRoute, stack); - router.get(baseRoute + ':page/', stack); + router.get(utils.url.urlJoin(baseRoute, ':page/'), stack); router.get('/feed/', function redirectToRSS(req, res) { - return utils.redirect301(res, utils.url.getSubdir() + req.baseUrl + baseRoute); + return utils.redirect301(res, utils.url.urlJoin(utils.url.getSubdir(), req.baseUrl, baseRoute)); }); router.param('page', handlePageParam); @@ -64,7 +64,7 @@ channelRouter = function router() { var channelsRouter = express.Router({mergeParams: true}), baseRoute = '/', - pageRoute = '/' + config.get('routeKeywords').page + '/:page/'; + pageRoute = utils.url.urlJoin('/', config.get('routeKeywords').page, ':page/'); _.each(channelConfig.list(), function (channel) { var channelRouter = express.Router({mergeParams: true}), @@ -78,7 +78,7 @@ channelRouter = function router() { if (channel.editRedirect) { channelRouter.get('/edit/', function redirect(req, res) { - res.redirect(utils.url.getSubdir() + channel.editRedirect.replace(':slug', req.params.slug)); + res.redirect(utils.url.urlJoin(utils.url.getSubdir(), channel.editRedirect.replace(':slug', req.params.slug))); }); } diff --git a/core/server/controllers/frontend/index.js b/core/server/controllers/frontend/index.js index fcf0e60b33..34dcdd2d53 100644 --- a/core/server/controllers/frontend/index.js +++ b/core/server/controllers/frontend/index.js @@ -71,8 +71,7 @@ frontendControllers = { // CASE: last param is of url is /edit, redirect to admin if (lookup.isEditURL) { - return res.redirect(utils.url.getSubdir() - + '/ghost/editor/' + post.id + '/'); + return res.redirect(utils.url.urlJoin(utils.url.getSubdir(), '/ghost/editor', post.id, '/')); } // CASE: permalink is not valid anymore, we redirect him permanently to the correct one diff --git a/core/server/data/importer/handlers/image.js b/core/server/data/importer/handlers/image.js index 781d85539d..01576252ae 100644 --- a/core/server/data/importer/handlers/image.js +++ b/core/server/data/importer/handlers/image.js @@ -37,9 +37,8 @@ ImageHandler = { return Promise.map(files, function (image) { return store.getUniqueFileName(store, image, image.targetDir).then(function (targetFilename) { - image.newPath = (utils.url.getSubdir() + '/' + - config.get('paths').imagesRelPath + '/' + path.relative(config.getContentPath('images'), targetFilename)) - .replace(new RegExp('\\' + path.sep, 'g'), '/'); + image.newPath = utils.url.urlJoin('/', utils.url.getSubdir(), config.get('paths').imagesRelPath, + path.relative(config.getContentPath('images'), targetFilename)); return image; }); diff --git a/core/server/data/meta/amp_url.js b/core/server/data/meta/amp_url.js index 56392d09d9..8843d1a25d 100644 --- a/core/server/data/meta/amp_url.js +++ b/core/server/data/meta/amp_url.js @@ -6,8 +6,7 @@ function getAmplUrl(data) { var context = data.context ? data.context : null; if (_.includes(context, 'post') && !_.includes(context, 'amp')) { - return utils.url.urlJoin(utils.url.getBaseUrl(false), - getUrl(data, false)) + 'amp/'; + return utils.url.urlJoin(utils.url.getBaseUrl(false), getUrl(data, false), 'amp/'); } return null; } diff --git a/core/server/data/meta/asset_url.js b/core/server/data/meta/asset_url.js index b02d85ea17..004c036650 100644 --- a/core/server/data/meta/asset_url.js +++ b/core/server/data/meta/asset_url.js @@ -4,14 +4,14 @@ var config = require('../../config'), function getAssetUrl(path, isAdmin, minify) { var output = ''; - output += utils.url.getSubdir() + '/'; + output += utils.url.urlJoin(utils.url.getSubdir(), '/'); if (!path.match(/^favicon\.ico$/) && !path.match(/^shared/) && !path.match(/^asset/)) { if (isAdmin) { - output += 'ghost/'; + output = utils.url.urlJoin(output, 'ghost/'); } - output += 'assets/'; + output = utils.url.urlJoin(output, 'assets/'); } // Get rid of any leading slash on the path diff --git a/core/server/data/meta/paginated_url.js b/core/server/data/meta/paginated_url.js index 501667e944..838f9e57bd 100644 --- a/core/server/data/meta/paginated_url.js +++ b/core/server/data/meta/paginated_url.js @@ -8,7 +8,7 @@ function getPaginatedUrl(page, data, absolute) { return null; } - var pagePath = '/' + config.get('routeKeywords').page + '/', + var pagePath = utils.url.urlJoin('/', config.get('routeKeywords').page, '/'), // Try to match the base url, as whatever precedes the pagePath baseUrlPattern = new RegExp('(.+)?(/' + config.get('routeKeywords').page + '/\\d+/)'), baseUrlMatch = data.relativeUrl.match(baseUrlPattern), @@ -17,18 +17,18 @@ function getPaginatedUrl(page, data, absolute) { newRelativeUrl; if (page === 'next' && data.pagination.next) { - newRelativeUrl = pagePath + data.pagination.next + '/'; + newRelativeUrl = utils.url.urlJoin(pagePath, data.pagination.next, '/'); } else if (page === 'prev' && data.pagination.prev) { - newRelativeUrl = data.pagination.prev > 1 ? pagePath + data.pagination.prev + '/' : '/'; + newRelativeUrl = data.pagination.prev > 1 ? utils.url.urlJoin(pagePath, data.pagination.prev, '/') : '/'; } else if (_.isNumber(page)) { - newRelativeUrl = page > 1 ? pagePath + page + '/' : '/'; + newRelativeUrl = page > 1 ? utils.url.urlJoin(pagePath, page, '/') : '/'; } else { // If none of the cases match, return null right away return null; } // baseUrl can be undefined, if there was nothing preceding the pagePath (e.g. first page of the index channel) - newRelativeUrl = baseUrl ? baseUrl + newRelativeUrl : newRelativeUrl; + newRelativeUrl = baseUrl ? utils.url.urlJoin(baseUrl, newRelativeUrl) : newRelativeUrl; return utils.url.urlFor({relativeUrl: newRelativeUrl, secure: data.secure}, absolute); } diff --git a/core/server/data/migration/backup.js b/core/server/data/migration/backup.js index 073906ff39..e8fcf5d5a8 100644 --- a/core/server/data/migration/backup.js +++ b/core/server/data/migration/backup.js @@ -6,12 +6,13 @@ var _ = require('lodash'), Promise = require('bluebird'), config = require('../../config'), exporter = require('../export'), + utils = require('../../utils'), writeExportFile, backup; writeExportFile = function writeExportFile(exportResult) { - var filename = path.resolve(config.get('paths').contentPath + '/data/' + exportResult.filename); + var filename = path.resolve(utils.url.urlJoin(config.get('paths').contentPath, 'data', exportResult.filename)); return Promise.promisify(fs.writeFile)(filename, JSON.stringify(exportResult.data)).return(filename); }; diff --git a/core/server/data/xml/rss/index.js b/core/server/data/xml/rss/index.js index 090cdf9193..f5fe795260 100644 --- a/core/server/data/xml/rss/index.js +++ b/core/server/data/xml/rss/index.js @@ -18,11 +18,11 @@ var crypto = require('crypto'), feedCache = {}; function isTag(req) { - return req.originalUrl.indexOf('/' + config.get('routeKeywords').tag + '/') !== -1; + return req.originalUrl.indexOf(utils.url.urlJoin('/', config.get('routeKeywords').tag, '/')) !== -1; } function isAuthor(req) { - return req.originalUrl.indexOf('/' + config.get('routeKeywords').author + '/') !== -1; + return req.originalUrl.indexOf(utils.url.urlJoin('/', config.get('routeKeywords').author, '/')) !== -1; } function handleError(next) { @@ -56,11 +56,11 @@ function getBaseUrl(req, slugParam) { var baseUrl = utils.url.getSubdir(); if (isTag(req)) { - baseUrl += '/' + config.get('routeKeywords').tag + '/' + slugParam + '/rss/'; + baseUrl = utils.url.urlJoin(baseUrl, config.get('routeKeywords').tag, slugParam, 'rss/'); } else if (isAuthor(req)) { - baseUrl += '/' + config.get('routeKeywords').author + '/' + slugParam + '/rss/'; + baseUrl = utils.url.urlJoin(baseUrl, config.get('routeKeywords').author, slugParam, 'rss/'); } else { - baseUrl += '/rss/'; + baseUrl = utils.url.urlJoin(baseUrl, 'rss/'); } return baseUrl; diff --git a/core/server/storage/local-file-store.js b/core/server/storage/local-file-store.js index c03af6075a..dfcd772ce6 100644 --- a/core/server/storage/local-file-store.js +++ b/core/server/storage/local-file-store.js @@ -37,10 +37,9 @@ LocalFileStore.prototype.save = function save(image, targetDir) { // The src for the image must be in URI format, not a file system path, which in Windows uses \ // For local file system storage can use relative path so add a slash var fullUrl = ( - utils.url.getSubdir() + '/' + - config.get('paths').imagesRelPath + - '/' + - path.relative(config.getContentPath('images'), targetFilename) + utils.url.urlJoin('/', utils.url.getSubdir(), + config.get('paths').imagesRelPath, + path.relative(config.getContentPath('images'), targetFilename)) ).replace(new RegExp('\\' + path.sep, 'g'), '/'); return fullUrl; diff --git a/core/server/utils/url.js b/core/server/utils/url.js index a1f3cd5b5c..a23a419b1a 100644 --- a/core/server/utils/url.js +++ b/core/server/utils/url.js @@ -47,6 +47,14 @@ function getProtectedSlugs() { } } +// ## urlJoin +// concats arguments to a path/URL +// Usage: +// urlJoin(getBaseUrl(), 'content', '/') -> http://my-ghost-blog.com/content/ +// Returns a URL or relative path +// Only to use for Ghost URLs and paths +// TODO: urlJoin needs to be optimised and to validate the URL/path properly. +// e. g. URLs should end with a trailing `/` at the end of the pathname. function urlJoin() { var args = Array.prototype.slice.call(arguments), prefixDoubleSlash = false, diff --git a/core/test/functional/routes/frontend_spec.js b/core/test/functional/routes/frontend_spec.js index 36923220f7..e485d60bfa 100644 --- a/core/test/functional/routes/frontend_spec.js +++ b/core/test/functional/routes/frontend_spec.js @@ -657,7 +657,7 @@ describe('Frontend Routing', function () { request.get('/') .expect(200) .expect(//) - .expect(/Ghost<\/a\>/) + .expect(/Ghost<\/a\>/) .end(doEnd(done)); });