Skip to content

Commit aafc245

Browse files
Sanjays2402Sanjays2402
committed
fix(control-ui): clear sticky chat UI on code:4001 revocation closes
Address codex P2 review on #72522: the previous reset condition only fired when the close carried a structured detail code via isAuthFailureDetailCode. Server-initiated revocation paths (shared-auth rotation, device removal, session revocation) close active sockets with code 4001 + a reason string, and the browser client forwards only {code, reason} for post-hello closes (error is set only for the pending connect error). Without classifying these directly, cached Control UI content survived explicit revocation until the next reconnect outcome. Add isAuthRevocationClose() that matches code 4001 + a lowercased reason substring against an allowlist (gateway auth changed/rotated/revoked, shared auth changed/rotated/revoked, device removed/revoked, session revoked, pairing revoked). Wire it into onClose so hasEverConnected is also dropped on these code-only revocation closes, preserving the existing transient-infra and structured-error paths. Tests cover all 8 revocation reasons (parametrized), plus two defensive cases: (a) a non-revocation 4001 reason stays sticky, (b) a non-4001 close that happens to mention 'device removed' in its transient reason stays sticky. 61 tests pass (was 51; +10 = 8 it.each + 2 single).
1 parent 6221173 commit aafc245

2 files changed

Lines changed: 123 additions & 1 deletion

File tree

‎ui/src/ui/app-gateway.node.test.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,77 @@ describe("connectGateway", () => {
509509
expect(host.hasEverConnected).toBe(true);
510510
});
511511

