fix(cli): windows logout credentials#5913
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dad653f8b4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3594087d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
6d35940 to
9c58701
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c58701c5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…n logout Go writes the Windows CredentialBlob as raw bytes that aren't always valid UTF-8/UTF-16. napi-rs/keyring's getPassword/findCredentials marshal that blob into a JS string and throw on those bytes, so the legacy TS port treated a present-but-opaque credential as absent and skipped deletion, leaving it behind after `supabase logout` reported success. Check existence via findCredentials with an exact target instead: a genuinely absent credential yields an empty result, while a throw means the target was found but couldn't be decoded, i.e. it exists. This deliberately avoids Entry.withTarget() for the probe itself, since constructing it writes an empty placeholder credential as a side effect on Windows (napi-rs/keyring), which would otherwise fabricate the very credential being checked for.
9c58701 to
60c1faf
Compare
7ttp
changed the title
There was a problem hiding this comment.
💡 Codex Review
Fix the mock so Windows sweeps match production
This new Windows sweep scenario passes only because the mock's no-target findCredentials branch includes Supabase CLI:<ref> keys; the real @napi-rs/keyring 1.3.0 Windows implementation enumerates *.${service}, while Go keyring writes targets as Supabase CLI:<account>. With only Go-written project DB-password credentials present, production deleteAllKeyringEntries never discovers this target-only entry, so logout can still leave the credential behind even though this test is green.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Three Windows-only correctness gaps in the credential layer, all matching Go's behavior: - Probe existence with a side-effect-free `findCredentials` read instead of `Entry.withTarget`, whose constructor writes an empty placeholder credential and would fabricate the target being probed for. - Distinguish a confirmed-absent target from an ambiguous probe failure (undecodable blob or enumeration error): only a confirmed-present target whose delete does not succeed becomes a hard error, and `deleteCredential`'s boolean is now checked rather than ignored, so a failed delete can no longer report a successful logout. - Sweep project credentials by enumerating Go's `Supabase CLI:<account>` namespace, mirroring Go's `DeleteAll` prefix match. napi's default enumeration filters the `<account>.<service>` form Go never writes on Windows, so the previous sweep left Go-written project credentials behind.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12e8b3fa60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The win32 sweep enumerated Supabase CLI:* to catch project database passwords, but findCredentials decodes every matched blob and throws for the whole call on one undecodable entry (the same failure mode this patch fixes for single-target lookups), aborting the sweep entirely. Verified against apps/cli-go: the only production keyring write is the access token (access_token.go:77, keyed by profile name) — link and every other command never write a project-ref-keyed credential. deleteAccessToken already removes both token forms before this sweep runs, so there is nothing in the Windows target form left to find. The sweep reverts to a plain, decode-only-what-you-write enumeration.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20420bfa37
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tate Entry.withTarget's constructor unconditionally writes an empty placeholder credential on Windows (verified in napi-rs/keyring's entry.rs), so by the time deleteCredential() runs, something always exists at that target — gating the failure path on a confirmed-present probe was withholding a real error signal whenever the probe was merely ambiguous. Any non-success delete outcome is now surfaced, since construction itself already resolved the existence question. Corrected two tests built on the same inaccurate assumption: an enumeration hiccup alone no longer implies not-logged-in (construction leaves something deletable regardless), and a failed Windows target write can leave an empty placeholder behind rather than nothing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b69a47a83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ders Two corrections, verified against apps/cli-go and the napi-rs/keyring source: - Restore the Windows-specific project-credential sweep in logout. Go's windowsKeychain.DeleteAll prefix-matches Supabase CLI: unconditionally (logout_test.go's "removes all Supabase CLI credentials" seeds and expects removal of project-keyed entries) — removing this sweep entirely broke that contract. findCredentials still decodes every matched blob and can abort the whole call on one undecodable entry; that's an upstream library constraint with no available workaround, so it stays best-effort. - probeWindowsTarget now requires a non-empty password before treating a match as present. Entry.withTarget's placeholder write means a prior failed save can leave an empty credential behind; matching on account name alone treated that placeholder as a real session and reported a fabricated successful logout.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e751666cef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…entry The prior test only checked that logout doesn't fail; it didn't assert whether the credentials actually survive. Now it does, making the known findCredentials all-or-nothing decode limitation explicit rather than implied. Verified there's no fix available to route around: diffed napi-rs/keyring's published 1.3.0 against its unreleased main branch (entry.rs is byte-identical) and confirmed no version exposes a decode-free bulk enumeration. The decode happens inside the Rust FFI boundary before results ever reach JS, unlike the raw CredEnumerateW Win32 API itself, which returns target names and blob bytes with no decode step at all.
|
@Coly010 can u have a look at this!? :) |
1 participant
TL;DR
Fixes stale Windows credentials surviving logout
Go handles this by never decoding the blob before deleting, but the TS port used a decode based existence check that threw on non-UTF16 bytes and treated present credentials as absent.
Now it uses a plain enumeration read with no decode step, so it's back to deleted or not, nothing else
(go parity: matches Go's own delete path exactly)
ref: