RateLimit didn't check if trusted proxy send the header x-forwarded-for

[thib3113]

Just found this reading the code :

https://github.com/moleculerjs/moleculer-web/blob/master/src/index.js#L1351C47-L1351C47

let opts = Object.assign({}, {
	window: 60 * 1000,
	limit: 30,
	headers: false,
	key: (req) => {
		return req.headers["x-forwarded-for"] ||
			req.connection.remoteAddress ||
			req.socket.remoteAddress ||
			req.connection.socket.remoteAddress;
	}
}, rateLimit);

getting ip from x-forwarded-for without checking if connection.remoteAddress is a trusted proxy is a security problem .

because I can send the header x-forwarded-for manually .

( not a big problem, but keep in mind that someone can bypass the rate limiter ) .

Also, while headers are lowercase in node.js, this is not the case for all implementations. For example, node.js in AWS Lambda does not convert headers to lowercase.

[icebob]

What is your proposal?

[thib3113]

@icebob in fact ... just let the user the responsibility of getting the ip .

Else, we need to manage trusted proxies, and so parse the x-forwarded-for header, to extract the first "not trusted proxy" . Also, sometimes "trusted proxies" can be a cidr, so, we need to check if the ips come from the cidr .... not an esay task .

Also, I think managing trusted proxies (like express do), can be interesting for moleculer-web … but It's not an easy task

[icebob]

Ok, but as you see, it's covered, because this logic just a default value, you can overwrite it in rateLimit options.

[thib3113]

yes I know for the default value . But if it's only a default value, I think it's better to don't use the header req.headers["x-forwarded-for"] .

Users without reverse proxy will be secure, and user with reverse proxy will need to adapt correctly depending on the reverse proxy configuration