Skip to content

Commit 002949a

Browse files
fix(doh): keep default guards when only one is overridden
createDohServer merged the caller's guard overrides onto DEFAULT_GUARDS and reported the result as server.guards, but built the handler from the raw overrides. A caller tuning a single guard programmatically (say maxResponseBytes) silently dropped rateLimit, ban and the amplification cap to createDohHandler's own null/null/0 defaults, while server.guards still reported the safe defaults were in force. A resolver that says it rate limits and does not is the open resolver these guards exist to prevent. Build the handler from the merged 'applied' set so the guards that run are the guards reported. --no-guards is unaffected: it produces a full guard set with explicit nulls, so those still override the defaults. Adds a regression test: with only maxResponseBytes overridden, a burst past the default burst size is still REFUSED once spent.
1 parent 0a8a90f commit 002949a

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

src/doh-server.mjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ export async function createDohServer({
141141
tldSet: new Set(await fetchTlds({ registryBase }).catch(() => [])),
142142
parkingAddress: await parkingAddress().catch(() => null),
143143
onQuery,
144-
...guards,
144+
...applied,
145145
});
146146

147147
const server = http.createServer(async (req, res) => {

test/doh-server.test.mjs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// The HTTP half of the DoH resolver.
2+
import http from "node:http";
23
import test from "node:test";
34
import assert from "node:assert/strict";
45

@@ -159,3 +160,49 @@ test("a server built with defaults actually enforces them", async (t) => {
159160
const body = Buffer.from(await limited.arrayBuffer());
160161
assert.equal(body.readUInt16BE(2) & 0x000f, 5, "REFUSED once the burst is spent");
161162
});
163+
164+
test("overriding one guard keeps the guards you did not mention", async (t) => {
165+
// A caller mounting this programmatically may tune a single guard and leave
166+
// the rest at their defaults. The un-mentioned guards have to stay the safe
167+
// defaults `server.guards` reports, not silently fall to the handler's own
168+
// off-by-default null/null/0 — a resolver that says it rate limits and does
169+
// not is exactly the open resolver these guards exist to prevent.
170+
const registry = http.createServer((req, res) => {
171+
// The only thing the resolver needs from the registry here is that our
172+
// ending is ours, so a query for it reaches the handler as an answerable
173+
// name (NXDOMAIN) rather than a not-ours REFUSED, which rate limiting also
174+
// returns and would mask the very thing under test.
175+
if ((req.url || "").startsWith("/api/moshpit/tlds")) {
176+
res.writeHead(200, { "content-type": "application/json" });
177+
res.end(JSON.stringify({ tlds: ["moshtest"], total: 1 }));
178+
} else {
179+
res.writeHead(404, { "content-type": "application/json" });
180+
res.end("{}");
181+
}
182+
});
183+
await new Promise((r) => registry.listen(0, "127.0.0.1", r));
184+
t.after(() => new Promise((r) => registry.close(r)));
185+
const registryBase = `http://127.0.0.1:${registry.address().port}`;
186+
187+
// Override only maxResponseBytes; rateLimit is left unmentioned.
188+
const server = await createDohServer({ port: 0, registryBase, maxResponseBytes: 4096 });
189+
t.after(() => server.close());
190+
assert.deepEqual(server.guards.rateLimit, { perSecond: 20, burst: 40 },
191+
"reports the default rate limit is in force");
192+
193+
const ask = () => fetch(`http://127.0.0.1:${server.port}${DOH_PATH}`, {
194+
method: "POST",
195+
headers: { "content-type": DNS_MESSAGE, "x-forwarded-for": "198.51.100.9" },
196+
body: query("x.moshtest"),
197+
});
198+
// Default burst is 40. Fire well past it at once so token refill during the
199+
// run cannot stand in for a limiter that was dropped.
200+
const rcodes = await Promise.all(
201+
Array.from({ length: 120 }, () => ask().then(async (r) =>
202+
Buffer.from(await r.arrayBuffer()).readUInt16BE(2) & 0x000f)),
203+
);
204+
assert.ok(rcodes.some((c) => c !== 5),
205+
"an ours-name below the limit is answered, not REFUSED");
206+
assert.ok(rcodes.some((c) => c === 5),
207+
"the unmentioned default rate limit still REFUSES once the burst is spent");
208+
});

0 commit comments

Comments
 (0)