Skip to content

Commit 499634f

Browse files
committed
fix(API): reject non-integer TRUST_PROXY values instead of NaN
parseInt() silently produced NaN for booleans and non-numeric strings, which Express treats as falsy and disables proxy trust. Add resolveTrustProxyHops() to enforce the documented non-negative integer contract, log a startup warning for invalid values, and fall back to 0.
1 parent be802d1 commit 499634f

5 files changed

Lines changed: 280 additions & 6 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,7 @@ In the `CHIA_ROOT` directory (usually `~/.chia/mainnet` on Linux), CADT will add
265265
* **APP**: This section contains shared configuration used by both V1 and V2 APIs.
266266
* **CW_PORT**: CADT port where the API will be available. 31310 by default.
267267
* **BIND_ADDRESS**: By default, CADT listens on localhost only. To enable remote connections to CADT, change this to `0.0.0.0` to listen on all network interfaces, or to an IP address to listen on a specific network interface.
268-
* **TRUST_PROXY**: Number of reverse-proxy hops between the internet and CADT. Used to correctly identify real client IP addresses for rate limiting and logging when CADT is deployed behind one or more proxies. Set to `0` (the default) when CADT is accessed directly with no proxy in front of it. Set to `1` when behind a single proxy such as nginx or Cloudflare. Set to `2` when behind two proxies such as Cloudflare in front of nginx (a common cloud/k8s deployment). Never set to `true`; that would trust the user-supplied IP in the `X-Forwarded-For` header and defeat rate limiting.
268+
* **TRUST_PROXY**: Number of reverse-proxy hops between the internet and CADT. Used to correctly identify real client IP addresses for rate limiting and logging when CADT is deployed behind one or more proxies. Must be a non-negative integer: set to `0` (the default) when CADT is accessed directly with no proxy in front of it, `1` when behind a single proxy such as nginx or Cloudflare, or `2` when behind two proxies such as Cloudflare in front of nginx (a common cloud/k8s deployment). Booleans (`true`/`false`) and non-numeric strings (e.g. `loopback`) are rejected, log a warning at startup, and fall back to `0`; in particular, never set to `true`, which would trust the user-supplied IP in the `X-Forwarded-For` header and defeat rate limiting.
269269
* **DATALAYER_URL**: URL and port to connect to the [Chia DataLayer RPC](https://docs.chia.net/datalayer-rpc). If Chia is installed locally with default settings, https://localhost:8562 will work.
270270
* **WALLET_URL**: URL and port to connect to the [Chia Wallet RPC](https://docs.chia.net/wallet-rpc). If Chia is installed on the same machine as CADT with default settings, https://localhost:9256 will work.
271271
* **USE_SIMULATOR**: Developer setting to populate CADT from a governance file and enable some extra APIs. Should always be "false" under normal usage.

src/middleware.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { OrganizationsV2 } from './models/v2/index.js';
2121
import { logger } from './config/logger.js';
2222
import { sendReadOnlyError } from './utils/read-only-response.js';
2323
import { getRateLimitRetryAfterSeconds } from './utils/rate-limit.js';
24+
import { resolveTrustProxyHops } from './utils/trust-proxy.js';
2425
import {
2526
checkDiskSpace,
2627
logDiskSpaceStatus,
@@ -64,7 +65,12 @@ const app = express();
6465
// This also silences the express-rate-limit ValidationError that fires
6566
// when X-Forwarded-For is present but trust proxy is disabled.
6667
// Never use `true`; it trusts the user-supplied leftmost IP.
67-
const trustProxy = parseInt(getConfig().APP.TRUST_PROXY ?? 0, 10);
68+
//
69+
// resolveTrustProxyHops enforces the non-negative integer contract and
70+
// logs a warning for any other value so that booleans / typos / strings
71+
// like "loopback" don't silently disable proxy trust via NaN (see
72+
// src/utils/trust-proxy.js for the rationale).
73+
const trustProxy = resolveTrustProxyHops(getConfig().APP.TRUST_PROXY);
6874
app.set('trust proxy', trustProxy);
6975

7076
app.use(

src/utils/defaultConfig.js

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,18 @@ export const defaultConfig = {
44
BIND_ADDRESS: 'localhost',
55
/**
66
* Number of reverse-proxy hops between the internet and CADT.
7-
* Passed directly to Express `trust proxy`.
7+
* Passed to Express `trust proxy` via `resolveTrustProxyHops`
8+
* (src/utils/trust-proxy.js), which requires a non-negative
9+
* integer:
810
* 0 – no proxy (default, direct access)
911
* 1 – one hop (nginx OR Cloudflare-only)
1012
* 2 – two hops (Cloudflare → nginx, typical k8s ingress setup)
11-
* Setting this correctly lets express-rate-limit see the real client IP
12-
* instead of the proxy IP. Never set to `true`; that trusts the
13-
* user-supplied leftmost IP and defeats rate limiting.
13+
* Setting this correctly lets express-rate-limit see the real
14+
* client IP instead of the proxy IP. Booleans (`true`/`false`)
15+
* and non-numeric strings (e.g. `"loopback"`) are explicitly
16+
* rejected and fall back to 0 with a warning log; `true` in
17+
* particular would trust the user-supplied leftmost IP and defeat
18+
* rate limiting.
1419
*/
1520
TRUST_PROXY: 0,
1621
DATALAYER_URL: 'https://localhost:8562',

src/utils/trust-proxy.js

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
'use strict';
2+
3+
import { logger } from '../config/logger.js';
4+
5+
// Express's `trust proxy` accepts booleans, numbers, strings (subnets,
6+
// CIDRs, "loopback", etc.), arrays and functions. CADT's contract is
7+
// intentionally narrower: TRUST_PROXY must be a non-negative integer
8+
// representing the number of reverse-proxy hops, as documented in
9+
// src/utils/defaultConfig.js.
10+
//
11+
// The previous implementation used `parseInt(rawValue, 10)`, which
12+
// silently produced `NaN` for any non-numeric input (`true`, `false`,
13+
// the strings "true"/"false"/"loopback", typos, etc.). Express treats
14+
// `NaN` as falsy and disables proxy trust, so an operator who set
15+
// `TRUST_PROXY=true` believing they were enabling proxy trust would
16+
// actually get the opposite outcome with no log message. Worse,
17+
// `docker-entrypoint.sh` converts the env var "true"/"false" to a YAML
18+
// boolean, so the bug fires for the most plausible "just enable it"
19+
// configuration mistake.
20+
//
21+
// `resolveTrustProxyHops` enforces the integer contract and logs a
22+
// clear warning when the input is outside it, defaulting to 0 (no
23+
// trust) so the failure mode is "rate limiting matches no proxy" rather
24+
// than "rate limiting silently trusts the world".
25+
26+
// Number.isSafeInteger already returns false for NaN, Infinity, and any
27+
// non-integer numeric value, so an explicit isFinite check would be
28+
// redundant.
29+
const isNonNegativeSafeInteger = (value) =>
30+
Number.isSafeInteger(value) && value >= 0;
31+
32+
/**
33+
* Coerce a raw TRUST_PROXY config value to a non-negative integer hop
34+
* count suitable for `app.set('trust proxy', n)`.
35+
*
36+
* @param {unknown} rawValue
37+
* @param {{warn: (msg: string) => void}} [log] Logger override for tests.
38+
* @returns {number}
39+
*/
40+
export const resolveTrustProxyHops = (rawValue, log = logger) => {
41+
if (rawValue === undefined || rawValue === null) {
42+
return 0;
43+
}
44+
45+
if (typeof rawValue === 'number') {
46+
if (isNonNegativeSafeInteger(rawValue)) {
47+
return rawValue;
48+
}
49+
log.warn(
50+
`[middleware]: TRUST_PROXY=${rawValue} is not a non-negative integer; ` +
51+
'falling back to 0 (no proxy trust). See defaultConfig.js for valid values.',
52+
);
53+
return 0;
54+
}
55+
56+
if (typeof rawValue === 'boolean') {
57+
// Booleans are explicitly unsupported because the comment in
58+
// defaultConfig.js says "Never set to `true`; that trusts the
59+
// user-supplied leftmost IP and defeats rate limiting". We also
60+
// reject `false` so operators get a clear log when their config
61+
// does not match the documented contract (instead of getting 0 by
62+
// accident).
63+
log.warn(
64+
`[middleware]: TRUST_PROXY is set to boolean \`${rawValue}\`; ` +
65+
'only non-negative integers (0, 1, 2, …) are supported. ' +
66+
'Falling back to 0 (no proxy trust). See defaultConfig.js.',
67+
);
68+
return 0;
69+
}
70+
71+
if (typeof rawValue === 'string') {
72+
const trimmed = rawValue.trim();
73+
if (trimmed === '') {
74+
return 0;
75+
}
76+
// Require an exact non-negative integer literal so "loopback",
77+
// "2 hops", "1.5" and "+2" don't get silently coerced.
78+
if (/^\d+$/.test(trimmed)) {
79+
const parsed = Number(trimmed);
80+
if (isNonNegativeSafeInteger(parsed)) {
81+
return parsed;
82+
}
83+
}
84+
log.warn(
85+
`[middleware]: TRUST_PROXY="${rawValue}" is not a non-negative integer string; ` +
86+
'falling back to 0 (no proxy trust). See defaultConfig.js.',
87+
);
88+
return 0;
89+
}
90+
91+
log.warn(
92+
`[middleware]: TRUST_PROXY has unsupported type \`${typeof rawValue}\`; ` +
93+
'falling back to 0 (no proxy trust).',
94+
);
95+
return 0;
96+
};
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
'use strict';
2+
3+
import { expect } from 'chai';
4+
import { resolveTrustProxyHops } from '../../../src/utils/trust-proxy.js';
5+
6+
// Pure-function spec for the TRUST_PROXY config resolver. Lives under
7+
// tests/v2/integration so it runs in the V2 mocha bucket, but the helper
8+
// itself is version-agnostic.
9+
//
10+
// The behaviour matters because the previous implementation used
11+
// `parseInt(rawValue, 10)`, which silently produced `NaN` for booleans
12+
// and non-numeric strings. `app.set('trust proxy', NaN)` is treated as
13+
// falsy by Express, so an operator who set `TRUST_PROXY=true` thinking
14+
// they were enabling proxy trust would actually disable it without any
15+
// log message.
16+
describe('resolveTrustProxyHops', function () {
17+
const makeLog = () => {
18+
const warnings = [];
19+
return {
20+
warnings,
21+
logger: {
22+
warn: (msg) => warnings.push(msg),
23+
},
24+
};
25+
};
26+
27+
describe('valid integer inputs (no warning)', function () {
28+
const validCases = [
29+
{ label: '0 (number)', input: 0, expected: 0 },
30+
{ label: '1 (number)', input: 1, expected: 1 },
31+
{ label: '2 (number)', input: 2, expected: 2 },
32+
{ label: '"0" (string)', input: '0', expected: 0 },
33+
{ label: '"1" (string)', input: '1', expected: 1 },
34+
{ label: '" 2 " (padded string)', input: ' 2 ', expected: 2 },
35+
{ label: '10 (large hop count)', input: 10, expected: 10 },
36+
];
37+
38+
validCases.forEach(({ label, input, expected }) => {
39+
it(`returns ${expected} for ${label}`, function () {
40+
const { logger, warnings } = makeLog();
41+
expect(resolveTrustProxyHops(input, logger)).to.equal(expected);
42+
expect(warnings, 'no warnings expected for valid input').to.have.length(0);
43+
});
44+
});
45+
});
46+
47+
describe('empty / unset inputs default to 0 silently', function () {
48+
[
49+
{ label: 'undefined', input: undefined },
50+
{ label: 'null', input: null },
51+
{ label: '"" (empty string)', input: '' },
52+
{ label: '" " (whitespace-only string)', input: ' ' },
53+
].forEach(({ label, input }) => {
54+
it(`returns 0 for ${label} with no warning`, function () {
55+
const { logger, warnings } = makeLog();
56+
expect(resolveTrustProxyHops(input, logger)).to.equal(0);
57+
expect(warnings, 'no warnings expected for unset input').to.have.length(0);
58+
});
59+
});
60+
});
61+
62+
describe('boolean inputs fall back to 0 with a warning', function () {
63+
// Booleans are the practical regression we are fixing: docker-entrypoint.sh
64+
// converts the env strings "true"/"false" to YAML booleans, and the
65+
// previous parseInt() produced NaN for both.
66+
[true, false].forEach((input) => {
67+
it(`returns 0 and warns for boolean \`${input}\``, function () {
68+
const { logger, warnings } = makeLog();
69+
expect(resolveTrustProxyHops(input, logger)).to.equal(0);
70+
expect(warnings).to.have.length(1);
71+
expect(warnings[0]).to.match(/TRUST_PROXY/);
72+
expect(warnings[0]).to.include(String(input));
73+
});
74+
});
75+
});
76+
77+
describe('invalid number inputs fall back to 0 with a warning', function () {
78+
[
79+
{ label: 'NaN', input: Number.NaN },
80+
{ label: 'Infinity', input: Number.POSITIVE_INFINITY },
81+
{ label: '-Infinity', input: Number.NEGATIVE_INFINITY },
82+
{ label: '-1 (negative)', input: -1 },
83+
{ label: '1.5 (non-integer)', input: 1.5 },
84+
].forEach(({ label, input }) => {
85+
it(`returns 0 and warns for ${label}`, function () {
86+
const { logger, warnings } = makeLog();
87+
expect(resolveTrustProxyHops(input, logger)).to.equal(0);
88+
expect(warnings).to.have.length(1);
89+
expect(warnings[0]).to.match(/TRUST_PROXY/);
90+
});
91+
});
92+
});
93+
94+
describe('non-integer string inputs fall back to 0 with a warning', function () {
95+
[
96+
'true',
97+
'false',
98+
'loopback',
99+
'linklocal',
100+
'10.0.0.1',
101+
'1.5',
102+
'+2',
103+
'-1',
104+
'2 hops',
105+
'two',
106+
].forEach((input) => {
107+
it(`returns 0 and warns for "${input}"`, function () {
108+
const { logger, warnings } = makeLog();
109+
expect(resolveTrustProxyHops(input, logger)).to.equal(0);
110+
expect(warnings).to.have.length(1);
111+
expect(warnings[0]).to.match(/TRUST_PROXY/);
112+
expect(warnings[0]).to.include(input);
113+
});
114+
});
115+
});
116+
117+
describe('unsupported types fall back to 0 with a warning', function () {
118+
[
119+
{ label: 'array', input: [1, 2, 3] },
120+
{ label: 'object', input: { hops: 1 } },
121+
{ label: 'function', input: () => 1 },
122+
].forEach(({ label, input }) => {
123+
it(`returns 0 and warns for ${label}`, function () {
124+
const { logger, warnings } = makeLog();
125+
expect(resolveTrustProxyHops(input, logger)).to.equal(0);
126+
expect(warnings).to.have.length(1);
127+
expect(warnings[0]).to.match(/TRUST_PROXY/);
128+
});
129+
});
130+
});
131+
132+
describe('default logger argument', function () {
133+
// Every other case explicitly injects a stub logger. This case covers
134+
// the production call-site shape (no second argument) so the default
135+
// logger path is exercised at least once. We do not assert against
136+
// the real logger's output because that would couple this spec to
137+
// winston transport behaviour; we only assert the resolved value.
138+
it('returns the correct integer without throwing when no logger is supplied', function () {
139+
expect(resolveTrustProxyHops(2)).to.equal(2);
140+
expect(resolveTrustProxyHops(0)).to.equal(0);
141+
});
142+
143+
it('falls back to 0 without throwing when no logger is supplied (invalid input)', function () {
144+
// The real logger should be invoked here; we accept whatever side
145+
// effects winston produces and only check the return value.
146+
expect(resolveTrustProxyHops(true)).to.equal(0);
147+
expect(resolveTrustProxyHops('loopback')).to.equal(0);
148+
});
149+
});
150+
151+
describe('regression coverage for the parseInt() bug', function () {
152+
// The previous implementation was:
153+
// const trustProxy = parseInt(getConfig().APP.TRUST_PROXY ?? 0, 10);
154+
// which produced NaN for all of these inputs.
155+
const regressions = [true, false, 'true', 'false', 'loopback'];
156+
157+
regressions.forEach((input) => {
158+
it(`does not produce NaN for legacy regression input ${JSON.stringify(input)}`, function () {
159+
const { logger } = makeLog();
160+
const result = resolveTrustProxyHops(input, logger);
161+
expect(result).to.be.a('number');
162+
expect(Number.isNaN(result), 'must not be NaN').to.equal(false);
163+
expect(Number.isFinite(result), 'must be finite').to.equal(true);
164+
});
165+
});
166+
});
167+
});

0 commit comments

Comments
 (0)