Conversation
- Add SearchProfileResponse DTO with only id, name, avatar - Update ProfileService.Search to return minimal DTO - Update swagger annotation - Add unit tests for search behavior - Add mocks for ProfileRepository, FriendshipRepository, Transactor
- Replace sync.Map with TTL-based eviction in ValueLimiter - Inject ValueLimiter into AuthHandler for testability - Add Stop() method with graceful shutdown wiring - Strip client-provided X-Rate-Key header in WithRateKey middleware - Add rate limit to PATCH /reset-password endpoint - Add tests for eviction and header stripping
- Add CaptchaService interface with Turnstile implementation - Noop bypass when AUTH_TURNSTILE_SECRET_KEY is empty (local dev) - Verify captcha token before processing password reset requests - Use http.NewRequestWithContext for proper context propagation - Add 10s HTTP client timeout - Add unit tests with httptest server
|
Warning Review limit reached
More reviews will be available in 42 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds Cloudflare Turnstile CAPTCHA verification and per-email rate-limiting for password reset requests, updates the profile search endpoint to return minimal response fields ( ChangesCAPTCHA Verification, Rate-Limiting, and Profile Search API Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/swagger.json (1)
3382-3392:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe generated Swagger artifacts are both stale for
SendPasswordResetRequest.Both specs still publish a password-reset request body with only
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/swagger.json` around lines 3382 - 3392, The Swagger spec is stale for dto.SendPasswordResetRequest and still only documents the email field; regenerate the OpenAPI artifacts from the updated DTO/annotations so the new CAPTCHA token field is included (and marked required if the DTO/validation requires it). Re-run the swagger generation/build step that produces docs/swagger.json (and any other published spec), verify dto.SendPasswordResetRequest now includes the captcha/token property with correct type and validation, then commit the regenerated artifacts.
🧹 Nitpick comments (4)
internal/domain/service/captcha_service_test.go (1)
21-32: ⚡ Quick winConsider adding test for non-200 HTTP status codes.
The current tests cover success, failure (success: false), and network errors, but don't verify behavior when Cloudflare returns a non-200 HTTP status code (e.g., 500, 503). This would help ensure the service handles Cloudflare service degradation gracefully.
🧪 Suggested test case
func TestTurnstileService_Verify_NonOKStatus(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) w.Write([]byte("Service unavailable")) })) defer srv.Close() svc := service.NewTurnstileServiceWithURL("test-secret", srv.URL) err := svc.Verify(context.Background(), "token") assert.Error(t, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/domain/service/captcha_service_test.go` around lines 21 - 32, Add a test that asserts Verify returns an error when the remote Turnstile endpoint responds with a non-200 status: create a httptest.NewServer handler that returns e.g. http.StatusInternalServerError and a body, instantiate the service with service.NewTurnstileServiceWithURL("test-secret", srv.URL), call svc.Verify(context.Background(), "token"), and assert an error is returned; place the test alongside TestTurnstileService_Verify_Success and reuse the same patterns (httptest server, defer srv.Close) so the Verify method behavior on non-OK HTTP responses is validated.internal/adapters/http/middlewares/ratelimit.go (1)
36-46: ⚡ Quick winDocument that Stop() must be called to prevent goroutine leak.
The constructor starts a background cleanup goroutine that runs indefinitely until
Stop()is called. If the caller doesn't callStop()during shutdown, the goroutine will leak.Consider adding a comment documenting this requirement, or verify that the graceful shutdown sequence always calls
Stop().📝 Suggested documentation
+// NewValueLimiter creates a new per-key rate limiter with the specified limit, burst, and TTL. +// The limiter starts a background cleanup goroutine that evicts idle entries. +// Call Stop() during shutdown to terminate the cleanup goroutine and prevent leaks. func NewValueLimiter(limit rate.Limit, burst int, ttl time.Duration) *ValueLimiter {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/http/middlewares/ratelimit.go` around lines 36 - 46, NewValueLimiter starts a background goroutine via vl.cleanup() that runs until ValueLimiter.Stop() is called, which can leak if Stop() isn't invoked; update the code comments and public docs to state that callers must call ValueLimiter.Stop() during shutdown (or ensure shutdown paths call Stop()), and add a short note above NewValueLimiter and the ValueLimiter type mentioning the background goroutine, the stop channel, and the requirement to call Stop() to prevent goroutine leaks (referencing NewValueLimiter, ValueLimiter, cleanup(), stop, and Stop()).internal/adapters/http/routes/api_routes.go (2)
37-52: 💤 Low valueLayered rate limiting on password-reset endpoints is correct.
Both the group-level limiter (20 req/min) and the per-endpoint limiters (3 req/15min for POST, 5 req/15min for PATCH) apply independently. The stricter per-endpoint limits effectively govern these sensitive operations while allowing normal usage of other auth endpoints within the group quota.
💡 Optional: Add inline comment explaining the layered strategy
authRoutes.GET("/verify-registration", handlers.Auth.HandleVerifyRegistration()) +// Layered IP-based limiter: 3 req/15min for password-reset (stricter than group limit) authRoutes.POST("/password-reset",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/http/routes/api_routes.go` around lines 37 - 52, Add an inline comment above the authRoutes.POST("/password-reset", ...) and authRoutes.PATCH("/reset-password", ...) blocks explaining the layered rate-limiting strategy: note that a group-level limiter (20 req/min) applies to the authRoutes group and these endpoint-level sentinelGin.RateLimit(httpserver.RateLimitConfig{...}) calls enforce stricter per-endpoint quotas (3 req/15min for HandleSendPasswordReset and 5 req/15min for HandleResetPassword), so both limiters apply and the per-endpoint limits will effectively govern these sensitive operations.
25-29: 💤 Low valueGroup-level IP rate limiting on auth routes looks correct.
The configuration allows 20 requests per 60 seconds (≈0.33 req/s) with a burst of 5 per IP across all
/authendpoints.💡 Optional: Add inline comment for maintainability
authRoutes := v1.Group("/auth") +// IP-based rate limit: 20 req/min with burst of 5 authRoutes.Use(sentinelGin.RateLimit(httpserver.RateLimitConfig{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/http/routes/api_routes.go` around lines 25 - 29, Add an inline comment above the authRoutes.Use(...) RateLimitConfig call explaining the per-IP rate limit calculation and rationale (20 requests per 60 seconds ≈ 0.33 req/s, burst=5) so future maintainers understand the intended throttling; reference the authRoutes.Use, sentinelGin.RateLimit, httpserver.RateLimitConfig and httpserver.KeyFuncByIP symbols when adding the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adapters/http/handler/auth_handler.go`:
- Around line 213-219: The per-email rate limit uses the raw request.Email and
is checked after the CAPTCHA call; normalize the email (trim spaces and
lowercase, e.g., canonicalEmail :=
strings.ToLower(strings.TrimSpace(request.Email))) and use that canonicalEmail
as the key to ah.emailLimiter.Allow to harden against case/spacing variants, and
move the Allow check to run before calling ah.captchaService.Verify so throttled
requests are short-circuited locally; update any variable names (e.g.,
canonicalEmail) and references in auth_handler.go for ah.emailLimiter.Allow and
ah.captchaService.Verify accordingly.
In `@internal/adapters/http/handler/handlers.go`:
- Line 37: The email rate limiter is hardcoded where NewAuthHandler is wired
(middlewares.NewValueLimiter(3.0/3600, 3, time.Hour)); add configurable
rate-limit fields to the core config (e.g., create or extend an AuthConfig in
internal/core/config to include rateLimitRate, rateLimitBurst, rateLimitWindow)
and update the services/handler wiring to read those values instead of literals
and pass them into middlewares.NewValueLimiter; update NewAuthHandler
construction to accept the config-derived limiter parameters and adjust any
tests that construct the limiter directly to use the new config-based values.
In `@internal/domain/service/captcha_service_test.go`:
- Line 1: The test file declares package service_test but must use the same
package as the code under test; change the package line in
captcha_service_test.go from "package service_test" to "package service" and
update any references/imports accordingly (replace any stdlib testing-only
assertions with testify/assert imports and calls) so the tests run in the same
package context as the production code and use testify/assert for assertions.
In `@internal/domain/service/captcha_service.go`:
- Around line 50-61: Before decoding the response from ts.httpClient.Do(req),
check resp.StatusCode (e.g., ensure it's 200); if it's not 200, read a small
snippet of resp.Body (or the full body if small) and return a wrapped error
indicating the unexpected HTTP status and include the body snippet for
diagnostics instead of attempting json.NewDecoder(resp.Body).Decode(&result);
update the error path in the captcha verification flow so that VerifyCaptcha
(the block using resp, ts.httpClient.Do and json.NewDecoder) returns a clear
error when resp.StatusCode != http.StatusOK.
---
Outside diff comments:
In `@docs/swagger.json`:
- Around line 3382-3392: The Swagger spec is stale for
dto.SendPasswordResetRequest and still only documents the email field;
regenerate the OpenAPI artifacts from the updated DTO/annotations so the new
CAPTCHA token field is included (and marked required if the DTO/validation
requires it). Re-run the swagger generation/build step that produces
docs/swagger.json (and any other published spec), verify
dto.SendPasswordResetRequest now includes the captcha/token property with
correct type and validation, then commit the regenerated artifacts.
---
Nitpick comments:
In `@internal/adapters/http/middlewares/ratelimit.go`:
- Around line 36-46: NewValueLimiter starts a background goroutine via
vl.cleanup() that runs until ValueLimiter.Stop() is called, which can leak if
Stop() isn't invoked; update the code comments and public docs to state that
callers must call ValueLimiter.Stop() during shutdown (or ensure shutdown paths
call Stop()), and add a short note above NewValueLimiter and the ValueLimiter
type mentioning the background goroutine, the stop channel, and the requirement
to call Stop() to prevent goroutine leaks (referencing NewValueLimiter,
ValueLimiter, cleanup(), stop, and Stop()).
In `@internal/adapters/http/routes/api_routes.go`:
- Around line 37-52: Add an inline comment above the
authRoutes.POST("/password-reset", ...) and authRoutes.PATCH("/reset-password",
...) blocks explaining the layered rate-limiting strategy: note that a
group-level limiter (20 req/min) applies to the authRoutes group and these
endpoint-level sentinelGin.RateLimit(httpserver.RateLimitConfig{...}) calls
enforce stricter per-endpoint quotas (3 req/15min for HandleSendPasswordReset
and 5 req/15min for HandleResetPassword), so both limiters apply and the
per-endpoint limits will effectively govern these sensitive operations.
- Around line 25-29: Add an inline comment above the authRoutes.Use(...)
RateLimitConfig call explaining the per-IP rate limit calculation and rationale
(20 requests per 60 seconds ≈ 0.33 req/s, burst=5) so future maintainers
understand the intended throttling; reference the authRoutes.Use,
sentinelGin.RateLimit, httpserver.RateLimitConfig and httpserver.KeyFuncByIP
symbols when adding the comment.
In `@internal/domain/service/captcha_service_test.go`:
- Around line 21-32: Add a test that asserts Verify returns an error when the
remote Turnstile endpoint responds with a non-200 status: create a
httptest.NewServer handler that returns e.g. http.StatusInternalServerError and
a body, instantiate the service with
service.NewTurnstileServiceWithURL("test-secret", srv.URL), call
svc.Verify(context.Background(), "token"), and assert an error is returned;
place the test alongside TestTurnstileService_Verify_Success and reuse the same
patterns (httptest server, defer srv.Close) so the Verify method behavior on
non-OK HTTP responses is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b99a5d70-cb6a-446d-a1ae-6659e0ba2ce4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.env.example.mockery.yamldocs/docs.godocs/swagger.jsondocs/swagger.yamlgo.modinternal/adapters/http/handler/auth_handler.gointernal/adapters/http/handler/handlers.gointernal/adapters/http/handler/profile_handler.gointernal/adapters/http/middlewares/ratelimit.gointernal/adapters/http/middlewares/ratelimit_integration_test.gointernal/adapters/http/middlewares/ratelimit_test.gointernal/adapters/http/register_routes.gointernal/adapters/http/routes/api_routes.gointernal/adapters/http/server.gointernal/core/config/auth_config.gointernal/domain/dto/auth_dto.gointernal/domain/dto/user_profile_dto.gointernal/domain/service/captcha_service.gointernal/domain/service/captcha_service_test.gointernal/domain/service/profile_service.gointernal/domain/service/profile_service_test.gointernal/domain/service/services.gointernal/mocks/mock_crud.gointernal/mocks/mock_crud_repository.gointernal/mocks/mock_repository.gointernal/mocks/mock_service.gointernal/provider/service_provider.go
- Normalize email (lowercase+trim) for rate limiter key - Check rate limiter before captcha to short-circuit locally - Check HTTP status code before decoding Turnstile response - Add test for non-200 status from Turnstile - Regenerate Swagger docs with captchaToken field
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security hardening to prevent automated abuse and information leakage.
Changes
Opaque search results (user enumeration prevention)
Rate limiter fixes & hardening
CAPTCHA on password reset (Cloudflare Turnstile)
CaptchaServiceinterface with Turnstile server-side verificationAUTH_TURNSTILE_SECRET_KEYis empty (local dev)Configuration
New env var:
AUTH_TURNSTILE_SECRET_KEY(leave empty to disable in dev)Testing
Summary by CodeRabbit
New Features
Documentation
Tests