diff --git a/api/AGENTS.md b/api/AGENTS.md index 9abfeb4f..c74bc0da 100644 --- a/api/AGENTS.md +++ b/api/AGENTS.md @@ -126,6 +126,8 @@ behind `dns-resolver` (gated by `tests/dns-resolvers-data.test.js`). - `requireValidReportId()` — `/api/report/:id` route param (22-char base64url). New param shape → new guard there, attached in `backend-server.js`. +Rewrites go through `setQueryParam()` there: Express 5's `req.query` re-parses +on every read, so a direct write is lost. ### Response enrichment lives in middleware too diff --git a/server/guards.js b/server/guards.js index 3160c152..575e20e9 100644 --- a/server/guards.js +++ b/server/guards.js @@ -11,6 +11,18 @@ import { STATUS_PROVIDER_IDS } from './service-status-providers.js'; import { DNS_RECORD_TYPE_SET } from '../common/dns-record-types.js'; import { parseAsnInput } from '../common/asn-input.js'; +// Express 5's req.query is a prototype getter that re-parses the URL on every +// read, so a direct write is lost. Pin the parsed object as an own property +// first; later guards, cacheable resolvers and the handler then share it. +const setQueryParam = (req, name, value) => { + if (!Object.hasOwn(req, 'query')) { + Object.defineProperty(req, 'query', { + value: req.query, writable: true, configurable: true, enumerable: true, + }); + } + req.query[name] = value; +}; + // Reject requests without an allowed referer. The error message variant // preserves the existing user-facing wording. export const requireReferer = (req, res, next) => { @@ -57,7 +69,7 @@ export const requireValidDomain = (paramName = 'domain') => (req, res, next) => if (!isValidDomain(domain)) { return res.status(400).json({ error: 'Invalid domain' }); } - req.query[paramName] = domain; + setQueryParam(req, paramName, domain); next(); }; @@ -89,7 +101,7 @@ export const requireValidASN = (paramName = 'asn') => (req, res, next) => { if (asn === null) { return res.status(400).json({ error: 'Invalid ASN' }); } - req.query[paramName] = String(asn); + setQueryParam(req, paramName, String(asn)); next(); }; @@ -104,7 +116,7 @@ export const normalizeAsnQuery = (paramName = 'q') => (req, res, next) => { if (asn === null) { return res.status(400).json({ error: 'Invalid ASN' }); } - req.query[paramName] = `AS${asn}`; + setQueryParam(req, paramName, `AS${asn}`); next(); }; @@ -119,7 +131,7 @@ export const requireValidCountry = (paramName = 'country') => (req, res, next) = if (!/^[A-Za-z]{2}$/.test(raw)) { return res.status(400).json({ error: 'Invalid country' }); } - req.query[paramName] = String(raw).toUpperCase(); + setQueryParam(req, paramName, String(raw).toUpperCase()); next(); }; @@ -145,7 +157,7 @@ export const requireValidRecordType = (paramName = 'type') => (req, res, next) = if (!DNS_RECORD_TYPE_SET.has(String(type).toUpperCase())) { return res.status(400).json({ error: 'Invalid record type' }); } - req.query[paramName] = String(type).toUpperCase(); + setQueryParam(req, paramName, String(type).toUpperCase()); next(); }; diff --git a/tests/guards.test.js b/tests/guards.test.js index 7c66a6cf..4eddc698 100644 --- a/tests/guards.test.js +++ b/tests/guards.test.js @@ -1,5 +1,6 @@ import assert from 'node:assert/strict'; import { describe, it } from 'node:test'; +import express from 'express'; import { requireReferer, requirePublicIP, requireValidPrefix, requireValidDomain, requireValidProviderId, requireValidRecordType, requireValidReportId, requireValidCountry, requireValidASN, normalizeAsnQuery } from '../server/guards.js'; @@ -407,3 +408,47 @@ describe('normalizeAsnQuery', () => { } }); }); + +// Express 5 defines req.query as a prototype getter that re-parses the URL +// on every read; a real request (never listening) catches rewrites that a +// plain-object stub would keep. +describe('guard rewrites on a real Express request', () => { + const app = express(); + const makeExpressReq = (url) => { + const req = Object.create(app.request); + req.url = url; + req.headers = {}; + return req; + }; + + it('reproduces the re-parsing getter', () => { + const req = makeExpressReq('/x?type=caa'); + assert.notEqual(req.query, req.query); + }); + + for (const [name, guard, url, param, expected] of [ + ['requireValidRecordType', requireValidRecordType(), '/x?type=caa', 'type', 'CAA'], + ['requireValidDomain', requireValidDomain(), '/x?domain=Example.COM', 'domain', 'example.com'], + ['requireValidASN', requireValidASN(), '/x?asn=AS013335', 'asn', '13335'], + ['normalizeAsnQuery', normalizeAsnQuery(), '/x?q=as13335', 'q', 'AS13335'], + ['requireValidCountry', requireValidCountry(), '/x?country=us', 'country', 'US'], + ]) { + it(`${name} rewrite survives a later req.query read`, () => { + const req = makeExpressReq(url); + let nextCalled = false; + guard(req, makeRes(), () => { nextCalled = true; }); + assert.equal(nextCalled, true); + assert.equal(req.query[param], expected); + }); + } + + it('keeps both rewrites when chained like /api/dnsresolver', () => { + const req = makeExpressReq('/x?type=caa&hostname=Example.COM'); + let calls = 0; + requireValidDomain('hostname')(req, makeRes(), () => { calls++; }); + requireValidRecordType()(req, makeRes(), () => { calls++; }); + assert.equal(calls, 2); + assert.deepEqual({ ...req.query }, { type: 'CAA', hostname: 'example.com' }); + assert.equal(req.query, req.query); + }); +});