Rate limiting can be bypassed with a spoofed ip

[nickjamesio]

First off, thanks for creating the epic stack! I'm learning about rate limiting so I thought I'd use epic-stack for reference. While looking over the repo, these two lines caught my attention:

• epic-stack/server/index.ts

  Line 152 in 800022d

  // Fly.io prevents spoofing of X-Forwarded-For

• epic-stack/server/index.ts

  Line 39 in 800022d

  app.set('trust proxy', true)

I was curious to see if it is indeed impossible to spoof X-Forwarded-For. I came across this forum post https://community.fly.io/t/recommended-setting-for-trust-proxy-on-express/6346 and I tried the curl request they provided and it looks like you can indeed spoof X-Forwarded-For.

Request for spoofing IP
curl -sv\ -H "Host: injected.host"\ -H "X-Forwarded-Proto: injected"\ -H "X-Forwarded-For: 1.2.3.4"\ -H "X-Forwarded-Port: 1234"\ -H "X-Forwarded-Ssl: maybe"\ https://debug.fly.dev/

While looking at express-rate-limit, it looks like it uses req.ip as the unique identifier. See here

The express docs have this to say about req.ip

The req.ip and req.ips values are populated based on the socket address and X-Forwarded-For header, starting at the first untrusted address.

I ran the epic stack locally and I could indeed get past the rate limiting with a spoofed IP. See this loom video

Based on this info, I am pretty sure rate limiting can be bypassed when an epic stack application is hosted in fly.io I think one solution would be to use the Fly-Client-Ip header since it would appear that header cannot be spoofed.

[kentcdodds]

Nice find! I welcome a PR to switch our implementation to use the Fly-Client-Ip header!

[nickjamesio]

I will def work on it!

[andrecasal]

Great work Nick!

[nickjamesio]

Been a busy week but I did get a chance to do some testing. It looks like Fly-Client-Ip can indeed work. One edge case though. If someone has a proxy in front of the app like Cloudflare, Fly-Client-Ip will contain the IP of Cloudflare, not the end user.

I did a quick search and it does not look like epic-stack requires the use of cloudflare. However, it can have unintended consequences if someone uses Cloudflare and is unaware. Do you have a recommendation on where to mention this or if it should be mentioned at all?

[kentcdodds]

Hmmmm... It definitely should be mentioned. Can probably just do a code comment. People making changes is always going to affect the assumptions we make and we can't over-complicate the codebase for every possible change people make to the stack.

[kentcdodds]

Hi @nickjamesio, are you still planning on working on this?

[nickjamesio]

Hey! Sorry about that and thanks for the nudge. I will be back from traveling in one week. I will take a look when I get back if no one has fixed it by then. Marking my calendar :)

[nickjamesio]

Just want to say I have some time this evening to work on this :)

[ancpetras]

@nickjamesio It may be of interest to you that Cloudflare sets the header CF-Connecting-IP to the original client's IP.

I don't know whether

• Special casing proxies makes any sense (though Cloudflare is super popular and possibly worth it)
• The fact that one can spoof CF-Connecting-IP outside of Cloudflare makes it impossible to write logic for
• CF-Connecting-IP is even forwarded by Fly Proxy

Just thought I'd mention it.

Cloudflare HTTP Header Docs

[nickjamesio]

Thanks! My plan is to add a code comment around CDNs but use Cloudflare as an example. I won't be adding any special logic for CF though.

Spoofing CF-Connecting-IP is something I have also thought of so thanks for the mention. I think I found a solution, however, it is Cloudflare specific. Authenticated origin pull

[andrecasal]

Hey @nickjamesio 👋 I'd like to write a test for this. Could you provide the request you've used to test it?

@kentcdodds would you want to add security tests to the Epic Stack?

[kentcdodds]

That sounds great depending on how complicated it is and how reliable they are. An example is always welcome!

[nickjamesio]

@andrecasal I did my testing in the browser but I can explain my process. I lowered the rate limit for the strongest endpoints down to 2 and tried logging in using invalid credentials. Attempt to login a few times and it will trigger the rate limit.

[andrecasal]

Thanks @nickjamesio!

@kentcdodds does this look good?

1. Selectively disable rate limits for certain tests:

test.describe('Rate Limit Tests', () => {

    test.beforeEach(() => {
        process.env.DISABLE_RATE_LIMIT = true;
    });

    test.afterEach(() => {
        process.env.DISABLE_RATE_LIMIT = false;
    });
    ...
}

2. Update middleware

const disableRateLimit = process.env.DISABLE_RATE_LIMIT || !IS_PROD || process.env.PLAYWRIGHT_TEST_BASE_URL
const rateLimitDefault = {
	windowMs: 60 * 1000,
	max: disableRateLimit ? Infinity : 1000,
	standardHeaders: true,
	legacyHeaders: false,
	// Fly.io prevents spoofing of X-Forwarded-For
	// so no need to validate the trustProxy config
	validate: { trustProxy: false },
	keyGenerator: (req: express.Request) => {
		return req.get('fly-client-ip') ?? `${req.ip}`
	},
}

3. Test the rate limits

// Helper function to make requests
async function makeRequests(url: string, count: number) {
    const responses = [];
    for (let i = 0; i < count; i++) {
        try {
            const response = await page.goto(url)
            responses.push(response.status);
        } catch (error) {
            if (error.response) {
                responses.push(error.response.status);
            } else {
                responses.push('error');
            }
        }
    }
    return responses;
}

// Tests
test.describe('Rate Limit Tests', () => {

    test('should allow 10 requests per minute for strongest rate limit', async () => {
        const url = `${BASE_URL}/strongest-endpoint`;
        const responses = await makeRequests(url, 12); // 10 allowed, 2 should be rate limited
        const successResponses = responses.filter(status => status === 200).length;
        const rateLimitedResponses = responses.filter(status => status === 429).length;

        expect(successResponses).toBe(10);
        expect(rateLimitedResponses).toBe(2);
    });

    test('should allow 100 requests per minute for strong rate limit', async () => {
        const url = `${BASE_URL}/strong-endpoint`;
        const responses = await makeRequests(url, 105); // 100 allowed, 5 should be rate limited
        const successResponses = responses.filter(status => status === 200).length;
        const rateLimitedResponses = responses.filter(status => status === 429).length;

        expect(successResponses).toBe(100);
        expect(rateLimitedResponses).toBe(5);
    });

    test('should allow 1000 requests per minute for general rate limit', async () => {
        const url = `${BASE_URL}/general-endpoint`;
        const responses = await makeRequests(url, 1005); // 1000 allowed, 5 should be rate limited
        const successResponses = responses.filter(status => status === 200).length;
        const rateLimitedResponses = responses.filter(status => status === 429).length;

        expect(successResponses).toBe(1000);
        expect(rateLimitedResponses).toBe(5);
    });
});

[kentcdodds]

I think this is kind of testing a dependency so I'm not sure I want to include it. Thanks though!

[andrecasal]

Good point. Would it be worth testing the IP spoofing?

[kentcdodds]

Perhaps! Definitely interested in seeing what a playwright test like that would look like.