512+
it.each([
513+
"gateway auth changed",
514+
"gateway auth rotated",
515+
"gateway auth revoked",
516+
"shared auth changed",
517+
"device removed",
518+
"device revoked",
519+
"session revoked",
520+
"pairing revoked",
521+
])(
522+
"clears hasEverConnected on code:4001 revocation reason %j with no error payload",
523+
(reason) => {
524+
// Server-initiated shared-auth rotation, device removal, and session
525+
// revocation closes use code 4001 + reason text. The browser client
526+
// forwards only `code` + `reason` for post-hello closes (`error` is
527+
// populated only from the pending connect error), so without
528+
// classifying the code/reason directly the sticky chat UI would
529+
// survive explicit revocation. Codex review on #72522.
530+
const host = createHost();
531+
532+
connectGateway(host);
533+
const client = gatewayClientInstances[0];
534+
client.emitHello();
535+
expect(host.hasEverConnected).toBe(true);
536+
537+
client.emitClose({ code: 4001, reason, error: undefined });
538+
expect(host.connected).toBe(false);
539+
expect(host.hasEverConnected).toBe(false);
540+
},
541+
);
542+
543+
it("keeps hasEverConnected on code:4001 with non-revocation reason and no error payload", () => {
544+
// Defensive: only the explicit revocation reason allowlist should drop
545+
// the sticky flag. A generic 4001 close (e.g. policy violation that is
546+
// not actually a credential rotation) must not be misclassified.
547+
const host = createHost();
548+
549+
connectGateway(host);
550+
const client = gatewayClientInstances[0];
551+
client.emitHello();
552+
expect(host.hasEverConnected).toBe(true);
553+
554+
client.emitClose({
555+
code: 4001,
556+
reason: "policy violation",
557+
error: undefined,
558+
});
559+
expect(host.connected).toBe(false);
560+
expect(host.hasEverConnected).toBe(true);
561+
});
562+
563+
it("keeps hasEverConnected on non-4001 close even if reason mentions revocation", () => {
564+
// Defensive: the reason allowlist only matters when paired with the
565+
// canonical 4001 revocation code. A 1006 transient close that happens
566+
// to embed the substring must stay sticky.
567+
const host = createHost();
568+
569+
connectGateway(host);
570+
const client = gatewayClientInstances[0];
571+
client.emitHello();
572+
expect(host.hasEverConnected).toBe(true);
573+
574+
client.emitClose({
575+
code: 1006,
576+
reason: "transient: device removed retry",
577+
error: undefined,
578+
});
579+
expect(host.connected).toBe(false);
580+
expect(host.hasEverConnected).toBe(true);
581+
});
582+
512583
it("preserves pending approval requests across reconnect", () => {
513584
const host = createHost();
514585
host.execApprovalQueue = [

‎ui/src/ui/app-gateway.ts‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,46 @@ const AUTH_FAILURE_DETAIL_CODES: ReadonlySet<string> = new Set([
103103
ConnectErrorDetailCodes.PAIRING_REQUIRED,
104104
]);
105105

106+
/**
107+
* Reason substrings on `code: 4001` close frames that indicate explicit
108+
* server-initiated auth/identity revocation, even when the WebSocket close
109+
* carries no structured error payload (e.g. shared-auth rotation closes
110+
* shared-auth sockets with reason `gateway auth changed`, device removal
111+
* closes with reason `device removed`, and session revocation closes use
112+
* `session revoked` / `gateway auth revoked`). The browser client only
113+
* forwards `code` + `reason` for these post-hello closes (`error` is set only
114+
* for the pending connect error), so the sticky `hasEverConnected` reset has
115+
* to look at the close code/reason directly.
116+
*
117+
* Match on lowercased substrings so reason text variants stay covered
118+
* without forcing the gateway to ship structured detail codes for closes
119+
* that already use `4001` + reason as the canonical revocation signal.
120+
*/
121+
const AUTH_REVOCATION_CLOSE_CODE = 4001;
122+
const AUTH_REVOCATION_REASON_SUBSTRINGS = [
123+
"gateway auth changed",
124+
"gateway auth rotated",
125+
"gateway auth revoked",
126+
"shared auth changed",
127+
"shared auth rotated",
128+
"shared auth revoked",
129+
"device removed",
130+
"device revoked",
131+
"session revoked",
132+
"pairing revoked",
133+
];
134+
135+
export function isAuthRevocationClose(close: { code?: number; reason?: string }): boolean {
136+
if (close.code !== AUTH_REVOCATION_CLOSE_CODE) {
137+
return false;
138+
}
139+
const reason = (close.reason ?? "").toLowerCase();
140+
if (!reason) {
141+
return false;
142+
}
143+
return AUTH_REVOCATION_REASON_SUBSTRINGS.some((needle) => reason.includes(needle));
144+
}
145+
106146
/**
107147
* Returns true when a gateway close detail code indicates an explicit
108148
* auth/identity failure that should force the credentials gate to reappear.
@@ -534,9 +574,20 @@ export function connectGateway(host: GatewayHost, options?: ConnectGatewayOption
534574
(typeof error?.code === "string" ? error.code : null);
535575
// If the close looks like an explicit auth/identity failure, drop the
536576
// sticky hasEverConnected flag so the renderer falls back to the full
537-
// credentials gate instead of holding the stale chat UI in place.
577+
// credentials gate instead of holding the stale chat UI in place. This
578+
// covers two distinct shapes:
579+
// 1. Structured error payloads on the connect path (lastErrorCode set
580+
// from resolveGatewayErrorDetailCode) — the original allowlist.
581+
// 2. Server-initiated `code: 4001` closes where the browser client
582+
// forwards only `code` + `reason` (no structured error). Shared-auth
583+
// rotation, device removal, and session revocation all use this
584+
// shape, so cached Control UI content would otherwise survive
585+
// explicit revocation until the next reconnect outcome (P2 codex
586+
// finding on #72522).
538587
if (host.lastErrorCode && isAuthFailureDetailCode(host.lastErrorCode)) {
539588
host.hasEverConnected = false;
589+
} else if (isAuthRevocationClose({ code, reason })) {
590+
host.hasEverConnected = false;
540591
}
541592
if (code !== 1012) {
542593
if (error?.message) {

0 commit comments

Comments
 (0)