💄 Improve URL consistency, Part 1: urlJoin (#7668)

refs #7666 Use urlJoin for more consistency instead of concatenating url strings.
TryGhost · Nov 14, 2016 · 06061d5 · 06061d5
1 parent 4a2ddbe
commit 06061d5
Show file tree

Hide file tree

Showing 18 changed files with 55 additions and 48 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -657,7 +657,7 @@ describe('Frontend Routing', function () {
             request.get('/')
                 .expect(200)
                 .expect(/<link rel="canonical" href="http:\/\/127.0.0.1:2370\/" \/\>/)
-                .expect(/<a href="http:\/\/127.0.0.1:2370">Ghost<\/a\>/)
+                .expect(/<a href="http:\/\/127.0.0.1:2370\/">Ghost<\/a\>/)
                 .end(doEnd(done));
         });