[SECUR-236] fix: grouped-paginator cursor bound + Project mass-assignment - #9486
[SECUR-236] fix: grouped-paginator cursor bound + Project mass-assignment#9486mguptahub wants to merge 4 commits into
Conversation
…imiting AppScan DAST remediation, ported from the plane-ee fix (#8591) and re-verified against a live plane-ce instance. - paginator: reject non-positive per_page (per_page=0 -> ZeroDivisionError -> HTTP 500) and bound the client-supplied cursor value/offset. The grouped paginators use cursor.value as the per-group page size: a negative value slices the queryset with a negative stop (ValueError -> HTTP 500) and a huge value fetches far more than max_per_page rows per group (cap bypass / DoS). One central guard in BasePaginator.paginate(); regression tests added. - auth: sign-in/sign-up (app + space) were plain Views with no rate limiting. Add the IP-based AuthenticationThrottle check plus a per-account throttle keyed on the normalized email (AuthenticationAccountThrottle). The IP key is bypassable by spoofing X-Forwarded-For (NUM_PROXIES unset); the per-account limiter caps credential guessing against a single account regardless of IP. - project serializer: mark created_by/updated_by read-only (fields="__all__" left them client-writable, allowing project ownership/attribution forgery). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThe paginator now validates cursor bounds before execution, with tests for invalid and maximum-bound cursors. The project serializer marks ChangesPagination validation
Project audit-field protection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/plane/authentication/rate_limit.py (1)
52-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new account throttle.
This PR adds 17 paginator unit tests but nothing exercises
AuthenticationAccountThrottle.get_cache_key(email vs. no-email fallback, normalization) orauthentication_account_throttle_allows. Given this is security-critical brute-force protection, a few focused unit tests (cache-key bucketing by normalized email, fallback to IP when email absent, allow/deny across the rate window) would be valuable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/authentication/rate_limit.py` around lines 52 - 76, Add focused unit tests for AuthenticationAccountThrottle.get_cache_key covering normalized email bucketing, distinct emails, and IP fallback when email is absent; also test authentication_account_throttle_allows across the configured rate window to verify initial requests are allowed and excess requests are denied.apps/api/plane/authentication/views/app/email.py (1)
33-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated throttle-check-and-redirect block into a shared helper. The same 16-line pattern (call
authentication_throttle_allows+authentication_account_throttle_allows, build aRATE_LIMIT_EXCEEDEDAuthenticationException, and safe-redirect) is copy-pasted across all four sign-in/sign-up handlers. A single helper (e.g. inrate_limit.py, parameterized byis_app/is_space) would remove the duplication and prevent future drift in this security-critical gating logic.
apps/api/plane/authentication/views/app/email.py#L33-L48: replace with a call to a sharedauthentication_rate_limit_redirect(request, next_path, is_app=True)helper that returnsNoneor the redirect.apps/api/plane/authentication/views/app/email.py#L158-L172: same replacement,is_app=True.apps/api/plane/authentication/views/space/email.py#L32-L47: same replacement,is_space=True.apps/api/plane/authentication/views/space/email.py#L133-L147: same replacement,is_space=True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/authentication/views/app/email.py` around lines 33 - 48, Extract the duplicated authentication throttle check and RATE_LIMIT_EXCEEDED redirect into a shared authentication_rate_limit_redirect helper, parameterized for app or space redirects and returning None when allowed. In apps/api/plane/authentication/views/app/email.py lines 33-48 and 158-172, replace each block with the helper using is_app=True; in apps/api/plane/authentication/views/space/email.py lines 32-47 and 133-147, replace each with is_space=True.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/authentication/rate_limit.py`:
- Around line 52-69: Update AuthenticationAccountThrottle.get_cache_key so an
attacker cannot exhaust a victim’s throttle bucket solely by submitting the
victim’s email; derive the key from server-validated identity when available, or
combine the normalized email with the client identity. Preserve email
normalization and the existing IP fallback for requests without an email.
- Around line 63-64: Validate AUTHENTICATION_ACCOUNT_RATE_LIMIT before assigning
or using it in the authentication account throttle, ensuring it contains exactly
a numeric request count and a supported period in the expected “count/period”
format. Fall back to the existing default rate when the environment value is
empty or malformed, so AuthenticationAccountThrottle initialization remains safe
for every auth request.
---
Nitpick comments:
In `@apps/api/plane/authentication/rate_limit.py`:
- Around line 52-76: Add focused unit tests for
AuthenticationAccountThrottle.get_cache_key covering normalized email bucketing,
distinct emails, and IP fallback when email is absent; also test
authentication_account_throttle_allows across the configured rate window to
verify initial requests are allowed and excess requests are denied.
In `@apps/api/plane/authentication/views/app/email.py`:
- Around line 33-48: Extract the duplicated authentication throttle check and
RATE_LIMIT_EXCEEDED redirect into a shared authentication_rate_limit_redirect
helper, parameterized for app or space redirects and returning None when
allowed. In apps/api/plane/authentication/views/app/email.py lines 33-48 and
158-172, replace each block with the helper using is_app=True; in
apps/api/plane/authentication/views/space/email.py lines 32-47 and 133-147,
replace each with is_space=True.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bfd2980-c1b1-4421-8d33-189cfabc326c
📒 Files selected for processing (6)
apps/api/plane/app/serializers/project.pyapps/api/plane/authentication/rate_limit.pyapps/api/plane/authentication/views/app/email.pyapps/api/plane/authentication/views/space/email.pyapps/api/plane/tests/unit/utils/test_paginator.pyapps/api/plane/utils/paginator.py
…+ rate guard - AuthenticationAccountThrottle keyed on (email + client IP) instead of email alone. An email-only key let anyone lock a victim out of their own account by spamming their address from other IPs (self-inflicted lockout DoS). Combining with the client IP prevents that while still capping single-source guessing. - Guard the AUTHENTICATION_ACCOUNT_RATE_LIMIT env value: a malformed rate would raise in DRF parse_rate() on every auth POST (throttle is instantiated per request), taking authentication down instance-wide. Falls back to the default. - Add regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — both valid, fixed in 3257ee9:
Added regression tests for both ( |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…9429 / #9335 Two parts of this PR fully overlapped existing open PRs against preview, so per the "drop the most recent on full overlap" call they are removed here: - per_page non-positive guard (get_per_page) -> covered identically by #9429. - password sign-in/sign-up rate limiting -> covered (more cleanly, via a decorator) by #9335 (GHSA-349j-pjw5-67q4). Reverted rate_limit.py and email.py (app + space) and removed the auth unit tests. This PR now carries only its unique, non-overlapping fixes: - grouped-paginator cursor bound in paginate() (cap-bypass DoS + negative-slice 500 that #9429 does not address), with TestCursorBounds. - Project created_by/updated_by read-only (mass-assignment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mguptahub
changed the title
Summary
AppScan DAST remediation for SECUR-236 (plane-ee counterpart: makeplane/plane-ee#8591). This PR was trimmed to its unique, non-overlapping fixes after two parts were found to fully overlap existing open PRs against
preview:What remains in this PR
🟠 MEDIUM + 🟡 LOW — grouped-paginator cursor bound (not addressed by #9429)
GroupedOffsetPaginatortakes its per-group page size from the clientcursor.value, not the cappedlimit.cursor=1000000:0:0→ up to 1M rows/group (bypassesmax_per_page, DoS; reachable unauthenticated on public boards);cursor=-1:0:0→ negative queryset slice →ValueError→ HTTP 500. Fix bounds the parsed cursor (0 <= value <= max_per_page,offset >= 0) centrally inpaginate(). Regression tests:TestCursorBounds.🟡 LOW — Project mass-assignment
ProjectSerializer(fields="__all__") leftcreated_by/updated_byclient-writable;BaseModel.save()only backfillscreated_bywhenNone, so a supplied value survived — allowing ownership/attribution forgery. Fix: add them toread_only_fields.Testing
TestCursorBoundscovers reject (-1:0:0,1000000:0:0,20:-1:0) + valid/at-max cursors.Note
Merge order: this PR's per_page/auth coverage now lives in #9429 and #9335 respectively — those should land for full SECUR-236 coverage on CE.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Security