Repository navigation
fix(api): add community write abuse admission - #82
Conversation
Close audit P1-35 (social writes) and P1-36 (team/forum writes): follow, friend request, team creation, public join, join request, thread creation and reply had no abuse budget, and a declined friend request could be re-sent without limit. Each of the seven creation routes now makes one atomic admission of a per-account and a per-IP bucket, after cheap validation and before the repository is touched. Keys name only the caller and the caller's address, never the target player or team, so writes aimed at a player cannot spend that player's budget. Five categories reuse existing shapes with the 10-account shared-NAT margin: socialInitiation (20/5min, 200/5min), teamCreation (5/h, 50/h), teamJoin (20/5min, 200/5min), forumThreadCreation (20/5min, 200/5min) and forumPostCreation (30/min, 300/min). Self-relations (in any UUID case), malformed slugs and blank names, titles and bodies are rejected by the domain's own validators before charging, with unchanged error contracts. Safety, clean-up and governance routes (unfollow, respond, block, unblock, leave/remove, role, transfer, join-request respond/cancel, thread/post edit/delete) stay unmetered, pinned by a structural test. The messaging budget validator becomes validateWriteRateLimits over all eight authenticated-write categories. The seven routes document 429 with Retry-After (follow and friend request also document 404), and openapi.json is regenerated.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe API applies per-user and per-IP rate limits to social, team, and forum write routes. Friend-request creation also has a sender-recipient pair limit. Selected input validation and self-relation checks occur before admission. The API documents rate-limit responses, and tests cover configuration, route behavior, concurrency, and shared admission across replicas. ChangesCommunity write admission
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant CommunityRoute
participant RateLimiter
participant Repository
Client->>CommunityRoute: Submit community write
CommunityRoute->>RateLimiter: Check configured admission buckets
RateLimiter-->>CommunityRoute: Admit or reject
CommunityRoute->>Repository: Persist admitted write
CommunityRoute-->>Client: Return result or rate-limit response
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds community write limits with documented refusals and coverage for rejected writes and concurrent requests. No concrete merge-blocking issue remains; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change adds abuse controls without a demonstrated new security weakness. Remaining uncertainty concerns deployment-wide enforcement and recovery after interrupted writes, rather than a verified bypass. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 6 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
/review |
|
@greptileai review |
|
@coderabbitai full review |
PR Summary by QodoAdd abuse admission to social, team, and forum writes
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
✅ Action performedFull review finished. |
Code Review by Qodo
1.
|
|
The account and address budgets alone still let one sender re-send a declined friend request to the same player about 5,760 times a day (exact-head Qodo finding). A friend request now also charges friendRequestRepeat.perPair (3 per 24 h), keyed friend-request:pair:<sender>:<recipient>, in the same atomic admission. The key names the sender first, so only the sender's own requests charge it: the recipient can still ask the sender, and others can still ask the recipient. The recipient is lower-cased so re-casing the UUID cannot reset the pair. The write-budget validator now carries each category's dimensions, so the new single-dimension policy is validated like the others. Both join routes admit inside their try block like the other five routes.
|
/review |
|
@greptileai review |
|
@coderabbitai full review |
|
|
Code review by qodo was updated up to the latest commit 7bd2cf6 |
An open PR does not reserve an increment; main's latest is 78.
|
/review |
|
@greptileai review |
|
@coderabbitai full review |
|
Code review by qodo was updated up to the latest commit 805081e |
✅ Action performedFull review finished. |
What this closes
Fable+Astra audit findings P1-35 (social writes) and P1-36 (team/forum writes). This is the "Social / messaging / teams / forums … no abuse budget" row of
docs/audits/FABLE_ASTRA_FULL_AUDIT.md.Reverified on
origin/main3a10e3c:Backend only. No
packages/web, ADR ordocs/audits/GEMINI_*changes, so no overlap with #81 or #80.Endpoint classification
POST /v1/social/follows/:playerIdsocialInitiationPOST /v1/social/friend-requestssocialInitiationDELETE /v1/social/follows/:playerIdPOST /v1/social/friend-requests/:id/respondPOST /v1/social/blocks/:playerIdDELETE /v1/social/blocks/:playerIdPOST /v1/messages/conversations,…/:id/messagesPOST /v1/teamsteamCreationPATCH /v1/teams/:idPOST /v1/teams/:id/membersteamJoinPOST /v1/teams/:id/join-requeststeamJoinDELETE /v1/teams/:id/members/:playerIdPATCH /v1/teams/:id/members/:playerIdPOST /v1/teams/:id/transfer-ownershipPOST /v1/teams/:id/join-requests/:reqId/respondDELETE /v1/teams/:id/join-requests/:reqIdPOST /v1/teams/:id/forum/threadsforumThreadCreationPATCH/DELETE /v1/teams/:id/forum/threads/:threadIdPOST /v1/teams/:id/forum/threads/:threadId/postsforumPostCreationPATCH/DELETE /v1/forum/posts/:postIdThe unmetered list is pinned by a structural test. Blocking an abuser, declining a request, leaving a team or deleting a post must never fail because a creation budget ran out.
Policy categories and rationale
socialInitiationconversationCreation: reaching out to another playerteamCreationregister/webauthnRegistershape for rarely repeated durable creationsteamJoinconversationCreation: reaching out to a teamforumThreadCreationconversationCreation: opening a threadforumPostCreationmessageSend: a replyfriendRequestRepeat(per sender → recipient pair)emailVerificationRequest.perUser(repeated sends to one inbox); a day-long window because the recipient bears the costf31385b: without it, the account budget alone let one sender re-send a declined request to the same player about 5,760 times a day. The key isfriend-request:pair:${sender}:${recipient-lowercased}, so only the sender can charge it. The recipient can still ask the sender, and others can still ask the recipient; tests prove both.NAT fairness and target lockout
seekCreation,conversationCreationandmessageSendalready use. Ten players behind one NAT can each use their full budget.<category>:user:${actorId}and<category>:ip:${ctx.ip ?? 'unknown'}. The one exception is the sender-first friend-request pair key. No key lets anyone but the caller spend it, so strangers aiming writes at a victim spend only their own budgets.Admission semantics
admit([user, ip]), the existing multi-bucket port, so a refusal charges neither bucket.Retry-After, and nothing is written.maindid not reject as 422.rateLimit.enabled = falsestill bypasses admission.validateMessagingRateLimitsbecamevalidateWriteRateLimitsover all eight authenticated-write categories. A missing, non-integer, non-positive or over-32-bit budget, or a malformed policy shape, fails startup.Retry-After. Follow and friend request also document their existing 404.openapi.jsonis regenerated.Evidence
Failing before: the final test files, run against
main'sroutes.tsandconfig.ts.maincommunity-admission.test.tsrate-limit-structure.test.tsconfig.test.tsThe 3 community tests that pass on
mainpin invariants that already held there: the victim's budgets are untouched, a disabled limiter charges nothing, and malformed input is rejected.Falsification: 23 disposable mutations of the final code, all caught. Sources were restored byte-identical from a disk backup after each.
enabled.socialInitiation(validator's final shape).friendRequestRepeat.Local validation:
check:*guard (8)The PostgreSQL suite includes a new two-replica test: shared budgets, refusal before write, and one-winner races for posts and for join vs. join request.
Reviews:
Retry-After/404 docs, and five test-strength gaps.f31385bit reported the same re-send gap as Qodo (fixed), a missingopenapi.json(a false positive: the file was only left out of the review patch), and one consistency point (fixed: both join routes now admit inside theirtrylike the other five).Not addressed here (pre-existing or out of scope)
POST /v1/studies/:id/collaboratorsis unmetered. It is study sharing, not P1-35/36, and is worth a follow-up.unknownIP bucket when a proxy dropsX-Forwarded-Foris a deployment concern shared by every metered route.editPosttakes the team advisory lock in PostgreSQL. Unmetered edits can contend with it, but deletion stays deliberately unmetered.PgRateLimitermeasures every bucket before rolling back a refusal. This is deliberate, soRetry-Afteris the longest wait, and it is unchanged.PROJECT_STATE
Appended as M15 Increment 79, the next number after main's latest (78); an open PR does not reserve an increment. Prior history is unchanged and append-only. Both this PR and #81 edit the
_Last updated_line, so whichever merges second renumbers its own entry and reconciles that line.Merge decision is the owner's.
Summary by CodeRabbit
429response with aRetry-Afterheader. Invalid requests are checked before they count toward limits.