Skip to content

Apply rate limiting to ConnectRPC path, deduplicate auth helpers#113

Merged
haasonsaas merged 3 commits intomainfrom
fix/connect-middleware-and-cleanup
Apr 12, 2026
Merged

Apply rate limiting to ConnectRPC path, deduplicate auth helpers#113
haasonsaas merged 3 commits intomainfrom
fix/connect-middleware-and-cleanup

Conversation

@haasonsaas
Copy link
Copy Markdown
Contributor

Summary

Follow-up to #111 addressing remaining Bugbot review feedback:

  • Rate limiting bypass (HIGH): ConnectRPC endpoints (/gate.v1.*) were served outside the chi router, bypassing rate limiter, body size limit, and security headers. Now wrapped with the same middleware. Note: this was pre-existing from the gRPC era — the old grpcMuxHandler also skipped chi middleware.
  • Duplicated authReq (LOW): Identical auth header methods in sync/connectclient.go and connector/registration.go replaced with shared controlplaneclient.ApplyAuth()
  • Set vs Add bug (LOW): The duplicated authReq used Set inside a multi-value loop. Eliminated by switching to ApplyAuth which sets headers directly.

Test plan

  • go build ./... passes
  • go vet ./... passes
  • go test ./... -race passes
  • Integration tests pass
  • Verify ConnectRPC endpoints are rate limited (send >200 burst requests)

🤖 Generated with Claude Code

- Wrap ConnectRPC handler with rate limiter, body size limit, and
  security headers — previously bypassed (pre-existing since gRPC era)
- Extract ApplyAuth() in controlplaneclient to replace duplicated
  authReq() methods in sync and connector packages
- Fixes Set-vs-Add header bug (simplified to direct Set calls)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cursor
Copy link
Copy Markdown

cursor bot commented Apr 12, 2026

PR Summary

High Risk
High: changes request handling for all ConnectRPC endpoints by adding rate limiting/body-size/security headers and RealIP, which could impact client behavior under load or behind proxies. Also refactors shared auth header application across connector/sync clients, so mistakes would affect control-plane connectivity.

Overview
ConnectRPC (/gate.v1.*) requests are now wrapped with the same middleware stack as REST routes (RealIP, rate limiting, max body size, and security headers), closing a bypass where RPC calls skipped these protections.

Auth header setting for ConnectRPC clients is deduplicated by replacing per-client authReq helpers with a shared controlplaneclient.ApplyAuth() used by connector registration and sync clients.

CI Buf breaking detection is updated to compare against origin/main instead of main.

Reviewed by Cursor Bugbot for commit 5476317. Bugbot is set up for automated code reviews on this repo. Configure here.

GitHub Actions checkout creates origin/main but not a local main branch.
buf breaking needs the remote ref.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, have a team admin raise the spend limit in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 37f9358. Configure here.

if orgID != "" {
req.Header().Set("X-Org-Id", orgID)
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New ApplyAuth duplicates existing withAuth helper logic

Low Severity

The new ApplyAuth function duplicates the header-setting logic of the existing withAuth function in the same file. Both set the same Authorization and X-Org-Id headers on a ConnectRPC request. The private withAuth could delegate to ApplyAuth internally to keep a single source of truth for auth header logic.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 37f9358. Configure here.

- Apply middleware.RealIP before rate limiter on ConnectRPC path so
  requests behind a reverse proxy are rate-limited by client IP, not
  proxy IP
- Remove OutgoingHeaders (no callers after ApplyAuth refactor)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@haasonsaas haasonsaas merged commit 4d4564e into main Apr 12, 2026
8 checks passed
@haasonsaas haasonsaas deleted the fix/connect-middleware-and-cleanup branch April 12, 2026 01:53
haasonsaas added a commit that referenced this pull request Apr 12, 2026
- Restore RealIP + rate limiter + body size + security headers on
  ConnectRPC path (lost during rebase from pre-#113 branch)
- Replace authReq with controlplaneclient.ApplyAuth (dedup from #113)
- Replace OutgoingHeaders with ApplyAuth (dedup from #115)
- withAuth delegates to ApplyAuth

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
haasonsaas added a commit that referenced this pull request Apr 12, 2026
* Import identity types from shared proto package

Remove local Organization, OrgMember, APIKey message definitions from
tenant.proto and import canonical types from identity/v1. Update all
converter functions, clients, and tests.

Closes #110

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Replace audit.Entry with shared auditv1.Event

Swap hand-written Entry struct for auditv1.Event from shared proto.
Sink interface accepts []*auditv1.Event. Logger methods construct proto
events with nested Actor/Resource. Metadata field overwrite prevented
by setting dedicated fields last. CI updated with shared proto symlinks
for buf lint and tenant.proto excluded from buf breaking during migration.

Closes #108

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Address review feedback: restore middleware, deduplicate auth helpers

- Restore RealIP + rate limiter + body size + security headers on
  ConnectRPC path (lost during rebase from pre-#113 branch)
- Replace authReq with controlplaneclient.ApplyAuth (dedup from #113)
- Replace OutgoingHeaders with ApplyAuth (dedup from #115)
- withAuth delegates to ApplyAuth

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.

1 participant