Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deleted_crates database table #9912

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 12, 2024

This table will be used to record when a crate was deleted, (optionally) why it was deleted, who deleted it, and when it will be available for re-creation again. I've kept the database dump visibility as private for now. We can decide later whether to make it public or not, and what fields to include.

The delete-crate admin tool is adjusted to fill the new table, optionally recording --deleted-by and --message parameters. Note that the admin tool defaults to available_at = deleted_at for now, to match the current behavior. This can be adjusted in a dedicated pull request if we decide to do so.

The publish endpoint is adjusted to check the new table for availability of the crate name before accepting the publish.

This PR should address #9904 (comment) by making it possible for the crate deletion endpoint to set available_at = deleted_at + 24 hours or whatever other value we choose (including 0 hours). It also addresses a couple of requests/comments over the years to track deletions in a more structured way than adding a comment to a Zulip thread.

This table will be used to record when a crate was deleted, (optionally) why it was deleted, who deleted it, and when it will be available for re-creation again.
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Nov 12, 2024
@Turbo87 Turbo87 requested a review from a team November 12, 2024 14:57
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 58.49057% with 44 lines in your changes missing coverage. Please review.

Project coverage is 88.98%. Comparing base (12e3436) to head (eb45233).

Files with missing lines Patch % Lines
src/bin/crates-admin/delete_crate.rs 0.00% 33 Missing ⚠️
src/models/user.rs 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9912      +/-   ##
==========================================
- Coverage   89.08%   88.98%   -0.10%     
==========================================
  Files         290      292       +2     
  Lines       30086    30189     +103     
==========================================
+ Hits        26802    26864      +62     
- Misses       3284     3325      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LawnGnome
Copy link
Contributor

@walterhpearce and I had a discussion this morning, and he's going to write it up in #9904, but we're probably going to need to track the versions in deleted crates as well. So let's hold this for now pending that write up, please.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 13, 2024

I don't think #9904 is the right place for this though. We already perform deletions through the admin tool and that PR is just enabling users to do it by themselves.

he's going to write it up

when? the RFC has been merged 2.5 months ago. would've been good to get input back then when it was still open...

@walterhpearce
Copy link

The main concern I discussed with @LawnGnome is around immediate takeover of a crate for malicious purposes.

The threat scenario is the event of a author deleting a crate, and an attacker publishing the same name and version, or the same name and a semver-compatible version.

In both of these cases, what mechanisms exist to notify the user that this is occuring? Does Cargo.lock checksums prevent the immediate update scenario? What about updates occuring for a semver-compat update?

I think it is an open question on where the responsibility for this lies - the open question was whether crates should enforce such a version-prevention mechanisms (force a major version bump), or should a flag be present for cargo to notify the user an author change occured?

My gut says option #2, the user notification, is the ideal scenario here.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 13, 2024

Does Cargo.lock checksums prevent the immediate update scenario?

yes, unless there is a bug in cargo that skips the checksum checks that should be the case

What about updates occuring for a semver-compat update?

the crate deletion RFC gives 4 different conditions that need to be met for users to be able to self-delete crates:

  • the crate is less than 3 days old (makes it unlikely for a large number of users to already rely on it, and, if so, makes it unlikely for the maintainer to not realize the impact and still delete it)
  • or the crate has less than X downloads, only one maintainer, and no (public) reverse dependencies

the combination of these IMHO makes this a somewhat unlikely scenario.

the open question was whether crates should enforce such a version-prevention mechanisms (force a major version bump)

the same could be said about crate transfers, which we already allow. but for the common use case of "I named my crate actix_web, but realized it should have been actix-web" it would be a bit unfortunate for the author to not be able to republish their existing versions. I guess it could be a limit only if a different author uses the name, but I'm not sure it is really worth the complexity.

should a flag be present for cargo to notify the user an author change occured?

I'm open to something like that, but that needs design work and a) I don't know enough about cargo to be able to do that and b) I don't think we should block the deletion tracking on an implementation on the cargo side, since that would likely delay the RFC implementation by months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants