timeout query with a serv-fail answer or a 4xx

Servfail answers are constructed as approp on errors with query-id & questions,
where possible. If not, the clients are sent a 4xx response instead of a 5xx:
Chromium-based and Firefox-based browser do not retry 5xx doh answers.

Response timeout timers were not closed once a dns query had been responded to
within specified timeout, but that is now dealt by a "deferred promise" which
resolves either on a timeout or on a valid response.

src/commons/dnsutil.js

@@ -44,6 +44,17 @@ export function servfail(qid, qs) {
   });
 }
 
+export function servfailQ(q) {
+  if (bufutil.emptyBuf(q)) return null;
+
+  try {
+    const p = decode(q);
+    return servfail(p.id, p.questions);
+  } catch (e) {
+    return null;
+  }
+}
+
 export function requestTimeout() {
   const t = envutil.workersTimeout();
   return t > _minRequestTimeout

src/commons/util.js

@@ -90,6 +90,7 @@ export function copyHeaders(request) {
   });
   return headers;
 }
+
 export function copyNonPseudoHeaders(req) {
   const headers = {};
   if (!req || !req.headers) return headers;
@@ -121,41 +122,59 @@ export function objOf(map) {
   return map.entries ? Object.fromEntries(map) : {};
 }
 
-export function timedOp(op, ms, cleanup) {
-  let tid = null;
-  let resolve = null;
-  let reject = null;
-  const promiser = (accept, deny) => {
-    resolve = accept;
-    reject = deny;
-  };
-  const p = new Promise(promiser);
+export function timedOp(op, ms, cleanup = () => {}) {
+  return new Promise((resolve, reject) => {
+    let timedout = false;
+    const tid = timeout(ms, () => {
+      timedout = true;
+      reject(new Error("timeout"));
+    });
 
-  let timedout = false;
-  tid = timeout(ms, () => {
-    timedout = true;
-    reject("timeout");
+    try {
+      op((out, ex) => {
+        if (timedout) {
+          cleanup(out);
+          return;
+        }
+
+        clearTimeout(tid);
+
+        if (ex) {
+          reject(ex);
+        } else {
+          resolve(out);
+        }
+      });
+    } catch (e) {
+      if (!timedout) reject(e);
+    }
   });
+}
 
-  try {
-    op((out, ex) => {
-      if (timedout) {
-        cleanup(out);
-        return;
-      }
-
-      clearTimeout(tid);
-
-      if (ex) {
-        reject(ex.message);
-      } else {
-        resolve(out);
-      }
+export function timedSafeAsyncOp(promisedOp, ms, defaultOut) {
+  // aggregating promises is a valid use-case for the otherwise
+  // "deferred promise anti-pattern". That is, using promise
+  // constructs (async, await, catch, then etc) within a
+  // "new Promise()" is an anti-pattern and hard to get right:
+  // stackoverflow.com/a/23803744 and stackoverflow.com/a/25569299
+  return new Promise((resolve, _) => {
+    let timedout = false;
+    const tid = timeout(ms, () => {
+      timedout = true;
+      resolve(defaultOut);
     });
-  } catch (ex) {
-    if (!timedout) reject(ex.message);
-  }
-  return p;
+
+    promisedOp()
+      .then((out) => {
+        if (!timedout) {
+          clearTimeout(tid);
+          resolve(out);
+        }
+      })
+      .catch((ignored) => {
+        if (!timedout) resolve(defaultOut);
+      });
+  });
 }
 
 export function timeout(ms, callback) {
@@ -293,6 +312,12 @@ export function respond405() {
   });
 }
 
+export function respond408() {
+  return new Response(null, {
+    status: 408, // timeout
+  });
+}
+
 export function respond503() {
   return new Response(null, {
     status: 503, // unavailable

src/core/doh.js

@@ -12,50 +12,41 @@ import * as util from "../commons/util.js";
 import * as dnsutil from "../commons/dnsutil.js";
 
 export function handleRequest(event) {
-  return Promise.race([
-    new Promise((accept, _) => {
-      accept(proxyRequest(event));
-    }),
-
-    // TODO: cancel timeout once proxyRequest is complete
-    // util.timedOp is one way to do so, but it results in a reject
-    // on timeouts which manifests as "exception" to upstream (server-
-    // -worker/deno/node) that then needs to handle it as approp.
-    new Promise((accept, _) => {
-      // on timeout, servfail
-      util.timeout(dnsutil.requestTimeout(), () => {
-        log.e("doh", "handle-request timeout");
-        accept(servfail());
-      });
-    }),
-  ]);
+  return util.timedSafeAsyncOp(
+    async () => proxyRequest(event),
+    dnsutil.requestTimeout(),
+    servfail()
+  );
 }
 
 async function proxyRequest(event) {
-  try {
-    if (optionsRequest(event.request)) return util.respond204();
+  if (optionsRequest(event.request)) return util.respond204();
+
+  const cr = new CurrentRequest();
 
-    const currentRequest = new CurrentRequest();
+  try {
     const plugin = new RethinkPlugin(event);
-    await plugin.executePlugin(currentRequest);
+    await plugin.executePlugin(cr);
 
+    // TODO: cors-headers are also set in server-node.js
+    // centralize setting these in just one place, if possible
     const ua = event.request.headers.get("User-Agent");
     if (util.fromBrowser(ua)) currentRequest.setCorsHeadersIfNeeded();
 
-    return currentRequest.httpResponse;
+    return cr.httpResponse;
   } catch (err) {
     log.e("doh", "proxy-request error", err);
-    return errorOrServfail(event.request, err);
+    return errorOrServfail(event.request, err, cr);
   }
 }
 
 function optionsRequest(request) {
   return request.method === "OPTIONS";
 }
 
-function errorOrServfail(request, err) {
+function errorOrServfail(request, err, currentRequest) {
   const ua = request.headers.get("User-Agent");
-  if (!util.fromBrowser(ua)) return servfail();
+  if (!util.fromBrowser(ua)) return servfail(currentRequest);
 
   const res = new Response(JSON.stringify(err.stack), {
     status: 503, // unavailable
@@ -64,6 +55,15 @@ function errorOrServfail(request, err) {
   return res;
 }
 
-function servfail() {
-  return util.respond503();
+function servfail(currentRequest) {
+  if (
+    util.emptyObj(currentRequest) ||
+    util.emptyObj(currentRequest.decodedDnsPacket)
+  ) {
+    return util.respond408();
+  }
+
+  const qid = currentRequest.decodedDnsPacket.id;
+  const qs = currentRequest.decodedDnsPacket.questions;
+  return dnsutil.servfail(qid, qs);
 }

src/core/plugin.js

@@ -23,7 +23,6 @@ export default class RethinkPlugin {
     this.parameter = new Map(envManager.getMap());
 
     const rxid = util.rxidFromHeader(event.request.headers) || util.xid();
-    // TODO: generate rxid in setRequest instead?
     this.registerParameter("rxid", "[rx." + rxid + "]");
 
     // caution: event isn't an event on nodejs, but event.request is a request

src/plugins/blocklist-wrapper/main.js

@@ -50,7 +50,7 @@ class BlocklistWrapper {
         return await this.initBlocklistConstruction(
           param.rxid,
           now,
-          envutil.dohResolver(),
+          envutil.blocklistUrl(),
           envutil.timestamp(),
           envutil.tdNodeCount(),
           envutil.tdParts()
@@ -212,7 +212,7 @@ async function fileFetch(url, typ) {
     throw new Error("Unknown conversion type at fileFetch");
   }
 
-  log.i("downloading", url);
+  log.i("downloading", url, typ);
   const res = await fetch(url, { cf: { cacheTtl: /* 2w */ 1209600 } });
 
   if (!res.ok) {
@@ -252,14 +252,12 @@ async function makeTd(baseurl, n) {
 
   log.i("tds downloaded");
 
-  return new Promise((resolve, reject) => {
-    try {
-      resolve(bufutil.concat(tds));
-    } catch (e) {
-      log.e("reject make-td", e);
-      reject(e.message);
-    }
-  });
+  try {
+    return bufutil.concat(tds);
+  } catch (e) {
+    log.e("reject make-td", e);
+    throw e;
+  }
 }
 
 export { BlocklistFilter, BlocklistWrapper };

src/server-deno.ts

@@ -165,9 +165,7 @@ async function handleTCPQuery(q: Uint8Array, conn: Deno.Conn) {
 }
 
 async function resolveQuery(q: Uint8Array) {
-  // Request Handler currently expects a FetchEvent containing request
-  // Using POST, as GET requests are capped at 2KB, where-as DNS-over-TCP
-  // has a much higher ceiling (even if rarely used)
+  // TODO: Sync code with server-node.js:resolveQuery
   const r: Response = (await handleRequest({
     request: new Request("https://example.com", {
       method: "POST",

src/server-node.js

@@ -349,14 +349,24 @@ async function handleTCPQuery(q, socket, host, flag) {
   const t = log.startTime("handle-tcp-query-" + rxid);
   try {
     const r = await resolveQuery(rxid, q, host, flag);
-    const rlBuf = bufutil.encodeUint8ArrayBE(r.byteLength, 2);
-    const chunk = new Uint8Array([...rlBuf, ...r]);
-
-    // writing to a destroyed socket crashes nodejs
-    if (!socket.destroyed) socket.write(chunk);
+    if (bufutil.emptyBuf(r)) {
+      log.w("empty ans from resolve");
+      ok = false;
+    } else {
+      const rlBuf = bufutil.encodeUint8ArrayBE(r.byteLength, 2);
+      const chunk = new Uint8Array([...rlBuf, ...r]);
+
+      // writing to a destroyed socket crashes nodejs
+      if (!socket.destroyed) {
+        socket.write(chunk);
+      } else {
+        ok = false;
+        log.w("send fail, tcp socket destroyed");
+      }
+    }
   } catch (e) {
     ok = false;
-    log.w(e);
+    log.w("send fail, err", e);
   }
   log.endTime(t);
 
@@ -374,8 +384,8 @@ async function handleTCPQuery(q, socket, host, flag) {
  * @return {Promise<Uint8Array>}
  */
 async function resolveQuery(rxid, q, host, flag) {
-  // Using POST, as GET requests are capped at 2KB, where-as DNS-over-TCP
-  // has a much higher ceiling (even if rarely used)
+  // Using POST, since GET requests cannot be greater than 2KB,
+  // where-as DNS-over-TCP msgs could be upto 64KB in size.
   const r = await handleRequest({
     request: new Request(`https://${host}/${flag}`, {
       method: "POST",
@@ -388,7 +398,12 @@ async function resolveQuery(rxid, q, host, flag) {
     }),
   });
 
-  return new Uint8Array(await r.arrayBuffer());
+  const ans = await r.arrayBuffer();
+  if (!bufutil.emptyBuf(ans)) {
+    return new Uint8Array(ans);
+  } else {
+    return dnsutil.servfailQ(q);
+  }
 }
 
 /**
@@ -455,12 +470,15 @@ async function handleHTTPRequest(b, req, res) {
 
     log.lapTime(t, "send-head");
 
-    const ans = Buffer.from(await fRes.arrayBuffer());
+    const ans = await fRes.arrayBuffer();
 
     log.lapTime(t, "recv-ans");
 
-    res.end(ans);
+    if (!bufutil.emptyBuf(ans)) {
+      res.end(bufutil.bufferOf(ans));
+    } // else: expect fRes.status to be set to non 2xx above
   } catch (e) {
+    res.writeHead(400); // bad request
     res.end();
     log.w(e);
   }