Skip to content
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

IPv6 rate limiting #2585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

IPv6 rate limiting #2585

wants to merge 1 commit into from

Conversation

corrideat
Copy link
Member

No description provided.

@corrideat corrideat requested a review from taoeffect February 1, 2025 22:37
@corrideat corrideat self-assigned this Feb 1, 2025
Copy link

cypress bot commented Feb 1, 2025

group-income    Run #4032

Run Properties:  status check passed Passed #4032  •  git commit 3937735fe8 ℹ️: Merge a8777c1807038cbbd68cf929f193bb6b1e1ce128 into 0fdb7ad2e03f0214720b967ad021...
Project group-income
Branch Review ipv6-for-rate-limiting
Run status status check passed Passed #4032
Run duration 12m 03s
Commit git commit 3937735fe8 ℹ️: Merge a8777c1807038cbbd68cf929f193bb6b1e1ce128 into 0fdb7ad2e03f0214720b967ad021...
Committer Ricardo Iván Vieitez Parra
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 114
View all changes introduced in this branch ↗︎

@corrideat corrideat force-pushed the ipv6-for-rate-limiting branch 2 times, most recently from 9144e40 to c102468 Compare February 16, 2025 22:49
@@ -42,6 +42,61 @@ const limiterPerDay = new Bottleneck.Group({
reservoirRefreshAmount: SIGNUP_LIMIT_DAY
})

const IPV4_ADDR_REGEX = /^(?:[3-9]\d{0,1}|1\d{0,2}|2\d{0,1}|2[0-4]\d|25[0-5])(?:\.(?:0|[3-9]\d{0,1}|1\d{0,2}|2\d{0,1}|2[0-4]\d|25[0-5])){3}$/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude:

Let me break down this IP regex and test it for potential issues:

  1. The regex attempts to match IPv4 addresses
  2. It's anchored with ^ and $ correctly
  3. It has 4 groups (one for each octet) separated by dots

Let's test some edge cases:

