diff --git a/src/utils/__tests__/avatar-test.js b/src/utils/__tests__/avatar-test.js index 18da866abba..482d72dbed7 100644 --- a/src/utils/__tests__/avatar-test.js +++ b/src/utils/__tests__/avatar-test.js @@ -71,12 +71,22 @@ describe('GravatarURL', () => { }); }); - test('uses hash from server, if provided', () => { + test('uses URL from server, if provided', () => { const email = 'user13313@chat.zulip.org'; - const hash = md5('cbobbe@zulip.com'); - const instance = GravatarURL.validateAndConstructInstance({ email, hash }); + const urlFromServer = + 'https://secure.gravatar.com/avatar/de6685f1d3eb74439c1dcda84f92543e?d=identicon&version=1'; + const instance = GravatarURL.validateAndConstructInstance({ + email, + urlFromServer, + }); SIZES_TO_TEST.forEach(size => { - expect(instance.get(size).toString()).toContain(hash); + const clonedUrlOfSize = new URL(instance.get(size).toString()); + const clonedUrlFromServer = new URL(urlFromServer); + clonedUrlFromServer.searchParams.delete('s'); + clonedUrlOfSize.searchParams.delete('s'); + // `urlFromServer` should equal the result for this size, modulo + // the actual size param. + expect(clonedUrlOfSize.toString()).toEqual(clonedUrlFromServer.toString()); }); }); diff --git a/src/utils/avatar.js b/src/utils/avatar.js index f8d5e7fe4d1..0cb184f98aa 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -3,7 +3,6 @@ /* eslint-disable no-use-before-define */ import md5 from 'blueimp-md5'; -import { tryParseUrl } from './url'; import * as logging from './logging'; import { ensureUnreachable } from '../types'; @@ -53,14 +52,17 @@ export class AvatarURL { // which to generate the correct hash; see // https://github.com/zulip/zulip/issues/15287. Implemented at // `do_events_register` in zerver/lib/events.py on the server.) - if (tryParseUrl(rawAvatarUrl)?.origin === GravatarURL.ORIGIN) { - const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname); - if (hashMatch === null) { - const msg = 'Unexpected Gravatar URL shape from server.'; - logging.error(msg, { value: rawAvatarUrl }); - throw new Error(msg); - } - return GravatarURL.validateAndConstructInstance({ email, hash: hashMatch[0] }); + if ( + rawAvatarUrl.startsWith( + // Best not to use an expensive `new URL` call, when the + // following is equivalent (assuming `rawAvatarUrl` is also + // valid through its /pathname, ?query=params, and + // #fragment). The trailing slash ensures that we've + // examined the full origin in `rawAvatarUrl`. + `${GravatarURL.ORIGIN}/`, + ) + ) { + return GravatarURL.validateAndConstructInstance({ email, urlFromServer: rawAvatarUrl }); } // Otherwise, it's a realm-uploaded avatar, either absolute or @@ -109,16 +111,46 @@ export class GravatarURL extends AvatarURL { /** * Construct from raw server data, or throw an error. * - * Pass the hash if the server provides it, to avoid computing it - * unnecessarily. + * Pass the Gravatar URL string from the server, if we've got it, to + * avoid doing an expensive `new URL` call. */ - static validateAndConstructInstance(args: {| email: string, hash?: string |}): GravatarURL { - const { email, hash = md5(email.toLowerCase()) } = args; - - const standardSizeUrl = new URL(`/avatar/${hash}`, GravatarURL.ORIGIN); - standardSizeUrl.searchParams.set('d', 'identicon'); + // We should avoid doing unnecessary `new URL` calls here. They are + // very expensive, and their use in these pseudo-constructors (which + // process data at the edge, just as it's received from the server) + // used to slow down `api.registerForEvents` quite a lot. + // + // In the past, we've been more conservative about validating a URL + // string that comes to us from the server at this point: we'd parse + // it with the URL constructor, grab the Gravatar hash from the + // result, and construct another URL with that hash and exactly the + // things we wanted it to have (like `d=identicon`). + // + // With some loss of validation, we've removed those two `new URL` + // calls in the path where the server provides a Gravatar URL + // string. Still, we should be able to trust that the server gives + // us properly formatted URLs; if it didn't, it seems like the kind + // of bug that would be fixed quickly. + static validateAndConstructInstance(args: {| + email: string, + urlFromServer?: string, + |}): GravatarURL { + const { email, urlFromServer } = args; - return new GravatarURL(standardSizeUrl); + if (urlFromServer !== undefined) { + // This may not be *quite* the URL we would have generated + // ourselves. In the wild, I've seen one with a `version=1`, for + // example. But we trust the server to give us one that works, + // anyway -- and perhaps any extra things we get will be a good + // bonus. + return new GravatarURL(urlFromServer); + } else { + return new GravatarURL( + // Thankfully, this string concatenation is quite safe: we + // know enough about our inputs here to compose a properly + // formatted URL with them, without using `new URL`. + `${GravatarURL.ORIGIN}/avatar/${md5(email.toLowerCase())}?d=identicon`, + ); + } } static ORIGIN = 'https://secure.gravatar.com'; @@ -195,18 +227,29 @@ export class FallbackAvatarURL extends AvatarURL { } /** - * Construct from raw server data, or throw an error. + * Construct from raw server data (the user ID), or throw an error. + * + * The `realm` must be already validated, e.g., by coming from the + * Redux state. */ + // We should avoid doing unnecessary `new URL` calls here. They are + // very expensive, and their use in these pseudo-constructors (which + // process data at the edge, just as it's received from the server) + // used to slow down `api.registerForEvents` quite a lot. static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL { const { realm, userId } = args; - return new FallbackAvatarURL(new URL(`/avatar/${userId.toString()}`, realm)); + // Thankfully, this string concatenation is quite safe: we know + // enough about our inputs here to compose a properly formatted + // URL with them, without using `new URL`. (In particular, + // `realm.origin` doesn't have a trailing slash.) + return new FallbackAvatarURL(`${realm.origin}/avatar/${userId.toString()}`); } /** * Standard URL from which to generate others. PRIVATE. * - * May be a string if the instance was constructed at rehydrate - * time, when URL validation is unnecessary. + * May start out as a string, and will be converted to a URL object + * in the first `.get()` call. */ _standardUrl: string | URL; @@ -214,13 +257,6 @@ export class FallbackAvatarURL extends AvatarURL { * PRIVATE: Make an instance from already-validated data. * * Not part of the public interface; use the static methods instead. - * - * It's private because we need a path to constructing an instance - * without constructing URL objects, which takes more time than is - * acceptable when we can avoid it, e.g., during rehydration. - * Constructing URL objects is a necessary part of validating data - * from the server, but we only need to validate the data once, when - * it's first received. */ constructor(standardUrl: string | URL) { super(); @@ -238,7 +274,7 @@ export class FallbackAvatarURL extends AvatarURL { */ get(sizePhysicalPx: number): URL { // `this._standardUrl` may have begun its life as a string, to - // avoid computing a URL object during rehydration + // avoid expensively calling the URL constructor if (typeof this._standardUrl === 'string') { this._standardUrl = new URL(this._standardUrl); }