fix(api): rate-limit state-changing endpoints on five controllers#332
Conversation
Five controllers had authenticated state-changing endpoints reachable without any distributed rate limiting, leaving them open to abuse/spam: - TagsController: create/update/delete/restore/assign (policy "tags", 60/min) - FriendsController: accept-request/remove-friend (policy "friend-mutations", 50/24h) - AchievementsController: report-event (policy "achievements", 30/min) - AiController: confirm/execute pending operation (tighter policy "ai-operations", 15/min) - AccountabilityController: accept/end/set-habits (policy "accountability-mutations", 50/24h) Applies the existing [DistributedRateLimit] attribute with per-user partitioning (automatic for authenticated requests) and registers the new policies in DistributedRateLimitService. Limits mirror the tightness of sibling policies; the AI ops get a deliberately tighter cap. Tests: attribute-presence coverage per controller plus service-level "blocks after permit limit" and per-user partition-isolation behavior for each new policy. Updates FriendsController coverage since accept/remove are now rate-limited. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #332
Scope: PR #332 (fix/rate-limit-state-changing-endpoints → main)
Recommendation: APPROVE
Summary
Purely additive, server-side hardening: applies the existing [DistributedRateLimit] attribute to previously-unthrottled, authenticated, state-changing endpoints on TagsController, FriendsController, AchievementsController, AiController, and AccountabilityController, plus five new policy entries in DistributedRateLimitService. No routes, DTOs, or response shapes change. All 12 changed files were read in full. No prior reviews/threads exist on this PR — this is a first pass.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info] Out-of-scope observation, not a defect in this diff: several other controllers with state-changing endpoints (
UserFactsController,SyncControllerMutations,GoalsController,ChecklistTemplatesController,ApiKeysController,GamificationController,CalendarController,ConfigController,NotificationController,ReferralController,SubscriptionController,HabitsControllerRequests) still carry zero[DistributedRateLimit]attributes. The PR body explicitly scopes itself to "five controllers," and this diff doesn't touch or regress those others — flagged only as a possible future follow-up. - [Info]
AiControllerRateLimitTestslacks the "reads/already-limited actions stay unaffected" negative test thatTagsControllerRateLimitTestsandAccountabilityControllerRateLimitTestsinclude. The two actually-changed actions (ConfirmPendingOperation,ExecutePendingOperation) are covered; this is a minor coverage-consistency nit only.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no findings. Verified: no remaining unprotected authenticated state-changing endpoint on the five touched controllers; partition key is user:{userId} for all five new policies (all controllers are class-level [Authorize], no IP/email fallback reachable); the fail-open "support" special-case doesn't match any of the five new policy names so they fail closed on a TryAcquireAsync exception, consistent with existing policies; new Policies dict entries use correct TimeSpan units and SegmentCount conventions matching sibling policies; no new [AllowAnonymous], no new DTO fields, no sensitive-data logging. |
| contract-aligner | NOT VERIFIABLE IN CI — orbit-ui-mobile not checked out. The diff itself changes no DTO, route, or request/response shape (only adds attributes + internal policy-dictionary entries), so there's nothing to drift-check even in principle. Backward-compat guard: no field add/remove/rename anywhere; a throttled request returns the existing 429 shape every client already handles from other pre-existing policies (chat, block, etc.). No ⚠️ breaks old mobile clients candidates. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped in CI per instructions; covered by separate required check |
| Tests (dotnet) | N/A — skipped in CI per instructions; covered by separate required check |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop) — N/A, no
apps/*files touched. - Dimension 9 (Parity web↔mobile) — N/A, backend-only diff.
- Dimension 10 (i18n) — N/A, no new user-facing strings.
- Dimension 14 (FEATURES.md parity) — N/A, no user-facing feature/screen/tool/plan-gating change (invisible infra hardening; a throttled request just gets a 429 clients already handle).
- Dimension 11 (Contract drift) — not independently verifiable in CI (sibling repo not checked out), but the diff contains no DTO/route/shape change to check.
- All 12 changed files were read in full and verdicted:
AccountabilityController.cs,AchievementsController.cs,AiController.cs,FriendsController.cs,TagsController.cs,DistributedRateLimitService.cs, and the six test files. Nothing deferred among changed files.
What's good
- Tight, mechanical scope: attribute additions + dictionary entries + matching unit tests only.
- Policy limits/windows consistently mirror existing sibling conventions (per-minute sliding windows for interactive bursts vs. 24h fixed windows for social/relationship mutations).
- Solid test discipline: attribute-presence coverage per controller, service-level permit-exhaustion tests for every new policy, per-user partition-isolation test for
tags; the existingFriendsControllerRateLimitTestswas correctly updated (moved, not just extended) since two actions changed category. - Correctly reasons about backward compatibility in the PR body — verified true against the actual diff.
Recommendation
Ready to merge as-is. No Critical/High findings; the two Info notes are optional follow-ups, not blockers.
There was a problem hiding this comment.
Code Review: PR #332 — fix(api): rate-limit state-changing endpoints on five controllers
Recommendation: APPROVE
Summary
The PR adds [DistributedRateLimit] to previously-unprotected authenticated, state-changing endpoints on TagsController, FriendsController, AchievementsController, AiController, and AccountabilityController, and registers five new policies (ai-operations, tags, achievements, friend-mutations, accountability-mutations) in DistributedRateLimitService. No routes, DTOs, or response shapes changed — server-side hardening only. New/updated tests cover attribute presence per action and service-level permit-exhaustion + per-user partition isolation for each new policy. Coverage was verified against every action on all five controllers; nothing abuse-prone was left unprotected, and the new limits are consistent with the existing policy scale.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[Info] New policy limits track existing sibling conventions
- location:
src/Orbit.Infrastructure/Services/DistributedRateLimitService.cs:20,25,26,29,33 ai-operations(15/min) mirrorshabit-suggest/tag-suggest;tags(60/min, shared across 5 CRUD actions) sits aboveuploads/public-profile(30/min) but is reasonable for a per-user CRUD bucket;friend-mutations/accountability-mutations(50/24h) matchblock/unblock/challenges.- risk: None — noted for the record, no change requested.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — full coverage confirmed across all five controllers' remaining mutating actions; filter-ordering ([Authorize] runs before the rate-limit filter, so partitioning is always user:{id} for these endpoints) and fail-closed behavior verified correct; no injection/data-exposure/logging issues. |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared type/endpoints.ts changed (attribute-only diff); not verifiable in this CI job since orbit-ui-mobile is not checked out here, but the diff itself contains no route/DTO edits. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | Skipped in this run — runs as separate required CI checks (Build / Unit Tests / SonarCloud). |
| Tests (dotnet) | Skipped in this run — same as above. |
Deferred — N/A dimensions
- DESIGN.md/AI-slop — N/A, no
apps/*UI files in this diff (backend-only repo). - Parity (web ↔ mobile) — not verifiable in this job (sibling
orbit-ui-mobilerepo not checked out); diff shows no contract-surface change (429 is a status every client already handles). - i18n — N/A, no new user-facing strings.
- FEATURES.md parity — N/A, pure enforcement hardening on existing endpoints, no new/changed/removed feature surface.
What's good
- Coverage is genuinely complete: cross-checked every mutating action on all five controllers, not just the ones the PR body lists, confirming no sibling endpoint was missed.
- Test strategy mirrors the existing
FriendsControllerRateLimitTestspattern exactly (attribute-presence theory + explicit non-rate-limited read actions), plus adds real permit-exhaustion and per-user-partition-isolation coverage at the service level for every new policy. - Policy values are chosen by analogy to existing sibling policies rather than invented ad hoc, keeping the rate-limit table internally consistent.
- Correctly scoped as server-only: no unnecessary contract churn, no speculative DTO changes.
Recommendation
Approve as-is. No follow-up required before merge.
There was a problem hiding this comment.
Review Summary — PR #332 (fix/rate-limit-state-changing-endpoints)
Recommendation: APPROVE
Scope
Adds distributed rate-limit policies (tags, achievements, ai-operations, friend-mutations, accountability-mutations) to five state-changing controllers, plus corresponding unit tests. Purely additive: attribute annotations + new Policies dictionary entries + new/extended tests. No route, DTO, or response-shape changes.
Verification performed
- Read the full diff (
gh pr diff 332) across all 12 changed files. - Confirmed
TagsController.SuggestTags(covered by the newTagsControllerRateLimitTests) already carried a pre-existingtag-suggestrate-limit attribute, untouched by this diff — the test addition is accurate, not speculative. - Read
src/Orbit.Api/RateLimiting/DistributedRateLimitAttribute.csin full:- Partition key resolution is
user:{GetUserId()}for any authenticated request; IP/email fallback only applies to unauthenticated requests under theauth/waitlistpolicies (irrelevant here). - On
TryAcquireAsyncfailure the filter fails closed (rethrows) for every policy except the hardcoded"support"fail-open case — none of the five new policy names match it.
- Partition key resolution is
- Grepped all five touched controllers and confirmed each carries class-level
[Authorize]with no[AllowAnonymous]override on the touched actions, so partitioning is genuinely per-user, not IP-shared, for all new limits.
Findings
- Critical: None
- High: None
- Medium: None
- Low/Info: None new. Prior review passes noted non-blocking observations on policy-limit rationale and other unthrottled controllers being out of scope — not re-litigated here as no new signal emerged.
Not verifiable in this CI job
- Cross-repo contract check (
orbit-ui-mobilenot checked out) — not applicable in principle either, since the diff has no DTO/route/contract surface change. - Build/Unit Tests/SonarCloud — run as separate required CI checks, skipped here per instructions.
This is the third independent pass over this PR (including two prior automated APPROVEs); this run corroborates the prior findings with direct code verification and surfaces nothing new.
There was a problem hiding this comment.
Automated /pr-review pass — see full report in conversation. Recommendation: APPROVE. No Critical/High findings survived adversarial verification; one High finding (MCP transport bypasses the new ai-operations REST rate limit) was downgraded to Medium/follow-up after a skeptic pass confirmed it's a pre-existing gap unrelated to this diff (AgentTools.cs is not touched by PR #332).
|
1 participant
What
Five controllers exposed authenticated, state-changing endpoints reachable without any distributed rate limiting, leaving them open to spam/abuse. This applies the existing
[DistributedRateLimit]attribute (per-user partitioning, automatic for authenticated requests) and registers sensible new policies inDistributedRateLimitService.TagsControllertagsFriendsControllerfriend-mutationsAchievementsControllerachievementsAiControllerai-operations(tighter)AccountabilityControlleraccountability-mutationsPolicy design mirrors sibling conventions: per-minute sliding windows (SegmentCount 4) for interactive write bursts (tags/achievements) and 24h fixed windows for social/relationship mutations (matching
block/unblock/friend-requests). The two AI agent operations get a deliberately tighter cap (15/min) since they can drive costly downstream execution, consistent withhabit-suggest/tag-suggest. Sharing one policy per controller domain follows the establishedchallenges/chatprecedent.Why not a client/contract change
Server-side hardening only — no new endpoints, no DTO fields, no renamed/removed contract. Request signatures and success responses are unchanged; a throttled request returns the standard
429every client already handles. Nopackages/sharedor consumer edit is warranted.Tests
FriendsControllerRateLimitTests) for Tags, Achievements, Ai, Accountability.tags(a second user is unaffected).FriendsControllerRateLimitTestssince AcceptRequest/RemoveFriend are now limited.dotnet buildclean; full suite green (4697 passed, 0 failed).Refs thomasluizon/orbit-ui-mobile#243