Repository navigation
Conversation
Deleting a git provider connection marked its credentials Revoked but kept every token and the connection's OAuth client secret decryptable in the database, and never told the provider, so an OAuth refresh token stayed valid at GitHub/GitLab while the UI said "Revoked". The delete now retires each credential through the same path a departure or an unlink uses (token blanked, Revoked, soft-deleted, act-as identities gone), nulls the client secret, and revokes each OAuth grant at the provider after the transaction commits, best effort and bounded, so a rolled-back delete never ends a grant. The refresh-then- access revoke a Disconnect already did moves into IOAuthGrantRevoker, shared by both paths. A connection-wide connection's group hooks outlived the delete too: still registered at GitLab/GitHub, still delivering, their HMAC secrets decryptable, and their registering token gone, so nothing could ever remove them. The delete now retires them first, while that token still works, the way a switch back to per-repository scope does, and blanks the secret of every hook row the connection keeps. Deleted variables, revoked storage credentials and rotated or cleared deployment storage defaults kept their ciphertext forever, because 0012's check demanded a ciphertext on every Secret row and the 0126 and 0173 guards refused every UPDATE. Migration 0249 lets a deleted Secret hold none, lets a revision be blanked exactly once and only when its credential is Revoked, lets a default's envelope be blanked exactly once and only when no template names it, and blanks what retirement already left behind. A superseded revision of an Active credential is kept: storage profiles pin an exact revision and the resolver still decrypts it for bytes stored through it. The materializer decrypts only the envelope its template names, so the one an edit replaces is blanked in the same transaction. The old checks reject shredded rows, so 0249's rollback note re-adds them NOT VALID; a test runs that note on a shredded database. Listing a credential's accessible repositories presents the token to the provider, yet needed only team membership, so a Viewer could spend the Owner's personal token to list private repositories outside the team. The query now takes IRequireCredentialSpend: the owner of a Personal credential, or a repos.manage holder; the rule is shared with IRequireCredentialManagement.
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.
Summary
ProviderInstanceService.DeleteAsync(backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs:241) first retires the connection's group/organization hooks (ProviderInstanceService.Retirement.cs:44).secret_encblanked.ICredentialRevocationService.RetireAsync: token blanked, Revoked, soft-deleted, act-as identities removed.oauth_client_secret_encis nulled.IOAuthGrantRevoker.backend/src/CodeSpace.Core/Persistence/DbUpFiles/0249_shred_retired_secrets.sql).chk_variable_value_exclusiveis relaxed for deleted rows only.StorageDefaultService.UpdateAsync(StorageDefaultService.cs:83) blanks the envelope that a rotation or a clear replaced, in the same transaction. The materializer decrypts only the envelope its template names, read under a share lock.external_id/owner_pathare kept so an operator can remove a hook still at the provider by hand; deliveries to it are refused as retired.NOT VALIDstatements and says to re-run 0126's and 0173's guard functions verbatim. A test runs those statements on a shredded database.ListAccessibleRepositoriesQueryimplementsIRequireCredentialSpend.CredentialOwnerOrPermission, shared withIRequireCredentialManagement. Instance Admins keep their Owner standing.No API shape changes. Behaviour changes, all security-required:
Test plan
OAuthGrantRevoker; variable retirement. Full unit suite: 13660 passed.RetiredSecretShredMigrationTests): a pre-0249 database upgraded through the productionDbUpRunner. Backfills, guards after the upgrade, and the documented rollback run on a shredded database./api/admin/storage-defaults.