Delete the credential a connection minted when the connection is removed - #1568
Delete the credential a connection minted when the connection is removed#1568GeiserX wants to merge 9 commits into
Conversation
Removing a connection deleted the tool, definition and connection rows and then returned. The secret itself stayed in the provider store, still decryptable — so a user who deleted a credential had not actually deleted it. Only ids the connection MINTED are removed. A connection can instead reference an item the user already had, and the provider contract is explicit that such a removal drops our routing and leaves the item intact; deleting one would destroy a credential we never created and cannot restore. The row carries no flag saying which is which, but it does not need one: minted ids are deterministic, so rebuilding them from the row and keeping only exact matches recovers the distinction with no schema change. It also leaves the OAuth app's shared client secret alone, which every connection minted through that app still needs.
Nothing stops a second connection referencing this one's minted item through the `from` origin — the reference path stores whatever id it is handed. Deleting the item on removal then pulled the credential out from under a connection that was still live and still using it, which is the same unrecoverable loss the minted-id check exists to prevent, arriving by a different route. The connection row is already deleted at that point, so anything still holding the id is by definition somebody else, and the item stays. This was recorded as a known gap when the deletion landed; the test that now covers it failed before this change.
The scan reads through the table's owner-visibility policy, so it sees the org partition and this caller's own rows but not another subject's. An alias held by a different subject is invisible to it. Left as-is on purpose: reading around a tenant-isolation boundary to widen a DELETE would be a worse defect than the narrow one it closes.
…vider A fresh review found a mutation that survived the entire suite: dropping the `:refresh` sibling from the rebuilt id set left a live, decryptable refresh token in the store after removal — the same orphan this change exists to close, in its most damaging form. Nothing in the SDK exercised an `oauth:` item id, so the security-relevant half was the untested one. An end-to-end authorization flow now covers it, and it fails under that mutation. The alias scan also compared id strings without regard to which provider held them. An id means nothing outside its own provider's namespace, so an unrelated connection holding the same string wrongly protected the item and left it behind. The scan is now scoped to the same provider, which narrows it too. Also records in the provider contract that an item id is unique only within an owner partition: the SDK's ids embed the owner literal but not the subject, so a provider keeping one flat namespace across subjects lets one member's write overwrite another's. The shipped stores file per (tenant, owner, subject); the contract now says so rather than leaving it to be discovered. Restores the JSDoc that the earlier insertion had detached from `connectionItemIds`.
|
Pushed a fix for a defect I found in my own patch while reviewing it. The credential deletion was running inside the transaction that removes the connection rows. The first fix was not enough, which is the more interesting part. Sequencing the deletion after the It now routes through the existing The regression test drives exactly that path — a plugin-owned outer transaction that removes the connection and then fails — and asserts the row came back and the credential survived. It fails against the original code and against the incomplete first fix, so it distinguishes all three states rather than merely passing on the final one. |
|
Pushed one more commit: the same orphan exists on a second path, and fixing only the first would have been misleading.
I found it while auditing which plugin lifecycle hooks run inside a transaction, not by looking for it. Worth mentioning because it means the two paths had drifted silently — merging this PR without it would leave a reviewer reasonably believing the orphan class was closed. The fix reuses the same Two mutations, each killing exactly one test: removing the deletion kills the "deletes what it minted" test, and running it inline instead of deferring kills the "rolled-back removal leaves the credentials intact" test. Neither kills both — the rollback test cannot bind the deferral on its own, because with the feature absent the credential survives trivially. |
|
Context for this one: #1585 explains why this PR and twelve others exist — they came out of a single pass over credential handling, asking for each credential where it ends up, how long it stays, and who can read it once it's there. This PR stands alone and doesn't depend on any of the others. |
1 participant
TL;DR
connections.removedeleted the connection row and left the credential it had minted in the store. The secret outlived the only thing that referenced it, with no surface left in the product to see or remove it.A user who disconnects an account still has that account's tokens held on their behalf.
What happens today
Connecting an account mints credential items and records a connection row pointing at them. Removing the connection deletes only the row. The items stay — unreferenced, invisible in the UI, and indefinitely live.
For an OAuth connection those items are an access token and a refresh token. A refresh token is long-lived by design, so "disconnected" and "revoked" quietly mean different things.
What this changes
Removal now also deletes the items the connection minted.
Known limits, stated rather than implied
The alias scan can only see connections the caller is allowed to see. A connection owned by someone else that points at the same item is invisible to it. That limit is recorded in a comment next to the code, because a reader who assumes the scan is global would draw the wrong conclusion about when an item is safe to delete.
Tests
Six tests covering: the OAuth path, the static path, the shared-item hold-back, provider scoping, and the id-matching being exact rather than fuzzy.
Scope
Independent of #1564. It touches
executor.tsand one line ofprovider.ts; both files are also touched by that PR, so expect a small textual conflict if both merge — the changes are in different functions and do not interact logically.