feat: add WSS support for HTTPS environments#158
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds WSS support but has several critical issues that need to be addressed. Most concerning: health checks will fail when --secure is used because they use HTTPS while browser clients expect the relay to be already running on WSS, and self-signed certificates are disabled without proper verification.
packages/relay/src/connection.ts
Outdated
| const httpProtocol = secure ? "https" : "http"; | ||
| const healthUrl = token | ||
| ? `http://localhost:${port}/health?${RELAY_TOKEN_PARAM}=${encodeURIComponent(token)}` | ||
| : `http://localhost:${port}/health`; | ||
| ? `${httpProtocol}://localhost:${port}/health?${RELAY_TOKEN_PARAM}=${encodeURIComponent(token)}` | ||
| : `${httpProtocol}://localhost:${port}/health`; |
There was a problem hiding this comment.
Critical: Health checks will fail in secure mode. When secure is true, this health check uses https://localhost:${port}/health, but Node.js fetch() will reject self-signed certificates by default. This means even if the server is running with valid mkcert certificates, the health check will fail, causing connectRelay to think the server isn't running and attempt to start a new one.
You need to disable certificate verification for the health check fetch, similar to how you do for WebSocket connections. Consider adding { agent: new https.Agent({ rejectUnauthorized: false }) } to the fetch options when secure is true, or set NODE_TLS_REJECT_UNAUTHORIZED=0 for the process (less secure but simpler).
packages/relay/src/connection.ts
Outdated
| : `${webSocketProtocol}://localhost:${port}`; | ||
| const socket = new WebSocket(connectionUrl, { | ||
| headers: { "x-relay-handler": "true" }, | ||
| rejectUnauthorized: false, |
There was a problem hiding this comment.
Security concern: This disables certificate verification for ALL WSS connections. While this is necessary for mkcert self-signed certificates, it means the WebSocket client will accept any certificate, making it vulnerable to man-in-the-middle attacks. This defeats much of the purpose of using WSS.
Consider only setting rejectUnauthorized: false when secure is true, and document that this is necessary for development with self-signed certificates. For production use cases, users should provide their own properly-signed certificates and this should be true.
| ); | ||
| } | ||
| await downloadMkcert(downloadUrl); | ||
| await runMkcert("-install"); |
There was a problem hiding this comment.
Security risk: mkcert -install modifies system trust store. Running mkcert -install adds the CA certificate to the system's trust store, which is a privileged operation that can fail or require user interaction (especially on macOS/Windows). This happens silently during server startup, which is unexpected behavior.
Consider:
- Only running
-installonce on first setup, and storing a flag in DATA_DIR to prevent re-running - Logging a clear message that the system trust store is being modified
- Catching and handling errors gracefully if the user denies permission
- Documenting this behavior clearly in the PR description and user-facing docs
| const downloadUrl = await fetchMkcertDownloadUrl(); | ||
| if (!downloadUrl) { | ||
| throw new Error( | ||
| `Unsupported platform: ${platform()} ${arch()}. Cannot download mkcert.`, | ||
| ); | ||
| } | ||
| await downloadMkcert(downloadUrl); | ||
| await runMkcert("-install"); |
There was a problem hiding this comment.
Race condition: Multiple processes could download mkcert simultaneously. If multiple providers start with --secure at the same time (not uncommon), they'll all check existsSync(MKCERT_PATH), see it doesn't exist, and simultaneously download/install mkcert. This could cause file corruption or race conditions.
Consider using a lock file or atomic file operations to prevent concurrent downloads.
| const response = await fetch(downloadUrl); | ||
| if (!response.ok || !response.body) { | ||
| throw new Error(`Failed to download mkcert: ${response.statusText}`); |
There was a problem hiding this comment.
Error handling: No verification of downloaded binary. The code downloads an executable from GitHub and runs it with elevated privileges (via -install) without any checksum verification. While GitHub release assets are generally trustworthy, this creates a supply-chain risk.
Consider:
- Verifying checksums of downloaded binaries against known good values
- At minimum, checking that the response is successful and the file size is reasonable
- Documenting this security consideration
packages/relay/src/mkcert.ts
Outdated
| if (!existsSync(KEY_PATH) || !existsSync(CERT_PATH)) { | ||
| await runMkcert( | ||
| `-key-file "${KEY_PATH}" -cert-file "${CERT_PATH}" ${hosts.join(" ")}`, | ||
| ); |
There was a problem hiding this comment.
Race condition: Multiple processes could generate certificates simultaneously. Similar to the mkcert download issue, if multiple processes start with --secure and certificates don't exist, they could all try to generate certificates at the same time, potentially causing corruption or errors.
Consider using a lock file or checking for the existence of temporary files that indicate certificate generation is in progress.
packages/relay/src/client.ts
Outdated
| const isSecure = | ||
| typeof window !== "undefined" && window.location.protocol === "https:"; | ||
| return `${isSecure ? "wss" : "ws"}://localhost:${DEFAULT_RELAY_PORT}`; |
There was a problem hiding this comment.
Logic issue: This auto-detection only works in browsers, not in Node.js clients. The window check ensures this only applies in browser environments, but if a Node.js process wants to connect to a relay running in secure mode, it will default to ws:// and fail to connect.
Consider allowing explicit configuration via options, or documenting that Node.js clients must explicitly specify wss:// in serverUrl when connecting to a secure relay.
| } | ||
|
|
||
| const getPlatformIdentifier = (): string => { | ||
| const architecture = arch() === "x64" ? "amd64" : arch(); |
There was a problem hiding this comment.
Platform support: arch() returns different values than expected. Node.js arch() can return values like arm64, arm, ia32 etc., but mkcert releases use arm64, arm, 386, etc. The code only handles x64 → amd64 mapping, meaning arm, ia32, and other architectures will fail to find matching releases.
Either add mappings for all Node.js arch values, or provide a clear error message listing supported platforms.
| }; | ||
|
|
||
| const runMkcert = async (args: string): Promise<void> => { | ||
| await exec(`"${MKCERT_PATH}" ${args}`, MKCERT_ENV); |
There was a problem hiding this comment.
Command injection risk: args is not properly escaped. While args is constructed from trusted sources in this codebase, the pattern exec(\"${MKCERT_PATH}" ${args}`)is vulnerable if args ever contains shell metacharacters. Since the paths containKEY_PATH, CERT_PATH, and DATA_DIR(which includeshomedir()`), a user with a malicious home directory path could potentially inject commands.
Consider using execFile() instead of exec() to avoid shell interpretation entirely.
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
@react-grab/utils
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 18 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/relay/src/connection.ts">
<violation number="1" location="packages/relay/src/connection.ts:146">
P1: Avoid disabling TLS certificate verification for WSS connections. `rejectUnauthorized: false` makes the client accept any certificate, which defeats TLS security and enables man‑in‑the‑middle attacks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/relay/src/connection.ts
Outdated
| : `${webSocketProtocol}://localhost:${port}`; | ||
| const socket = new WebSocket(connectionUrl, { | ||
| headers: { "x-relay-handler": "true" }, | ||
| rejectUnauthorized: false, |
There was a problem hiding this comment.
P1: Avoid disabling TLS certificate verification for WSS connections. rejectUnauthorized: false makes the client accept any certificate, which defeats TLS security and enables man‑in‑the‑middle attacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/relay/src/connection.ts, line 146:
<comment>Avoid disabling TLS certificate verification for WSS connections. `rejectUnauthorized: false` makes the client accept any certificate, which defeats TLS security and enables man‑in‑the‑middle attacks.</comment>
<file context>
@@ -114,15 +132,18 @@ const connectToExistingRelay = async (
+ : `${webSocketProtocol}://localhost:${port}`;
const socket = new WebSocket(connectionUrl, {
headers: { "x-relay-handler": "true" },
+ rejectUnauthorized: false,
});
</file context>
| rejectUnauthorized: false, | |
| // rely on default certificate validation | |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/relay/src/client.ts">
<violation number="1" location="packages/relay/src/client.ts:38">
P2: Default relay URL now follows the page hostname, which breaks local relay connectivity when the UI is hosted remotely. Keep `localhost` as the default host and only switch the protocol to `wss` for HTTPS pages.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Bugbot Autofix prepared fixes for 4 of the 4 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 4 of the 4 bugs found in the latest run.
|
| const getDefaultWebSocketUrl = (): string => { | ||
| if (typeof window === "undefined") { | ||
| return `ws://localhost:${DEFAULT_RELAY_PORT}`; | ||
| } |
There was a problem hiding this comment.
Browser client on custom hostname fails WebSocket connection due to certificate validation error when relay server generates certificates only for localhost
|
|
||
| const CA_INSTALLED_FLAG = join(DATA_DIR, ".ca-installed"); | ||
|
|
||
| export const ensureCertificates = async ( |
There was a problem hiding this comment.
Race condition in mkcert download: Multiple concurrent processes can check existsSync(MKCERT_PATH) simultaneously, see it doesn't exist, and all proceed to download and write the same file concurrently, causing file corruption or permission errors.
|
Bugbot Autofix prepared fixes for 4 of the 4 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
| await chmod(MKCERT_PATH, 0o755); | ||
| } catch (error) { | ||
| const { unlink } = await import("node:fs/promises"); | ||
| await unlink(MKCERT_PATH).catch(() => {}); |
There was a problem hiding this comment.
Unnecessary dynamic import for unlink
Low Severity
unlink is dynamically imported from node:fs/promises at line 71, but this module is already statically imported at line 3 for chmod, mkdir, readFile, and writeFile. Simply add unlink to the existing static import instead of using await import().
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
- Move close handler outside open handler in attemptConnection to ensure reconnection retries work even when connection attempts fail - Return detected protocol from health check to prevent WebSocket protocol mismatch when secure parameter is undefined - Add try-catch in connect() to clear stale rejected promise and prevent permanent connection failure
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/relay/src/connection.ts">
<violation number="1" location="packages/relay/src/connection.ts:182">
P2: `connectRelay` may still connect with `ws://` after the relay auto-upgrades to HTTPS if `secure` is explicitly false, because `checkIfRelayServerIsRunning` doesn’t return the actual protocol in that branch. This causes a protocol mismatch and failed connection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/relay/src/connection.ts
Outdated
| if (secure !== undefined) { | ||
| const result = await tryProtocol(secure); | ||
| if (result) return true; | ||
| return await tryProtocol(!secure); |
There was a problem hiding this comment.
Protocol fallback loses actual server mode information
High Severity
When secure is explicitly specified but the server is running on the opposite protocol, checkIfRelayServerIsRunning returns only a boolean (true/false) instead of an object with isSecure information. This causes detectedSecure to be undefined, and connectToExistingRelay then uses the originally requested secure value instead of the actual protocol the server is using. For example, if --secure is passed but the server runs HTTP, the code will attempt a WSS connection to an HTTP server, which fails.
Additional Locations (1)
| } catch {} | ||
| }); | ||
|
|
||
| const proxyServer: RelayServer = { |
There was a problem hiding this comment.
Orphaned reconnect loop on initial connection failure
Medium Severity
When the initial WebSocket connection in attemptConnection fails, both error and close events fire. The error event rejects the promise (causing connectToExistingRelay to throw), but the close event also fires and schedules a reconnect because isExplicitDisconnect is still false. Since no proxyServer is returned when the initial connection fails, there's no way to set isExplicitDisconnect = true, resulting in orphaned reconnection attempts that run indefinitely in the background and may eventually register duplicate handlers.
| continue; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Network errors silently succeed while HTTP errors throw
Medium Severity
In ensureServerReady, when the last retry attempt encounters a network error (catch block), the function silently returns instead of throwing. However, when all attempts receive non-OK HTTP responses or "upgrading" status, the loop exits normally and throws an error. This inconsistency means network errors are silently swallowed while HTTP errors properly propagate, which can mask connectivity issues and make debugging harder.
… loop bugs - Fix protocol fallback to return isSecure info even when secure param is explicitly set - Fix ensureServerReady to throw on network errors after max retries instead of silent return - Fix orphaned reconnect loop by tracking hasConnectedOnce flag to prevent reconnects on initial connection failure
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
|
| ? healthCheckResult.isSecure | ||
| : undefined; | ||
|
|
||
| let isSecureMode = secure ?? detectedSecure ?? false; |
There was a problem hiding this comment.
Startup message shows wrong protocol when connecting to existing relay
Medium Severity
isSecureMode is calculated as secure ?? detectedSecure ?? false, which prioritizes the user's --secure flag over the detected server mode. However, connectToExistingRelay is called with detectedSecure ?? secure, which has the opposite priority. When a user passes --secure but an existing server is running in HTTP mode, isSecureMode becomes true while the actual connection uses HTTP (detectedSecure = false). This causes printStartupMessage to display wss://localhost:4722 when the actual connection is ws://localhost:4722, misleading users about the connection state.
Additional Locations (1)
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
| handler, | ||
| token, | ||
| useSecure, | ||
| ); |
There was a problem hiding this comment.
EADDRINUSE handler ignores detected secure mode
High Severity
When handling EADDRINUSE in startServer, the code calls checkIfRelayServerIsRunning which returns { isRunning: true, isSecure: boolean } with the actual protocol the running server uses. However, connectToExistingRelay is called with useSecure (the requested mode) instead of extracting isSecure from isNowRunning. This causes connection failures when Process A runs a relay in HTTP mode and Process B tries to start in HTTPS mode—it will attempt a WSS connection to a WS server. Compare to lines 179-187 where this is handled correctly by extracting detectedSecure from the health check result.
| const socket = new WebSocket(connectionUrl, { | ||
| headers: { "x-relay-handler": "true" }, | ||
| rejectUnauthorized: false, | ||
| }); |
There was a problem hiding this comment.
Handler reconnection fails after server secure upgrade
High Severity
When a relay server upgrades from HTTP to HTTPS (triggered by a browser health check with secure=true), remote handlers connected via connectToExistingRelay cannot reconnect. The secure parameter is captured once at initial connection time and reused for all reconnection attempts. After upgrade, reconnection attempts use ws:// against an HTTPS server requiring wss://, causing permanent disconnection with infinite failed retry loops.
Additional Locations (1)
| readFile(CERT_PATH), | ||
| ]); | ||
| return { key, cert }; | ||
| }; |
There was a problem hiding this comment.
Concurrent mkcert operations can corrupt certificate files
Medium Severity
The ensureCertificates function lacks file locking or atomic write patterns. If multiple provider processes start simultaneously with --secure (e.g., in a monorepo running multiple services), they can concurrently download mkcert, install the CA, or generate certificates to the same paths. Concurrent createWriteStream calls to the same file interleave data, corrupting the binary or certificates and causing cryptic failures on subsequent runs.
Additional Locations (1)
Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| await delay(); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
HTTPS endpoint never tried when HTTP fails
High Severity
In ensureServerReady, when the browser is in a secure context, the code only attempts the HTTP health endpoint while didTriggerUpgrade is false. If the HTTP fetch fails (due to mixed content blocking on non-localhost HTTPS pages, or because the server is already running in HTTPS-only mode), didTriggerUpgrade is never set to true, so the HTTPS endpoint is never tried. This causes all connection attempts to time out after MAX_UPGRADE_RETRIES, even when the server is healthy and running in HTTPS mode.
| setTimeout(() => { | ||
| attemptConnection().catch(() => {}); | ||
| }, HANDLER_RECONNECT_DELAY_MS); | ||
| } |
There was a problem hiding this comment.
Handler reconnects with wrong protocol after server upgrade
Medium Severity
In connectToExistingRelay, the secure parameter is captured once at function call time and reused for all reconnection attempts. When the server upgrades from HTTP to HTTPS while handlers are connected, handlers' sockets close, triggering reconnects using the original protocol value (ws:// instead of wss://). Since the server is now HTTPS-only, these reconnects fail, but the close event schedules another reconnect, creating an infinite loop of failed connection attempts.
- Fix Bug 1: HTTPS endpoint now tried when HTTP fails in secure context by tracking consecutive HTTP failures and falling back to HTTPS after threshold - Fix Bug 2: Handler reconnects with correct protocol after server upgrade by dynamically checking server protocol before reconnection attempts
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
| token, | ||
| actualSecure, | ||
| ); | ||
| isSecureMode = actualSecure ?? false; |
There was a problem hiding this comment.
Secure flag silently ignored when connecting to existing server
Medium Severity
The --secure flag is silently ignored when connecting to an existing HTTP relay server. At line 191, actualSecure = detectedSecure ?? secure uses the nullish coalescing operator, which only substitutes for null/undefined, not false. When an HTTP server is running (detectedSecure = false) and the user passes --secure (secure = true), the result is false, causing the handler to connect via WS instead of WSS. This is inconsistent with line 115 where secure ?? detectedSecure ?? false correctly prioritizes user preference when starting a new server.
| setTimeout(() => { | ||
| attemptConnection().catch(() => {}); | ||
| }, HANDLER_RECONNECT_DELAY_MS); | ||
| } |
There was a problem hiding this comment.
Missing reconnection timeout cleanup causes resource leak
Medium Severity
The reconnection setTimeout at line 322 doesn't store its timeout ID, so it cannot be cancelled when stop() is called. If the socket closes and then stop() is called before the reconnection delay expires, the timeout still fires and creates a new connection. This leaves an orphan handler connected to the relay server after stop() was called. The client-side code (client.ts) correctly stores reconnectTimeoutId and clears it in disconnect(), but this new reconnection logic lacks equivalent cleanup.
Additional Locations (1)
…imeout - Fix secure flag being silently ignored when connecting to existing server by prioritizing user's explicit --secure preference over detected mode - Fix resource leak by storing and clearing reconnection timeout ID in stop() and unregisterHandler() to prevent orphan connections after explicit disconnect
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/relay/src/connection.ts">
<violation number="1" location="packages/relay/src/connection.ts:191">
P2: Use the detected server protocol when a relay is already running; prioritizing the caller-provided `secure` flag can force `ws://` against a `wss://` server and break connections.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/relay/src/connection.ts
Outdated
| }; | ||
|
|
||
| if (isRelayServerRunning) { | ||
| const actualSecure = secure ?? detectedSecure; |
There was a problem hiding this comment.
Protocol mismatch when connecting to existing relay
Medium Severity
The expression secure ?? detectedSecure incorrectly prioritizes the user's secure flag over the detected server protocol. The nullish coalescing operator (??) only falls back when the left operand is null or undefined, not false. If a handler is started with explicit secure: false but the relay server is running HTTPS, actualSecure becomes false, causing the handler to attempt a ws:// connection to a wss:// server, which fails. The same issue exists in the EADDRINUSE recovery path where useSecure is passed instead of isNowRunning.isSecure.
Additional Locations (1)
- Prioritize detected protocol over user preference when connecting to existing servers - Use isNowRunning.isSecure instead of useSecure in EADDRINUSE recovery path - This prevents ws:// connection attempts to wss:// servers and vice versa
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
| } | ||
| const protocol = isSecureContext() ? "wss:" : "ws:"; | ||
| return `${protocol}//${window.location.hostname}:${DEFAULT_RELAY_PORT}`; | ||
| }; |
There was a problem hiding this comment.
WSS hostname mismatch causes connection failures for non-localhost
Medium Severity
The browser client's getDefaultWebSocketUrl uses window.location.hostname for WebSocket connections, but ensureCertificates generates certificates only for localhost and 127.0.0.1 by default. When users access pages via custom hostnames (e.g., myapp.local with localias), the browser will try to connect to wss://myapp.local:4722, but the certificate isn't valid for that hostname, causing certificate validation failures. Additionally, the upgrade mechanism sends HTTP requests from HTTPS pages to trigger server upgrades, which browsers block as mixed content for non-localhost hostnames—so the server never receives the upgrade request and stays in HTTP mode. This contradicts the PR's claim of "Compatibility with local SSL setups (e.g., localias)".
Additional Locations (1)
2 participants
Summary
https:protocol and useswss://instead ofws://--secureflag for WSS connectionsUsage
This enables:
Test plan
--secureflag with providersCloses #156
Note
Medium Risk
Touches core relay connectivity (protocol selection, upgrade flow, and TLS certificate generation), which can cause connection failures or upgrade loops if misbehaving, but changes are scoped and include retries/health checks and explicit
--secureopt-in.Overview
Enables secure relay connections (HTTPS + WSS) end-to-end: the browser
RelayClientnow defaults towss://when running onhttps:pages, preflights/healthwith retries, and can trigger/await an HTTP→HTTPS upgrade before opening the WebSocket.The relay server can now run in secure mode using locally generated certificates via new
mkcertintegration, exposessecurestatus (and anupgradingstate) on/health, supports CORS/OPTIONS, and improves shutdown semantics to avoid port races. Provider CLIs are simplified to a sharedspawnDetachedServerhelper and provider servers now accept--secureto request secure relay mode; handler connections gain ws/wss-aware health checks and automatic reconnect behavior.Minor cleanups include formatting-only tweaks in UI/e2e tests, adding
certificatestopackages/gym/.gitignore, and updating@types/nodefor@react-grab/utils.Written by Cursor Bugbot for commit c2472f4. This will update automatically on new commits. Configure here.
Summary by cubic
Adds WSS support for HTTPS pages with auto‑upgrade, mkcert certs, and hostname‑aware client defaults. Improves reliability with protocol‑aware health checks, handler auto‑reconnect, safer upgrade/cert handling, honors the --secure flag; closes #156.
New Features
Bug Fixes
Written for commit c2472f4. Summary will update on new commits.