Skip to content

feat: validate usernames to avoid RepoJacking - #239

Open
typed-sigterm wants to merge 2 commits into
OpenRailAssociation:mainfrom
typed-sigterm:feat/validate-usernames
Open

typed-sigterm wants to merge 2 commits into
OpenRailAssociation:mainfrom
typed-sigterm:feat/validate-usernames

Conversation

@typed-sigterm

Copy link
Copy Markdown
Contributor

Resolve #234.

@mxmehl
mxmehl self-requested a review August 12, 2026 13:55

@mxmehl mxmehl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread gh_org_mgr/_gh_org.py
configured_users.update(self.configured_org_owners)

# Collect configured team members and maintainers
for team_attrs in self.configured_teams.values():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check username-ID mapping before each run

2 participants