fix(mitm): don’t let dns errors go unhandled

Author: blakebyrnes

mitm/handlers/RequestSession.ts

@@ -222,7 +222,7 @@ export default class RequestSession extends TypedEventEmitter<IRequestSessionEve
       try {
         return await this.dns.lookupIp(host);
       } catch (error) {
-        log.error('DnsLookup.Error', {
+        log.info('DnsLookup.Error', {
           sessionId: this.sessionId,
           error,
         });

mitm/lib/Dns.ts

@@ -54,9 +54,9 @@ export class Dns {
   }
 
   private async systemLookup(host: string): Promise<IDnsEntry> {
+    const dnsEntry = createPromise<IDnsEntry>();
+    Dns.dnsEntries.set(host, dnsEntry);
     try {
-      const dnsEntry = createPromise<IDnsEntry>();
-      Dns.dnsEntries.set(host, dnsEntry);
       const lookupAddresses = await dns.lookup(host.split(':').shift(), {
         all: true,
         family: 4,
@@ -68,22 +68,20 @@ export class Dns {
         })),
       };
       dnsEntry.resolve(entry);
-      return entry;
     } catch (error) {
-      Dns.dnsEntries.get(host)?.reject(error);
+      dnsEntry.reject(error);
       Dns.dnsEntries.delete(host);
-      throw error;
     }
+    return dnsEntry.promise;
   }
 
   private async lookupDnsEntry(host: string): Promise<IDnsEntry> {
     const existing = Dns.dnsEntries.get(host);
     if (existing && !existing.isResolved) return existing.promise;
 
+    const dnsEntry = createPromise<IDnsEntry>();
+    Dns.dnsEntries.set(host, dnsEntry);
     try {
-      const dnsEntry = createPromise<IDnsEntry>();
-      Dns.dnsEntries.set(host, dnsEntry);
-
       if (!this.socket) {
         this.socket = new DnsOverTlsSocket(
           this.dnsServer,
@@ -103,12 +101,11 @@ export class Dns {
           })),
       };
       dnsEntry.resolve(entry);
-      return entry;
     } catch (error) {
-      Dns.dnsEntries.get(host)?.reject(error);
+      dnsEntry.reject(error);
       Dns.dnsEntries.delete(host);
-      throw error;
     }
+    return dnsEntry.promise;
   }
 
   private nextIp(dnsEntry: IDnsEntry): string {

mitm/test/dns.test.ts

@@ -115,3 +115,30 @@ test('should lookup in the local machine if not found in DoT', async () => {
   expect(lookupSpy).toHaveBeenCalledTimes(1);
   expect(systemLookupSpy).toHaveBeenCalledTimes(1);
 });
+
+test('should properly expose errors if nothing is found', async () => {
+  const lookupSpy = jest.spyOn(nodeDns, 'lookup').mockClear();
+  const dotLookup = jest
+    .spyOn<any, any>(dns, 'lookupDnsEntry')
+    .mockClear()
+    .mockImplementationOnce(() => {
+      throw new Error('Not found');
+    });
+  const systemLookupSpy = jest.spyOn<any, any>(dns, 'systemLookup').mockClear();
+
+  let unhandledErrorCalled = false;
+  const handler = () => {
+    unhandledErrorCalled = true;
+  };
+  process.once('unhandledRejection', handler);
+
+  await expect(dns.lookupIp('not-real-123423423443433434343-fake-domain.com')).rejects.toThrowError(
+    'Not found',
+  );
+  await new Promise(resolve => setTimeout(resolve, 100));
+  expect(unhandledErrorCalled).toBe(false);
+  expect(dotLookup).toHaveBeenCalledTimes(1);
+  expect(lookupSpy).toHaveBeenCalledTimes(1);
+  expect(systemLookupSpy).toHaveBeenCalledTimes(1);
+  process.off('unhandledRejection', handler);
+});