Can't remove deleted organizations from crate owners

[DianaNites]

Title. Also see similar? issue #1512, which the author closed.

The error is

error: failed to remove owners from crate

Caused by:
  api errors (status 404 Not Found): Not Found

A workaround exists, which is to recreate the organization and team, which lets you delete it.

But that may also be a potential security issue, what if someone else creates it and maliciously abuses their newly granted publish privileges? What about for organization renames?

[miraclx]

Any progress here?

[carols10cents]

If there was progress, it would be posted to the issue.

[carols10cents]

Over on #7042, @nic-hartley said:

Happy to put in a PR about this, it doesn't seem like it'd be a huge change. Though I'd need a pointer to. uh. where in this repo to start looking. it's quite large and intimidating.

All of the github related code is in https://github.com/rust-lang/crates.io/blob/main/src/github.rs; hopefully that's a useful starting place? I can provide more pointers later if needed.

The problem is that the current code is asking GitHub for existence of the org/team before removing it from the crate's owners, and the code should not need to check anything with GitHub in this case; it should just change the record in crate_owners for that team+crate to deleted='t'.

[nic-hartley]

@carols10cents I appreciate the pointer!

A quick fix does seem entirely doable to me (another variant or parameter for Team::create_or_update to disable the remote check and only look locally).

But as pointed out in the original post I do think this is a security issue, if only a minor one, so it might be better to build in some "does it still exist" check for crate owners, and delete GitHub organizations when they stop existing. Not sure how to build that, though, especially considering edge cases (what if the last owner of a crate gets deleted?)

I'll start a pull request (hopefully tonight! But I've got some boxes to carry.) with the quick fix, hopefully folks more experienced with the codebase have ideas for the autoremove. I don't think this issue occurs with normal users but I'll test it and make sure.

[carols10cents]

We don't currently poll GitHub for deletions of orgs/teams/users at all, and I don't think we need to add that. There are GitHub organization IDs and team IDs that change if ownership of the GitHub organization changes.

Also, people who own a crate only via a team can't modify owners (they can only publish/yank). And we have a check that won't let you remove the last owner of a crate.

So here's what crates.io sees:

• User abcd publishes (and now owns) crate some_crate.
• User abcd adds GitHub team some_org/some_team as an owner of some_crate.
• Crates.io asks GitHub about some_org/some_team and GitHub tells crates.io that some_org has org id 123 and some_team has team id 456, which crates.io stores in our database.
• If the team gets deleted in GitHub, then user abcd tells crates.io "remove some_org/some_team as an owner of some_crate", we shouldn't need to ask GitHub anything, we can just mark that crate_owner relationship as deleted in our database. It doesn't much matter whether the team is in GitHub anymore or not (but currently we require that it exists, that's the bug).
• If the org gets deleted in GitHub, then user abcd tells crates.io "remove some_org/some_team as an owner of some_crate", we also shouldn't need to ask GitHub anything.
• If the org gets deleted in GitHub, then user zzzz creates the org some_org in Github, and creates team some_team in GitHub, this some_org won't have GitHub ID 123. User zzzz won't have any permissions to do anything with some_crate in crates.io because the IDs won't match. User abcd should still be able to remove some_org/some_team as an owner of some_crate at this point.

Am I missing some case you're thinking of that would be a security problem?

[nic-hartley]

@carols10cents

Ah, okay, I didn't realize we were tracking org ID rather than org name, my bad. Org name is all that gets presented in the UI and I only just started looking into the codebase. As long as there's no way to "reacquire" an old org ID, then we're good.

Current plan is to basically add a boolean parameter that's actually used in Team::create_or_update_github_team but gets passed in from as far up as Crate::owner_add/Crate::owner_remove, indicating whether to look up the team in the GitHub API or the database. I'll put up the PR just as soon as I finish the first commit.

[edmorley]

I'm running into a similar issue, where the org/team I'm trying to delete still exists, but the GitHub org has since enabled the IP allow-listing GitHub feature, so crates.io is no longer able to verify if the org/team exists.

Attempts to delete, result in:

Failed to remove the team <team> as crate owner: could not find the github team <team>

I'm presuming the switch to looking up from the DB (and not making requests to GitHub) in #7051 will presumably fix our case too?

[nic-hartley]

@edmorley I'm reasonably sure you're correct that this will fix your case, yes. With 7051 we wouldn't hit the API at all for owner removal, so whatever the API would say is irrelevant.