Discovery resends its search and takes only IPv4; the idle limit spares a connecting socket; descriptions compare as routers store them (2.0.4)

README.md

@@ -45,6 +45,8 @@ await client.removeMapping({ public: 8080 });
 
 `getGateway()` discovers the UPnP gateway via SSDP and returns an `UpnpInfo` object. The SSDP socket is created for discovery and automatically closed when done — no cleanup needed.
 
+The search goes out again 250, 750, 1750 and 3750 ms after the first until a gateway answers, since UDP loses datagrams and some routers drop searches for a second or more at a time; it stops at the first answer, or when `timeout` (5 s by default) runs out. Only an answer whose location is an IPv4 address is taken: some routers answer twice, with an IPv6 location first, and both the mappings and the host's address are IPv4.
+
 Device info and capabilities are fetched lazily on first access and cached; the local address is resolved on every call. All three come at once via `getAll()`:
 
 ```typescript
@@ -63,7 +65,7 @@ const addr = await info.getLocalAddress();
 
 With `cacheGateway: true` the gateway found by the first discovery is kept for later calls. A request to it that fails with anything but a UPnP fault - a connection refused or reset, no answer within the request timeout, an answer that is not a SOAP reply - drops it and searches again, since a router whose UPnP service restarts answers on a new port. If the router now answers at a different location, the request is sent there once more; at the same location the original error stands. A UPnP fault, an action the router does not support, and an abort by the caller keep the cached gateway.
 
-A request sent again may find that the first one landed before the router went quiet. `createMapping` then gets 718 for its own mapping, and resolves if the router holds exactly that mapping (same internal client, internal port and description). `removeMapping` gets 714, and `removeMappingRange` 714 or 730, and they resolve. On a first request each of these codes throws as usual. `createAnyMapping` is never sent twice, because the router picks its port.
+A request sent again may find that the first one landed before the router went quiet. `createMapping` then gets 718 for its own mapping, and resolves if the router holds exactly that mapping (same internal client, internal port and description, compared as `canonicalDescription` gives it). `removeMapping` gets 714, and `removeMappingRange` 714 or 730, and they resolve. On a first request each of these codes throws as usual. `createAnyMapping` is never sent twice, because the router picks its port.
 
 ### Device Info
 
@@ -119,6 +121,11 @@ const mapping = await client.getMapping({ public: 8080, protocol: "TCP" });
 const all = await client.getMappings();
 const local = await client.getMappings({ local: true });
 const filtered = await client.getMappings({ description: /^Flux_/ });
+
+// A string filter is matched as routers store descriptions: FRITZ!Box keeps
+// "Flux_App_web" as "Flux.App.web", and both match "Flux_App_". Compare any
+// stored description the same way with canonicalDescription():
+canonicalDescription("Flux.App.web") === canonicalDescription("Flux_App_web"); // true
 ```
 
 ## Network Info
@@ -213,7 +220,7 @@ The gateway, its description and its capabilities are fetched once and shared be
 
 | Option | Type | Default | Description |
 |--------|------|---------|-------------|