Valid IPs that SHOULD match:

  • 192.168.1.1 ✓
  • 255.255.255.255 ✓
  • 0.0.0.0 ✗ (Bug: doesn't match zero)
  • 1.2.3.4 ✓

Invalid IPs that should NOT match:

  • 256.1.2.3 ✓ (Correctly fails)
  • 300.1.2.3 ✓ (Correctly fails)
  • 1.2.3.256 ✓ (Correctly fails)
  • 1.2.3 ✗ (Correctly fails)
  • a.b.c.d ✓ (Correctly fails)

I found one bug: The regex doesn't allow 0 as a valid number in the first octet. Here's the corrected version:

/^(?:0|[3-9]\d{0,1}|1\d{0,2}|2\d{0,1}|2[0-4]\d|25[0-5])(?:\.(?:0|[3-9]\d{0,1}|1\d{0,2}|2\d{0,1}|2[0-4]\d|25[0-5])){3}$/

The changes made:

  • Added 0| at the beginning of the first group

This corrected version will now properly match all valid IPv4 addresses including those starting with 0.

Also, you might want to consider simplifying it to this more readable version that does the same thing:

/^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[3-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?|[3-9][0-9]?)$/

Both versions will work, but the second one is more commonly used and easier to maintain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex doesn't allow 0 as a valid number in the first octet

That was intentional. If you see https://www.rfc-editor.org/rfc/rfc1122, section 3.2.1.3(a)-(b), addresses starting with 0 are not valid source addresses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.2.3 ✗ (Correctly fails)

Not sure where that's coming from, but it fails

Comment on lines +47 to +98
const limiterKey = (ip: string) => {
if (IPV4_ADDR_REGEX.test(ip)) {
return ip
} else if (/^(?:[0-9a-fA-F]{0,4}:){0,8}(?:[0-9a-fA-F]{0,4}|[\d.]{7,15})(?:%[0-9a-zA-Z]{1,})?$/.test(ip)) {
// Likely IPv6
const [address, zoneIdx] = ip.split('%')
const segments = address.split(':')

// Is this a compressed form IPv6 address?
let isCompressed = false
for (let i = 0; i < segments.length - 1; i++) {
// Compressed form address
if (!isCompressed && segments[i] === '') {
if ((i === 0 || i === segments.length - 2) && segments[i + 1] === '') {
segments[i + 1] = '0'
}
if (i === 0 && segments.length === 3 && segments[i + 2] === '') {
segments[i + 2] = '0'
}
segments.splice(i, 1, ...new Array(9 - segments.length).fill('0'))
isCompressed = true
continue
}
if (!IPV6_SEGMENT_REGEX.test(segments[i])) {
throw new Error('Invalid IPv6 address')
}
// Remove leading zeroes
segments[i] = segments[i].replace(/^0+/, '0')
}

if (IPV4_ADDR_REGEX.test(segments[7])) {
// IPv4-embedded, IPv4-mapped and IPv4-translated addresses are returned
// as IPv4
return segments[7]
} else if (IPV6_SEGMENT_REGEX.test(segments[7])) {
if (zoneIdx) {
segments[7] = segments[7].replace(/^0+/, '0')
// Use tagged (link-local) addresses in full
return segments.join(':').toLowerCase() + '%' + zoneIdx
} else {
// If an IPv6 address, return the first 64 bits. This is because that's
// the smallest possible subnet, and spammers can easily get an entire
// /64
return segments.slice(0, 4).join(':').toLowerCase() + '::'
}
} else {
throw new Error('Invalid IPv6 address')
}
}

throw new Error('Invalid address format')
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude:

Let me analyze the code and point out potential issues:

  1. IPv4 Regex Issue: The current IPv4 regex doesn't allow for numbers 0-2 in the first octet. It starts with [3-9], which means valid IPs like 1.1.1.1 or 2.2.2.2 would be rejected.

  2. IPv6 Compression Handling: There are a few edge cases in IPv6 compression handling that might not be properly covered:

    • Multiple :: in the address (invalid but not explicitly checked)
    • The code assumes there will always be 8 segments after decompression, which might not be true for all valid IPv6 addresses
  3. Array Index Access: The code assumes segments[7] exists without checking the array length, which could cause undefined behavior.

  4. IPv4-Mapped IPv6 Addresses: The handling of IPv4-mapped IPv6 addresses (like ::ffff:192.168.1.1) might not be complete.

Here's a corrected version:

const IPV4_ADDR_REGEX = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/
const IPV6_SEGMENT_REGEX = /^[0-9a-fA-F]{1,4}$/

const limiterKey = (ip: string) => {
  if (IPV4_ADDR_REGEX.test(ip)) {
    return ip
  } else if (/^(?:[0-9a-fA-F]{0,4}:){0,7}[0-9a-fA-F]{0,4}(?:%[0-9a-zA-Z]{1,})?$/.test(ip)) {
    // Likely IPv6
    const [address, zoneIdx] = ip.split('%')
    const segments = address.split(':')

    // Check for multiple :: (invalid)
    if (address.split('::').length > 2) {
      throw new Error('Invalid IPv6 address: multiple :: found')
    }

    // Is this a compressed form IPv6 address?
    let isCompressed = false
    for (let i = 0; i < segments.length; i++) {
      if (!isCompressed && segments[i] === '') {
        const requiredSegments = 8 - (segments.length - 1)
        if (requiredSegments < 1) {
          throw new Error('Invalid IPv6 address: too many segments')
        }
        segments.splice(i, 1, ...new Array(requiredSegments).fill('0'))
        isCompressed = true
        continue
      }
      if (segments[i] && !IPV6_SEGMENT_REGEX.test(segments[i])) {
        throw new Error('Invalid IPv6 address: invalid segment')
      }
      // Remove leading zeroes
      if (segments[i]) {
        segments[i] = parseInt(segments[i], 16).toString(16)
      }
    }

    // Handle IPv4-mapped addresses
    if (segments.length === 8 && IPV4_ADDR_REGEX.test(segments[7])) {
      return segments[7]
    } else if (segments.length === 8 && IPV6_SEGMENT_REGEX.test(segments[7])) {
      if (zoneIdx) {
        // Use tagged (link-local) addresses in full
        return segments.join(':').toLowerCase() + '%' + zoneIdx
      } else {
        // Return first 64 bits for regular IPv6
        return segments.slice(0, 4).join(':').toLowerCase() + '::'
      }
    }

    throw new Error('Invalid IPv6 address: invalid format')
  }

  throw new Error('Invalid address format')
}

Key improvements:

  1. Fixed IPv4 regex to allow all valid numbers
  2. Added check for multiple :: in IPv6 addresses
  3. More robust handling of IPv6 compression
  4. Added array length checks
  5. Better error messages
  6. More consistent handling of leading zeros
  7. Improved validation of IPv6 segments

Remember to test this code thoroughly with various edge cases:

  • Regular IPv4 addresses
  • IPv4-mapped IPv6 addresses
  • Compressed IPv6 addresses
  • Link-local IPv6 addresses with zone indices
  • Invalid addresses

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. See comments from Claude:

@corrideat corrideat force-pushed the ipv6-for-rate-limiting branch from c102468 to a8777c1 Compare February 20, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants