Skip to content

security(workplaces): rate-limit unauthenticated connector pairing#40

Open
DeryFerd wants to merge 1 commit into
anvie:mainfrom
DeryFerd:security/rate-limit-connector-pairing
Open

security(workplaces): rate-limit unauthenticated connector pairing#40
DeryFerd wants to merge 1 commit into
anvie:mainfrom
DeryFerd:security/rate-limit-connector-pairing

Conversation

@DeryFerd
Copy link
Copy Markdown
Contributor

Summary

Cloud workplaces pair through POST /api/connector/pair, which is intentionally unauthenticated so the Evonet binary can exchange a short pairing code for a long-lived connector_token. That design makes sense for the connector flow, but it also means anyone who can reach the server can keep guessing codes until one works.

Pairing codes are six characters from a reduced alphabet (uppercase letters and digits, with ambiguous characters removed). The search space is large enough that random guessing is not practical in a single request, yet still small enough that a patient attacker could hammer the endpoint for the full TTL window (default five minutes) without hitting any throttle today.

This PR adds a simple per-IP rate limiter for pairing attempts. Before finalize_pairing() runs, the route checks whether the client IP has already used up its budget in the current window. If so, it returns 429 with a short error message. Every pairing request counts toward the limit, including wrong or expired codes, because those are exactly the guesses an attacker would send.

The limiter lives in backend/connector_pair_rate_limit.py and mirrors the in-memory style already used for failed login attempts in routes/auth.py: thread-safe timestamps keyed by IP, old entries pruned on each check. Defaults are aligned with pairing semantics:

  • Window: CONNECTOR_PAIRING_CODE_TTL (default 300 seconds)
  • Max attempts: CONNECTOR_PAIR_MAX_ATTEMPTS (default 30), configurable via .env

Legitimate pairing should need only a handful of tries (typo, clock skew, code regenerated once). Thirty attempts per IP per code lifetime is generous for real connectors while making online brute force much slower.

What changed

  • New module backend/connector_pair_rate_limit.py with is_rate_limited(), record_attempt(), and reset_for_tests().
  • routes/workplaces.py: rate check + record_attempt() at the start of api_connector_pair().
  • config.py: CONNECTOR_PAIR_MAX_ATTEMPTS env knob (min 5, max 200).
  • unit_tests/test_connector_pair_rate_limit.py: under-limit allowed, at-limit blocked, per-IP isolation.

No change to how codes are generated, stored, or validated — only how often a single IP may call the pair endpoint.

Risk / compatibility

  • Multi-instance deployments: like the login limiter, state is in-process memory. Behind multiple app workers without sticky sessions, limits are per worker, not global. That is weaker than a shared store but consistent with existing patterns in this repo.
  • Reverse proxies: limiting uses request.remote_addr. If you terminate TLS or proxy in front of Evonic, ensure the deployment forwards the real client IP (or all pairing traffic may appear as one address).
  • False positives: an office NAT sharing one public IP could hit the cap if many connectors pair at once. Operators can raise CONNECTOR_PAIR_MAX_ATTEMPTS or TTL if needed.

Validation

  • python -m pytest unit_tests/test_connector_pair_rate_limit.py -q

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Owner

@anvie anvie left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Overall solid implementation with clear threat model and good alignment with existing login rate limiter patterns.

Key findings from review:

  1. Rate limiter uses time.monotonic() and threading.Lock - correct and consistent with auth.py patterns.
  2. Pruning on every is_rate_limited() call prevents unbounded growth.
  3. Minor race condition: is_rate_limited() and record_attempt() are called separately. Consider merging into an atomic check_and_record().
  4. getattr fallback for config values could silently default to 30 if config not loaded. Direct import would be safer.
  5. No logging when rate limit triggers - useful for monitoring false positives in production.
  6. Tests are clean but missing window expiry test and edge case (None IP) test.

Full review report saved in artifacts/pr40-review.md.

APPROVED with minor suggestions - nothing blocking.

Best,
Aisyah

Robin Syihab's agent.

anvie added a commit that referenced this pull request May 16, 2026
Task #260 — Review PR #40 by DeryFerd: add per-IP rate limiter for
unauthenticated connector pairing endpoint. Implementation approved
with minor suggestions (atomic check-and-record, logging, tests).

- artifacts/pr40-review.md: full review report
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