-| `timeout` | `number` | `1800` | SSDP discovery timeout (ms) |
+| `timeout` | `number` | `5000` | SSDP discovery timeout (ms) |
 | `cacheGateway` | `boolean` | `false` | Keep the discovered gateway between calls; see [Caching the Gateway](#caching-the-gateway) |
 | `url` | `string` | — | Bypass SSDP, connect directly |
 | `localAddress` | `string` | — | Required when using `url` |

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@runonflux/nat-upnp",
-  "version": "2.0.3",
+  "version": "2.0.4",
   "main": "build/src/index.js",
   "types": "build/src/index.d.ts",
   "files": [

src/index.ts

@@ -19,7 +19,7 @@ export type {
 export { Ssdp } from "./nat-upnp/ssdp";
 export type { SearchCallback, ISsdp, SsdpEmitter } from "./nat-upnp/ssdp";
 
-export { Client, UpnpInfo } from "./nat-upnp/client";
+export { Client, UpnpInfo, canonicalDescription } from "./nat-upnp/client";
 export type {
   GetMappingOpts,
   GetMappingRangeOpts,

src/nat-upnp/client.ts

@@ -1,3 +1,4 @@
+import net from "net";
 import { RawResponse } from "../index";
 import Device, {
   GatewayDevice,
@@ -113,7 +114,7 @@ export class Client implements IClient {
       throw new Error("`localAddress` must be supplied if using `url`");
     }
 
-    this.timeout = options.timeout || 1800;
+    this.timeout = options.timeout || 5000;
     this.url = options.url || null;
     this.localAddress = options.localAddress || null;
     this.cacheGateway = options.cacheGateway || false;
@@ -170,7 +171,7 @@ export class Client implements IClient {
           held !== null &&
           held.mapping.private.host === internalClient &&
           held.mapping.private.port === ports.internal.port &&
-          held.mapping.description === description;
+          canonicalDescription(held.mapping.description) === canonicalDescription(description);
         if (!isThisMapping) throw err;
         return held.raw;
       }
@@ -632,6 +633,11 @@ export class Client implements IClient {
 
       p.on("device", (headers) => {
         if (resolved) return;
+        // Mappings are IPv4 and the node's address is read from its route to
+        // the router, so only an IPv4 location will do. Some routers answer
+        // an IPv4 search twice, with an IPv6 location first; the IPv4 answer
+        // that follows is the one taken.
+        if (!isIPv4Location(headers.location)) return;
         resolved = true;
         p.emit("end");
         clearTimeout(timeout);
@@ -745,6 +751,14 @@ function validPort(value: unknown, what: string, lowest: 0 | 1 = 1): number {
  * from strict firmware, or a mapping to nothing reported as success from
  * lenient firmware. The absence is an error before the wire, not an element.
  */
+function isIPv4Location(location: string): boolean {
+  try {
+    return net.isIPv4(new URL(location).hostname);
+  } catch {
+    return false;
+  }
+}
+
 function internalClientFor(host: string | undefined, localAddress: string): string {
   const client = host || localAddress;
   if (!client) {
@@ -837,10 +851,22 @@ function findResponseKey(data: RawResponse, prefix: string): any | null {
   return key ? data[key] : null;
 }
 
+/**
+ * A mapping description as routers keep it, whatever they did to its
+ * punctuation: every run of characters other than letters and digits becomes
+ * one underscore. FRITZ!Box stores "Flux_App_web" as "Flux.App.web" and
+ * "FluxOS Reserved" as "FluxOS.Reserved"; all three forms of each compare
+ * equal here. Compare descriptions a router has stored this way, never as
+ * they were sent.
+ */
+export function canonicalDescription(description: string): string {
+  return String(description ?? "").replace(/[^A-Za-z0-9]+/g, "_");
+}
+
 function matchesDescription(desc: string, filter: RegExp | string): boolean {
   if (typeof desc !== "string") return false;
   if (filter instanceof RegExp) return filter.test(desc);
-  return desc.indexOf(filter) !== -1;
+  return canonicalDescription(desc).includes(canonicalDescription(filter));
 }
 
 export default Client;

src/nat-upnp/device.ts

@@ -107,11 +107,25 @@ function text(value: unknown): string {
  * mapping table does within 50 ms.
  *
  * A kept connection is closed from this side after KEPT_CONNECTION_IDLE_MS
- * without a request: sooner than those routers close one themselves (4 to 5 s),
- * so a request never crosses the router's close, and the connection does not
- * hold one of the router's few slots between unrelated calls.
+ * unused in the pool: sooner than those routers close one themselves (4 to
+ * 5 s), so a request never crosses the router's close, and the connection does
+ * not hold one of the router's few slots between unrelated calls. The limit is
+ * the pool's alone. Given to the agent as its `timeout` it would apply to every
+ * socket from creation, connecting included, and a router slow to accept - a
+ * busy miniupnpd, one loop behind a backlog of five - would have requests cut
+ * off at the idle limit instead of at their own timeout.
  */
 class RouterAgent extends http.Agent {
+  constructor() {
+    super({ keepAlive: true });
+    // After the agent's own "free" handler, which sets a pooled socket's
+    // timeout to the agent's - none.
+    this.on("free", (socket: Socket) => {
+      const pooled = Object.values(this.freeSockets).some((sockets) => sockets?.includes(socket));
+      if (pooled && !socket.destroyed) socket.setTimeout(KEPT_CONNECTION_IDLE_MS);
+    });
+  }
+
   // Node returns whether the socket was kept; @types/node declares void.
   keepSocketAlive(socket: Socket): void {
     // The request the socket has just carried, and the router's answer to it.
@@ -124,7 +138,8 @@ class RouterAgent extends http.Agent {
   }
 }
 export const KEPT_CONNECTION_IDLE_MS = 3000;
-const upnpAgent = new RouterAgent({ keepAlive: true, timeout: KEPT_CONNECTION_IDLE_MS });
+/** Exported for the tests, which slow a connection's connect through it. */
+export const upnpAgent = new RouterAgent();
 
 // Prevent OOM from malicious/broken router responses
 const MAX_RESPONSE_BYTES = 2 * 1024 * 1024;

src/nat-upnp/ssdp.ts

@@ -17,6 +17,14 @@ export class Ssdp implements ISsdp {
    */
   private static readonly INTERNET_ROUTE_PROBE = "8.8.8.8";
 
+  /**
+   * After the first M-SEARCH, the same search goes out again at these offsets
+   * (ms) until it ends: UDP loses some, and a router can drop searches for a
+   * second or more at a time. The gaps double, as DHCP's retransmissions do,
+   * so the last copies still reach a router that was silent for the first.
+   */
+  static readonly RESEND_AFTER_MS: readonly number[] = [250, 750, 1750, 3750];
+
   private readonly sourcePort: number;
   private readonly multicast = "239.255.255.250";
   private readonly port = 1900;
@@ -26,6 +34,7 @@ export class Ssdp implements ISsdp {
   private bound = false;
   private closed = false;
   private readonly pendingSearches: [string, SsdpEmitter][] = [];
+  private readonly resendTimers = new Set<NodeJS.Timeout>();
 
   constructor(options?: { sourcePort?: number }) {
     this.sourcePort = options?.sourcePort || 0;
@@ -138,6 +147,17 @@ export class Ssdp implements ISsdp {
 
     let ended = false;
 
+    const resends = Ssdp.RESEND_AFTER_MS.map((after) => {
+      const timer = setTimeout(() => {
+        this.resendTimers.delete(timer);
+        // a socket that died and was not replaced has no one to send for
+        if (!this.socket) return;
+        this.socket.send(query, 0, query.length, this.port, this.multicast);
+      }, after);
+      this.resendTimers.add(timer);
+      return timer;
+    });
+
     const ondevice: SearchCallback = (headers) => {
       if (ended || headers.st !== device) return;
       emitter!.emit("device", headers);
@@ -155,6 +175,10 @@ export class Ssdp implements ISsdp {
       this.ssdpEmitter.removeListener("device", ondevice);
       this.ssdpEmitter.removeListener("error", onerror);
       ended = true;
+      for (const timer of resends) {
+        clearTimeout(timer);
+        this.resendTimers.delete(timer);
+      }
     });
 
     return emitter;
@@ -166,6 +190,8 @@ export class Ssdp implements ISsdp {
     this.bound = false;
     this.pendingSearches.length = 0;
     this.ssdpEmitter.removeAllListeners();
+    for (const timer of this.resendTimers) clearTimeout(timer);
+    this.resendTimers.clear();
 
     if (this.socket) {
       try {