#6236 Improve URL paths handling

extension/chrome/dev/ci_unit_test.ts

@@ -8,6 +8,7 @@ import { AttachmentUI } from '../../js/common/ui/attachment-ui.js';
 import { ApiErr } from '../../js/common/api/shared/api-error.js';
 import { Mime } from '../../js/common/core/mime.js';
 import { Attachment } from '../../js/common/core/attachment.js';
+import { Attester } from '../../js/common/api/key-server/attester.js';
 import { Wkd } from '../../js/common/api/key-server/wkd.js';
 import { MsgUtil } from '../../js/common/core/crypto/pgp/msg-util.js';
 import { Sks } from '../../js/common/api/key-server/sks.js';
@@ -32,6 +33,7 @@ const libs: unknown[] = [
   ApiErr,
   Attachment,
   AttachmentUI,
+  Attester,
   Buf,
   ExpirationCache,
   KeyUtil,

extension/chrome/settings/index.ts

@@ -163,8 +163,7 @@ View.run(
         this.setHandler(async () => {
           const prvs = await KeyStoreUtil.parse(await KeyStore.getRequired(this.acctEmail!));
           const mostUsefulPrv = KeyStoreUtil.chooseMostUseful(prvs, 'EVEN-IF-UNUSABLE');
-          const escapedFp = Xss.escape(mostUsefulPrv!.key.id);
-          await Settings.renderSubPage(this.acctEmail, this.tabId, 'modules/my_key.htm', `&fingerprint=${escapedFp}`);
+          await Settings.renderSubPage(this.acctEmail, this.tabId, 'modules/my_key.htm', { fingerprint: mostUsefulPrv!.key.id });
         })
       );
       $('.action_show_encrypted_inbox').on(
@@ -538,7 +537,7 @@ View.run(
         }
 
         const escapedEmail = Xss.escape(KeyUtil.getPrimaryEmail(prv) || '');
-        const escapedLink = `<a href="#" data-test="action-show-key-${originalIndex}" class="action_show_key" page="modules/my_key.htm" addurltext="&fingerprint=${ki.id}">${escapedEmail}</a>`;
+        const escapedLink = `<a href="#" data-test="action-show-key-${originalIndex}" class="action_show_key" page="modules/my_key.htm" addurltext="&fingerprint=${encodeURIComponent(ki.id)}">${escapedEmail}</a>`;
         const fpHtml = `<span class="good" style="font-family: monospace;">${Str.spaced(Xss.escape(ki.fingerprints[0]))}</span>`;
 
         const rowClass = isExpired ? 'key-content-row expired' : 'key-content-row';

extension/js/common/api/account-servers/external-service.ts

@@ -5,7 +5,7 @@ import { AuthenticationConfiguration } from '../../authentication-configuration.
 import { ClientConfigurationError, ClientConfigurationJson } from '../../client-configuration.js';
 import { Attachment } from '../../core/attachment.js';
 import { Buf } from '../../core/buf.js';
-import { Dict, Str } from '../../core/common.js';
+import { Dict, Str, Url } from '../../core/common.js';
 import { FLAVOR, SHARED_TENANT_API_HOST } from '../../core/const.js';
 import { ErrorReport } from '../../platform/error-report.js';
 import { Serializable } from '../../platform/store/abstract-store.js';
@@ -95,13 +95,18 @@ export class ExternalService extends Api {
 
   public fetchAndSaveClientConfiguration = async (): Promise<ClientConfigurationJson> => {
     const auth = await this.request<AuthenticationConfiguration>(
-      `/api/${this.apiVersion}/client-configuration/authentication?domain=${this.domain}`,
+      Url.create(`/api/${this.apiVersion}/client-configuration/authentication`, { domain: this.domain }),
       undefined,
       undefined,
       false
     );
     await AcctStore.set(this.acctEmail, { authentication: auth });
-    const r = await this.request<FesRes.ClientConfiguration>(`/api/${this.apiVersion}/client-configuration?domain=${this.domain}`, undefined, undefined, false);
+    const r = await this.request<FesRes.ClientConfiguration>(
+      Url.create(`/api/${this.apiVersion}/client-configuration`, { domain: this.domain }),
+      undefined,
+      undefined,
+      false
+    );
     if (r.clientConfiguration && !r.clientConfiguration.flags) {
       throw new ClientConfigurationError('missing_flags');
     }
@@ -173,7 +178,7 @@ export class ExternalService extends Api {
   };
 
   public messageGatewayUpdate = async (externalId: string, emailGatewayMessageId: string) => {
-    await this.request(`/api/${this.apiVersion}/message/${externalId}/gateway`, {
+    await this.request(`/api/${this.apiVersion}/message/${encodeURIComponent(externalId)}/gateway`, {
       fmt: 'JSON',
       data: {
         emailGatewayMessageId,
@@ -278,5 +283,5 @@ export class ExternalService extends Api {
       cc: process(recipients.cc),
       bcc: process(recipients.bcc),
     };
-  }
+  };
 }

extension/js/common/api/authentication/google/google-oauth.ts

@@ -41,7 +41,7 @@ export class GoogleOAuth extends OAuth {
   public static async getTokenInfo(accessToken: string): Promise<GoogleTokenInfo> {
     return await Api.ajax(
       {
-        url: `${OAUTH_GOOGLE_API_HOST}/tokeninfo?access_token=${accessToken}`,
+        url: `${OAUTH_GOOGLE_API_HOST}/tokeninfo?${new URLSearchParams([['access_token', accessToken]]).toString()}`,
         method: 'GET',
         timeout: 10000,
         stack: CatchHelper.stackTrace(),

extension/js/common/api/email-provider/gmail/gmail.ts

@@ -36,7 +36,12 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
 
   public threadGet = async (threadId: string, format?: GmailResponseFormat, progressCb?: ProgressCb, retryCount = 0): Promise<GmailRes.GmailThread> => {
     try {
-      return await Google.gmailCall<GmailRes.GmailThread>(this.acctEmail, `threads/${threadId}`, { method: 'GET', data: { format } }, { download: progressCb });
+      return await Google.gmailCall<GmailRes.GmailThread>(
+        this.acctEmail,
+        `threads/${encodeURIComponent(threadId)}`,
+        { method: 'GET', data: { format } },
+        { download: progressCb }
+      );
     } catch (e) {
       if (ApiErr.isRateLimit(e) && retryCount < MAX_RATE_LIMIT_ERROR_RETRY_COUNT) {
         await Time.sleep(1000);
@@ -60,7 +65,7 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
   };
 
   public threadModify = async (id: string, rmLabels: string[], addLabels: string[]): Promise<GmailRes.GmailThread> => {
-    return await Google.gmailCall<GmailRes.GmailThread>(this.acctEmail, `threads/${id}/modify`, {
+    return await Google.gmailCall<GmailRes.GmailThread>(this.acctEmail, `threads/${encodeURIComponent(id)}/modify`, {
       method: 'POST',
       data: {
         removeLabelIds: rmLabels || [], // todo - insufficient permission - need https://github.com/FlowCrypt/flowcrypt-browser/issues/1304
@@ -79,11 +84,11 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
   };
 
   public draftDelete = async (id: string): Promise<GmailRes.GmailDraftDelete> => {
-    return await Google.gmailCall<GmailRes.GmailDraftDelete>(this.acctEmail, 'drafts/' + id, { method: 'DELETE' });
+    return await Google.gmailCall<GmailRes.GmailDraftDelete>(this.acctEmail, `drafts/${encodeURIComponent(id)}`, { method: 'DELETE' });
   };
 
   public draftUpdate = async (id: string, mimeMsg: string, threadId: string): Promise<GmailRes.GmailDraftUpdate> => {
-    return await Google.gmailCall<GmailRes.GmailDraftUpdate>(this.acctEmail, `drafts/${id}`, {
+    return await Google.gmailCall<GmailRes.GmailDraftUpdate>(this.acctEmail, `drafts/${encodeURIComponent(id)}`, {
       method: 'PUT',
       data: {
         message: { raw: Buf.fromUtfStr(mimeMsg).toBase64UrlStr(), threadId },
@@ -92,7 +97,7 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
   };
 
   public draftGet = async (id: string, format: GmailResponseFormat = 'full'): Promise<GmailRes.GmailDraftGet> => {
-    return await Google.gmailCall<GmailRes.GmailDraftGet>(this.acctEmail, `drafts/${id}`, { method: 'GET', data: { format } });
+    return await Google.gmailCall<GmailRes.GmailDraftGet>(this.acctEmail, `drafts/${encodeURIComponent(id)}`, { method: 'GET', data: { format } });
   };
 
   public draftList = async (): Promise<GmailRes.GmailDraftList> => {
@@ -139,7 +144,7 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
     try {
       return await Google.gmailCall<GmailRes.GmailMsg>(
         this.acctEmail,
-        `messages/${msgId}`,
+        `messages/${encodeURIComponent(msgId)}`,
         { method: 'GET', data: { format: format || 'full' } },
         progressCb ? { download: progressCb } : undefined
       );
@@ -164,10 +169,13 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
   };
 
   public attachmentGet = async (attachment: Attachment, progress?: { download: ProgressCb }): Promise<GmailRes.GmailAttachment> => {
+    if (!attachment.msgId || !attachment.id) {
+      throw new Error('Attachment is missing Gmail message id or attachment id');
+    }
     type RawGmailAttRes = { attachmentId: string; size: number; data: string };
     const { attachmentId, size, data } = await Google.gmailCall<RawGmailAttRes>(
       this.acctEmail,
-      `messages/${attachment.msgId}/attachments/${attachment.id}`,
+      `messages/${encodeURIComponent(attachment.msgId)}/attachments/${encodeURIComponent(attachment.id)}`,
       { method: 'GET' },
       progress
     );
@@ -227,7 +235,7 @@ export class Gmail extends EmailProviderApi implements EmailProviderInterface {
       };
       GoogleOAuth.googleApiAuthHeader(this.acctEmail)
         .then(async authHeader => {
-          const url = `${GMAIL_GOOGLE_API_HOST}/gmail/v1/users/me/messages/${msgId}/attachments/${attachmentId}`;
+          const url = `${GMAIL_GOOGLE_API_HOST}/gmail/v1/users/me/messages/${encodeURIComponent(msgId)}/attachments/${encodeURIComponent(attachmentId)}`;
           const response: Response = await fetch(url, {
             method: 'GET',
             headers: authHeader,

extension/js/common/api/email-provider/gmail/google.ts

@@ -11,7 +11,7 @@ import { GoogleOAuth } from '../../authentication/google/google-oauth.js';
 import { CatchHelper } from '../../../platform/catch-helper.js';
 export class Google {
   public static webmailUrl = (acctEmail: string) => {
-    return `https://mail.google.com/mail/u/${acctEmail}`;
+    return `https://mail.google.com/mail/u/${encodeURIComponent(acctEmail)}`;
   };
 
   public static gmailCall = async <RT>(acctEmail: string, path: string, params?: AjaxParams, progress?: ProgressCbs): Promise<RT> => {

extension/js/common/api/flowcrypt-website.ts

@@ -11,7 +11,7 @@ namespace FlowCryptWebsiteRes {
 
 export class FlowCryptWebsite extends Api {
   public static pubKeyUrl = (resource: string) => {
-    return `https://flowcrypt.com/pub/${resource}`;
+    return `https://flowcrypt.com/pub/${encodeURIComponent(resource)}`;
   };
 
   public static retrieveBlogPosts = async (): Promise<FlowCryptWebsiteRes.FcBlogPost[]> => {

extension/js/common/api/key-server/attester.ts

@@ -42,7 +42,8 @@ export class Attester extends Api {
   public doLookupLdap = async (email: string, server?: string): Promise<PubkeysSearchResult> => {
     const ldapServer = server ?? `keys.${Str.getDomainFromEmailAddress(email)}`;
     try {
-      const r = await this.pubCall(`ldap-relay?server=${ldapServer}&search=${email}`);
+      const query = new URLSearchParams({ server: ldapServer, search: email });
+      const r = await this.pubCall(`ldap-relay?${query.toString()}`);
       return await this.getPubKeysSearchResult(r);
     } catch (e) {
       // treat error 500 as error 404 on this particular endpoint
@@ -73,7 +74,7 @@ export class Attester extends Api {
     if (!this.clientConfiguration.canSubmitPubToAttester()) {
       throw new Error('Cannot replace pubkey at attester because your organisation rules forbid it');
     }
-    await this.pubCall(`pub/${email}`, pubkey, { authorization: `Bearer ${idToken}` });
+    await this.pubCall(`pub/${encodeURIComponent(email)}`, pubkey, { authorization: `Bearer ${idToken}` });
   };
 
   /**
@@ -85,7 +86,7 @@ export class Attester extends Api {
     if (!this.clientConfiguration.canSubmitPubToAttester()) {
       throw new Error('Cannot replace pubkey at attester because your organisation rules forbid it');
     }
-    return await this.pubCall(`pub/${email}`, pubkey);
+    return await this.pubCall(`pub/${encodeURIComponent(email)}`, pubkey);
   };
 
   public welcomeMessage = async (email: string, pubkey: string, idToken: string | undefined): Promise<{ sent: boolean }> => {
@@ -109,7 +110,7 @@ export class Attester extends Api {
 
   private doLookup = async (email: string): Promise<PubkeysSearchResult> => {
     try {
-      const r = await this.pubCall(`pub/${email}`);
+      const r = await this.pubCall(`pub/${encodeURIComponent(email)}`);
       return await this.getPubKeysSearchResult(r);
     } catch (e) {
       if (ApiErr.isNotFound(e)) {

extension/js/common/browser/ui.ts

@@ -367,7 +367,7 @@ export class Ui {
   };
 
   public static getTestCompatibilityLink = (acctEmail: string): string => {
-    return `<a href="/chrome/settings/modules/compatibility.htm?acctEmail=${acctEmail}" target="_blank">Test your OpenPGP key compatibility</a>`;
+    return `<a href="${Url.create('/chrome/settings/modules/compatibility.htm', { acctEmail })}" target="_blank">Test your OpenPGP key compatibility</a>`;
   };
 
   public static getScreenDimensions = (): ScreenDimensions => {

extension/js/service_worker/bg-handlers.ts

@@ -182,6 +182,9 @@ export class BgHandlers {
   };
 
   public static thunderbirdOpenPassphraseDialog = async (r: Bm.ThunderbirdOpenPassphraseDialog): Promise<Bm.Res.ThunderbirdOpenPassphraseDialog> => {
-    await BgUtils.openExtensionTab(`chrome/elements/passphrase.htm?type=message&parentTabId=0&acctEmail=${r.acctEmail}&longids=${r.longids}`, true);
+    await BgUtils.openExtensionTab(
+      Url.create('chrome/elements/passphrase.htm', { type: 'message', parentTabId: '0', acctEmail: r.acctEmail, longids: r.longids }),
+      true
+    );
   };
 }

test/source/mock/attester/attester-endpoints.ts

@@ -24,7 +24,7 @@ export interface AttesterConfig {
 export const getMockAttesterEndpoints = (oauth: OauthMock, attesterConfig: AttesterConfig): HandlersDefinition => {
   return {
     '/attester/pub/?': async ({ body }, req) => {
-      const emailOrLongid = req.url.split('/').pop()!.toLowerCase().trim();
+      const emailOrLongid = decodeURIComponent(req.url.split('/').pop()!).toLowerCase().trim();
       if (isGet(req)) {
         if (!attesterConfig?.pubkeyLookup) {
           throw new HttpClientErr('Method not allowed', 405);

test/source/mock/fes/customer-url-fes-endpoints.ts

@@ -106,7 +106,7 @@ export const getMockCustomerUrlFesEndpoints = (config: FesConfig | undefined): H
       const port = parsePort(req);
       const gatewayMatch = /\/api\/v1\/messages\/([^/]+)\/gateway/.exec(req.url);
       if (gatewayMatch && parseAuthority(req) === standardFesUrl(port) && req.method === 'POST') {
-        const externalId = gatewayMatch[1];
+        const externalId = decodeURIComponent(gatewayMatch[1]);
         if (externalId === 'FES-MOCK-EXTERNAL-FOR-GATEWAYFAILURE@EXAMPLE.COM-ID') {
           throw new HttpClientErr(`Test error`, Status.BAD_REQUEST);
         }
@@ -141,7 +141,7 @@ export const getMockCustomerUrlFesEndpoints = (config: FesConfig | undefined): H
       const port = parsePort(req);
       const gatewayMatch = /\/api\/v1\/message\/([^/]+)\/gateway/.exec(req.url);
       if (gatewayMatch && parseAuthority(req) === standardFesUrl(port) && req.method === 'POST') {
-        const externalId = gatewayMatch[1];
+        const externalId = decodeURIComponent(gatewayMatch[1]);
         if (externalId === 'FES-MOCK-EXTERNAL-FOR-GATEWAYFAILURE@EXAMPLE.COM-ID') {
           throw new HttpClientErr(`Test error`, Status.BAD_REQUEST);
         }

test/source/mock/fes/shared-tenant-fes-endpoints.ts

@@ -214,7 +214,7 @@ export const getMockSharedTenantFesEndpoints = (config: FesConfig | undefined):
     '/shared-tenant-fes/api/v1/messages/?': async ({ body }, req) => {
       const gatewayMatch = /\/shared-tenant-fes\/api\/v1\/messages\/([^/]+)\/gateway/.exec(req.url);
       if (gatewayMatch && req.method === 'POST') {
-        const externalId = gatewayMatch[1];
+        const externalId = decodeURIComponent(gatewayMatch[1]);
         if (externalId === 'FES-MOCK-EXTERNAL-FOR-GATEWAYFAILURE@EXAMPLE.COM-ID') {
           throw new HttpClientErr(`Test error`, Status.BAD_REQUEST);
         }
@@ -253,7 +253,7 @@ export const getMockSharedTenantFesEndpoints = (config: FesConfig | undefined):
     '/shared-tenant-fes/api/v1/message/?': async ({ body }, req) => {
       const gatewayMatch = /\/shared-tenant-fes\/api\/v1\/message\/([^/]+)\/gateway/.exec(req.url);
       if (gatewayMatch && req.method === 'POST') {
-        const externalId = gatewayMatch[1];
+        const externalId = decodeURIComponent(gatewayMatch[1]);
         if (externalId === 'FES-MOCK-EXTERNAL-FOR-GATEWAYFAILURE@EXAMPLE.COM-ID') {
           throw new HttpClientErr(`Test error`, Status.BAD_REQUEST);
         }

test/source/tests/browser-unit-tests/unit-Attester.js

@@ -0,0 +1,35 @@
+/* ©️ 2016 - present FlowCrypt a.s. Limitations apply. Contact human@flowcrypt.com */
+
+/**
+ * These tests use JavaScript instead of TypeScript to avoid dealing with types in cross-environment setup.
+ * (tests are injected from NodeJS through puppeteer into a browser environment)
+ * While this makes them less convenient to write, the result is more flexible.
+ *
+ * Import your lib to `ci_unit_test.ts` to resolve `ReferenceError: SomeClass is not defined`
+ *
+ * Each test must return "pass" to pass. To reject, throw an Error.
+ *
+ * Each test must start with one of (depending on which flavors you want it to run):
+ *  - BROWSER_UNIT_TEST_NAME(`some test name`);
+ *  - BROWSER_UNIT_TEST_NAME(`some test name`).enterprise;
+ *  - BROWSER_UNIT_TEST_NAME(`some test name`).consumer;
+ *
+ * This is not a JavaScript file. It's a text file that gets parsed, split into chunks, and
+ *    parts of it executed as javascript. The structure is very rigid. The only flexible place is inside
+ *    the async functions. For the rest, do not change the structure or our parser will get confused.
+ *    Do not put any code whatsoever outside of the async functions.
+ */
+
+BROWSER_UNIT_TEST_NAME(`Attester lookup encodes email path segment`);
+(async () => {
+  const maliciousEmail = '%2e%2e/ldap-relay?server=attacker.example&search=x@example.com@flowcrypt.test';
+  const attester = new Attester({
+    canLookupThisRecipientOnAttester: () => true,
+    canSubmitPubToAttester: () => true,
+  });
+  const { pubkeys } = await attester.lookupEmail(maliciousEmail);
+  if (pubkeys.length) {
+    throw Error(`Expected no pubkey for encoded path traversal email, got ${pubkeys.length}`);
+  }
+  return 'pass';
+})();

test/source/tests/unit-browser.ts

@@ -59,6 +59,15 @@ export const defineUnitBrowserTests = (testVariant: TestVariant, testWithBrowser
                   returnError: new HttpClientErr('Pubkey not found', Status.NOT_FOUND),
                 },
               },
+              attester: {
+                ldapRelay: {
+                  'x@example.com@flowcrypt.test': {
+                    pubkey: somePubkey,
+                    domainToCheck: 'attacker.example',
+                  },
+                },
+                pubkeyLookup: {},
+              },
             });
           }
           /* The "test" parameter is utilized by the unit test named "Catcher does not include query string on report." */

tsconfig.json

@@ -14,6 +14,7 @@
     "strict": true,
     "useUnknownInCatchVariables": false,
     "outDir": "build/generic-extension-wip",
+    "rootDir": "./extension",
     "moduleResolution": "bundler",
     "skipLibCheck": true,
     "paths": {