Revert "Caddy internal route" #101
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoves Caddy IP-based routing for /api/generate-signature/* and adds IP validation inside PublicMiddleware. Requests now reach the API through the general /api/* proxy, where middleware enforces IP presence and allowed-list checks (in production) before timestamp validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Caddy as Caddy (/api/*)
participant API as API Service (api:8080)
participant MW as PublicMiddleware
Client->>Caddy: HTTP request /api/generate-signature/...
Caddy->>API: Proxy request (headers forwarded)
API->>MW: Handle(request)
rect rgb(235, 245, 255)
note right of MW: Existing rate limiting
MW->>MW: Check rate limit
alt Rate limit exceeded
MW-->>API: Error (rate limit)
API-->>Caddy: 429
Caddy-->>Client: 429 Too Many Requests
else Allowed
note right of MW: New IP validation
MW->>MW: HasInvalidIP(request)
alt Missing/Disallowed IP
MW-->>API: Error (invalid IP)
API-->>Caddy: 400/403
Caddy-->>Client: Error response
else IP OK
note right of MW: Existing timestamp validation and downstream handling
MW->>MW: Validate timestamp
MW-->>API: Continue
API-->>Caddy: Success/other response
Caddy-->>Client: Response
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✨ 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. Comment |
![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 @gocanto, 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!
This pull request primarily reverts a previous change that introduced an internal route with IP restrictions in the Caddy web server configuration. To maintain necessary security, the IP validation logic has been migrated and re-implemented directly within the Go application's public middleware. This ensures that access control for sensitive API endpoints is handled programmatically within the application, providing a more integrated and potentially flexible approach to IP-based access management.
Highlights
- Caddy Configuration Reverted: The Caddyfile.prod configuration has been reverted, removing the Caddy-level IP restriction for the /api/generate-signature/* route. This effectively undoes the change introduced in #100.
- Go Middleware IP Validation Added: New IP validation logic has been introduced within the pkg/middleware/public_middleware.go. This middleware now checks the client's IP address for validity.
- Production IP Enforcement: The newly added Go middleware enforces that in a production environment, client requests must originate from a specific, pre-configured allowed IP address, returning an error for unauthorized IPs or missing client IPs.
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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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 reverts a previous change, moving the IP-based access control for the /api/generate-signature endpoint from the Caddy configuration back into the application's middleware. This is a reasonable change to centralize application logic. My review focuses on the new Go implementation of the IP check. I've identified an opportunity to make it more robust and flexible, bringing it closer to the capabilities of the Caddy ip_range directive it replaces. Specifically, I'm suggesting improvements to handle multiple allowed IPs and use proper IP address object comparison instead of string comparison.
| if p.isProduction && ip != p.allowedIP { | ||
| return mwguards.InvalidRequestError("The given IP is not allowed", "unauthorised ip: "+ip) | ||
| } |
There was a problem hiding this comment.
The current IP address check uses a direct string comparison, which can be brittle. For instance, it wouldn't correctly handle different string representations of the same IP address (e.g., IPv4 vs. IPv4-mapped IPv6 addresses).
Furthermore, the implementation only allows a single IP, which is a regression from the previous Caddy configuration that supported an ip_range (like 127.0.0.1 ::1 for both IPv4 and IPv6 localhost).
To make this more robust and flexible, I suggest parsing the IP strings into net.IP objects and using the Equal() method for comparison. To support multiple allowed IPs, p.allowedIP could be treated as a comma-separated list.
Here's a suggested implementation that incorporates these improvements. Note that you will need to add import "net" to the file.
if p.isProduction {
clientIP := net.ParseIP(ip)
if clientIP == nil {
return mwguards.InvalidRequestError("The given IP is not allowed", "unauthorised ip: "+ip)
}
isAllowed := false
allowedIPs := strings.Split(p.allowedIP, ",")
for _, allowedIPStr := range allowedIPs {
allowedIP := net.ParseIP(strings.TrimSpace(allowedIPStr))
if allowedIP != nil && clientIP.Equal(allowedIP) {
isAllowed = true
break
}
}
if !isAllowed {
return mwguards.InvalidRequestError("The given IP is not allowed", "unauthorised ip: "+ip)
}
}
Reverts #100
Summary by CodeRabbit
Bug Fixes
Chores