From 8d88f5b6a50ff42eb1f3b06ce56f78139da20eff Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Wed, 11 Jan 2017 10:45:56 +0000 Subject: [PATCH] urlencode navigation URLs rather than HTML escape (#7836) closes #7826 - expose raw url value inside `{{navigation}}` helper - modify `{{url}}` helper to urlencode values and mark as HTML-safe to avoid Handlebars additional HTML-escaping --- core/server/helpers/navigation.js | 2 +- core/server/helpers/url.js | 10 ++- .../unit/server_helpers/navigation_spec.js | 37 +++++++++ core/test/unit/server_helpers/url_spec.js | 75 ++++++++++++------- 4 files changed, 93 insertions(+), 31 deletions(-) diff --git a/core/server/helpers/navigation.js b/core/server/helpers/navigation.js index 759c1bbf3f..42ab985e30 100644 --- a/core/server/helpers/navigation.js +++ b/core/server/helpers/navigation.js @@ -66,7 +66,7 @@ navigation = function (options) { out.current = _isCurrentUrl(e.url, currentUrl); out.label = e.label; out.slug = _slugify(e.label); - out.url = hbs.handlebars.Utils.escapeExpression(e.url); + out.url = e.url; out.secure = self.secure; return out; }); diff --git a/core/server/helpers/url.js b/core/server/helpers/url.js index 5457af13b7..ae9bc754fa 100644 --- a/core/server/helpers/url.js +++ b/core/server/helpers/url.js @@ -4,12 +4,16 @@ // Returns the URL for the current object scope i.e. If inside a post scope will return post permalink // `absolute` flag outputs absolute URL, else URL is relative -var getMetaDataUrl = require('../data/meta/url'); +var hbs = require('express-hbs'), + getMetaDataUrl = require('../data/meta/url'); function url(options) { - var absolute = options && options.hash.absolute; + var absolute = options && options.hash.absolute, + url = getMetaDataUrl(this, absolute); - return getMetaDataUrl(this, absolute); + url = encodeURI(decodeURI(url)); + + return new hbs.SafeString(url); } module.exports = url; diff --git a/core/test/unit/server_helpers/navigation_spec.js b/core/test/unit/server_helpers/navigation_spec.js index 3bcbf6e7f6..727f61d1c6 100644 --- a/core/test/unit/server_helpers/navigation_spec.js +++ b/core/test/unit/server_helpers/navigation_spec.js @@ -143,6 +143,43 @@ describe('{{navigation}} helper', function () { rendered.string.should.containEql('nav-foo nav-current'); rendered.string.should.containEql('nav-bar"'); }); + + it('doesn\'t html-escape URLs', function () { + var firstItem = {label: 'Foo', url: '/?foo=bar&baz=qux'}, + rendered; + + optionsData.data.blog.navigation = [firstItem]; + rendered = helpers.navigation(optionsData); + + should.exist(rendered); + rendered.string.should.not.containEql('='); + rendered.string.should.not.containEql('&'); + rendered.string.should.containEql('/?foo=bar&baz=qux'); + }); + + it('encodes URLs', function () { + var firstItem = {label: 'Foo', url: '/?foo=space bar&'}, + rendered; + + optionsData.data.blog.navigation = [firstItem]; + rendered = helpers.navigation(optionsData); + + should.exist(rendered); + rendered.string.should.containEql('foo=space%20bar'); + rendered.string.should.not.containEql(''); + rendered.string.should.containEql('%3Cscript%3Ealert(%22gotcha%22)%3C/script%3E'); + }); + + it('doesn\'t double-encode URLs', function () { + var firstItem = {label: 'Foo', url: '/?foo=space%20bar'}, + rendered; + + optionsData.data.blog.navigation = [firstItem]; + rendered = helpers.navigation(optionsData); + + should.exist(rendered); + rendered.string.should.not.containEql('foo=space%2520bar'); + }); }); describe('{{navigation}} helper with custom template', function () { diff --git a/core/test/unit/server_helpers/url_spec.js b/core/test/unit/server_helpers/url_spec.js index a654540c15..66beafcd58 100644 --- a/core/test/unit/server_helpers/url_spec.js +++ b/core/test/unit/server_helpers/url_spec.js @@ -49,7 +49,7 @@ describe('{{url}} helper', function () { }); should.exist(rendered); - rendered.should.equal('/slug/'); + rendered.string.should.equal('/slug/'); }); it('should output an absolute URL if the option is present', function () { @@ -59,7 +59,7 @@ describe('{{url}} helper', function () { ); should.exist(rendered); - rendered.should.equal('http://testurl.com/slug/'); + rendered.string.should.equal('http://testurl.com/slug/'); }); it('should output an absolute URL with https if the option is present and secure', function () { @@ -70,7 +70,7 @@ describe('{{url}} helper', function () { ); should.exist(rendered); - rendered.should.equal('https://testurl.com/slug/'); + rendered.string.should.equal('https://testurl.com/slug/'); }); it('should output an absolute URL with https if secure', function () { @@ -81,7 +81,7 @@ describe('{{url}} helper', function () { ); should.exist(rendered); - rendered.should.equal('https://testurl.com/slug/'); + rendered.string.should.equal('https://testurl.com/slug/'); }); it('should return the slug with a prefixed /tag/ if the context is a tag', function () { @@ -93,32 +93,32 @@ describe('{{url}} helper', function () { }); should.exist(rendered); - rendered.should.equal('/tag/the-tag/'); + rendered.string.should.equal('/tag/the-tag/'); }); it('should return / if not a post or tag', function () { rendered = helpers.url.call({markdown: 'ff', title: 'title', slug: 'slug'}); should.exist(rendered); - rendered.should.equal('/'); + rendered.string.should.equal('/'); rendered = helpers.url.call({html: 'content', title: 'title', slug: 'slug'}); should.exist(rendered); - rendered.should.equal('/'); + rendered.string.should.equal('/'); rendered = helpers.url.call({html: 'content', markdown: 'ff', slug: 'slug'}); should.exist(rendered); - rendered.should.equal('/'); + rendered.string.should.equal('/'); rendered = helpers.url.call({html: 'content', markdown: 'ff', title: 'title'}); should.exist(rendered); - rendered.should.equal('/'); + rendered.string.should.equal('/'); }); it('should return a relative url if passed through a nav context', function () { rendered = helpers.url.call( {url: '/foo', label: 'Foo', slug: 'foo', current: true}); should.exist(rendered); - rendered.should.equal('/foo'); + rendered.string.should.equal('/foo'); }); it('should return an absolute url if passed through a nav context', function () { @@ -126,7 +126,7 @@ describe('{{url}} helper', function () { {url: '/bar', label: 'Bar', slug: 'bar', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://testurl.com/bar'); + rendered.string.should.equal('http://testurl.com/bar'); }); it('should return an absolute url with https if context is secure', function () { @@ -134,7 +134,7 @@ describe('{{url}} helper', function () { {url: '/bar', label: 'Bar', slug: 'bar', current: true, secure: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('https://testurl.com/bar'); + rendered.string.should.equal('https://testurl.com/bar'); }); it('external urls should be retained in a nav context', function () { @@ -142,7 +142,7 @@ describe('{{url}} helper', function () { {url: 'http://casper.website/baz', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://casper.website/baz'); + rendered.string.should.equal('http://casper.website/baz'); }); it('should handle hosted urls in a nav context', function () { @@ -150,7 +150,7 @@ describe('{{url}} helper', function () { {url: 'http://testurl.com/qux', label: 'Qux', slug: 'qux', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://testurl.com/qux'); + rendered.string.should.equal('http://testurl.com/qux'); }); it('should handle hosted urls in a nav context with secure', function () { @@ -159,7 +159,7 @@ describe('{{url}} helper', function () { secure: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('https://testurl.com/qux'); + rendered.string.should.equal('https://testurl.com/qux'); }); it('should handle hosted https urls in a nav context with secure', function () { @@ -168,7 +168,7 @@ describe('{{url}} helper', function () { secure: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('https://testurl.com/qux'); + rendered.string.should.equal('https://testurl.com/qux'); }); it('should handle hosted urls with the wrong protocol in a nav context', function () { @@ -176,7 +176,7 @@ describe('{{url}} helper', function () { {url: 'https://testurl.com/quux', label: 'Quux', slug: 'quux', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://testurl.com/quux'); + rendered.string.should.equal('http://testurl.com/quux'); }); it('should pass through protocol-less URLs regardless of absolute setting', function () { @@ -184,13 +184,13 @@ describe('{{url}} helper', function () { {url: '//casper.website/baz', label: 'Baz', slug: 'baz', current: true}, {hash: {}}); should.exist(rendered); - rendered.should.equal('//casper.website/baz'); + rendered.string.should.equal('//casper.website/baz'); rendered = helpers.url.call( {url: '//casper.website/baz', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('//casper.website/baz'); + rendered.string.should.equal('//casper.website/baz'); }); it('should pass through URLs with alternative schemes regardless of absolute setting', function () { @@ -198,25 +198,25 @@ describe('{{url}} helper', function () { {url: 'tel:01234567890', label: 'Baz', slug: 'baz', current: true}, {hash: {}}); should.exist(rendered); - rendered.should.equal('tel:01234567890'); + rendered.string.should.equal('tel:01234567890'); rendered = helpers.url.call( {url: 'mailto:example@ghost.org', label: 'Baz', slug: 'baz', current: true}, {hash: {}}); should.exist(rendered); - rendered.should.equal('mailto:example@ghost.org'); + rendered.string.should.equal('mailto:example@ghost.org'); rendered = helpers.url.call( {url: 'tel:01234567890', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('tel:01234567890'); + rendered.string.should.equal('tel:01234567890'); rendered = helpers.url.call( {url: 'mailto:example@ghost.org', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('mailto:example@ghost.org'); + rendered.string.should.equal('mailto:example@ghost.org'); }); it('should pass through anchor-only URLs regardless of absolute setting', function () { @@ -224,13 +224,34 @@ describe('{{url}} helper', function () { {url: '#thatsthegoodstuff', label: 'Baz', slug: 'baz', current: true}, {hash: {}}); should.exist(rendered); - rendered.should.equal('#thatsthegoodstuff'); + rendered.string.should.equal('#thatsthegoodstuff'); rendered = helpers.url.call( {url: '#thatsthegoodstuff', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('#thatsthegoodstuff'); + rendered.string.should.equal('#thatsthegoodstuff'); + }); + + it('should not HTML-escape URLs', function () { + rendered = helpers.url.call( + {url: '/foo?foo=bar&baz=qux', label: 'Foo', slug: 'foo', current: true}); + should.exist(rendered); + rendered.string.should.equal('/foo?foo=bar&baz=qux'); + }); + + it('should encode URLs', function () { + rendered = helpers.url.call( + {url: '/foo?foo=bar&baz=qux&', label: 'Foo', slug: 'foo', current: true}); + should.exist(rendered); + rendered.string.should.equal('/foo?foo=bar&baz=qux&%3Cscript%3Ealert(%22gotcha%22)%3C/script%3E'); + }); + + it('should not double-encode URLs', function () { + rendered = helpers.url.call( + {url: '/?foo=space%20bar', label: 'Foo', slug: 'foo', current: true}); + should.exist(rendered); + rendered.string.should.equal('/?foo=space%20bar'); }); describe('with subdir', function () { @@ -240,7 +261,7 @@ describe('{{url}} helper', function () { {url: 'http://casper.website/baz', label: 'Baz', slug: 'baz', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://casper.website/baz'); + rendered.string.should.equal('http://casper.website/baz'); }); it('should handle subdir being set in nav context', function () { @@ -250,7 +271,7 @@ describe('{{url}} helper', function () { {url: '/xyzzy', label: 'xyzzy', slug: 'xyzzy', current: true}, {hash: {absolute: 'true'}}); should.exist(rendered); - rendered.should.equal('http://testurl.com/blog/xyzzy'); + rendered.string.should.equal('http://testurl.com/blog/xyzzy'); }); }); });