Skip to content

Commit

Permalink
馃悰 Fixed preview url and Zapier on subdirectory (#9683)
Browse files Browse the repository at this point in the history
closes #9675

- with dynamic routing we have introduced a breaking change, which we have overseen
- Ghost does not return absolute urls, that's why the clients need to concat the blog url and the resource url
- with 1.24.0 Ghost returned resource urls including the subdirectory
- this caused trouble for e.g. zapier or the preview feature in the admin client
- revert breaking change and ensure we only expose resource urls without subdirectory
  • Loading branch information
kirrg001 authored and kevinansfield committed Jun 12, 2018
1 parent fe8c073 commit 7b0d5d4
Show file tree
Hide file tree
Showing 22 changed files with 193 additions and 68 deletions.
3 changes: 1 addition & 2 deletions core/server/apps/amp/lib/router.js
Expand Up @@ -35,7 +35,6 @@ function _renderer(req, res, next) {
function getPostData(req, res, next) {
req.body = req.body || {};

const urlWithSubdirectoryWithoutAmp = req.originalUrl.match(/(.*?\/)amp/)[1];
const urlWithoutSubdirectoryWithoutAmp = res.locals.relativeUrl.match(/(.*?\/)amp/)[1];

/**
Expand All @@ -58,7 +57,7 @@ function getPostData(req, res, next) {
*
* The challenge is to design different types of apps e.g. extensions of routers, standalone pages etc.
*/
const permalinks = urlService.getPermalinkByUrl(urlWithSubdirectoryWithoutAmp, {withUrlOptions: true});
const permalinks = urlService.getPermalinkByUrl(urlWithoutSubdirectoryWithoutAmp, {withUrlOptions: true});

if (!permalinks) {
return next(new common.errors.NotFoundError({
Expand Down
2 changes: 1 addition & 1 deletion core/server/apps/subscribers/lib/router.js
Expand Up @@ -67,7 +67,7 @@ function handleSource(req, res, next) {
delete req.body.location;
delete req.body.referrer;

const resource = urlService.getResource(urlService.utils.absoluteToRelative(req.body.subscribed_url));
const resource = urlService.getResource(urlService.utils.absoluteToRelative(req.body.subscribed_url, {withoutSubdirectory: true}));

if (resource) {
req.body.post_id = resource.data.id;
Expand Down
4 changes: 2 additions & 2 deletions core/server/data/meta/author_url.js
Expand Up @@ -6,11 +6,11 @@ function getAuthorUrl(data, absolute) {
context = context === 'amp' ? 'post' : context;

if (data.author) {
return urlService.getUrlByResourceId(data.author.id, {absolute: absolute, secure: data.author.secure});
return urlService.getUrlByResourceId(data.author.id, {absolute: absolute, secure: data.author.secure, withSubdirectory: true});
}

if (data[context] && data[context].primary_author) {
return urlService.getUrlByResourceId(data[context].primary_author.id, {absolute: absolute, secure: data[context].secure});
return urlService.getUrlByResourceId(data[context].primary_author.id, {absolute: absolute, secure: data[context].secure, withSubdirectory: true});
}

return null;
Expand Down
2 changes: 1 addition & 1 deletion core/server/data/meta/url.js
Expand Up @@ -13,7 +13,7 @@ function sanitizeAmpUrl(url) {

function getUrl(data, absolute) {
if (schema.isPost(data) || schema.isTag(data) || schema.isUser(data)) {
return urlService.getUrlByResourceId(data.id, {secure: data.secure, absolute: absolute});
return urlService.getUrlByResourceId(data.id, {secure: data.secure, absolute: absolute, withSubdirectory: true});
}

if (schema.isNav(data)) {
Expand Down
2 changes: 1 addition & 1 deletion core/server/helpers/author.js
Expand Up @@ -30,7 +30,7 @@ module.exports = function author(options) {
if (this.author && this.author.name) {
if (autolink) {
output = templates.link({
url: urlService.getUrlByResourceId(this.author.id),
url: urlService.getUrlByResourceId(this.author.id, {withSubdirectory: true}),
text: _.escape(this.author.name)
});
} else {
Expand Down
2 changes: 1 addition & 1 deletion core/server/helpers/authors.js
Expand Up @@ -31,7 +31,7 @@ module.exports = function authors(options) {
function createAuthorsList(authors) {
function processAuthor(author) {
return autolink ? templates.link({
url: urlService.getUrlByResourceId(author.id),
url: urlService.getUrlByResourceId(author.id, {withSubdirectory: true}),
text: _.escape(author.name)
}) : _.escape(author.name);
}
Expand Down
2 changes: 1 addition & 1 deletion core/server/helpers/tags.js
Expand Up @@ -30,7 +30,7 @@ module.exports = function tags(options) {
function createTagList(tags) {
function processTag(tag) {
return autolink ? templates.link({
url: urlService.getUrlByResourceId(tag.id),
url: urlService.getUrlByResourceId(tag.id, {withSubdirectory: true}),
text: _.escape(tag.name)
}) : _.escape(tag.name);
}
Expand Down
15 changes: 5 additions & 10 deletions core/server/services/routing/controllers/entry.js
Expand Up @@ -50,7 +50,7 @@ module.exports = function entryController(req, res, next) {
*
* That's why we have to check against the router type.
*/
if (urlService.getResource(post.url).config.type !== res.locals.routerOptions.type) {
if (urlService.getResourceById(post.id).config.type !== res.locals.routerOptions.type) {
debug('not my resource type');
return next();
}
Expand All @@ -60,20 +60,15 @@ module.exports = function entryController(req, res, next) {
* This should only happen if you have date permalinks enabled and you change
* your publish date.
*
* @NOTE
* @NOTE:
*
* The resource url always contains the subdirectory. This was different before dynamic routing.
* That's why we have to use the original url, which contains the sub-directory.
*
* @NOTE
*
* post.url contains the subdirectory if configured.
* Ensure we redirect to the correct post url including subdirectory.
*/
if (post.url !== url.parse(req.originalUrl).pathname) {
if (post.url !== req.path) {
debug('redirect');

return urlService.utils.redirect301(res, url.format({
pathname: post.url,
pathname: urlService.utils.createUrl(post.url, false, false, true),
search: url.parse(req.originalUrl).search
}));
}
Expand Down
2 changes: 1 addition & 1 deletion core/server/services/routing/controllers/preview.js
Expand Up @@ -30,7 +30,7 @@ module.exports = function previewController(req, res, next) {
}

if (post.status === 'published') {
return urlService.utils.redirect301(res, urlService.getUrlByResourceId(post.id));
return urlService.utils.redirect301(res, urlService.getUrlByResourceId(post.id, {withSubdirectory: true}));
}

helpers.secure(req, post);
Expand Down
6 changes: 2 additions & 4 deletions core/server/services/url/UrlGenerator.js
Expand Up @@ -133,13 +133,11 @@ class UrlGenerator {
}

/**
* We currently generate relative urls.
* We currently generate relative urls without subdirectory.
*/
_generateUrl(resource) {
const permalink = this.router.getPermalinks().getValue();
const url = localUtils.replacePermalink(permalink, resource.data);

return localUtils.createUrl(url, false, false, true);
return localUtils.replacePermalink(permalink, resource.data);
}

/**
Expand Down
21 changes: 21 additions & 0 deletions core/server/services/url/UrlService.js
Expand Up @@ -125,6 +125,19 @@ class UrlService {
return objects[0].resource;
}

getResourceById(resourceId) {
const object = this.urls.getByResourceId(resourceId);

if (!object) {
throw new common.errors.InternalServerError({
message: 'Resource not found.',
code: 'URLSERVICE_RESOURCE_NOT_FOUND'
});
}

return object.resource;
}

hasFinished() {
return this.finished;
}
Expand All @@ -150,13 +163,21 @@ class UrlService {
return this.utils.createUrl(obj.url, options.absolute, options.secure);
}

if (options.withSubdirectory) {
return this.utils.createUrl(obj.url, false, options.secure, true);
}

return obj.url;
}

if (options.absolute) {
return this.utils.createUrl('/404/', options.absolute, options.secure);
}

if (options.withSubdirectory) {
return this.utils.createUrl('/404/', false, options.secure);
}

return '/404/';
}

Expand Down
19 changes: 17 additions & 2 deletions core/server/services/url/utils.js
Expand Up @@ -408,9 +408,24 @@ function makeAbsoluteUrls(html, siteUrl, itemUrl) {
return htmlContent;
}

function absoluteToRelative(urlToModify) {
function absoluteToRelative(urlToModify, options) {
options = options || {};

const urlObj = url.parse(urlToModify);
return urlObj.pathname;
const relativePath = urlObj.pathname;

if (options.withoutSubdirectory) {
const subDir = getSubdir();

if (!subDir) {
return relativePath;
}

const subDirRegex = new RegExp('^' + subDir);
return relativePath.replace(subDirRegex, '');
}

return relativePath;
}

function deduplicateDoubleSlashes(url) {
Expand Down
28 changes: 14 additions & 14 deletions core/test/integration/services/url/UrlService_spec.js
Expand Up @@ -542,7 +542,7 @@ describe('Unit: services/url/UrlService', function () {
let router1, router2, router3, router4, router5;

beforeEach(function (done) {
configUtils.set('url', 'http://localhost:2388/blog');
configUtils.set('url', 'http://localhost:2388/blog/');

urlService = new UrlService();

Expand Down Expand Up @@ -694,44 +694,44 @@ describe('Unit: services/url/UrlService', function () {
});

let url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.posts[0].id);
url.should.eql('/blog/collection/2015/html-ipsum/');
url.should.eql('/collection/2015/html-ipsum/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.posts[1].id);
url.should.eql('/blog/collection/2015/ghostly-kitchen-sink/');
url.should.eql('/collection/2015/ghostly-kitchen-sink/');

// featured
url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.posts[2].id);
url.should.eql('/blog/podcast/short-and-sweet/');
url.should.eql('/podcast/short-and-sweet/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.tags[0].id);
url.should.eql('/blog/category/kitchen-sink/');
url.should.eql('/category/kitchen-sink/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.tags[1].id);
url.should.eql('/blog/category/bacon/');
url.should.eql('/category/bacon/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.tags[2].id);
url.should.eql('/blog/category/chorizo/');
url.should.eql('/category/chorizo/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.tags[3].id);
url.should.eql('/blog/category/pollo/');
url.should.eql('/category/pollo/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.tags[4].id);
url.should.eql('/blog/category/injection/');
url.should.eql('/category/injection/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.users[0].id);
url.should.eql('/blog/persons/joe-bloggs/');
url.should.eql('/persons/joe-bloggs/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.users[1].id);
url.should.eql('/blog/persons/smith-wellingsworth/');
url.should.eql('/persons/smith-wellingsworth/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.users[2].id);
url.should.eql('/blog/persons/jimothy-bogendath/');
url.should.eql('/persons/jimothy-bogendath/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.users[3].id);
url.should.eql('/blog/persons/slimer-mcectoplasm/');
url.should.eql('/persons/slimer-mcectoplasm/');

url = urlService.getUrlByResourceId(testUtils.DataGenerator.forKnex.users[4].id);
url.should.eql('/blog/persons/contributor/');
url.should.eql('/persons/contributor/');
});
});
});
2 changes: 1 addition & 1 deletion core/test/integration/web/site_spec.js
Expand Up @@ -595,7 +595,7 @@ describe('Integration - Web - Site', function () {
},

'/': {
permalink: '/:slug'
permalink: '/:slug/'
}
},

Expand Down
2 changes: 1 addition & 1 deletion core/test/unit/apps/amp/router_spec.js
Expand Up @@ -137,7 +137,7 @@ describe('Unit - apps/amp/lib/router', function () {
req.originalUrl = '/blog/welcome/amp';
res.locals.relativeUrl = '/welcome/amp';

urlService.getPermalinkByUrl.withArgs('/blog/welcome/').returns('/:slug/');
urlService.getPermalinkByUrl.withArgs('/welcome/').returns('/:slug/');

helpers.postLookup.withArgs('/welcome/', {permalinks: '/:slug/'}).resolves({
post: post
Expand Down
8 changes: 4 additions & 4 deletions core/test/unit/data/meta/author_url_spec.js
Expand Up @@ -22,7 +22,7 @@ describe('getAuthorUrl', function () {
}
};

urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('author url');

should.exist(getAuthorUrl({
Expand All @@ -39,7 +39,7 @@ describe('getAuthorUrl', function () {
}
};

urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: true, secure: undefined})
urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: true, secure: undefined, withSubdirectory: true})
.returns('absolute author url');

should.exist(getAuthorUrl({
Expand All @@ -56,7 +56,7 @@ describe('getAuthorUrl', function () {
}
};

urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(post.primary_author.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('author url');

should.exist(getAuthorUrl({
Expand All @@ -71,7 +71,7 @@ describe('getAuthorUrl', function () {
slug: 'test-author'
};

urlService.getUrlByResourceId.withArgs(author.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(author.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('author url');

should.exist(getAuthorUrl({
Expand Down
12 changes: 6 additions & 6 deletions core/test/unit/data/meta/url_spec.js
Expand Up @@ -18,7 +18,7 @@ describe('getUrl', function () {
it('should return url for a post', function () {
const post = testUtils.DataGenerator.forKnex.createPost();

urlService.getUrlByResourceId.withArgs(post.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(post.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('post url');

getUrl(post).should.eql('post url');
Expand All @@ -27,7 +27,7 @@ describe('getUrl', function () {
it('should return absolute url for a post', function () {
const post = testUtils.DataGenerator.forKnex.createPost();

urlService.getUrlByResourceId.withArgs(post.id, {absolute: true, secure: undefined})
urlService.getUrlByResourceId.withArgs(post.id, {absolute: true, secure: undefined, withSubdirectory: true})
.returns('absolute post url');

getUrl(post, true).should.eql('absolute post url');
Expand All @@ -49,7 +49,7 @@ describe('getUrl', function () {
// the tag object contains a `parent` attribute. the tag model contains a `parent_id` attr.
tag.parent = null;

urlService.getUrlByResourceId.withArgs(tag.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(tag.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('tag url');

getUrl(tag).should.eql('tag url');
Expand All @@ -66,7 +66,7 @@ describe('getUrl', function () {
// @TODO: WTF O_O
tag.secure = true;

urlService.getUrlByResourceId.withArgs(tag.id, {absolute: undefined, secure: true})
urlService.getUrlByResourceId.withArgs(tag.id, {absolute: undefined, secure: true, withSubdirectory: true})
.returns('secure tag url');

getUrl(tag).should.eql('secure tag url');
Expand All @@ -75,7 +75,7 @@ describe('getUrl', function () {
it('should return url for a author', function () {
const author = testUtils.DataGenerator.forKnex.createUser();

urlService.getUrlByResourceId.withArgs(author.id, {absolute: undefined, secure: undefined})
urlService.getUrlByResourceId.withArgs(author.id, {absolute: undefined, secure: undefined, withSubdirectory: true})
.returns('author url');

getUrl(author).should.eql('author url');
Expand All @@ -87,7 +87,7 @@ describe('getUrl', function () {
// @TODO: WTF
author.secure = true;

urlService.getUrlByResourceId.withArgs(author.id, {absolute: true, secure: true})
urlService.getUrlByResourceId.withArgs(author.id, {absolute: true, secure: true, withSubdirectory: true})
.returns('absolute secure author url');

getUrl(author, true).should.eql('absolute secure author url');
Expand Down

0 comments on commit 7b0d5d4

Please sign in to comment.