Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a connection-approval workflow enabling users to approve or deny pending client connections via challenge codes and JWT-based authentication. It adds an ApprovalService to manage pending connections, protocol messages for approval state transitions, backend CLI options, a terminal UI for approvals, and frontend support for JWT-based authentication and approval overlays. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ApprovalService
participant Approver
Client->>Server: connect (no password, optional JWT)
Server->>ApprovalService: addPending(connection metadata)
ApprovalService->>Server: emit pending-added
Server->>Client: auth_pending (challenge code)
Approver->>ApprovalService: view pending connections
Note over Approver: Review connection details
alt Approve
Approver->>ApprovalService: approve(id)
ApprovalService->>ApprovalService: generate JWT
ApprovalService->>Server: emit approved
Server->>Client: auth_approved (JWT, clientId)
Client->>Client: store JWT
Client->>Server: reconnect with JWT
Server->>Client: auth_ok
else Deny
Approver->>ApprovalService: deny(id)
ApprovalService->>Server: emit denied
Server->>Client: auth_denied (reason)
Client->>Client: clear pending state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
UI Screenshots
Screenshots are available as a build artifact. Files captured: drawer-open.png, main-view.png Download the |
CI Feedback 🧐(Feedback updated until commit d51408a)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
guysmoilov
force-pushed
the
feat/issue-21-connection-approval-tui
branch
from
cd43792 to
7474844
Compare
guysmoilov
force-pushed
the
feat/issue-21-connection-approval-tui
branch
from
7474844 to
439d7c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/App.tsx (1)
294-306:⚠️ Potential issue | 🟡 MinorForward
jwtValueto the terminal socket in theauth_okhandler for independent endpoint authentication.The control and terminal WebSocket endpoints must authenticate independently. Currently, line 305 calls
openTerminalSocket(passwordValue)without forwardingjwtValue. If the control socket authenticated via stored JWT and the server sendsauth_ok(instead of the typicalauth_approved), the terminal socket would lack the JWT and fail authentication despite the control socket succeeding.While the server currently sends
auth_approvedwith JWT for JWT-based auth, this defensive pattern ensures terminal endpoint authentication remains independent and resilient to any auth response variation.Proposed fix
case "auth_ok": setErrorMessage(""); setPasswordErrorMessage(""); setAuthReady(true); setNeedsPasswordInput(false); setPendingApproval(false); if (message.requiresPassword && passwordValue) { localStorage.setItem("tmux-mobile-password", passwordValue); } else { localStorage.removeItem("tmux-mobile-password"); } - openTerminalSocket(passwordValue); + openTerminalSocket(passwordValue, jwtValue); return;
🧹 Nitpick comments (9)
src/backend/config.ts (1)
11-12:jwtLifetimeSecsis required even whenapprovalEnabled: false.
jwtLifetimeSecshas no effect whenapprovalEnabledisfalse, yet allRuntimeConfigconsumers must supply it. Making it optional (or grouping the two fields into an optionalapprovalConfigsub-object) would make the relationship between the fields explicit and remove dead config from non-approval setups.♻️ Proposed refactor
export interface RuntimeConfig { port: number; host: string; password?: string; tunnel: boolean; defaultSession: string; scrollbackLines: number; pollIntervalMs: number; token: string; frontendDir: string; - approvalEnabled: boolean; - jwtLifetimeSecs: number; + approvalEnabled: boolean; + jwtLifetimeSecs?: number; // only meaningful when approvalEnabled = true }package.json (1)
55-64: React 19 → 18 downgrade is driven by ink 5.x's incompatibility with React 19.
inkv5.x requires React 18. The downgrade from React 19 to 18 is the correct fix for the chosen ink version. However,ink@^6requires React >=19.0.0 and was developed to address React 19 compatibility. If the project wants to stay on React 19 long-term, upgrading toink@^6would unblock that—but this would also require upgrading React back to 19.The current pairing (ink 5 + React 18) is valid and requires no changes, but it's worth tracking if React 19 compatibility becomes important later.
src/backend/auth/approval-service.ts (2)
19-19: Challenge code character set includes non-alphanumeric symbols.
randomToken(3)produces base64url output which includes-and_. AftertoUpperCase(), the 4-char code could contain characters like_or-(e.g.,A_B-), which may look odd in a user-facing approval UI. Consider restricting the alphabet to alphanumeric only for a cleaner UX.🔧 Proposed fix
- const challengeCode = randomToken(3).slice(0, 4).toUpperCase(); + const challengeCode = randomToken(6) + .replace(/[^A-Za-z0-9]/g, "") + .slice(0, 4) + .toUpperCase();
63-71:verifyJwtdiscards the token payload — caller cannot identify the authenticated client.The server will likely need the
clientIdclaim from a verified JWT (e.g., on reconnection). Currently the caller gets only{ valid: true }with no way to know who the token belongs to without decoding it again separately.♻️ Proposed fix
- public async verifyJwt(token: string): Promise<{ valid: boolean; reason?: string }> { + public async verifyJwt(token: string): Promise<{ valid: boolean; reason?: string; clientId?: string }> { try { - await jwtVerify(token, this.jwtSecret); - return { valid: true }; + const { payload } = await jwtVerify(token, this.jwtSecret); + return { valid: true, clientId: payload.clientId as string }; } catch (error) { const reason = error instanceof Error ? error.message : "invalid token"; return { valid: false, reason }; } }src/backend/tui/approval-tui.tsx (1)
72-76: Swallowed rejection fromservice.approve().
void service.approve(...)discards the promise. If JWT signing fails (e.g., invalid key state), the error is silently lost and the TUI gives no feedback. Consider adding a.catch()to surface failures.🔧 Proposed fix
const connection = pending[selectedIndex]; if (connection) { - void service.approve(connection.id); + service.approve(connection.id).catch(() => { + // Approval failure is surfaced by the absence of the "approved" event + }); }src/backend/cli.ts (1)
130-134:jwtSecretis allocated unconditionally even when approval is disabled.When
--no-approveis set,approvalServiceisundefinedand the secret is never used. Move the secret generation inside the conditional to avoid the unnecessary allocation.♻️ Proposed fix
const approvalEnabled = !args.noApprove; - const jwtSecret = crypto.randomBytes(32); const approvalService = approvalEnabled - ? new ApprovalService({ jwtSecret, jwtLifetimeSecs: args.jwtLifetime }) + ? new ApprovalService({ jwtSecret: crypto.randomBytes(32), jwtLifetimeSecs: args.jwtLifetime }) : undefined;tests/integration/approval-flow.test.ts (1)
217-217: FixedsetTimeoutdelays are fragile synchronizationThree tests use
await new Promise((resolve) => setTimeout(resolve, 50))to wait for async side-effects (close propagation, auth processing). These can silently pass on fast machines and flake on slow CI.
- Line 217 (after
control1.close()): wait for close to propagate before reconnecting.- Line 283 (after
control.close()): wait forremovePendingto run.- Line 302 (after terminal auth): wait for JWT verification to set
ctx.authed.For lines 217 and 283 (socket close), polling
approvalService.getPending().lengthwith a short poll loop or using a one-shot"pending-removed"event fromapprovalServicewould be deterministic. For line 302, having the server send a small acknowledgement on terminal auth success (or pollingterminal.readyState) would be better.Also applies to: 283-283, 302-302
src/backend/server.ts (2)
313-323: Control WS lacks theauthInProgressguard present on terminal WSTerminal WS sets
ctx.authInProgress = truebeforeawait approvalService.verifyJwt(...)and back tofalseafter, dropping any messages that arrive during the async window. Control WS has no equivalent. Becausesocket.on("message", async ...)handlers run concurrently in the Node.js event loop, a second auth message arriving while the first is still inawait approvalService.verifyJwt(...)passes the!context.authedcheck (not yet set totrue) and enters the auth block again — potentially triggering twocompleteAuthcalls, twoauth_approvedmessages, and twoensureAttachedSessioninvocations.Add a
pendingAuthflag toControlContext(analogous toauthInProgressinDataContext) and guard early:interface ControlContext { socket: WebSocket; authed: boolean; clientId: string; pendingApprovalId?: string; + authInProgress?: boolean; }if (!context.authed) { + if (context.authInProgress) return; if (message.type !== "auth") { ... } if (message.jwt && approvalService) { + context.authInProgress = true; const jwtResult = await approvalService.verifyJwt(message.jwt); + context.authInProgress = false; ... } ... }Also applies to: 401-429
208-228:approvalServiceevent listeners are never removed onstop()
approvalService.on("approved", ...)andapprovalService.on("denied", ...)are registered once at creation time and never removed. If anApprovalServiceinstance outlives one server lifecycle (e.g., shared across a restart or held by a test), the old listeners — which close over the first server'scontrolClientsset — remain active and would fire against a stale context.Consider removing the listeners in
stop():+ let approvedListener: ((e: {...}) => void) | undefined; + let deniedListener: ((e: {...}) => void) | undefined; if (approvalService) { - approvalService.on("approved", (event) => { ... }); - approvalService.on("denied", (event) => { ... }); + approvedListener = (event) => { ... }; + deniedListener = (event) => { ... }; + approvalService.on("approved", approvedListener); + approvalService.on("denied", deniedListener); } // inside stop(): + if (approvalService && approvedListener) approvalService.off("approved", approvedListener); + if (approvalService && deniedListener) approvalService.off("denied", deniedListener);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/backend/auth/approval-service.tssrc/backend/auth/approval-types.tssrc/backend/cli.tssrc/backend/config.tssrc/backend/server.tssrc/backend/tui/approval-tui.tsxsrc/backend/types/protocol.tssrc/backend/util/geoip.tssrc/frontend/App.tsxsrc/frontend/styles/app.csssrc/frontend/types/protocol.tstests/backend/approval-service.test.tstests/backend/geoip.test.tstests/e2e/harness/test-server.tstests/integration/approval-flow.test.tstests/integration/server.test.tstsconfig.backend.json
🧰 Additional context used
📓 Path-based instructions (2)
**/types/protocol.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep protocol types in sync between backend (
src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts)
Files:
src/frontend/types/protocol.tssrc/backend/types/protocol.ts
**/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
Preserve independent auth gating on both
/ws/controland/ws/terminalWebSocket endpoints
Files:
src/backend/server.ts
🧠 Learnings (3)
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/server.ts : Preserve independent auth gating on both `/ws/control` and `/ws/terminal` WebSocket endpoints
Applied to files:
src/backend/auth/approval-types.tssrc/frontend/types/protocol.tstests/integration/approval-flow.test.tssrc/backend/types/protocol.tstests/backend/approval-service.test.tssrc/backend/cli.tssrc/backend/server.tstests/integration/server.test.tssrc/frontend/App.tsx
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/types/protocol.ts : Keep protocol types in sync between backend (`src/backend/types/protocol.ts`) and frontend (`src/frontend/types/protocol.ts`)
Applied to files:
src/backend/auth/approval-types.tssrc/frontend/types/protocol.tstsconfig.backend.jsonsrc/backend/types/protocol.tssrc/backend/server.ts
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/tmux/**/*.ts : Use `execFile` with argument arrays in tmux command execution to prevent shell injection attacks
Applied to files:
tests/integration/server.test.ts
🧬 Code graph analysis (6)
src/backend/auth/approval-service.ts (2)
src/backend/auth/approval-types.ts (2)
PendingConnection(3-12)ApprovalServiceOptions(14-17)src/backend/util/random.ts (1)
randomToken(3-4)
tests/backend/geoip.test.ts (1)
src/backend/util/geoip.ts (1)
resolveGeo(6-14)
tests/backend/approval-service.test.ts (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/auth/approval-types.ts (1)
PendingConnection(3-12)
src/backend/cli.ts (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/tui/approval-tui.tsx (1)
renderApprovalTui(126-128)
src/backend/server.ts (5)
src/backend/tmux/types.ts (1)
TmuxGateway(9-24)src/backend/pty/pty-adapter.ts (1)
PtyFactory(9-11)src/backend/auth/auth-service.ts (1)
AuthService(8-32)src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/util/geoip.ts (1)
resolveGeo(6-14)
src/backend/tui/approval-tui.tsx (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/auth/approval-types.ts (1)
PendingConnection(3-12)
🪛 Stylelint (17.3.0)
src/frontend/styles/app.css
[error] 680-680: Unexpected quotes around "Menlo" (font-family-name-quotes)
(font-family-name-quotes)
[error] 680-680: Unexpected quotes around "Monaco" (font-family-name-quotes)
(font-family-name-quotes)
🔇 Additional comments (14)
tests/e2e/harness/test-server.ts (1)
44-48: LGTM — correctly satisfies the new requiredRuntimeConfigfields.src/backend/util/geoip.ts (1)
1-14: LGTM — clean implementation with helpful privacy comment.src/backend/auth/approval-types.ts (1)
1-17: LGTM — well-typed interfaces;import typefor WebSocket avoids unnecessary runtime import.tsconfig.backend.json (1)
10-10: No action needed — the basetsconfig.jsonalready has"jsx": "react-jsx"set, so TypeScript will successfully compile the.tsxfiles included intsconfig.backend.jsonwithout error. The configuration is correct as-is.Likely an incorrect or invalid review comment.
src/backend/types/protocol.ts (1)
50-52: The frontend protocol file properly mirrors the backend auth variants.The three new
ControlServerMessagevariants (auth_pending,auth_approved,auth_denied) are correctly defined in bothsrc/backend/types/protocol.tsandsrc/frontend/types/protocol.tswith matching field signatures.src/backend/tui/approval-tui.tsx (1)
30-85: LGTM — well-structured TUI component.Event subscriptions are properly cleaned up, the selected-index clamping is correct, and the keyboard input handling covers the expected interactions cleanly.
tests/integration/server.test.ts (1)
21-23: LGTM — test config updated to match the expandedRuntimeConfiginterface.tests/backend/approval-service.test.ts (1)
1-143: LGTM — solid test coverage for ApprovalService.Good coverage of the core lifecycle (add/approve/deny/remove), event emissions, JWT signing/verification, and cross-secret rejection. The tests are clean and well-organized.
src/backend/cli.ts (1)
186-189: Good use of dynamic import for the TUI module.This avoids loading Ink and React when approval is disabled — nice optimization for the common
--no-approvepath.src/frontend/App.tsx (4)
59-64: JWT stored in localStorage — comment adequately documents the trade-off.The security comment is appreciated. The single-origin, no-third-party-scripts assumption is reasonable for this use case. If third-party scripts are ever introduced, this should be revisited.
867-886: Approval overlay UI looks good.Clean implementation with challenge code display, spinner, and a fallback to password entry. The
data-testidattributes enable integration testing.
236-248: Auth payload construction —jwtandpasswordare mutually exclusive.The
if/else iflogic means that if bothjwtValueandpasswordValueare provided, only the JWT is sent. This is the correct precedence, but worth noting for reviewers that a JWT always takes priority over a password in the auth payload.
515-521:switchToPasswordEntrycloses the control socket — user must re-authenticate.Calling
controlSocketRef.current?.close()means the user loses the WebSocket connection. When they later submit a password viasubmitPassword(),openControlSocket(password)is called (line 512), which creates a new socket. This flow is correct and clean.src/frontend/types/protocol.ts (1)
36-38: The protocol types between backend (src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts) are already in sync. The three newControlServerMessagevariants—auth_pending,auth_approved, andauth_denied—match identically across both files with the correct shapes and types.
| public async approve(id: string): Promise<{ connectionId: string; jwt: string } | null> { | ||
| const connection = this.pending.get(id); | ||
| if (!connection) { | ||
| return null; | ||
| } | ||
|
|
||
| const jwt = await new SignJWT({ clientId: connection.clientId }) | ||
| .setProtectedHeader({ alg: "HS256" }) | ||
| .setIssuedAt() | ||
| .setExpirationTime(`${this.jwtLifetimeSecs}s`) | ||
| .sign(this.jwtSecret); | ||
|
|
||
| this.pending.delete(id); | ||
| const result = { connectionId: id, jwt }; | ||
| this.emit("approved", { ...result, clientId: connection.clientId }); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicated JWT signing logic between approve() and signJwt().
Lines 41–45 replicate the same SignJWT chain found in signJwt() (lines 73–78). approve() should delegate to signJwt() to keep the signing configuration in one place.
♻️ Proposed refactor
public async approve(id: string): Promise<{ connectionId: string; jwt: string } | null> {
const connection = this.pending.get(id);
if (!connection) {
return null;
}
- const jwt = await new SignJWT({ clientId: connection.clientId })
- .setProtectedHeader({ alg: "HS256" })
- .setIssuedAt()
- .setExpirationTime(`${this.jwtLifetimeSecs}s`)
- .sign(this.jwtSecret);
+ const jwt = await this.signJwt(connection.clientId);
this.pending.delete(id);
const result = { connectionId: id, jwt };
this.emit("approved", { ...result, clientId: connection.clientId });
return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async approve(id: string): Promise<{ connectionId: string; jwt: string } | null> { | |
| const connection = this.pending.get(id); | |
| if (!connection) { | |
| return null; | |
| } | |
| const jwt = await new SignJWT({ clientId: connection.clientId }) | |
| .setProtectedHeader({ alg: "HS256" }) | |
| .setIssuedAt() | |
| .setExpirationTime(`${this.jwtLifetimeSecs}s`) | |
| .sign(this.jwtSecret); | |
| this.pending.delete(id); | |
| const result = { connectionId: id, jwt }; | |
| this.emit("approved", { ...result, clientId: connection.clientId }); | |
| return result; | |
| } | |
| public async approve(id: string): Promise<{ connectionId: string; jwt: string } | null> { | |
| const connection = this.pending.get(id); | |
| if (!connection) { | |
| return null; | |
| } | |
| const jwt = await this.signJwt(connection.clientId); | |
| this.pending.delete(id); | |
| const result = { connectionId: id, jwt }; | |
| this.emit("approved", { ...result, clientId: connection.clientId }); | |
| return result; | |
| } |
| try { | ||
| await ensureAttachedSession(context.socket); | ||
| } catch (error) { | ||
| logger.error("initial attach failed", error); | ||
| sendJson(context.socket, { | ||
| type: "error", | ||
| message: error instanceof Error ? error.message : String(error) | ||
| }); | ||
| } | ||
| await monitor?.forcePublish(); |
There was a problem hiding this comment.
monitor?.forcePublish() is outside the try/catch in completeAuth
The try/catch at lines 195–203 covers only ensureAttachedSession. If monitor.forcePublish() rejects, the exception propagates to the caller. When completeAuth is invoked via void completeAuth(...) (line 213), that rejection is silently dropped. Extend the guard or wrap the forcePublish call:
- } catch (error) {
+ } catch (error) { // covers ensureAttachedSession
logger.error("initial attach failed", error);
sendJson(context.socket, { ... });
}
- await monitor?.forcePublish();
+ try {
+ await monitor?.forcePublish();
+ } catch (error) {
+ logger.error("forcePublish failed", error);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await ensureAttachedSession(context.socket); | |
| } catch (error) { | |
| logger.error("initial attach failed", error); | |
| sendJson(context.socket, { | |
| type: "error", | |
| message: error instanceof Error ? error.message : String(error) | |
| }); | |
| } | |
| await monitor?.forcePublish(); | |
| try { | |
| await ensureAttachedSession(context.socket); | |
| } catch (error) { // covers ensureAttachedSession | |
| logger.error("initial attach failed", error); | |
| sendJson(context.socket, { | |
| type: "error", | |
| message: error instanceof Error ? error.message : String(error) | |
| }); | |
| } | |
| try { | |
| await monitor?.forcePublish(); | |
| } catch (error) { | |
| logger.error("forcePublish failed", error); | |
| } |
| if (message.jwt && approvalService) { | ||
| const jwtResult = await approvalService.verifyJwt(message.jwt); | ||
| if (jwtResult.valid) { | ||
| logger.log("control ws jwt auth ok", context.clientId); | ||
| const freshJwt = await approvalService.signJwt(context.clientId); | ||
| await completeAuth(context, freshJwt); | ||
| return; | ||
| } | ||
| logger.log("control ws jwt invalid", context.clientId, jwtResult.reason); | ||
| // JWT invalid — fall through to password check | ||
| } |
There was a problem hiding this comment.
JWT auth path skips message.token validation — access token can be bypassed
When a JWT is present and valid, completeAuth is called immediately without verifying message.token against authService.token. A caller who possesses a valid JWT but does not know the server's access token (e.g., JWT leaked from localStorage while the URL token is not known) can authenticate on the control WebSocket. The same gap exists on the terminal WebSocket (lines 420–429).
The server's access token is the first factor; the JWT is a derived credential issued only after the token was verified. The JWT path should still enforce the first factor:
🔒 Proposed fix — check token before accepting JWT (control WS)
// 1. JWT authentication (if approvalService available and JWT provided)
if (message.jwt && approvalService) {
+ const tokenCheck = authService.verify({ token: message.token });
+ if (!tokenCheck.ok) {
+ sendJson(socket, { type: "auth_error", reason: tokenCheck.reason ?? "invalid token" });
+ return;
+ }
const jwtResult = await approvalService.verifyJwt(message.jwt);
if (jwtResult.valid) {Apply the same guard to the terminal WS JWT path at lines 420–429.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (message.jwt && approvalService) { | |
| const jwtResult = await approvalService.verifyJwt(message.jwt); | |
| if (jwtResult.valid) { | |
| logger.log("control ws jwt auth ok", context.clientId); | |
| const freshJwt = await approvalService.signJwt(context.clientId); | |
| await completeAuth(context, freshJwt); | |
| return; | |
| } | |
| logger.log("control ws jwt invalid", context.clientId, jwtResult.reason); | |
| // JWT invalid — fall through to password check | |
| } | |
| if (message.jwt && approvalService) { | |
| const tokenCheck = authService.verify({ token: message.token }); | |
| if (!tokenCheck.ok) { | |
| sendJson(socket, { type: "auth_error", reason: tokenCheck.reason ?? "invalid token" }); | |
| return; | |
| } | |
| const jwtResult = await approvalService.verifyJwt(message.jwt); | |
| if (jwtResult.valid) { | |
| logger.log("control ws jwt auth ok", context.clientId); | |
| const freshJwt = await approvalService.signJwt(context.clientId); | |
| await completeAuth(context, freshJwt); | |
| return; | |
| } | |
| logger.log("control ws jwt invalid", context.clientId, jwtResult.reason); | |
| // JWT invalid — fall through to password check | |
| } |
| .challenge-code { | ||
| font-family: "Menlo", "Monaco", "Courier New", monospace; | ||
| font-size: 2.4rem; | ||
| font-weight: 700; | ||
| letter-spacing: 0.35em; | ||
| color: var(--accent); | ||
| padding: 0.6rem 0; | ||
| } |
There was a problem hiding this comment.
Remove unnecessary quotes around single-word font names.
Stylelint flags "Menlo" and "Monaco" — single-word font family names don't require quotes. Multi-word names like "Courier New" are fine as-is.
🔧 Proposed fix
.challenge-code {
- font-family: "Menlo", "Monaco", "Courier New", monospace;
+ font-family: Menlo, Monaco, "Courier New", monospace;
font-size: 2.4rem;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .challenge-code { | |
| font-family: "Menlo", "Monaco", "Courier New", monospace; | |
| font-size: 2.4rem; | |
| font-weight: 700; | |
| letter-spacing: 0.35em; | |
| color: var(--accent); | |
| padding: 0.6rem 0; | |
| } | |
| .challenge-code { | |
| font-family: Menlo, Monaco, "Courier New", monospace; | |
| font-size: 2.4rem; | |
| font-weight: 700; | |
| letter-spacing: 0.35em; | |
| color: var(--accent); | |
| padding: 0.6rem 0; | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 680-680: Unexpected quotes around "Menlo" (font-family-name-quotes)
(font-family-name-quotes)
[error] 680-680: Unexpected quotes around "Monaco" (font-family-name-quotes)
(font-family-name-quotes)
| test("returns location string for known public IP", () => { | ||
| // 8.8.8.8 is Google DNS — geoip-lite has data for it | ||
| const result = resolveGeo("8.8.8.8"); | ||
| // Should be a non-empty string (exact result depends on geoip-lite DB version) | ||
| expect(result).not.toBe("Unknown"); | ||
| expect(result.length).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
The 8.8.8.8 assertion is environment-dependent and could become a flaky test.
The test asserts a non-"Unknown" result for 8.8.8.8 relying on the geoip-lite bundled MaxMind snapshot always containing data for Google's DNS server. While currently reliable, the assertion would silently fail if the database is stale, stripped, or if geoip-lite changes how it ships its DB. Consider adding a vi.mock or a describe.skipIf guard for CI environments where geolocation data might not be guaranteed, or restructuring to test the formatting logic independently from the live lookup.
| return new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| const idx = waiters.findIndex((w) => w.resolve === resolve); | ||
| if (idx >= 0) waiters.splice(idx, 1); | ||
| reject(new Error(`Timed out waiting for message. Received: ${JSON.stringify(messages.map((m) => m.type))}`)); | ||
| }, timeoutMs); | ||
|
|
||
| waiters.push({ | ||
| matcher, | ||
| resolve: (msg) => { | ||
| clearTimeout(timeout); | ||
| resolve(msg); | ||
| }, | ||
| reject | ||
| }); |
There was a problem hiding this comment.
waiters stale entry on timeout — waiter is never cleaned up
The timeout callback searches:
const idx = waiters.findIndex((w) => w.resolve === resolve);But the pushed entry stores the wrapper function — (msg) => { clearTimeout(timeout); resolve(msg); } — not the original resolve from new Promise(...). The identity comparison always returns -1, so the waiter is never spliced out when the timeout fires. The entry stays in the array until the next matching message arrives and removes it as part of the normal notification loop.
Practical risk: if a test times out and a later message in the same socket session matches the stale predicate, the wrapper is called on the already-rejected promise (no-op), but the stale entry still consumes an array slot and affects iteration count.
🛠 Proposed fix — capture wrapper reference for cleanup
return new Promise((resolve, reject) => {
const timeout = setTimeout(() => {
- const idx = waiters.findIndex((w) => w.resolve === resolve);
+ const idx = waiters.findIndex((w) => w.resolve === wrappedResolve);
if (idx >= 0) waiters.splice(idx, 1);
reject(new Error(`Timed out waiting for message. Received: ${JSON.stringify(messages.map((m) => m.type))}`));
}, timeoutMs);
+ const wrappedResolve = (msg: Record<string, unknown>): void => {
+ clearTimeout(timeout);
+ resolve(msg);
+ };
waiters.push({
matcher,
- resolve: (msg) => {
- clearTimeout(timeout);
- resolve(msg);
- },
+ resolve: wrappedResolve,
reject
});
});| test("multiple pending connections are tracked independently", async () => { | ||
| const control1 = await openSocket(`${baseWsUrl}/ws/control`); | ||
| const control2 = await openSocket(`${baseWsUrl}/ws/control`); | ||
| const { waitFor: waitFor1 } = collectMessages(control1); | ||
| const { waitFor: waitFor2 } = collectMessages(control2); | ||
|
|
||
| control1.send(JSON.stringify({ type: "auth", token: "test-token" })); | ||
| control2.send(JSON.stringify({ type: "auth", token: "test-token" })); | ||
|
|
||
| const pending1 = await waitFor1((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | ||
| const pending2 = await waitFor2((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | ||
|
|
||
| expect(approvalService.getPending()).toHaveLength(2); | ||
| expect(pending1.challengeCode).toBeTruthy(); | ||
| expect(pending2.challengeCode).toBeTruthy(); | ||
|
|
||
| // Approve only the first | ||
| const pendingList = approvalService.getPending(); | ||
| await approvalService.approve(pendingList[0].id); | ||
|
|
||
| const approved = await waitFor1((msg) => msg.type === "auth_approved") as { type: string }; | ||
| expect(approved.type).toBe("auth_approved"); | ||
|
|
||
| // Second should still be pending | ||
| expect(approvalService.getPending()).toHaveLength(1); | ||
|
|
||
| control1.close(); | ||
| control2.close(); | ||
| }); |
There was a problem hiding this comment.
Ordering assumption on pendingList[0] may cause intermittent failures
Lines 343–346 assume pendingList[0] belongs to control1, then wait for auth_approved on control1 via waitFor1. The Map preserves insertion order, but the server processes control1.send and control2.send based on which message arrives first in the event loop — not send-call order. If control2's message is processed first, pendingList[0] is control2's entry; approving it sends auth_approved to control2, not control1, and waitFor1 times out.
Use the challengeCode from the already-awaited pending1 to find the correct pending entry:
const pendingList = approvalService.getPending();
- await approvalService.approve(pendingList[0].id);
+ const entry1 = pendingList.find((p) => p.challengeCode === pending1.challengeCode);
+ expect(entry1).toBeDefined();
+ await approvalService.approve(entry1!.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("multiple pending connections are tracked independently", async () => { | |
| const control1 = await openSocket(`${baseWsUrl}/ws/control`); | |
| const control2 = await openSocket(`${baseWsUrl}/ws/control`); | |
| const { waitFor: waitFor1 } = collectMessages(control1); | |
| const { waitFor: waitFor2 } = collectMessages(control2); | |
| control1.send(JSON.stringify({ type: "auth", token: "test-token" })); | |
| control2.send(JSON.stringify({ type: "auth", token: "test-token" })); | |
| const pending1 = await waitFor1((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | |
| const pending2 = await waitFor2((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | |
| expect(approvalService.getPending()).toHaveLength(2); | |
| expect(pending1.challengeCode).toBeTruthy(); | |
| expect(pending2.challengeCode).toBeTruthy(); | |
| // Approve only the first | |
| const pendingList = approvalService.getPending(); | |
| await approvalService.approve(pendingList[0].id); | |
| const approved = await waitFor1((msg) => msg.type === "auth_approved") as { type: string }; | |
| expect(approved.type).toBe("auth_approved"); | |
| // Second should still be pending | |
| expect(approvalService.getPending()).toHaveLength(1); | |
| control1.close(); | |
| control2.close(); | |
| }); | |
| test("multiple pending connections are tracked independently", async () => { | |
| const control1 = await openSocket(`${baseWsUrl}/ws/control`); | |
| const control2 = await openSocket(`${baseWsUrl}/ws/control`); | |
| const { waitFor: waitFor1 } = collectMessages(control1); | |
| const { waitFor: waitFor2 } = collectMessages(control2); | |
| control1.send(JSON.stringify({ type: "auth", token: "test-token" })); | |
| control2.send(JSON.stringify({ type: "auth", token: "test-token" })); | |
| const pending1 = await waitFor1((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | |
| const pending2 = await waitFor2((msg) => msg.type === "auth_pending") as { type: string; challengeCode: string }; | |
| expect(approvalService.getPending()).toHaveLength(2); | |
| expect(pending1.challengeCode).toBeTruthy(); | |
| expect(pending2.challengeCode).toBeTruthy(); | |
| // Approve only the first | |
| const pendingList = approvalService.getPending(); | |
| const entry1 = pendingList.find((p) => p.challengeCode === pending1.challengeCode); | |
| expect(entry1).toBeDefined(); | |
| await approvalService.approve(entry1!.id); | |
| const approved = await waitFor1((msg) => msg.type === "auth_approved") as { type: string }; | |
| expect(approved.type).toBe("auth_approved"); | |
| // Second should still be pending | |
| expect(approvalService.getPending()).toHaveLength(1); | |
| control1.close(); | |
| control2.close(); | |
| }); |
) - Create Ink-based server-side TUI for approving/denying connections - Add frontend waiting overlay with challenge code display - Replace cleartext password storage with JWT in localStorage - Handle auth_pending, auth_approved, auth_denied message types - Add "Enter Password Instead" fallback from approval overlay - Add CSS for approval card, challenge code, and spinner Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Unit tests for ApprovalService (14 tests): pending management, JWT signing/verification, events - Unit tests for geoip resolver (3 tests): private IPs, invalid input, public IP lookup - Integration tests for approval flow (15 tests): password auth with JWT issuance, pending approval state, approve/deny flow, JWT reconnection, invalid JWT fallback, socket cleanup, terminal WS JWT auth, multiple pending connections, /api/config endpoint, --no-approve behavior - Fix existing test configs to include new approvalEnabled/jwtLifetimeSecs fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
guysmoilov
force-pushed
the
feat/issue-21-connection-approval-tui
branch
from
439d7c7 to
533141c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/frontend/styles/app.css (1)
702-716: Add aprefers-reduced-motionguard for the spinner animation.The spin animation runs unconditionally, which is problematic for users who have configured their OS to reduce motion.
♿ Proposed fix for reduced-motion accessibility
+@media (prefers-reduced-motion: reduce) { + .approval-spinner { + animation: none; + border-top-color: var(--accent); + opacity: 0.6; + } +}src/backend/tui/approval-tui.tsx (1)
67-85: Minor: down-arrow on an empty list setsselectedIndexto-1.When
pendingis empty, pressing down-arrow computesMath.min(-1, i + 1), resulting inselectedIndex = -1. The clamp effect on line 62 won't correct it because-1 >= 0is false. This has no visible impact since the empty-state branch renders, but it leaves state inconsistent. Consider guarding like you already do fora/d:Proposed fix
if (key.upArrow) { setSelectedIndex((i) => Math.max(0, i - 1)); } else if (key.downArrow) { - setSelectedIndex((i) => Math.min(pending.length - 1, i + 1)); + if (pending.length > 0) { + setSelectedIndex((i) => Math.min(pending.length - 1, i + 1)); + } } else if (input === "a" && pending.length > 0) {src/backend/auth/approval-service.ts (1)
17-29: Challenge code generation is subtly coupled to base64url output length.
randomToken(3)produces exactly 4 base64url characters for 3 bytes, so.slice(0, 4)is always a no-op. If someone changes the byte size, the challenge code length changes unpredictably. Consider making the intent explicit:Proposed clarification
- const challengeCode = randomToken(3).slice(0, 4).toUpperCase(); + // 3 bytes → 4 base64url chars; slice ensures exactly 4 chars + const challengeCode = randomToken(3).slice(0, 4).toUpperCase();Or decouple entirely:
- const challengeCode = randomToken(3).slice(0, 4).toUpperCase(); + const challengeCode = crypto.randomBytes(2).toString("hex").toUpperCase(); // 4 hex charssrc/backend/cli.ts (1)
129-133:jwtSecretis generated even when approval is disabled.
crypto.randomBytes(32)on line 130 runs unconditionally. WhenapprovalEnabledis false, the secret is never used. This is trivial overhead, but moving it inside the conditional would make the intent clearer.Proposed change
const approvalEnabled = !args.noApprove; - const jwtSecret = crypto.randomBytes(32); const approvalService = approvalEnabled - ? new ApprovalService({ jwtSecret, jwtLifetimeSecs: args.jwtLifetime }) + ? new ApprovalService({ jwtSecret: crypto.randomBytes(32), jwtLifetimeSecs: args.jwtLifetime }) : undefined;src/backend/server.ts (1)
249-270: Approval event listeners are never removed on server stop.The
approvalService.on("approved", ...)andon("denied", ...)listeners registered here are never cleaned up in thestop()method. If the server is stopped and a new one created sharing the sameapprovalServiceinstance (e.g. in tests), the stale listeners will fire against a deadcontrolClientsset.♻️ Proposed fix — remove listeners on stop
Store the handler references and remove them in
stop():+ let approvedHandler: ((event: { connectionId: string; jwt: string; clientId: string }) => void) | undefined; + let deniedHandler: ((event: { connectionId: string }) => void) | undefined; + if (approvalService) { - approvalService.on("approved", (event: { connectionId: string; jwt: string; clientId: string }) => { + approvedHandler = (event: { connectionId: string; jwt: string; clientId: string }) => { // ...existing logic... - }); - approvalService.on("denied", (event: { connectionId: string }) => { + }; + deniedHandler = (event: { connectionId: string }) => { // ...existing logic... - }); + }; + approvalService.on("approved", approvedHandler); + approvalService.on("denied", deniedHandler); }Then in
stop():stopPromise = (async () => { logger.log("server shutdown begin"); monitor?.stop(); + if (approvalService && approvedHandler) approvalService.off("approved", approvedHandler); + if (approvalService && deniedHandler) approvalService.off("denied", deniedHandler);src/frontend/App.tsx (4)
294-305:auth_okpath still persists cleartext password inlocalStorage.The PR objective states "Replace cleartext password storage with JWT stored in localStorage," but when the server responds with
auth_ok(non-approval path, line 294), the cleartext password is still saved at line 301. This means users on servers without approval enabled continue to have their password stored in the clear.If the intent is to phase out cleartext storage entirely, this path should also use a JWT (when
approvalServiceis present on the server, the server already issues one in theauth_okflow). If this is intentional for backward compatibility, consider adding a comment.
236-247: Terminal socket auth payload sends either JWT or password, never both — silent auth failure if JWT is present but invalid.When
jwtValueis truthy (line 242), the password is omitted from the auth payload. If the JWT turns out to be invalid/expired on the server, the terminal WS currently closes with4001(line 513 on server). The client then shows "terminal authentication failed" but has no automatic fallback to re-try with the password.This is acceptable if the control socket has already validated the JWT (and refreshed it), but worth noting that a race between JWT expiry and terminal socket open could cause a transient failure with no retry.
59-64: JWT stored inlocalStoragewith no expiry-awareness on the client side.The JWT is stored and re-used on reconnect, but there's no client-side check of the JWT's expiry (
expclaim) before sending it. This means the client will always attempt a round-trip with a potentially expired JWT before discovering it's invalid.This is a minor inefficiency — not a correctness issue — since the server validates it. But parsing the
expclaim and skipping the JWT if it's expired would save a wasted connection attempt.
867-886: Approval overlay looks good — minor accessibility note.The overlay correctly renders the challenge code as text content (safe from XSS in React) and provides a password fallback button. The
data-testidattributes are present for testing.One small accessibility improvement: consider adding
aria-live="polite"to the overlay or the challenge code element so screen readers announce the approval state when it appears.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/backend/auth/approval-service.tssrc/backend/auth/approval-types.tssrc/backend/cli.tssrc/backend/config.tssrc/backend/server.tssrc/backend/tui/approval-tui.tsxsrc/backend/types/protocol.tssrc/backend/util/geoip.tssrc/frontend/App.tsxsrc/frontend/styles/app.csssrc/frontend/types/protocol.tstests/backend/approval-service.test.tstests/backend/geoip.test.tstests/e2e/harness/test-server.tstests/integration/approval-flow.test.tstests/integration/server.test.tstsconfig.backend.json
🧰 Additional context used
📓 Path-based instructions (2)
**/types/protocol.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep protocol types in sync between backend (
src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts)
Files:
src/frontend/types/protocol.tssrc/backend/types/protocol.ts
**/server.ts
📄 CodeRabbit inference engine (AGENTS.md)
Preserve independent auth gating on both
/ws/controland/ws/terminalWebSocket endpoints
Files:
src/backend/server.ts
🧠 Learnings (3)
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/types/protocol.ts : Keep protocol types in sync between backend (`src/backend/types/protocol.ts`) and frontend (`src/frontend/types/protocol.ts`)
Applied to files:
src/backend/auth/approval-types.tssrc/frontend/types/protocol.tssrc/backend/types/protocol.tstsconfig.backend.jsonsrc/backend/server.ts
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/server.ts : Preserve independent auth gating on both `/ws/control` and `/ws/terminal` WebSocket endpoints
Applied to files:
src/backend/auth/approval-types.tstests/integration/approval-flow.test.tstests/backend/approval-service.test.tssrc/frontend/types/protocol.tssrc/backend/types/protocol.tstests/integration/server.test.tssrc/backend/server.tssrc/frontend/App.tsx
📚 Learning: 2026-02-14T22:15:23.148Z
Learnt from: CR
Repo: DagsHub/tmux-mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-14T22:15:23.148Z
Learning: Applies to **/tmux/**/*.ts : Use `execFile` with argument arrays in tmux command execution to prevent shell injection attacks
Applied to files:
tests/integration/server.test.ts
🧬 Code graph analysis (6)
tests/integration/approval-flow.test.ts (4)
src/backend/server.ts (2)
RunningServer(48-53)createTmuxMobileServer(86-648)src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/auth/auth-service.ts (1)
AuthService(8-32)tests/harness/ws.ts (1)
openSocket(34-38)
src/backend/tui/approval-tui.tsx (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/auth/approval-types.ts (1)
PendingConnection(3-12)
tests/backend/approval-service.test.ts (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/auth/approval-types.ts (1)
PendingConnection(3-12)
tests/backend/geoip.test.ts (1)
src/backend/util/geoip.ts (1)
resolveGeo(6-14)
src/backend/auth/approval-service.ts (2)
src/backend/auth/approval-types.ts (2)
PendingConnection(3-12)ApprovalServiceOptions(14-17)src/backend/util/random.ts (1)
randomToken(3-4)
src/backend/cli.ts (2)
src/backend/auth/approval-service.ts (1)
ApprovalService(6-88)src/backend/tui/approval-tui.tsx (1)
renderApprovalTui(126-128)
🪛 GitHub Check: test-and-build
tests/integration/approval-flow.test.ts
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > terminal WS authenticates with JWT
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 305-305: tests/integration/approval-flow.test.ts > approval flow integration > terminal WS authenticates with JWT
AssertionError: expected 3 to be 1 // Object.is equality
- Expected
- Received
- 1
- 3
❯ tests/integration/approval-flow.test.ts:305:33
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > JWT reconnection skips password and approval
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 232-232: tests/integration/approval-flow.test.ts > approval flow integration > JWT reconnection skips password and approval
AssertionError: expected 'tmux-mobile-client-zfgwUT7YFxBOhrYL' to be 'main' // Object.is equality
Expected: "main"
Received: "tmux-mobile-client-zfgwUT7YFxBOhrYL"
❯ tests/integration/approval-flow.test.ts:232:30
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > approving a pending connection sends auth_approved with JWT
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 185-185: tests/integration/approval-flow.test.ts > approval flow integration > approving a pending connection sends auth_approved with JWT
AssertionError: expected 'tmux-mobile-client-ETVBJEOvFIG1Tw86' to be 'main' // Object.is equality
Expected: "main"
Received: "tmux-mobile-client-ETVBJEOvFIG1Tw86"
❯ tests/integration/approval-flow.test.ts:185:30
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > password auth succeeds and issues JWT via auth_approved
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 142-142: tests/integration/approval-flow.test.ts > approval flow integration > password auth succeeds and issues JWT via auth_approved
AssertionError: expected 'tmux-mobile-client-SrOJwKInnQGS-1y1' to be 'main' // Object.is equality
Expected: "main"
Received: "tmux-mobile-client-SrOJwKInnQGS-1y1"
❯ tests/integration/approval-flow.test.ts:142:30
🪛 Stylelint (17.3.0)
src/frontend/styles/app.css
[error] 688-688: Unexpected quotes around "Menlo" (font-family-name-quotes)
(font-family-name-quotes)
[error] 688-688: Unexpected quotes around "Monaco" (font-family-name-quotes)
(font-family-name-quotes)
🔇 Additional comments (16)
src/backend/util/geoip.ts (1)
1-14: LGTM — clean and correct implementation.The null guard,
filter(Boolean)for missing city/region/country fields, and the dual"Unknown"fallback all handle the edge cases correctly. The privacy notice is a helpful reminder for operators.src/backend/config.ts (1)
11-24: LGTM — clean additions to both interfaces.The naming distinction between
jwtLifetime(CliArgs) andjwtLifetimeSecs(RuntimeConfig) is idiomatic for CLI-to-runtime mapping. Making both newRuntimeConfigfields required (non-optional) ensures all construction sites are updated.src/backend/types/protocol.ts (1)
2-2: LGTM —jwt?on the auth message enables JWT-based reconnection cleanly.tests/integration/server.test.ts (1)
21-24: LGTM — config correctly extended with the new required fields.
approvalEnabled: falseensures existing integration tests are unaffected by the approval flow, andjwtLifetimeSecs: 86400is a sensible default for test scenarios.package.json (1)
55-61: React downgraded from v19 to v18 for Ink 5.x compatibility.Ink 5.x does not work with React 19; the downgrade to React 18 is necessary. The frontend has no React 19-specific APIs in use, so the downgrade is safe.
src/frontend/types/protocol.ts (1)
36-38: New auth flow message variants look correct.The three new union members align with the approval flow:
auth_pendingcarries achallengeCode,auth_approvedreturns ajwt+clientId, andauth_deniedprovides areason. These are consistent with what the tests and server-side code expect.Per the coding guideline, verify these types are mirrored exactly in the backend protocol file.
[approve_code_changes, request_verification]
#!/bin/bash # Verify that the backend protocol types include the same auth_pending, auth_approved, auth_denied variants echo "=== Backend protocol.ts ===" fd 'protocol.ts' src/backend/types --exec cat {} echo "" echo "=== Comparing auth variants ===" rg -n 'auth_pending|auth_approved|auth_denied' src/backend/types/protocol.ts src/frontend/types/protocol.tsAs per coding guidelines:
**/types/protocol.ts: "Keep protocol types in sync between backend (src/backend/types/protocol.ts) and frontend (src/frontend/types/protocol.ts)"src/backend/tui/approval-tui.tsx (1)
126-128: LGTM — clean entry point.The
renderApprovalTuiexport cleanly mounts the Ink app with the provided service instance. Straightforward.src/backend/auth/approval-types.ts (1)
1-17: LGTM — clean type definitions.The
PendingConnectionandApprovalServiceOptionsinterfaces are well-structured and properly consumed by the service, server, and tests.tests/e2e/harness/test-server.ts (1)
45-47: LGTM — sensible defaults for E2E harness.Approval is disabled by default in the E2E test server, consistent with pre-existing tests not needing the approval flow.
tests/backend/approval-service.test.ts (1)
1-143: Solid unit test suite.Comprehensive coverage of the
ApprovalServicepublic API: add/approve/deny/remove flows, event emissions, JWT signing/verification, and cross-secret rejection. Well-organized with clear assertions.src/backend/cli.ts (1)
185-188: LGTM — lazy import of TUI.Dynamic
import()of the Ink-based TUI avoids pulling in React/Ink when approval is disabled. Clean approach.tests/integration/approval-flow.test.ts (2)
140-142:attached.sessionassertion fails in CI — server returns clientId, not session name.Static analysis shows multiple tests failing here with values like
"tmux-mobile-client-SrOJwKInnQGS-1y1"instead of"main". The same failure occurs at lines 185 and 232. The server'sattachedmessage appears to be sendingclientIdin thesessionfield rather than the actual tmux session name. Investigate the server-side attach flow after approval to ensure it sends the correct session name.[raise_critical_issue, request_verification]
#!/bin/bash # Find the server-side code that sends the "attached" message to understand what value is placed in the session field rg -n '"attached"' src/backend/server.ts -C 5 echo "---" # Also check how the session is determined after auth_approved rg -n 'attached.*session' src/backend/server.ts -C 3
288-309: Terminal WS JWT auth test fails — socket closes instead of staying open.CI reports
readyStateis3(CLOSED) instead of1(OPEN) at line 305. The 50ms sleep on line 302 is likely insufficient for the async auth to complete, but more importantly the terminal WS auth path may not properly handle JWT-only auth (no password). Verify the server-side terminal WS auth handles the JWT path and doesn't close the socket.Additionally, the test lacks a proper assertion mechanism — checking
readyStateafter a fixed delay is inherently racy. Consider listening for a specific auth success/error message or using a close event with a timeout.[raise_critical_issue, request_verification]
#!/bin/bash # Inspect the terminal WS authentication handling in server.ts rg -n 'terminal' src/backend/server.ts | head -30 echo "---" # Look at how JWT auth is handled on the terminal WS path rg -nP 'jwt|terminal.*auth' src/backend/server.ts -C 5src/backend/server.ts (2)
97-103:/api/configexposesapprovalEnabled— verify this doesn't leak sensitive posture info.Exposing whether approval mode is active tells unauthenticated clients how auth works. This is likely intentional (the frontend needs it to decide the UI flow), but worth confirming that revealing this is acceptable from a security standpoint.
414-431: No changes needed. The narrow condition at line 414 (authResult.reason === "invalid password") is intentional design, not a bug.The AuthService distinguishes between two failure modes: "invalid token" (client lacks proper credentials entirely) and "invalid password" (client has valid token but wrong password). Approval flow only triggers for the latter because it represents a known client requesting access with partial credentials—the intended approval scenario. Invalid tokens are rejected outright. This design is documented in code comments (line 34) and validated by tests. Both
/ws/controland/ws/terminalmaintain independent auth gating as required.Likely an incorrect or invalid review comment.
src/frontend/App.tsx (1)
515-521:switchToPasswordEntrycorrectly closes the pending socket — server cleans up the pending approval.Closing the control socket here triggers the server's
socket.on("close")handler (server.ts line 457), which removes the pending approval. The user can then submit a password viasubmitPassword, which opens a fresh control socket. This flow is clean.
| if (authMessage.jwt && approvalService) { | ||
| ctx.authInProgress = true; | ||
| const jwtResult = await approvalService.verifyJwt(authMessage.jwt); | ||
| ctx.authInProgress = false; | ||
| if (jwtResult.valid) { | ||
| ctx.authed = true; | ||
| logger.log("terminal ws jwt auth ok"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Terminal WS JWT-auth path never sets controlClientId / controlContext — terminal I/O is silently broken.
When the JWT path succeeds, only ctx.authed is set to true (line 501) and the handler returns (line 503). ctx.controlClientId and ctx.controlContext are never assigned, and the terminal socket is never added to controlContext.terminalClients.
Consequences:
- Incoming terminal data hits
ctx.controlContext?.runtime?.write(...)(line 531) which isundefined?.runtime?.write(...)→ silently dropped. - Resize messages are similarly lost (line 545).
- The control context never knows about this terminal client, so
broadcastStateterminal output is never forwarded.
The non-JWT path (lines 507-526) correctly looks up controlContext and wires everything. The JWT path must do the same.
🐛 Proposed fix — wire controlContext after JWT auth
if (authMessage.jwt && approvalService) {
ctx.authInProgress = true;
const jwtResult = await approvalService.verifyJwt(authMessage.jwt);
ctx.authInProgress = false;
if (jwtResult.valid) {
- ctx.authed = true;
- logger.log("terminal ws jwt auth ok");
- return;
+ const controlContext = getControlContext(clientId);
+ if (!controlContext || !controlContext.authed) {
+ socket.close(4001, "unauthorized");
+ return;
+ }
+ ctx.authed = true;
+ ctx.controlClientId = clientId;
+ ctx.controlContext = controlContext;
+ controlContext.terminalClients.add(ctx);
+ logger.log("terminal ws jwt auth ok");
+ return;
}
}| | { type: "auth_pending"; challengeCode: string } | ||
| | { type: "auth_approved"; jwt: string; clientId: string } | ||
| | { type: "auth_denied"; reason: string } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the frontend protocol file exists and view its contents
if [ -f src/frontend/types/protocol.ts ]; then
echo "=== Frontend Protocol File ===" && cat src/frontend/types/protocol.ts
else
echo "File not found at src/frontend/types/protocol.ts"
# Try to find the file
find . -name "protocol.ts" -type f 2>/dev/null | head -20
fiRepository: DagsHub/tmux-mobile
Length of output: 1281
🏁 Script executed:
# Also verify the backend file to understand the complete context
echo "=== Backend Protocol File (lines around 50-52) ===" && sed -n '40,65p' src/backend/types/protocol.tsRepository: DagsHub/tmux-mobile
Length of output: 822
🏁 Script executed:
# Search for ControlClientMessage definition in both files
echo "=== Backend ControlClientMessage ===" && grep -A 10 "ControlClientMessage" src/backend/types/protocol.ts
echo ""
echo "=== Frontend ControlClientMessage ===" && grep -A 10 "ControlClientMessage" src/frontend/types/protocol.tsRepository: DagsHub/tmux-mobile
Length of output: 725
Add ControlClientMessage type definition to the frontend protocol file.
The three new ControlServerMessage variants (auth_pending, auth_approved, auth_denied) are correctly synced between backend and frontend. However, the frontend protocol file is missing the entire ControlClientMessage type definition, which includes the required jwt?: string field. The frontend cannot properly type auth messages without this type.
Add the following to src/frontend/types/protocol.ts:
export type ControlClientMessage =
| { type: "auth"; token?: string; password?: string; clientId?: string; jwt?: string }
| { type: "select_session"; session: string }
| { type: "new_session"; name: string }
| { type: "new_window"; session: string }
| { type: "select_window"; session: string; windowIndex: number }
| { type: "kill_window"; session: string; windowIndex: number }
| { type: "select_pane"; paneId: string }
| { type: "split_pane"; paneId: string; orientation: "h" | "v" }
| { type: "kill_pane"; paneId: string }
| { type: "zoom_pane"; paneId: string }| // Try JWT first if stored | ||
| const storedJwt = jwtRef.current; | ||
| if (storedJwt) { | ||
| openControlSocket("", storedJwt); | ||
| return; | ||
| } | ||
|
|
||
| // If password required and no password stored, show prompt | ||
| if (config.passwordRequired && !password) { | ||
| // If approval is enabled and no password, connect without password to trigger approval | ||
| if (config.approvalEnabled) { | ||
| openControlSocket(""); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Stored JWT that is expired/revoked triggers a connection with no automatic fallback.
If jwtRef.current contains an expired JWT (line 402), openControlSocket("", storedJwt) is called. The server responds with auth_error, which clears the JWT (lines 338-341). However, the useEffect has already returned — there's no retry logic to re-run the connection attempt with password or approval flow after the JWT is cleared.
The user would see an error and the password prompt (if passwordRequired), but for approval-enabled servers without a password requirement, they'd be stuck with just an error message and no way to re-trigger the approval flow without a page refresh.
💡 Suggested approach — retry after JWT failure
In the auth_error handler (around line 338), after clearing the invalid JWT, automatically retry the connection:
if (jwtValue) {
jwtRef.current = null;
localStorage.removeItem("tmux-mobile-jwt");
+ // Retry without JWT to trigger normal auth/approval flow
+ if (serverConfig?.approvalEnabled) {
+ openControlSocket("");
+ return;
+ }
}Note: be cautious of infinite retry loops — only retry once after JWT clearance.
| beforeEach(async () => { | ||
| await startServer({ password: "secret123" }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await runningServer.stop(); | ||
| }); |
There was a problem hiding this comment.
afterEach hook times out due to unclosed sockets from failing tests.
CI reports "Hook timed out in 10000ms" at the afterEach (line 121). When preceding test assertions fail, sockets may remain open, preventing runningServer.stop() from completing. Consider forcefully closing all tracked connections before stopping the server, or increase the hook timeout for integration tests.
🛠 Suggested approach — add a cleanup timeout
afterEach(async () => {
- await runningServer.stop();
+ // Force close to avoid hanging when tests leave sockets open
+ await Promise.race([
+ runningServer.stop(),
+ new Promise<void>((resolve) => setTimeout(() => resolve(), 5000))
+ ]);
});A better long-term fix is to track all opened sockets in the test and close them in afterEach before calling stop().
🧰 Tools
🪛 GitHub Check: test-and-build
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > terminal WS authenticates with JWT
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > JWT reconnection skips password and approval
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > approving a pending connection sends auth_approved with JWT
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
[failure] 121-121: tests/integration/approval-flow.test.ts > approval flow integration > password auth succeeds and issues JWT via auth_approved
Error: Hook timed out in 10000ms.
If this is a long-running hook, pass a timeout value as the last argument or configure it globally with "hookTimeout".
❯ tests/integration/approval-flow.test.ts:121:3
1 participant
Implementation Details
Dependencies & Tooling
geoip-litefor IP-based geolocation resolution of pending connectionsinkfor the server-side TUI renderingjosefor JWT cryptographic operations (HS256)Core Services & Types
pending-added,approved,denied,pending-removedauth_pending(challenge code)auth_approved(JWT and clientId)auth_denied(reason)Server-Side Changes
--no-approve(disable approval) and--jwt-lifetime(configure JWT expiration, default 86400s)/api/configendpoint now exposesapprovalEnabledflagFrontend Integration
.approval-card,.challenge-code, and spinner animation CSS for UI presentationTesting
/api/configreflection, and--no-approvebehaviorConfiguration
approvalEnabledandjwtLifetimeSecsfieldspendingApprovalIdfield to track approval state per connectionauthInProgressflag to guard terminal authentication during pending approvals