Skip to content

fix: add rate limiting to webhooks and auth endpoints, and add caching to user session#2591

Merged
kmendell merged 1 commit into
mainfrom
fix/rate-limit
May 13, 2026
Merged

fix: add rate limiting to webhooks and auth endpoints, and add caching to user session#2591
kmendell merged 1 commit into
mainfrom
fix/rate-limit

Conversation

@kmendell
Copy link
Copy Markdown
Member

@kmendell kmendell commented May 13, 2026

Checklist

  • This PR is not opened from my fork’s main branch

What This PR Implements

Fixes:

Changes Made

Testing Done

  • Development environment started: ./scripts/development/dev.sh start
  • Frontend verified at http://localhost:3000
  • Backend verified at http://localhost:3552
  • Manual testing completed (describe):
  • No linting errors (e.g., just lint all)
  • Backend tests pass: just test backend

AI Tool Used (if applicable)

AI Tool:
Assistance Level:
What AI helped with:
I reviewed and edited all AI-generated output:
I ran all required tests and manually verified changes:

Additional Context

Disclaimer Greptiles Reviews use AI, make sure to check over its work.

To better help train Greptile on our codebase, if the comment is useful and valid Like the comment, if its not helpful or invalid Dislike

To have Greptile Re-Review the changes, mention greptileai.

Greptile Summary

This PR adds per-IP rate limiting to auth and webhook endpoints, introduces a 15-second in-process token-verification cache to reduce DB load, and adds TRUSTED_PROXIES configuration to correctly identify client IPs behind reverse proxies.

  • Rate limiting: PerIPRateLimitForPaths creates one independent token bucket per path, so a login burst does not consume the token-refresh or webhook budget. Auth endpoints are capped at 5 req/min (burst 5); the webhook trigger at 60 req/min (burst 10).
  • Token cache: VerifyToken now caches validated (User, SessionID) tuples for 15 s, keyed by SHA-256 of the raw JWT. Cache entries are eagerly purged in RevokeSession, ChangePassword, and the new InvalidateUserTokenCache, which is also called from UpdateUser and DeleteUser to address stale roles/deletion.
  • Auth bridge refactor: Public routes now run opportunisticBearerAuthInternal, which populates user/session context without rejecting the request, enabling logout to revoke the correct session even though the route has no security requirement.

Confidence Score: 5/5

Safe to merge; all new code is well-tested and the only concern is a minor configuration edge-case that does not affect deployments using the default settings.

All three major features (rate limiting, token cache, auth bridge refactor) are covered by new tests and the logic is sound. The one rough edge — TRUSTED_PROXIES falling back to ExtractIPFromXFFHeader() with an empty option set when every CIDR is invalid — only affects operators who set the env var to a malformed value and does not cause a security hole, just potentially less-effective rate limiting.

backend/internal/bootstrap/router_bootstrap.go — the TRUSTED_PROXIES fallback when all CIDRs are invalid.

Fix All in Codex Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
backend/internal/bootstrap/router_bootstrap.go:123-138
When `TRUSTED_PROXIES` is set but every CIDR string is invalid (e.g., a typo), `opts` remains empty and `echo.ExtractIPFromXFFHeader()` is called with no trust options. Echo's implementation with an empty trust set returns `RemoteAddr` directly — the same as `ExtractIPDirect()` — so XFF headers are silently ignored. Behind a reverse proxy this means all requests appear to come from the proxy's IP and they all share a single rate-limit bucket, making per-IP limiting ineffective. Falling back to `ExtractIPDirect()` explicitly when no valid CIDRs were parsed makes the intent clear and matches the `TRUSTED_PROXIES=""` path.

```suggestion
	} else {
		var opts []echo.TrustOption
		for _, cidr := range strings.Split(cfg.TrustedProxies, ",") {
			cidr = strings.TrimSpace(cidr)
			if cidr == "" {
				continue
			}
			_, ipnet, err := net.ParseCIDR(cidr)
			if err != nil {
				slog.Warn("invalid TRUSTED_PROXIES CIDR, ignoring", "cidr", cidr, "error", err)
				continue
			}
			opts = append(opts, echo.TrustIPRange(ipnet))
		}
		if len(opts) == 0 {
			slog.Warn("TRUSTED_PROXIES set but no valid CIDRs found; falling back to direct IP extraction")
			e.IPExtractor = echo.ExtractIPDirect()
		} else {
			e.IPExtractor = echo.ExtractIPFromXFFHeader(opts...)
		}
	}
```

Reviews (2): Last reviewed commit: "fix: add rate limiting to webhooks and a..." | Re-trigger Greptile

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kmendell kmendell marked this pull request as ready for review May 13, 2026 04:50
@kmendell kmendell requested a review from a team May 13, 2026 04:50
@getarcaneappbot
Copy link
Copy Markdown
Contributor

getarcaneappbot commented May 13, 2026

Container images for this PR have been built successfully!

  • Manager: ghcr.io/getarcaneapp/arcane:pr-2591
  • Agent: ghcr.io/getarcaneapp/arcane-headless:pr-2591

Built from commit c199f8a

Comment thread backend/internal/services/auth_service.go
Comment thread backend/pkg/utils/cache/ttl.go
Comment thread backend/internal/middleware/rate_limit_middleware.go
Comment thread backend/api/middleware/auth_test.go Outdated
@kmendell kmendell force-pushed the fix/rate-limit branch 2 times, most recently from bb1bd66 to 88f5b3a Compare May 13, 2026 05:33
@kmendell kmendell merged commit ab6415d into main May 13, 2026
21 checks passed
@kmendell kmendell deleted the fix/rate-limit branch May 13, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants