fix(desktop): prevent proxy password mask corruption - #3704
Conversation
Generated-by: OpenAI Codex
b94e9ab to
c5ec403
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head c5ec40335dcaa0d19756bff067bced9cf2cf687c. I found two blocking correctness issues in the new proxy-credential write path: one cross-client race that can recreate a credential after authentication was disabled, and one partial-commit path that applies proxy policy even though the API reports failure. The hosted checks are green and the branch merges cleanly, but these state-consistency issues need to be resolved before approval.
| else await setCredential(client, PROXY_CREDENTIAL, proxy.password); | ||
| } | ||
| else if (proxy.credential?.kind === "replace") | ||
| await setCredential(client, PROXY_CREDENTIAL, proxy.credential.secret); |
There was a problem hiding this comment.
[P1] Make proxy policy and credential replacement one Host-owned operation across clients. The new queue is local to one RuntimeHostSettingsModule, so two supported Desktop clients have independent lanes. Client A can start a replacement and pause at the credential CAS; client B can then set authEnabled=false and delete the credential; when A receives credential_stale, its retry rereads the now-empty locator and recreates the secret with expected: null. A production-adapter/CAS probe ended with authEnabled=false while the vault again contained A's replacement secret. The existing "disable wins" test uses one module and therefore cannot cover this inter-client ordering. Please move the policy decision plus keep/replace/delete into one Runtime Host atomic or recoverable operation that validates both the policy revision and credential basis, and add a two-client test asserting the vault remains empty after disable.
There was a problem hiding this comment.
Fixed in 2948461. Desktop no longer performs a client-local credential CAS/retry. It sends the observed policy revision, credential basis, complete proxy policy, and keep/replace/delete intent through the Host-owned runtime.policy.network-proxy.update operation. The Host rejects a stale client before it can recreate a deleted credential.
Added coordinator and storage tests with two clients observing the same initial basis: client B disables authentication, then client A attempts the stale replacement. The replacement is rejected, the final policy remains authEnabled: false, and the vault remains empty.
| ): Promise<void> { | ||
| if (patch.network?.proxy) { | ||
| const proxy = patch.network.proxy; | ||
| await client.updateRuntimePolicy((policy) => ({ |
There was a problem hiding this comment.
[P2] Do not commit proxy policy before the credential write can still fail. A normal update containing both policy fields and credential: replace/delete first commits set_network_proxy here, then performs vault persistence below. If that second step rejects, the caller receives an error even though the Host is already using the new host/auth/username with the old credential. This is also reachable from the schema-v1 settings-plus-credentials import path. A production-adapter failure probe observed the rejected call with enabled=true, authEnabled=true, username=new-user, and the old secret still stored; the success control stored the replacement. Existing failure coverage sends a credential-only patch, so it never checks policy after the failure. Please make this a single Host-side compound operation, or define an explicit recoverable commit protocol, and test each persistence cut for committed/unchanged/known recovery semantics.
There was a problem hiding this comment.
Fixed in 2948461. Proxy policy and credential changes now use one Host-side recoverable operation backed by a persisted transaction intent, so Desktop no longer commits policy before a credential write that can independently fail.
Added persistence-cut coverage for the compound operation and a Desktop regression test verifying that a rejected credential replacement does not expose newly requested proxy policy fields with the old credential. Recovery leaves the pair either unchanged or consistently committed, and the transaction intent is cleared after recovery.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
|
CI status note: This PR does not change the slash-command/menu implementation or that test relative to current |
Summary
settings.jsonFixes #3696
UI behavior
The recording shows sequential draft input, working show/hide, safe reload state, Escape cancellation without closing Settings, replacement after a saved password, and the proxy-only removal of Copy.
Verification
Proxy-Authorizationnpm run lintnpm run format:checknpm run buildnpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uiAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the fix, tests, verification, and review-driven corrections.
Checklist
Does this PR entail a change in behavior?