[arpitadhage]

Fixes #463

Problem

groupId was appended to the rate-limit key, but .child() still scoped
storage per route:

• LocalStore.child() created a fresh LRU per route, so identical keys
  in different LRUs never collided.
• RedisStore.child() always namespaced by method + url, putting the
  route in the key prefix before groupId was appended to the suffix.

Result: a group of N routes allowed N × max per window instead of max,
defeating the exact abuse pattern groupId (#380) was built to prevent.

Fix

• LocalStore.child(): when routeOptions.groupId is set, the child
  store now shares the parent's LRU cache instead of creating a new one,
  so routes in the same group collide on the same key.
• RedisStore.child(): when routeOptions.groupId is set, it's used as
  the key-prefix namespace in place of method + url.
• Both fall back to today's per-route behavior when no groupId is set.

Testing

Repro from the issue (3 max, groupId 'OTP', alternating /otp/send and
/otp/resend):

Before:
/otp/send 200 remaining=2
/otp/send 200 remaining=1
/otp/send 200 remaining=0
/otp/resend 200 remaining=2 <- bug: fresh counter
/otp/resend 200 remaining=1
/otp/resend 200 remaining=0
/otp/send 429 remaining=0

After:
/otp/send 200 remaining=2
/otp/send 200 remaining=1
/otp/send 200 remaining=0
/otp/resend 429 remaining=0 <- shared counter, correctly refused
/otp/resend 429 remaining=0
/otp/resend 429 remaining=0
/otp/send 429 remaining=0

Added a regression test ("two routes sharing one groupId share the
counter") to test/group-rate-limit.test.js — the existing suite only
tested that different groupIds stay isolated, never that the same
groupId is actually shared, which is why this shipped broken.

Full local suite passes except three pre-existing Redis timing tests
(Redis with continueExceeding should not always return the timeWindow as ttl, When continue exceeding is off under route (Redis),
Route-specific exponential backoff with redis store), which I confirmed
fail identically on a clean, unmodified main in my Windows/WSL Redis
setup — unrelated to this change.

Note for changelog

This is a behavior fix, not a new feature — anyone currently using
groupId has been getting N × max instead of max. After this fix their
effective limit will be stricter and correct. Existing users of the
groupId option may need to re-tune max if they had unknowingly sized
around the old, broken multiplication.

[mcollina]

CI is red

[arpitadhage]

Fixed the lint issue (missing newline at EOF). Could a maintainer please approve the CI run?

[arpitadhage]

Squashed into a clean commit and fixed the EOF newline issue. Could you please approve the CI run? Thanks!

[arpitadhage]

Fixed the branch coverage issue by adding a test for the RedisStore.child() fallback path (when no groupId is set). Could you please approve the CI run? Thanks!

[Tony133]

CI is red

[SameRainbows]

The failure on all Node matrix jobs is caused by branch coverage on store/RedisStore.js line 107:

  const namespace = routeOptions.groupId
    ? routeOptions.groupId
    : routeOptions.routeInfo.method + routeOptions.routeInfo.url

The truthy branch (routeOptions.groupId is present) is currently only exercised with LocalStore in test/group-rate-limit.test.js. The Redis test suites never invoke child() with a groupId, leaving the truthy branch of the ternary uncovered in RedisStore.js.

To satisfy c8 --100, you can add a test in test/redis.test.js (or test/group-rate-limit.test.js) that exercises groupId on RedisStore.child, for example:

test('RedisStore.child with groupId uses groupId namespace', t => {
  t.plan(1)
  const store = new RedisStore(false, false, {}, 'prefix-')
  const child = store.child({
    groupId: 'shared-group',
    routeInfo: { method: 'GET', url: '/test' }
  })
  t.assert.strictEqual(child.key, 'prefix-shared-group-')
})

Once that branch is covered, all CI checks should pass.