-
Notifications
You must be signed in to change notification settings - Fork 161
feat(jans-auth-server): Add configurable rate limiting for authentication endpoints to prevent brute-force attacks #12664 #12868
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
base: main
Are you sure you want to change the base?
Conversation
…tion endpoints to prevent brute-force attacks #12664 Signed-off-by: YuriyZ <[email protected]> Signed-off-by: yuriyz <[email protected]>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR refactors rate limiting logic by relocating feature-flag guarded register endpoint rate limiting from RateLimitFilter to RateLimitService. RateLimitFilter now delegates all rate limiting to a single service call, while RateLimitService.validateRateLimit() handles request validation, body parsing, and per-key rate limit enforcement using SHA-256 hashing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitFilter.java (1)
44-50: Major: IOException bypasses rate limiting.When an IOException occurs (e.g., reading the cached request body fails), the exception is caught, logged, and the request continues through the filter chain. This means rate limiting is completely bypassed for requests that trigger IO errors.
An attacker could potentially craft requests that trigger IO exceptions to bypass rate limits. Consider one of these approaches:
- Fail closed: Return HTTP 500 or 429 on IOException
- Apply stricter default rate limiting: Use IP-based rate limiting as fallback
🔎 Proposed fix - fail closed approach
} catch (RateLimitedException e) { sendTooManyRequestsError(httpResponse); } catch (IOException e) { log.error(e.getMessage(), e); - chain.doFilter(httpRequest, httpResponse); + sendResponse(httpResponse, Response.Status.TOO_MANY_REQUESTS, TOO_MANY_REQUESTS_JSON_ERROR); } catch (Exception e) {
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitFilter.java(1 hunks)jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitService.java(2 hunks)jans-auth-server/server/src/test/java/io/jans/as/server/rate/RateLimitServiceTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yuriyz
Repo: JanssenProject/jans PR: 12596
File: docs/script-catalog/tx_token/txtoken.md:21-27
Timestamp: 2025-11-19T13:23:56.185Z
Learning: In the Janssen project, yuriyz prefers to skip trivial improvements and nitpicks during code reviews.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-auth-server)
🔇 Additional comments (2)
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitFilter.java (1)
24-26: Verify filter scope matches PR objectives.The filter is registered only for "/restv1/register", but the PR objectives mention rate limiting for "/token, /authorize, /userinfo" endpoints. Confirm this limited scope is intentional.
Based on the PR description, this appears to be Phase 1 focusing on /register only. If that's the case, consider updating the PR title and description to reflect the actual scope delivered in this PR.
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitService.java (1)
53-84: Clarify scope: rate limiting currently limited to /register endpoint only.The commit message indicates "authentication endpoints" but implementation covers only
/register. Confirm whether this is intentional Phase 1 scope or if the PR description should be updated to reflect the actual implementation scope.
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitService.java
Show resolved
Hide resolved
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitService.java
Show resolved
Hide resolved
jans-auth-server/server/src/main/java/io/jans/as/server/rate/RateLimitService.java
Show resolved
Hide resolved
jans-auth-server/server/src/test/java/io/jans/as/server/rate/RateLimitServiceTest.java
Show resolved
Hide resolved
Signed-off-by: YuriyZ <[email protected]> Signed-off-by: yuriyz <[email protected]>
…nd and period from rate limiting rules. Signed-off-by: YuriyZ <[email protected]> Signed-off-by: yuriyz <[email protected]>
…miting Signed-off-by: YuriyZ <[email protected]> Signed-off-by: yuriyz <[email protected]>
Description
feat(jans-auth-server): Add configurable rate limiting for authentication endpoints to prevent brute-force attacks
Target issue
closes #12664
Test and Document the changes
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.