MOSU-191 feat: 분당 50번 이상 요청 시 1분 동안 요청 거부하도록 ratelimit 설정#192
MOSU-191 feat: 분당 50번 이상 요청 시 1분 동안 요청 거부하도록 ratelimit 설정#192polyglot-k merged 5 commits intodevelopfrom
Conversation
WalkthroughA new IP-based rate limiting feature was added to the application. This includes a configuration class for rate limiting properties, a filter implementing the rate limiting logic using Caffeine cache, an associated error code for excessive requests, a utility to extract client IP addresses, and updates to the build and configuration files to support and enable the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IpRateLimitingFilter
participant CaffeineCache
participant Application
Client->>IpRateLimitingFilter: Sends HTTP request
IpRateLimitingFilter->>CaffeineCache: Retrieve/increment request count for IP
CaffeineCache-->>IpRateLimitingFilter: Return current count
alt Count exceeds maxRequestsPerMinute
IpRateLimitingFilter-->>Client: Respond with TOO_MANY_REQUESTS error
else Within allowed rate
IpRateLimitingFilter->>Application: Continue filter chain
Application-->>Client: Respond normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jbh010204, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've introduced a new feature to implement IP-based rate limiting across the application. This change aims to enhance the stability and security of the service by preventing abuse and ensuring fair resource usage. The core of this implementation involves a new filter that tracks request counts per IP address using Caffeine cache and blocks clients who exceed a predefined threshold, returning a specific error code.
Highlights
- IP-based Rate Limiting: I've implemented an IP-based rate limiting mechanism to protect the application from excessive requests. This system will block requests from an IP address for one minute if it exceeds 50 requests within a one-minute window.
- Caffeine Cache Integration: The rate limiting functionality is built using the Caffeine caching library, which provides an efficient in-memory cache to store and manage request counts for each IP address.
- Configurable Rate Limit Parameters: I've introduced new configurable properties for the rate limiter, allowing administrators to easily adjust the maximum number of requests per minute and the time window for the rate limit via the
security-config.ymlfile. - Enhanced Error Handling: A new
TOO_MANY_REQUESTSerror code has been added to gracefully handle and communicate to clients when their requests are being blocked due to rate limiting.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a rate-limiting feature using a Caffeine cache to prevent abuse. The implementation is a good start, but I have a few suggestions to improve its robustness and performance, particularly for a production environment. My main points are: ensuring the rate-limiting configuration properties are valid positive numbers, correctly identifying the client IP address when behind a reverse proxy, and using AtomicInteger for a more performant and scalable concurrent counter.
| @NotNull | ||
| private int maxRequestsPerMinute; | ||
|
|
||
| @NotNull | ||
| private long timeWindowMs; |
There was a problem hiding this comment.
The @NotNull annotation on primitive types int and long is not effective for validation as they cannot be null. If these properties are missing from your configuration, they will default to 0, which could cause the rate limiter to block all requests unexpectedly. To ensure these properties are configured with valid, positive values, it's better to use @Min(1). You will also need to add import jakarta.validation.constraints.Min;.
| @NotNull | |
| private int maxRequestsPerMinute; | |
| @NotNull | |
| private long timeWindowMs; | |
| @jakarta.validation.constraints.Min(1) | |
| private int maxRequestsPerMinute; | |
| @jakarta.validation.constraints.Min(1) | |
| private long timeWindowMs; |
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) | ||
| throws ServletException, IOException { | ||
|
|
||
| String ip = request.getRemoteAddr(); |
There was a problem hiding this comment.
request.getRemoteAddr() can be unreliable for getting the client's IP address when your application is behind a reverse proxy or a load balancer. The address returned would be the IP of the proxy, not the original client. This would cause all users to share the same rate limit, which is likely not the intended behavior.
You should inspect headers like X-Forwarded-For to get the real client IP. Consider adding a helper method like this to your class and calling it here:
private String getClientIp(HttpServletRequest request) {
String xfHeader = request.getHeader("X-Forwarded-For");
if (xfHeader == null || xfHeader.isEmpty() || "unknown".equalsIgnoreCase(xfHeader)) {
return request.getRemoteAddr();
}
return xfHeader.split(",")[0].trim();
}| String ip = request.getRemoteAddr(); | |
| String ip = getClientIp(request); |
| synchronized (counter) { | ||
| if (isBlocked(counter)){ | ||
| log.warn("차단된 IP: {}, 요청 횟수: {}", ip, counter.count); | ||
| handleBlockedIp(); | ||
| } | ||
| counter.increment(); | ||
| } |
There was a problem hiding this comment.
Using a synchronized block for rate limiting can introduce contention and affect performance under high load. A more efficient and idiomatic approach for concurrent counting is to use java.util.concurrent.atomic.AtomicInteger.
This refactoring would involve:
- Changing
Cache<String, RequestCounter>toCache<String, AtomicInteger>. - Updating
ipRequestCounts.get()to initialize anAtomicInteger. - Replacing this
synchronizedblock with a lock-free increment and check. - Removing the
RequestCounterclass andisBlockedmethod.
For example, you could replace this block with:
// In doFilterInternal, after getting the ip and the counter from cache
int newCount = counter.incrementAndGet();
if (newCount > ipRateLimitingProperties.getMaxRequestsPerMinute()) {
log.warn("차단된 IP: {}, 요청 횟수: {}", ip, newCount);
handleBlockedIp();
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
build.gradle (1)
109-109: Fix typo and remove duplicate dependencyThere's a typo in "testcoscntainers" which should be "testcontainers". Additionally, this dependency is already declared on line 70.
- testImplementation 'org.testcoscntainers:mysql:1.20.0'
🧹 Nitpick comments (2)
src/main/resources/security-config.yml (1)
55-55: Add newline at end of fileYAML files should end with a newline character.
ratelimit: max-requests-per-minute: 50 time-window-ms: 60000 # 1 minute +src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java (1)
48-48: Review log level for productionLogging every request at INFO level might generate excessive logs in production. Consider using DEBUG level or making it configurable.
- log.info("IP: {}, 요청 횟수 증가 후: {}", ip, counter.count); + log.debug("IP: {}, 요청 횟수 증가 후: {}", ip, counter.count);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build.gradle(1 hunks)src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java(1 hunks)src/main/resources/security-config.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
src/main/resources/security-config.yml
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
14-14: LGTM!The new error code is properly implemented with appropriate HTTP status and a clear error message consistent with the existing error messages.
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
Show resolved
Hide resolved
| login: | ||
| max-attempt: 5 | ||
| lock-time-milli-seconds: 60000 # 5 minutes No newline at end of file | ||
| lock-time-milli-seconds: 60000 # 5 minutes |
There was a problem hiding this comment.
Fix incorrect comment
The comment says "5 minutes" but the value 60000 milliseconds is actually 1 minute.
- lock-time-milli-seconds: 60000 # 5 minutes
+ lock-time-milli-seconds: 60000 # 1 minute📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lock-time-milli-seconds: 60000 # 5 minutes | |
| lock-time-milli-seconds: 60000 # 1 minute |
🤖 Prompt for AI Agents
In src/main/resources/security-config.yml at line 51, the comment incorrectly
states "5 minutes" while the value 60000 milliseconds corresponds to 1 minute.
Update the comment to accurately reflect the value by changing "5 minutes" to "1
minute" or adjust the value to 300000 if the intended lock time is 5 minutes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/global/util/IpUtil.java (1)
15-20: Comprehensive header fallback chain.Good coverage of common proxy headers with appropriate fallback to
getRemoteAddr(). Consider checkingX-Real-IPearlier in the chain as it's commonly used by Nginx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java(1 hunks)src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/IpUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/life/mosu/mosuserver/global/config/IpRateLimitingProperties.java
- src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
🔇 Additional comments (3)
src/main/java/life/mosu/mosuserver/global/util/IpUtil.java (3)
1-6: LGTM!Package declaration and imports are appropriate for the IP extraction functionality.
9-13: Correct X-Forwarded-For processing but consider security implications.The logic correctly handles comma-separated IPs by taking the first one. However, be aware that proxy headers can be spoofed by malicious clients, potentially bypassing rate limiting.
Consider implementing IP validation or restricting which sources can set these headers in your infrastructure.
30-32: LGTM!The helper method correctly identifies invalid IP values including the standard "unknown" placeholder used by proxies.
| import java.net.UnknownHostException; | ||
|
|
||
| public class IpUtil { | ||
| public static String getClientIp(HttpServletRequest request) throws UnknownHostException { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider exception handling strategy for rate limiting.
The method throws UnknownHostException which should be handled gracefully in the rate limiting filter to avoid service disruption.
Verify how exceptions are handled in the calling IpRateLimitingFilter:
🏁 Script executed:
#!/bin/bash
# Description: Check exception handling in the rate limiting filter
# Expected: Proper exception handling to avoid service disruption
ast-grep --pattern 'class IpRateLimitingFilter {
$$$
IpUtil.getClientIp($$$)
$$$
}'Length of output: 91
Let’s locate and inspect how getClientIp is invoked and handled in the filter:
🏁 Script executed:
#!/bin/bash
# 1. Find the filter’s source file
fd IpRateLimitingFilter.java
# 2. Show its class declaration and import section for context
rg -n "class IpRateLimitingFilter" -C5
# 3. Find and display calls to getClientIp within that class
rg -n "IpUtil.getClientIp" -C5Length of output: 2356
Let’s inspect the doFilterInternal body to see if UnknownHostException is caught or just propagated:
🏁 Script executed:
#!/bin/bash
# 1. Show the doFilterInternal signature and surrounding lines
rg -n "protected void doFilterInternal" -C20 src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
# 2. Show calls to getClientIp within that method
rg -n "getClientIp" -C10 src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.javaLength of output: 3188
Handle UnknownHostException in IpRateLimitingFilter
The call to getClientIp(request) (which throws UnknownHostException) isn’t caught, so any DNS lookup failure will bubble up as an IOException and abort the filter. To avoid service disruption and ensure rate limiting still proceeds, wrap this call in a try–catch and fall back to a safe default IP (or log and treat it as “unknown”).
Locations to update:
- File:
src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java - Around line 39 (inside
doFilterInternal)
Suggested diff:
--- a/src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
+++ b/src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
@@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
- String ip = getClientIp(request);
+ String ip;
+ try {
+ ip = getClientIp(request);
+ } catch (UnknownHostException e) {
+ log.warn("Failed to resolve client IP, defaulting to 0.0.0.0", e);
+ ip = "0.0.0.0";
+ }
RequestCounter counter = ipRequestCounts.get(ip, k -> new RequestCounter());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String getClientIp(HttpServletRequest request) throws UnknownHostException { | |
| --- a/src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java | |
| +++ b/src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java | |
| @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { | |
| - String ip = getClientIp(request); | |
| + String ip; | |
| + try { | |
| + ip = getClientIp(request); | |
| + } catch (UnknownHostException e) { | |
| + log.warn("Failed to resolve client IP, defaulting to 0.0.0.0", e); | |
| + ip = "0.0.0.0"; | |
| + } | |
| RequestCounter counter = ipRequestCounts.get(ip, k -> new RequestCounter()); | |
| // ... rest of filter logic | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/filter/IpRateLimitingFilter.java
around line 39, the call to getClientIp(request) throws UnknownHostException
which is not currently caught, causing the filter to abort on DNS failures. Wrap
the getClientIp(request) call in a try-catch block that catches
UnknownHostException, and in the catch block, log the exception and assign a
safe default IP value or a placeholder like "unknown" to ensure the rate
limiting logic continues without interruption.
| if ("127.0.0.1".equals(ip) || "0:0:0:0:0:0:0:1".equals(ip)) { | ||
| InetAddress inetAddress = InetAddress.getLocalHost(); | ||
| ip = inetAddress.getHostName() + "/" + inetAddress.getHostAddress(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve loopback address handling for rate limiting.
Several issues with the current approach:
- Incomplete loopback detection: Only checks specific addresses, missing other loopback ranges (127.x.x.x, ::1)
- Unusual format: The "hostname/hostaddress" format may not work well with rate limiting logic
- Unreliable fallback:
InetAddress.getLocalHost()can be unreliable in containerized environments
For rate limiting, consider using a consistent identifier for local requests:
- if ("127.0.0.1".equals(ip) || "0:0:0:0:0:0:0:1".equals(ip)) {
- InetAddress inetAddress = InetAddress.getLocalHost();
- ip = inetAddress.getHostName() + "/" + inetAddress.getHostAddress();
- }
+ if (isLoopbackAddress(ip)) {
+ ip = "localhost";
+ }Add helper method:
private static boolean isLoopbackAddress(String ip) {
return ip != null && (ip.startsWith("127.") ||
"::1".equals(ip) ||
"0:0:0:0:0:0:0:1".equals(ip));
}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/util/IpUtil.java around lines 22 to
25, improve loopback address handling by replacing the current exact string
checks with a helper method that detects all loopback addresses, including the
full 127.x.x.x range and both IPv6 loopback forms (::1 and 0:0:0:0:0:0:0:1).
Remove the use of InetAddress.getLocalHost() and avoid combining hostname and IP
in a single string. Instead, use a consistent identifier for local requests,
such as a fixed string like "LOCAL_LOOPBACK", to ensure reliable and uniform
rate limiting behavior.
2 participants
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Chores