Fix immediate security findings#733
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds SIWE nonce purposes and validation helpers, enforces staff-only profile mutations, centralizes submission gating and duplicate-evidence timestamp logic, auto-detects evidence URL types, and sanitizes rendered Markdown on the frontend. ChangesEthereum Authentication with Nonce Purposes
Profile Mutation Authorization
Contribution Submission and Steward Review Flow
Frontend Markdown Sanitization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/.env.example (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
testservertoALLOWED_HOSTSin the example env.
ALLOWED_HOSTSis missingtestserver, which can break test requests that use Django’s default test host.Suggested change
-ALLOWED_HOSTS=localhost,127.0.0.1 +ALLOWED_HOSTS=localhost,127.0.0.1,testserverAs per coding guidelines: "
backend/**/.env*: Add 'testserver' to ALLOWED_HOSTS in .env configuration for tests to work properly".🤖 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 `@backend/.env.example` at line 4, Update the ALLOWED_HOSTS entry in the example environment to include "testserver" so Django tests using the default test host succeed; specifically modify the ALLOWED_HOSTS variable string (ALLOWED_HOSTS=localhost,127.0.0.1) to also contain testserver (e.g., ALLOWED_HOSTS=localhost,127.0.0.1,testserver).backend/contributions/management/commands/review_submissions.py (1)
184-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
skip_pendingbefore consultingurl_to_sub_ids.
handle()setsskip_pending_duplicatesfor--submission-id, but_check_single_url_duplicate()never uses that flag. Single-submission runs will still reject against pending/newer submissions viaurl_to_sub_ids, which reintroduces the order-dependent behavior this PR is trying to remove.Suggested fix
if normalized in accepted_urls: return ( 'Reject: Duplicate Submission', f'Tier 1 auto-reject: Evidence URL already exists in an ' f'accepted contribution: {evidence.url[:100]}', ) + + if skip_pending: + return None + # Check pending/accepted submitted contributions (exclude self) others = (url_to_sub_ids.get(normalized) or set()) - {submission.id}🤖 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 `@backend/contributions/management/commands/review_submissions.py` around lines 184 - 216, The _check_single_url_duplicate function currently consults url_to_sub_ids and rejects against pending submissions even when the command set skip_pending_duplicates; update _check_single_url_duplicate to respect that flag by filtering out pending submission IDs from others before returning or loading created_at data: when skip_pending_duplicates is True remove any IDs whose status is pending/new (e.g. via an existing status map or a DB lookup on SubmittedContribution.objects.filter(id__in=others).values_list('id','status')) so that url_to_sub_ids and missing_created_at_ids only include non-pending submissions; ensure the function signature or caller passes skip_pending_duplicates (from handle()) if needed and keep the later duplicate-created_at comparison unchanged.
🤖 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 `@backend/validators/tests/test_api.py`:
- Around line 42-73: Add a negative test to backend/validators/tests/test_api.py
that verifies PATCH /api/v1/validators/me/ rejects unsupported keys: call
self.client.patch('/api/v1/validators/me/', {'unsupported_field': 'x'}) and
assert response.status_code == status.HTTP_400_BAD_REQUEST and that no Validator
was created for self.user; place this alongside
test_patch_validator_profile_does_not_create_missing_profile so the
unsupported-field guard on the view/serializer (PATCH /validators/me/) is
covered and will fail if future changes allow arbitrary keys.
In `@frontend/src/lib/markdownLoader.js`:
- Around line 4-22: Update the SANITIZE_CONFIG object in markdownLoader.js to
use an explicit whitelist and tighter rules: replace or augment FORBID_TAGS with
an ALLOWED_TAGS array listing only the safe HTML tags your markdown needs (e.g.,
p, a, ul, ol, li, strong, em, img, h1-h6, pre, code), add ALLOW_DATA_ATTR: false
to disable data-* attributes, and add an ALLOWED_URI_REGEXP entry that permits
only safe schemes (e.g., ^(https?|mailto):) for link/src attributes; keep
FORBID_ATTR: ['style'] (and remove any overlapping forbidden tags) so DOMPurify
uses the explicit whitelist and URI regexp for stronger defense-in-depth when
sanitizing via SANITIZE_CONFIG.
In `@frontend/src/tests/markdownLoader.test.js`:
- Around line 13-27: Add extra edge-case unit tests to markdownLoader.test.js
that call parseMarkdown and assert the output strips additional XSS vectors:
include inputs with data:text/html URLs, other event handlers (onload,
onmouseover) on multiple elements, <object> and <embed> tags, <form> and <input>
elements, and SVG payloads like <svg onload="...">; for each case assert the
returned HTML does not contain the forbidden tags/attributes/URLs (e.g.,
'data:text/html', 'onload', 'onmouseover', '<object', '<embed', '<form',
'<input', '<svg') and reference the existing FORBID_TAGS behavior to ensure
parity with sanitizer expectations.
---
Outside diff comments:
In `@backend/.env.example`:
- Line 4: Update the ALLOWED_HOSTS entry in the example environment to include
"testserver" so Django tests using the default test host succeed; specifically
modify the ALLOWED_HOSTS variable string (ALLOWED_HOSTS=localhost,127.0.0.1) to
also contain testserver (e.g., ALLOWED_HOSTS=localhost,127.0.0.1,testserver).
In `@backend/contributions/management/commands/review_submissions.py`:
- Around line 184-216: The _check_single_url_duplicate function currently
consults url_to_sub_ids and rejects against pending submissions even when the
command set skip_pending_duplicates; update _check_single_url_duplicate to
respect that flag by filtering out pending submission IDs from others before
returning or loading created_at data: when skip_pending_duplicates is True
remove any IDs whose status is pending/new (e.g. via an existing status map or a
DB lookup on
SubmittedContribution.objects.filter(id__in=others).values_list('id','status'))
so that url_to_sub_ids and missing_created_at_ids only include non-pending
submissions; ensure the function signature or caller passes
skip_pending_duplicates (from handle()) if needed and keep the later
duplicate-created_at comparison unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3b5b5cce-bee2-403f-8d72-051054b262a0
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
backend/.env.examplebackend/2025-05-27.sqlite3backend/builders/tests.pybackend/builders/views.pybackend/contributions/management/commands/review_submissions.pybackend/contributions/models.pybackend/contributions/serializers.pybackend/contributions/tests/test_review_submissions.pybackend/contributions/tests/test_steward_permissions.pybackend/contributions/tests/test_validator_category_gating.pybackend/contributions/views.pybackend/docker-compose.ymlbackend/ethereum_auth/migrations/0002_nonce_purpose.pybackend/ethereum_auth/models.pybackend/ethereum_auth/siwe_utils.pybackend/ethereum_auth/tests.pybackend/ethereum_auth/views.pybackend/poaps/tests/test_poaps.pybackend/poaps/views.pybackend/stewards/tests.pybackend/stewards/views.pybackend/validators/tests.pybackend/validators/tests/test_api.pybackend/validators/views.pyfrontend/package.jsonfrontend/src/lib/auth.jsfrontend/src/lib/markdownLoader.jsfrontend/src/routes/PoapRecovery.sveltefrontend/src/tests/markdownLoader.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/contributions/models.py (1)
749-757:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale
url_typewhen the URL is removed.The new auto-detection never resets
url_typein theelsebranch, so an evidence row can become description-only while still carrying its previous URL classification.🔧 Proposed fix
else: + self.url_type = None self.normalized_url = ''🤖 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 `@backend/contributions/models.py` around lines 749 - 757, The save method in the Contribution model leaves a stale url_type when self.url is empty; update the else branch in save to clear the URL classification by setting self.url_type = None and self.url_type_id = None (and ensure self.normalized_url = '' remains), so removal of a URL resets both the foreign-key object and its id; reference the save method and the fields/methods url, url_type, url_type_id, normalized_url, detect_url_type, normalize_url when making this change.backend/contributions/views.py (1)
47-47: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBackfill
url_typewhen copying accepted Evidence viabulk_create.
bulk_create()bypassesEvidence.save(), so if sourceEvidence.url_typeisNULL, the accepted copy staysNULL. Duplicate checking has fallbacks forurl_type=NULL, so this isn’t anallow_duplicate-correctness issue, but it does leave accepted Evidence with missingurl_typein API/UI data.🔧 Proposed fix
-from .url_utils import normalize_url +from .url_utils import detect_url_type, normalize_url ... - url_type=evidence.url_type, + url_type=( + evidence.url_type + or (detect_url_type(evidence.url) if evidence.url else None) + ), normalized_url=normalize_url(evidence.url) if evidence.url else '',🤖 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 `@backend/contributions/views.py` at line 47, When copying accepted Evidence objects via bulk_create, ensure you backfill url_type for each new Evidence because bulk_create bypasses Evidence.save(); update the code that prepares the list for Evidence.objects.bulk_create(...) to set instance.url_type = source.url_type or, if source.url_type is None, compute and assign it (e.g., call normalize_url(source.url) or the existing URL-type helper) so every created Evidence has a non-null url_type for API/UI usage.
🤖 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 `@backend/poaps/views.py`:
- Around line 360-364: This action exposes another user's POAPs by resolving an
arbitrary `address`; before calling get_object_or_404 you must enforce a
self-or-staff guard: verify that request.user either is staff
(request.user.is_staff or request.user.is_superuser) or that the resolved
account/wallet belongs to request.user (compare request.user.wallet_address /
account.id / username as appropriate), and return a 403/raise PermissionDenied
if neither condition holds; alternatively apply an explicit permission check
(e.g., a custom IsOwnerOrStaff permission) at the start of the viewset action to
protect the address-based lookup in this action.
---
Outside diff comments:
In `@backend/contributions/models.py`:
- Around line 749-757: The save method in the Contribution model leaves a stale
url_type when self.url is empty; update the else branch in save to clear the URL
classification by setting self.url_type = None and self.url_type_id = None (and
ensure self.normalized_url = '' remains), so removal of a URL resets both the
foreign-key object and its id; reference the save method and the fields/methods
url, url_type, url_type_id, normalized_url, detect_url_type, normalize_url when
making this change.
In `@backend/contributions/views.py`:
- Line 47: When copying accepted Evidence objects via bulk_create, ensure you
backfill url_type for each new Evidence because bulk_create bypasses
Evidence.save(); update the code that prepares the list for
Evidence.objects.bulk_create(...) to set instance.url_type = source.url_type or,
if source.url_type is None, compute and assign it (e.g., call
normalize_url(source.url) or the existing URL-type helper) so every created
Evidence has a non-null url_type for API/UI usage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91cd8d1c-de08-4797-ad7b-a5f53e38815f
📒 Files selected for processing (7)
backend/contributions/models.pybackend/contributions/tests/test_steward_permissions.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/poaps/tests/test_poaps.pybackend/poaps/views.pybackend/validators/views.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/contributions/models.py (1)
749-757:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale
url_typewhen the URL is removed.The new auto-detection never resets
url_typein theelsebranch, so an evidence row can become description-only while still carrying its previous URL classification.🔧 Proposed fix
else: + self.url_type = None self.normalized_url = ''🤖 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 `@backend/contributions/models.py` around lines 749 - 757, The save method in the Contribution model leaves a stale url_type when self.url is empty; update the else branch in save to clear the URL classification by setting self.url_type = None and self.url_type_id = None (and ensure self.normalized_url = '' remains), so removal of a URL resets both the foreign-key object and its id; reference the save method and the fields/methods url, url_type, url_type_id, normalized_url, detect_url_type, normalize_url when making this change.backend/contributions/views.py (1)
47-47: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBackfill
url_typewhen copying accepted Evidence viabulk_create.
bulk_create()bypassesEvidence.save(), so if sourceEvidence.url_typeisNULL, the accepted copy staysNULL. Duplicate checking has fallbacks forurl_type=NULL, so this isn’t anallow_duplicate-correctness issue, but it does leave accepted Evidence with missingurl_typein API/UI data.🔧 Proposed fix
-from .url_utils import normalize_url +from .url_utils import detect_url_type, normalize_url ... - url_type=evidence.url_type, + url_type=( + evidence.url_type + or (detect_url_type(evidence.url) if evidence.url else None) + ), normalized_url=normalize_url(evidence.url) if evidence.url else '',🤖 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 `@backend/contributions/views.py` at line 47, When copying accepted Evidence objects via bulk_create, ensure you backfill url_type for each new Evidence because bulk_create bypasses Evidence.save(); update the code that prepares the list for Evidence.objects.bulk_create(...) to set instance.url_type = source.url_type or, if source.url_type is None, compute and assign it (e.g., call normalize_url(source.url) or the existing URL-type helper) so every created Evidence has a non-null url_type for API/UI usage.
🤖 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 `@backend/poaps/views.py`:
- Around line 360-364: This action exposes another user's POAPs by resolving an
arbitrary `address`; before calling get_object_or_404 you must enforce a
self-or-staff guard: verify that request.user either is staff
(request.user.is_staff or request.user.is_superuser) or that the resolved
account/wallet belongs to request.user (compare request.user.wallet_address /
account.id / username as appropriate), and return a 403/raise PermissionDenied
if neither condition holds; alternatively apply an explicit permission check
(e.g., a custom IsOwnerOrStaff permission) at the start of the viewset action to
protect the address-based lookup in this action.
---
Outside diff comments:
In `@backend/contributions/models.py`:
- Around line 749-757: The save method in the Contribution model leaves a stale
url_type when self.url is empty; update the else branch in save to clear the URL
classification by setting self.url_type = None and self.url_type_id = None (and
ensure self.normalized_url = '' remains), so removal of a URL resets both the
foreign-key object and its id; reference the save method and the fields/methods
url, url_type, url_type_id, normalized_url, detect_url_type, normalize_url when
making this change.
In `@backend/contributions/views.py`:
- Line 47: When copying accepted Evidence objects via bulk_create, ensure you
backfill url_type for each new Evidence because bulk_create bypasses
Evidence.save(); update the code that prepares the list for
Evidence.objects.bulk_create(...) to set instance.url_type = source.url_type or,
if source.url_type is None, compute and assign it (e.g., call
normalize_url(source.url) or the existing URL-type helper) so every created
Evidence has a non-null url_type for API/UI usage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 91cd8d1c-de08-4797-ad7b-a5f53e38815f
📒 Files selected for processing (7)
backend/contributions/models.pybackend/contributions/tests/test_steward_permissions.pybackend/contributions/views.pybackend/ethereum_auth/views.pybackend/poaps/tests/test_poaps.pybackend/poaps/views.pybackend/validators/views.py
🛑 Comments failed to post (1)
backend/poaps/views.py (1)
360-364:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce self-or-staff access on this address-based POAP route.
This action still resolves an arbitrary
addressand returns that account’s POAP history, so any authenticated caller can enumerate another user’s claims by wallet. Authentication alone does not close the IDOR here; gate the action to the owner of the address or staff beforeget_object_or_404().🔒 Suggested guard
def poaps(self, request, address=None): from django.shortcuts import get_object_or_404 from users.models import User + if ( + not request.user.is_staff + and normalize_wallet_address(address) != normalize_wallet_address(request.user.address) + ): + return Response({'detail': 'Not found.'}, status=status.HTTP_404_NOT_FOUND) + user = get_object_or_404(User, address__iexact=address)As per coding guidelines,
backend/{...}/views.py: “Flag any handler/viewset/action that fetches a resource by id, slug, address, wallet, username, path param, or query param without proving object ownership, steward/admin role, or an explicit permission check on that exact resource.”🤖 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 `@backend/poaps/views.py` around lines 360 - 364, This action exposes another user's POAPs by resolving an arbitrary `address`; before calling get_object_or_404 you must enforce a self-or-staff guard: verify that request.user either is staff (request.user.is_staff or request.user.is_superuser) or that the resolved account/wallet belongs to request.user (compare request.user.wallet_address / account.id / username as appropriate), and return a 403/raise PermissionDenied if neither condition holds; alternatively apply an explicit permission check (e.g., a custom IsOwnerOrStaff permission) at the start of the viewset action to protect the address-based lookup in this action.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the immediate security fixes across SIWE/POAP nonce purpose validation, Markdown sanitization, role profile mutation restrictions, submission update gating, steward review hardening, duplicate-evidence ordering, and removal of the tracked SQLite artifact. Also updates local SIWE config handling for localhost:5173 and adds focused regression coverage for the touched auth, POAP, role, submission, review, duplicate-evidence, and Markdown flows. Verification: backend touched-surface suite passed with 132 tests OK; focused frontend Markdown sanitizer test passed.
Summary by CodeRabbit
Bug Fixes
New Features
Tests