-
-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rate limit signups #2280
rate limit signups #2280
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment for concrete changes. In addition, I have the following comment:
I think that the documentation (which now doesn't exist, so next best thing is adding a comment somewhere in this file) should explain the limitations of this approach:
- Limiting by IP addresses is maybe the simplest way of doing rate-limiting, but it has serious limitations:
a. Because of CGNAT, multiple users (potentially millions, but more likely a few hundred) can be sharing a single IP address. This makes this PR break things for legitimate users (if they're all using Group Income).
b. On the flip side, it doesn't really stop attacks, because attackers that are rate-limited can easily get a new IP address.
c. Because of the previous point, this PR can be used to cause a DoS by filling up the list of IP addresses.
d. At the very least, the key used should be the /64 block for IPv6 addresses. Not doing this makes this PR pointless and exposes the server to very easy DoS. - Because this state is local to the server, it doesn't work under load balancing scenarios.
// rate limit signups in production | ||
if (process.env.NODE_ENV === 'production') { | ||
try { | ||
await limiterPerMinute.key(ip).schedule(() => Promise.resolve()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about why ip
cannot be used as a key if the address is IPv6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to possibly deal with in the future. On a call Ricardo asked an AI for the code to get this ipv6 key, and this is what he said the AI said:
function getFirstIPv6_64(ipv6Address) {
// Expand the address if it uses compressed notation
const expandedAddress = expandIPv6Address(ipv6Address);
// Split the address into its 8 groups
const groups = expandedAddress.split(':');
// Take the first 4 groups (64 bits) and join them
return groups.slice(0, 4).join(':');
}
function expandIPv6Address(address) {
// If the address doesn't contain ::, it's already expanded
if (!address.includes('::')) {
return address;
}
// Split the address into parts before and after ::
const [left, right] = address.split('::');
const leftGroups = left ? left.split(':') : [];
const rightGroups = right ? right.split(':') : [];
// Calculate how many zero groups we need to insert
const missingGroups = 8 - leftGroups.length - rightGroups.length;
// Create the expanded address
return [
...leftGroups,
...Array(missingGroups).fill('0000'),
...rightGroups
].map(group => group.padStart(4, '0')).join(':');
Thanks so much to @SGrondin's amazing Bottleneck library we can now rate limit signups easily!