Skip to content

Commit

Permalink
url: Make getFullUrl use URL constructor.
Browse files Browse the repository at this point in the history
This function relied on some hacky heuristics to give its output,
and if something was wrong with its input, the effect wasn't very
clear. Now,

- no more hacky heuristics -- the URL constructor (as polyfilled by
  `react-native-url-polyfill`) will apply appropriate rules to give
  the correct output, given an acceptable input, and
- if something is wrong with the input, we get a clear "INVALID_URL"
  error.

Note that this function will now throw errors in cases where it
didn't before. This is better than giving a (maybe subtly) wrong
output, but it does suggest we should do some stress-testing with
real data to be sure we weren't somehow taking advantage of the
implementation mistakes that were there before.

So, do that here. On CZO, I didn't encounter any INVALID_URL errors,
even in the following likely-looking places:

- Copying an in-message link to the clipboard -- both a relative
  upload URL and an absolute non-upload URL (Jitsi)
- Avatar URLs in several places
- Realm icon on the AuthScreen on CZO
- Both links on LegalScreen

The new implementation of `getFullUrl` is so simple that it'll be
more transparent to inline it at all call sites, which we'll do in
the next commit.
  • Loading branch information
chrisbobbe committed Sep 16, 2020
1 parent ceedce9 commit ee2764b
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/utils/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ export const tryParseUrl = (url: string, base?: string | URL): URL | void => {
* @param url an absolute URL, or a relative URL on the realm.
* @param realm the URL of the current realm.
*/
// Uses some crude heuristics. TODO: Revisit this after or during #4081.
// We'll be inlining this at all call sites very soon.
export const getFullUrl = (url: string = '', realm: string): string =>
!url.startsWith('http') ? `${realm}${url.startsWith('/') ? '' : '/'}${url}` : url;
new URL(url, realm).toString();

export const isUrlOnRealm = (url: string = '', realm: string): boolean =>
url.startsWith('/') || url.startsWith(realm) || !/^(http|www.)/i.test(url);
Expand Down

0 comments on commit ee2764b

Please sign in to comment.