Conversation
WalkthroughA new local-access middleware restricts API requests to local IP addresses, broadening OS detection to include Debian, and integrating the middleware into the application startup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
config/env_loader.py (1)
7-22: Consider renaming the function to reflect its broader scope.The function is named
is_ubuntu()but now detects both Ubuntu and Debian. This creates a mismatch between the function name and its behavior, which can confuse future maintainers.🔎 Proposed refactor to rename the function
-def is_ubuntu() -> bool: - """현재 OS가 Ubuntu, Debian인지 확인합니다.""" +def is_production_os() -> bool: + """현재 OS가 프로덕션 환경(Ubuntu, Debian)인지 확인합니다.""" try: # /etc/os-release 파일 확인 if os.path.exists("/etc/os-release"): with open("/etc/os-release", "r") as f: content = f.read().lower() return "ubuntu" in content or "debian" in content # platform 모듈을 통한 확인 return ( "ubuntu" in platform.platform().lower() or "debian" in platform.platform().lower() ) except Exception as e: return FalseAnd update the call site at line 31:
- is_prod = is_ubuntu() + is_prod = is_production_os()middleware/local_access.py (1)
30-50: Enhance local IP detection to support IPv6 and private IP ranges.The current implementation only checks IPv4 loopback addresses (127.x.x.x). Consider adding support for:
- IPv6 loopback address
::1- Private IP ranges if the middleware should allow access from the local network (192.168.x.x, 10.x.x.x, 172.16-31.x.x)
🔎 Proposed enhancement for IPv6 support
+import ipaddress + def _is_local_ip(self, client_ip: str) -> bool: """ IP 주소가 로컬 주소인지 확인 Args: client_ip: 확인할 IP 주소 Returns: 로컬 IP인 경우 True, 그렇지 않으면 False """ if not client_ip: return False + # IPv6 loopback + if client_ip == "::1": + return True + if client_ip.startswith("127."): return True # 기본 로컬 IP 확인 if client_ip in self.ALLOWED_LOCAL_IPS: return True return FalseIf you also want to allow private IP ranges, you can use the
ipaddressmodule:try: ip = ipaddress.ip_address(client_ip) return ip.is_loopback or ip.is_private # Add is_private if needed except ValueError: return client_ip in self.ALLOWED_LOCAL_IPSmain.py (1)
42-42: Add explicit type annotations for Optional parameters.Several endpoint parameters use implicit
Optional(defaultNone), which is discouraged by PEP 484. Consider making them explicit usingT | Nonesyntax.🔎 Proposed type annotation fixes
@app.post("/discord/direct-messages") -async def create_direct_message(authorization: str = Header(None), data: dict = None): +async def create_direct_message(authorization: str = Header(None), data: dict | None = None): validate_api_key(authorization)Apply similar changes to:
- Line 86:
data: dict | None = None- Line 109:
data: dict | None = None- Line 121:
channel_id: str | None = None- Line 132:
notice_type: str | None = None- Line 141:
notice_type: str | None = None- Line 142:
last_notice_link: str | None = NoneAlso applies to: 86-86, 109-109, 121-121, 132-132, 141-142
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/env_loader.py(1 hunks)main.py(7 hunks)middleware/__init__.py(1 hunks)middleware/local_access.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main.py (2)
middleware/local_access.py (1)
LocalAccessMiddleware(13-101)config/db_config.py (10)
DBManager(15-138)read_direct_messages_list(27-30)create_direct_message(37-50)read_server_channels_list(32-35)create_server_channel(77-91)update_server_channel(102-110)delete_server_channel(112-120)read_notice_list(122-125)read_scraper_type_list(132-135)read_category_list(127-130)
🪛 Ruff (0.14.8)
middleware/local_access.py
20-20: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
20-20: Possible binding to all interfaces
(S104)
main.py
42-42: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
86-86: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
109-109: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
121-121: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
132-132: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
141-141: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
142-142: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
171-171: Possible binding to all interfaces
(S104)
🔇 Additional comments (3)
middleware/__init__.py (1)
1-5: LGTM!Clean package initialization with proper exports.
main.py (1)
16-17: LGTM! Middleware properly registered.The
LocalAccessMiddlewareis registered correctly and will execute before route handlers, providing the intended access control.middleware/local_access.py (1)
76-101: Middleware registration order is correct.LocalAccessMiddleware is properly registered as the only middleware in the application, ensuring it executes first and blocks external IP access with a 403 response before any route handlers are reached. API key validation remains in place at the handler level. No changes needed.
| import uvicorn | ||
| uvicorn.run("main:app", host="0.0.0.0", port=8000, reload=True) No newline at end of file | ||
|
|
||
| uvicorn.run("main:app", host="0.0.0.0", port=8000, reload=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the LocalAccessMiddleware file
find . -name "local_access.py" -type f | head -20Repository: kookmin-feed/kookmin-feed-database-api-server
Length of output: 117
🏁 Script executed:
cat -n ./middleware/local_access.pyRepository: kookmin-feed/kookmin-feed-database-api-server
Length of output: 3307
🏁 Script executed:
cat -n ./main.py | head -200Repository: kookmin-feed/kookmin-feed-database-api-server
Length of output: 7056
Remove "0.0.0.0" from ALLOWED_LOCAL_IPS—it's an invalid client IP that indicates misunderstanding of binding configuration.
The middleware correctly restricts access by checking client IPs against ALLOWED_LOCAL_IPS and returns 403 for external requests. However, line 20 of middleware/local_access.py incorrectly includes "0.0.0.0" in the allowed list. Since "0.0.0.0" is never a valid client IP (only a server binding address), this configuration error should be removed. The middleware will continue to block external access because "0.0.0.0" will never appear as a client_ip, but the misconfiguration should be corrected for clarity.
Additionally, for production deployment:
- Disable
reload=Truein the uvicorn configuration - Deploy behind a firewall or reverse proxy
- Verify the deployment environment only allows local requests to reach the application
🧰 Tools
🪛 Ruff (0.14.8)
171-171: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
In main.py line 171 and middleware/local_access.py line 20: remove "0.0.0.0"
from ALLOWED_LOCAL_IPS (it's a server bind address, not a client IP), ensure the
allowed list contains only valid client IPs like "127.0.0.1" and "::1" (or other
actual internal client IPs you intend to allow), and update any tests/config
that referenced "0.0.0.0"; also change uvicorn.run in main.py to not use
reload=True for production (either remove the reload flag or set reload=False)
so the server does not run with auto-reload in production.
| Starlette의 BaseHTTPMiddleware를 상속받아 구현 | ||
| """ | ||
|
|
||
| ALLOWED_LOCAL_IPS = ["127.0.0.1", "0.0.0.0", "localhost"] |
There was a problem hiding this comment.
Critical: Remove "0.0.0.0" from ALLOWED_LOCAL_IPS - major security vulnerability.
The IP address "0.0.0.0" represents all network interfaces, not a local address. Including it in ALLOWED_LOCAL_IPS effectively disables the middleware's access restriction, allowing connections from any IP address.
🔎 Proposed fix to remove 0.0.0.0
- ALLOWED_LOCAL_IPS = ["127.0.0.1", "0.0.0.0", "localhost"]
+ ALLOWED_LOCAL_IPS: ClassVar[list[str]] = ["127.0.0.1", "localhost"]Note: Also added ClassVar annotation as recommended by static analysis.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
20-20: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
20-20: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
In middleware/local_access.py around line 20, ALLOWED_LOCAL_IPS incorrectly
contains "0.0.0.0" which allows all interfaces and nullifies the access
restriction; remove "0.0.0.0" from the list so it only includes true local
addresses (e.g., "127.0.0.1", "localhost") and, per static analysis suggestion,
annotate the constant with typing.ClassVar[str] (or ClassVar[list[str]] as
appropriate) to make its intent explicit.
| def _get_client_ip(self, request: Request) -> str | None: | ||
| """ | ||
| 클라이언트 IP 주소를 추출 | ||
|
|
||
| 프록시나 로드밸런서 환경을 고려하여 X-Forwarded-For 헤더도 확인 | ||
|
|
||
| Args: | ||
| request: Starlette Request 객체 | ||
|
|
||
| Returns: | ||
| 클라이언트 IP 주소 또는 None | ||
| """ | ||
| # X-Forwarded-For 헤더 확인 (프록시 환경 고려) | ||
| forwarded_for = request.headers.get("X-Forwarded-For") | ||
| if forwarded_for: | ||
| # 여러 프록시를 거친 경우 첫 번째 IP가 실제 클라이언트 IP | ||
| return forwarded_for.split(",")[0].strip() | ||
|
|
||
| # 직접 연결인 경우 | ||
| if request.client: | ||
| return request.client.host | ||
|
|
||
| return None |
There was a problem hiding this comment.
Security concern: X-Forwarded-For header can be spoofed.
The middleware trusts the X-Forwarded-For header without validation. An attacker can easily spoof this header to bypass the local IP restriction by setting X-Forwarded-For: 127.0.0.1.
If you're deploying behind a trusted reverse proxy (nginx, Apache, or a cloud load balancer), ensure the proxy strips client-provided X-Forwarded-For headers and sets its own. Otherwise, consider:
- Only trusting
request.client.hostwhen not behind a proxy - Using proxy-specific headers that cannot be spoofed (e.g.,
X-Real-IPset by your proxy) - Validating that requests are coming through your trusted proxy first
Based on learnings, the security implications of header spoofing should be carefully evaluated in production environments.
🤖 Prompt for AI Agents
In middleware/local_access.py around lines 52 to 74, the current _get_client_ip
trusts X-Forwarded-For unconditionally which is spoofable; change it to only use
forwarded headers when the immediate peer (request.client.host) is a configured
trusted proxy, otherwise always use request.client.host; implement a
trusted_proxies configuration (list of IPs or CIDRs), validate
request.client.host against that list, prefer X-Real-IP if present, parse and
sanitize the header (take first value, strip whitespace) and validate it as a
well-formed IP (using the ipaddress module) before returning it; if validation
fails or request.client.host is not a trusted proxy, return request.client.host
or None.
2 participants
https://www.notion.so/prod-discord-bot-2c9615c17a49806e9871e264c032e56c?source=copy_link
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.