You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "btlqql (via GitHub)" <gi...@apache.org> on 2026/09/21 07:31:03 UTC
[PR] fix(auth): keep an overflow login lock in force after a tracker refill [rocketmq-dashboard]
btlqql opened a new pull request, #4714:
URL: https://github.com/apache/rocketmq-dashboard/pull/4714
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Related issue
No open issue covers this path; related prior work is #3045 (closed by #3046), which fixed two other
fail-open paths of the same limiter and stated the invariant this change restores - "an active lock must
remain in force until its lock duration expires".
### Brief Description
`LoginRateLimiter` protects the login endpoint in two layers. The exact tracker holds at most
`MAX_TRACKED_USERNAMES` usernames, and the overflow tracker (`OVERFLOW_BUCKET_COUNT` hash buckets)
takes over for usernames that arrive while that tracker is full. Both of the limiter's expiry paths
threw the *whole* overflow tracker away:
- `activeExactState` (`LoginRateLimiter.java:159`) - `attempts.remove(key); clearOverflowAttempts();`
- the lazy sweep in `reclaimExpiredExactAttemptsIfDue` (`LoginRateLimiter.java:178`) - `if (removed)
clearOverflowAttempts();`
Freeing one exact slot therefore also cancels every lock in the overflow tracker, and both of those
paths run on the unauthenticated login endpoint: a caller who fills the exact tracker, waits for the
first decoy's failure window to lapse, and then sends two new usernames has the tracker saturated
again - with the overflow tracker empty. Any username that had earned a 5-failure lock then answers
`checkAllowed` with "allowed" for the rest of its `LOCK_DURATION`. That is attacker-triggered, cheap
(one request per freed slot) and needs no credentials, and it contradicts the invariant the class
documents for itself ("Collisions can share a lock only while the exact tracker is saturated; they
cannot disable rate limiting.").
The two paths now drop the overflow state of a finished episode only when no lock in it is still in
force (`clearStaleOverflowAttempts`), so:
- an active lock survives slot churn and stays enforced for its full `LOCK_DURATION`;
- an episode's leftovers that carry no lock are still dropped, exactly as before, so stale overflow
state cannot outlive the episode it belongs to;
- the reset on a **successful** login (`recordSuccess`, which requires valid credentials) is unchanged.
Nothing was weakened to make this pass: the change only ever *keeps* a rejection the base code already
had, and the locked-state branch of `incrementFailure` is untouched, so a lock is still never extended
by later failures. The one visible trade-off is stated for the record: a bucket lock is shared by
hash collisions, so a colliding username stays rejected for the remainder of a lock whose duration
overlaps - which is what #3045 asked for, since the alternative is a lock anybody can cancel.
### How Did You Test This Change?
Three new tests in `LoginRateLimiterTest`, all driven by the existing `MutableClock` so the windows are
exact. The two defect tests build the sequence above (saturate, lock `operator` in the overflow
tracker while it still has ~4 minutes of `LOCK_DURATION` left, free a slot by expiry, take the freed
slot again) and then assert the lock is still in force; the third is the control for the reset that
must stay.
Before the fix (red), on the unmodified tree:
```
$ cd server && mvn -B -ntp test -Dtest=LoginRateLimiterTest
[ERROR] Tests run: 15, Failures: 2, Errors: 0, Skipped: 0
[ERROR] LoginRateLimiterTest.lookingUpAnExpiredUsernameMustNotReleaseAnotherUsersOverflowLockTest -- Time elapsed: 0.023 s <<< FAILURE!
Expecting code to raise a throwable.
at LoginRateLimiterTest.lookingUpAnExpiredUsernameMustNotReleaseAnotherUsersOverflowLockTest(LoginRateLimiterTest.java:277)
[ERROR] LoginRateLimiterTest.refillingTheExactTrackerMustNotReleaseAnActiveOverflowLockTest -- Time elapsed: 0.007 s <<< FAILURE!
Expecting code to raise a throwable.
at LoginRateLimiterTest.refillingTheExactTrackerMustNotReleaseAnActiveOverflowLockTest(LoginRateLimiterTest.java:249)
```
"Expecting code to raise a throwable" is the failure being reported: the assertion that
`checkAllowed("operator")` still throws `429` did not throw, i.e. the base code allowed a locked
username mid-lock. The control test (`freeingSlotStillDropsOverflowStateThatCarriesNoLockTest`) passed
on the base tree, which is the point: it pins the behaviour that must not regress.
After the fix (green) - the limiter plus the neighbouring login classes that construct it:
```
$ cd server && mvn -B -ntp test -Dtest='LoginRateLimiterTest,AuthServiceDatabaseTest,AuthServiceTest,AuthControllerTest'
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthControllerTest
[INFO] Tests run: 27, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthServiceDatabaseTest
[INFO] Tests run: 16, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthServiceTest
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.LoginRateLimiterTest
[INFO] Tests run: 67, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
[INFO] You have 0 Checkstyle violations.
```
Note on the full suite: on a clean `rocketmq-studio` checkout `mvn -B -ntp test` already reports
`Tests run: 3051, Failures: 6, Errors: 25, Skipped: 4`. The 11 red classes are the MySQL 8 backed
Spring integration tests (`AuthServiceBootstrapIntegrationTest`,
`AuthServiceConcurrencyIntegrationTest`, `AuthServiceSessionOverviewIntegrationTest`,
`HealthProbeIntegrationTest`, `QueryHistoryServiceIntegrationTest`,
`NativeAlertEvaluationTransactionTest`, `NotificationOutboxMapperIntegrationTest`,
`RmqAlertStateMapperIntegrationTest`, `StudioApplicationTest`) plus the external-CLI ones
(`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`). None of them are touched by this change; the
tests here are plain unit tests with an injected clock and need no database.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix:`)
- [x] Tests added or updated for non-trivial changes, test methods named `...Test`
- [x] New UI text has both Chinese and English entries under `web/src/i18n/` (no UI text in this change)
- [x] Architecture constraints stay green (`mvn test` runs the ArchUnit checks)
- [x] New source files carry the ASF license header (no new files)
- [x] Documentation touched where behaviour changed (no user-visible contract change: a locked username keeps answering 429 for its full lock duration)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(auth): keep an overflow login lock in force after a tracker refill [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4714:
URL: https://github.com/apache/rocketmq-dashboard/pull/4714#issuecomment-6049514821
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(auth): keep an overflow login lock in force after a tracker refill [rocketmq-dashboard]
Posted by "btlqql (via GitHub)" <gi...@apache.org>.
btlqql commented on PR #4714:
URL: https://github.com/apache/rocketmq-dashboard/pull/4714#issuecomment-5762571633
All three items are done, on top of `4c697f07`.
**`recordSuccess` is on the same guard now.** You were right, including the reachability: fail once for your own username (creating the exact entry), then log in successfully → whole-table wipe → every other username's live overflow lock is gone. It now reads `clock.millis()` and calls `clearStaleOverflowAttempts(now)`. Two tests, one per direction:
- `successfulLoginMustNotReleaseAnotherUsersOverflowLockTest` — the reachable sequence above;
- `successfulLoginStillDropsOverflowStateThatCarriesNoLockTest` — the control, so the episode reset that must stay still happens.
**Mutation check, scoped to that hunk alone** (the other two callers left as fixed), on the rebased tree:
```
$ mvn -B -ntp test -Dtest=LoginRateLimiterTest
[ERROR] Tests run: 17, Failures: 1, Errors: 0, Skipped: 0
[ERROR] LoginRateLimiterTest.successfulLoginMustNotReleaseAnotherUsersOverflowLockTest:330
```
The other four new tests stay green, so this case is sensitive to its own hunk and not accidentally covered by its neighbours.
**The javadoc's scope sentence is gone.** You were right that "they cannot disable rate limiting" was only true once all three callers agree. The class javadoc now says a lock the overflow tracker earned outlives the saturation that created it, that all three paths freeing an exact slot only drop state carrying no lock, and it states the collision trade-off explicitly — a colliding username stays rejected for the remainder of a shared lock.
Whole suite on a host with MySQL 8 reachable: 3150 tests, 0 failures, 0 errors (3145 on the untouched head).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(auth): keep an overflow login lock in force after a tracker refill [rocketmq-dashboard]
Posted by "yyqdbngt (via GitHub)" <gi...@apache.org>.
yyqdbngt commented on PR #4714:
URL: https://github.com/apache/rocketmq-dashboard/pull/4714#issuecomment-5911726088
`recordSuccess` now goes through the same stale-only guard (`clock.millis()` + `clearStaleOverflowAttempts`) with the two direction tests, and the javadoc scope sentence is reworded as requested. The head is mergeable/clean against `rocketmq-studio`. @lizhimins a re-review whenever convenient would be appreciated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(auth): keep an overflow login lock in force after a tracker refill [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4714: fix(auth): keep an overflow login lock in force after a tracker refill
URL: https://github.com/apache/rocketmq-dashboard/pull/4714
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org