url: Consistently parse URLs for isUrlOnRealm (CVE-2022-35962)

The ad-hoc parsing we'd been doing here had the consequence that it
could return true for some URL strings that in reality resolve to
URLs that aren't on the Zulip realm.

If another (authenticated) user sends a message containing an image
link crafted to trigger this bug, and the receiving user taps on the
image in order to expand it in the lightbox, this could cause the
user's login credentials to be disclosed.  This is CVE-2022-35962.

To fix the issue, use `new URL` to parse the URL, the same way we're
going to do at each of its two call sites.

This still isn't a great structure, because it'd be easy for a call
site to change (or a new one to be added) so that the URL gets used
in a way other than by passing to our `new URL`, or with a different
base URL.  Add a TODO to refactor accordingly, and mark the function
as deprecated to discourage new callers.

Author: chrisbobbe

src/utils/__tests__/url-test.js

@@ -118,16 +118,22 @@ describe('isUrlOnRealm', () => {
 
   test('when link is on realm, return true', () => {
     expect(isUrlOnRealm('/#narrow/stream/jest', realm)).toBe(true);
-
     expect(isUrlOnRealm('https://example.com/#narrow/stream/jest', realm)).toBe(true);
-
     expect(isUrlOnRealm('#narrow/#near/1', realm)).toBe(true);
-  });
 
-  test('when link is on not realm, return false', () => {
-    expect(isUrlOnRealm('https://demo.example.com', realm)).toBe(false);
+    // Absolutizes to https://example.com/www.google.com
+    expect(isUrlOnRealm('www.google.com', realm)).toBeTrue();
+
+    expect(isUrlOnRealm('https://example.com', realm)).toBeTrue();
+    expect(isUrlOnRealm('https://example.com/', realm)).toBeTrue();
+    expect(isUrlOnRealm('https://example.com/foo/bar.baz', realm)).toBeTrue();
+  });
 
-    expect(isUrlOnRealm('www.google.com', realm)).toBe(false);
+  test('when link is not on realm, return false', () => {
+    expect(isUrlOnRealm('https://demo.example.com/', realm)).toBeFalse();
+    expect(isUrlOnRealm('https://demo.example/', realm)).toBeFalse();
+    expect(isUrlOnRealm('//demo.example/', realm)).toBeFalse();
+    expect(isUrlOnRealm(' https://demo.example/', realm)).toBeFalse();
   });
 });
 

src/utils/url.js

@@ -91,10 +91,26 @@ export const tryParseUrl = (url: string, base?: string | URL): URL | void => {
   }
 };
 
-// TODO: Work out what this does, write a jsdoc for its interface, and
-// reimplement using URL object (not just for the realm)
-export const isUrlOnRealm = (url: string, realm: URL): boolean =>
-  url.startsWith('/') || url.startsWith(realm.toString()) || !/^(http|www.)/i.test(url);
+/**
+ * Test if the given URL string resolves on the realm, with realm as base.
+ *
+ * DEPRECATED because URL strings are complicated.  Callers should construct
+ *   a URL object before this point, and use the same URL object both for
+ *   asking any questions like this one and for subsequently making any
+ *   network requests.
+ *
+ * This returns true just if `new URL(url, realm)` gives a URL that is
+ * within the same Zulip realm.
+ *
+ * This performs a call to `new URL` and therefore may take a fraction of a
+ * millisecond.  Avoid using in a context where it might be called more than
+ * 10 or 100 times per user action.
+ */
+// TODO: Take a URL object instead of a string, and remove warnings in jsdoc.
+export const isUrlOnRealm = (url: string, realm: URL): boolean => {
+  const parsedUrl = tryParseUrl(url, realm);
+  return parsedUrl ? parsedUrl.origin === realm.origin : false;
+};
 
 const getResourceWithAuth = (uri: string, auth: Auth) => ({
   uri: new URL(uri, auth.realm).toString(),