Repository navigation
Fix Parameter #50/#52 authz on repo approval and email association (includes ME_DATA escape) - #444
Merged
Merged
Conversation
Fixes a stored XSS: app_html injects MeView.get_me_data() into a <script> block via json.dumps(), which does not escape <, >, or &. A repo's emailmap contains attacker-influenced sender email addresses, so a crafted from_email could break out of the script element and execute in a repo admin's browser. Escape <, >, & to their \uXXXX forms before injection (U+2028/U+2029 are already escaped by json.dumps' ensure_ascii default), matching Django's json_script scheme. Also add HSTS response headers to production (SECURE_HSTS_SECONDS=1y, includeSubDomains, preload), which were previously unset. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Parameter #52: POST /api/github/associate-email/<msg_uuid>/ only checked is_authenticated, so any logged-in user could create an EmailMap on any repo. The offer-associate email now links to /associate-email/<msg_uuid>/<token>/, where the token is a signed, expiring (30 day) django.core.signing payload of the message uuid and sender address. GET and POST require that token, or that the caller is an admin of the repo. A duplicate (repo, email) now returns 409 instead of a 500 IntegrityError. Parameter #50: POST /api/github/repository/<uuid>/approve/ was AllowAny and approved the repo for any user whose (self-chosen) Django username matched the inviter's GitHub login. The AllowAny override is gone, and approval now requires the caller's linked GitHub SocialAccount uid to equal original_invitation_data['inviter']['id'], still only while the repo is pending inviter approval. The anonymous "Verify your GitHub" page used the approve endpoint to show the repo name and inviter login; it now shows generic copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Accounts are only meant to come from GitHub login. Open local signup let anyone create an account with an arbitrary username, which is what made #50 and #52 reachable for outsiders. The social adapter stays open so GitHub signup keeps working (by default it defers to the account adapter). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uv rejects the git:// URL for the pinned django-allauth commit, which fails the "Requirements lockfile" check on master and this branch. Same change as #443: use git+https for the same commit, record that source in the lock, add a setuptools build constraint for its legacy setup.py, and have lock checks keep the existing pins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Fixes two open Parameter pentest highs on fire.fundersclub.com. The branch is cut from
security/xss-escape-and-hsts, so it includes d4f4e41 (ME_DATA JSON escape + HSTS, also open as #442). Merging this PR ships all three; #442 can be closed afterwards or merged first.#52: EmailMap write with no object authorization
POST /api/github/associate-email/<msg_uuid>/only checkedis_authenticated, so any logged-in user could create an EmailMap on any repo./associate-email/<msg_uuid>/<token>/. The token isdjango.core.signing, saltassociate-email, payload{msg uuid, from_email}, and it expires after 30 days (fb_github/utils.py).AssociateEmailViewGET and POST need a valid token for that exact message and sender, or the caller must be an admin of the repo (Repository.is_editable_by). A baremsg_uuidnow returns 404 on GET and 403 on POST.(repo, email)returns 409 instead of a 500IntegrityError.associate-email/:uuid/:tokenroute. The token stays in the path, so it survives the OAuth login redirect. The token-less route is kept for admins.#50: Repo approval gated by a self-chosen username
POST /api/github/repository/<uuid>/approve/:dispatchoverride that setAllowAny, soIsAuthenticatedapplies.approve()only approves when the caller's linked GitHubSocialAccount.uidequalsoriginal_invitation_data['inviter']['id'], and only while the status ispending-inviter-approval. It no longer compares the inviter login with the Django username.authenticate/:uuid) called this endpoint anonymously to show the repo name and inviter login. It now shows generic copy. Logged-in flows are unchanged.Also: close local password signup
Added
ACCOUNT_ADAPTER.is_open_for_signup → False, so outsiders can no longer make a local account with an arbitrary username. That is what made both findings reachable.SOCIALACCOUNT_ADAPTER.is_open_for_signup → Truekeeps GitHub signup open: by default allauth's social adapter defers to the account adapter, which would otherwise close GitHub signup too. Existing local accounts can still log in. It's in its own commit (08bce73) so it can be reverted separately.Behaviour changes to be aware of
EmailMap.loginis stillrequest.user.username, andapprove_by_inviteralso uses the username. With signup closed, new accounts come from GitHub, but any existing local accounts keep their self-chosen usernames.Overlap with #443
git://allauth URL. I applied Handle repeat email associations and restrict error testing #443's four dependency-file changes unchanged (py-requirements/base.txt,src/requirements.lock.txt,src/scripts/deps.sh,src/build-constraints.txt), so those hunks should merge cleanly whichever PR lands first. This switches the locked allauth from PyPI 0.40.0 to the pinned git commit (0.41.0-dev). The tests pass against it.AssociateEmailView.post(get_or_create plus a 409) and adds tests that associate with a baremsg_uuidas a non-admin. This PR deliberately rejects that, so whichever lands second needs a manual merge, and Handle repeat email associations and restrict error testing #443'stest_email_association.pywill need the token URL.Tests
test_approverewritten: anonymous calls get 403, and the repo is only approved by the inviter's GitHub uid. New negative cases cover a local user whose username equals the inviter login, and a different linked GitHub account.AssociateEmailAPITestCase: a raw uuid is rejected, and so are bad, foreign, wrong-salt, wrong-email and expired tokens. Covers the sender with a token, the repo admin without one, an admin of another repo (rejected), and the duplicate (409).test_association_emailchecks the emailed link has a valid token.test_signup: local signup is closed, GitHub signup is open.python manage.py test: 20 tests, OK (flake8 included). Run locally on Python 3.10, with psycopg2 2.8.6 swapped in locally and acollections.abcshim for the old celery. The lockfile is unchanged. The Angular changes were not compiled locally (no node_modules). They're small, and I kept them TS 2.0 / AOT-safe.🤖 Generated with Claude Code