From c117f6de04beef7a58aee3f14495999e7e6c62cd Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Thu, 4 Apr 2024 15:39:51 +0200 Subject: [PATCH] DEV: Redo `DiscourseURL.isInternal()` (#26504) Simplify the implementation and make it return `true` for prefix-less relative urls, like `docs` --- .../javascripts/discourse/app/lib/url.js | 49 ++++++++++-------- .../tests/unit/lib/click-track-test.js | 4 +- .../discourse/tests/unit/lib/url-test.js | 51 ++++++++++++++----- 3 files changed, 66 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/url.js b/app/assets/javascripts/discourse/app/lib/url.js index f1d12162f10..c4d4546be0d 100644 --- a/app/assets/javascripts/discourse/app/lib/url.js +++ b/app/assets/javascripts/discourse/app/lib/url.js @@ -286,27 +286,32 @@ const DiscourseURL = EmberObject.extend({ // Determines whether a URL is internal or not isInternal(url) { - if (url && url.length) { - if (url.startsWith("//")) { - url = "http:" + url; - } - if (url.startsWith("#")) { - return true; - } - if (url.startsWith("/")) { - return true; - } - if (url.startsWith(this.origin())) { - return true; - } - if (url.replace(/^http/, "https").startsWith(this.origin())) { - return true; - } - if (url.replace(/^https/, "http").startsWith(this.origin())) { - return true; - } + if (!url?.length) { + return false; } - return false; + + if (url.startsWith("//")) { + url = "http:" + url; + } + + if (url.startsWith("http://") || url.startsWith("https://")) { + return ( + url.startsWith(this.origin) || + url.replace(/^http/, "https").startsWith(this.origin) || + url.replace(/^https/, "http").startsWith(this.origin) + ); + } + + try { + const parsedUrl = new URL(url, this.origin); + if (parsedUrl.protocol !== "http:" && parsedUrl.protocol !== "https:") { + return false; + } + } catch { + return false; + } + + return true; }, /** @@ -388,8 +393,8 @@ const DiscourseURL = EmberObject.extend({ }, // This has been extracted so it can be tested. - origin() { - let prefix = getURL("/"); + get origin() { + const prefix = getURL("/"); return window.location.origin + (prefix === "/" ? "" : prefix); }, diff --git a/app/assets/javascripts/discourse/tests/unit/lib/click-track-test.js b/app/assets/javascripts/discourse/tests/unit/lib/click-track-test.js index 30c694ffe1d..c13f159064b 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/click-track-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/click-track-test.js @@ -81,7 +81,7 @@ module("Unit | Utility | click-track", function (hooks) { assert.true(false, "should not request a csrf token"); }); - sinon.stub(DiscourseURL, "origin").returns("http://discuss.domain.com"); + sinon.stub(DiscourseURL, "origin").get(() => "http://discuss.domain.com"); const done = assert.async(); pretender.post("/clicks/track", (request) => { @@ -101,7 +101,7 @@ module("Unit | Utility | click-track", function (hooks) { }); test("does not track attachments", async function (assert) { - sinon.stub(DiscourseURL, "origin").returns("http://discuss.domain.com"); + sinon.stub(DiscourseURL, "origin").get(() => "http://discuss.domain.com"); pretender.post("/clicks/track", () => assert.true(false)); diff --git a/app/assets/javascripts/discourse/tests/unit/lib/url-test.js b/app/assets/javascripts/discourse/tests/unit/lib/url-test.js index 5a6d98c4326..f47c6de8712 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/url-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/url-test.js @@ -14,54 +14,77 @@ module("Unit | Utility | url", function (hooks) { setupTest(hooks); test("isInternal with a HTTP url", function (assert) { - sinon.stub(DiscourseURL, "origin").returns("http://eviltrout.com"); + sinon.stub(DiscourseURL, "origin").get(() => "http://eviltrout.com"); - assert.notOk(DiscourseURL.isInternal(null), "a blank URL is not internal"); - assert.ok(DiscourseURL.isInternal("/test"), "relative URLs are internal"); - assert.ok( + assert.false(DiscourseURL.isInternal(null), "a blank URL is not internal"); + assert.true(DiscourseURL.isInternal("/test"), "relative URLs are internal"); + assert.true( + DiscourseURL.isInternal("docs"), + "non-prefixed relative URLs are internal" + ); + assert.true(DiscourseURL.isInternal("#foo"), "anchor URLs are internal"); + assert.true( DiscourseURL.isInternal("//eviltrout.com"), "a url on the same host is internal (protocol-less)" ); - assert.ok( + assert.true( DiscourseURL.isInternal("http://eviltrout.com/tophat"), "a url on the same host is internal" ); - assert.ok( + assert.true( DiscourseURL.isInternal("https://eviltrout.com/moustache"), "a url on a HTTPS of the same host is internal" ); - assert.notOk( + assert.false( DiscourseURL.isInternal("//twitter.com.com"), "a different host is not internal (protocol-less)" ); - assert.notOk( + assert.false( DiscourseURL.isInternal("http://twitter.com"), "a different host is not internal" ); + assert.false( + DiscourseURL.isInternal("ftp://eviltrout.com"), + "a different protocol is not internal" + ); + assert.false( + DiscourseURL.isInternal("ftp::/eviltrout.com"), + "an invalid URL is not internal" + ); }); test("isInternal with a HTTPS url", function (assert) { - sinon.stub(DiscourseURL, "origin").returns("https://eviltrout.com"); - assert.ok( + sinon.stub(DiscourseURL, "origin").get(() => "https://eviltrout.com"); + assert.true( DiscourseURL.isInternal("http://eviltrout.com/monocle"), "HTTPS urls match HTTP urls" ); + assert.true( + DiscourseURL.isInternal("https://eviltrout.com/monocle"), + "HTTPS urls match HTTPS urls" + ); }); test("isInternal on subfolder install", function (assert) { - sinon.stub(DiscourseURL, "origin").returns("http://eviltrout.com/forum"); - assert.notOk( + sinon.stub(DiscourseURL, "origin").get(() => "http://eviltrout.com/forum"); + assert.false( DiscourseURL.isInternal("http://eviltrout.com"), "the host root is not internal" ); - assert.notOk( + assert.false( DiscourseURL.isInternal("http://eviltrout.com/tophat"), "a url on the same host but on a different folder is not internal" ); - assert.ok( + assert.true( DiscourseURL.isInternal("http://eviltrout.com/forum/moustache"), "a url on the same host and on the same folder is internal" ); + assert.true(DiscourseURL.isInternal("/test"), "relative URLs are internal"); + assert.true( + DiscourseURL.isInternal("docs"), + "non-prefixed relative URLs are internal" + ); + assert.true(DiscourseURL.isInternal("#foo"), "anchor URLs are internal"); }); test("userPath", function (assert) {