DEV: Redo `DiscourseURL.isInternal()` (#26504)

Simplify the implementation and make it return `true` for prefix-less relative urls, like `docs`

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?.length) {
+      return false;
+    }
+
     if (url.startsWith("//")) {
       url = "http:" + url;
     }
-      if (url.startsWith("#")) {
+
-        return true;
+    if (url.startsWith("http://") || url.startsWith("https://")) {
-      }
+      return (
-      if (url.startsWith("/")) {
+        url.startsWith(this.origin) ||
-        return true;
+        url.replace(/^http/, "https").startsWith(this.origin) ||
-      }
+        url.replace(/^https/, "http").startsWith(this.origin)
-      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;
-      }
     }
+
+    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() {
+  get origin() {
-    let prefix = getURL("/");
+    const prefix = getURL("/");
     return window.location.origin + (prefix === "/" ? "" : prefix);
   },
 

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));
 

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.false(DiscourseURL.isInternal(null), "a blank URL is not internal");
-    assert.ok(DiscourseURL.isInternal("/test"), "relative URLs are internal");
+    assert.true(DiscourseURL.isInternal("/test"), "relative URLs are internal");
-    assert.ok(
+    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");
+    sinon.stub(DiscourseURL, "origin").get(() => "https://eviltrout.com");
-    assert.ok(
+    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");
+    sinon.stub(DiscourseURL, "origin").get(() => "http://eviltrout.com/forum");
-    assert.notOk(
+    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) {