feat: validate usernames to avoid RepoJacking - #239
typed-sigterm wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks for tackling this! I tried to reproduce in a real case. I renamed a test user in our test org and ran the tool with the current main. Since GitHub doesn't redirect old logins for the REST user endpoint (confirmed in their docs: "links to your previous profile page... will return a 404 error"), _resolve_gh_username('john1') fails, gets logged as an ERROR, and the sync just skips them and continues - which then leads to the renamed account (john2, still a real, current member) being treated as unconfigured and removed from the team.
But in your PR, you run a get_user() for every user. In an org with 100+ members, this is costly in terms of rate limit and time. Also, the detection of renaming does not seem to work at all, as users don't get redirected.
As an alternative, I'd suggest something smaller than a dedicated upfront validation pass: _resolve_gh_username() already detects exactly this failure mode (it's the source of the "does not exist on GitHub. Spelling error or did they rename themselves?" error). Right now callers just continue past that failure. If we instead treat that as fatal (log CRITICAL and abort the whole run) we get the same protection without:
- a new method or new upfront resolution pass (no added API calls beyond what's already made)
- reordering anything in
manage.py - doubling
get_user()calls for the same username (once in validation, potentially again during the actual add/update)
I looked through sync_teams_members: on a rename, the "configured but missing from current" loop (old username) always runs before the "current but not configured" loop (new username), and both hang off the same "members differ" check. So aborting on the first loop's resolution failure reliably stops execution before the second loop can act on the now-orphaned new login, at least for team members. I haven't traced sync_org_owners/collaborators as closely but the same pattern should apply since they use the same helper.
Trade-off: this only fires when the mismatch actually causes a diff (which a rename always does, since it produces both a missing old name and an unconfigured new name). A purely coincidental/already-in-sync case wouldn't hit this at all, but there's nothing to protect in that case anyway.
What do you think about this proposal? Have I missed something?
EDIT: This is probably related to #247
| configured_users.update(self.configured_org_owners) | ||
|
|
||
| # Collect configured team members and maintainers | ||
| for team_attrs in self.configured_teams.values(): |
There was a problem hiding this comment.
configured_repos_collaborators isn't included in the configured_users set, so individually-added repo collaborators aren't covered by this check. Since they can hold direct repo permissions, they're just as exposed to the same attack as team members/owners. Worth adding them here too.
Resolve #234.