Conversation
- Add bootstrap-token pairing flow and signed auth cookies - Require browser origin/cookie checks for webSocket and session routes - Update web client to exchange pairing tokens before connecting
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return; | ||
| } | ||
|
|
||
| const cookieHeader = yield* browserAuth.createAuthCookie(url.protocol === "https:"); |
There was a problem hiding this comment.
Cookie Secure flag can never be set
Low Severity
The isSecure argument to createAuthCookie is computed as url.protocol === "https:", but url is always constructed with http://localhost:${port} as the base URL. This means url.protocol is always "http:", so the Secure cookie flag can never be set regardless of the actual transport. The conditional logic in formatAuthCookie for adding the Secure attribute is dead code.
Additional Locations (1)
|
|
||
| cachedApi = createWsNativeApi(); | ||
| return cachedApi; | ||
| } |
There was a problem hiding this comment.
initializeNativeApi duplicates existing readNativeApi logic
Low Severity
initializeNativeApi has an identical body to readNativeApi in browser contexts — both check the cache, fall back to window.nativeApi, then create via createWsNativeApi(). The only difference is the non-browser branch (throw vs. return undefined). The shared logic could be consolidated to avoid maintaining two copies.
Additional Locations (1)
| message: Schema.String, | ||
| cause: Schema.optional(Schema.Defect), | ||
| }, | ||
| ) {} |
There was a problem hiding this comment.
BrowserAuthError exported but never used anywhere
Low Severity
BrowserAuthError is defined and exported but never referenced anywhere in the codebase — not in wsServer.ts, tests, or any other module. This is dead code that adds surface area without being used.
| return () => { | ||
| active = false; | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
StrictMode double-effect causes bootstrap token race condition
Medium Severity
Under React.StrictMode (enabled at line 96), the effect runs twice. The first invocation of ensureBrowserPairing synchronously clears the bootstrap token from the URL hash and fires the exchange fetch. Cleanup then sets active = false. The second invocation finds no token in the URL and immediately calls fetchSession, which races against the still-in-flight exchange POST. Since the auth cookie hasn't been set yet, the session check returns false, and the component transitions to the "unpaired" screen even though the exchange succeeded. The one-time server-side token is consumed, so refreshing won't help.
Additional Locations (1)
| state.expiresAt > now && | ||
| state.token === token; | ||
| return [matches, matches ? { ...state, consumedAt: now } : state] as const; | ||
| }); |
There was a problem hiding this comment.
Bootstrap token compared without timing-safe equality
Low Severity
consumeBootstrapToken compares the user-supplied token to the stored secret using state.token === token (simple string equality), while verifyCookie correctly uses crypto.timingSafeEqual for HMAC comparison. The bootstrap token is a security-sensitive secret and the same timing-safe approach applies, though the practical risk is mitigated by the token's one-time use, 5-minute TTL, and loopback-only scope.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium
t3code/apps/server/src/wsServer.ts
Lines 459 to 465 in e980a2f
At line 544, url.protocol === "https:" always evaluates to false because the url object is constructed at line 458 with a hardcoded http://localhost:${port} base. Since req.url is only a path, the protocol is always "http:", so browserAuth.createAuthCookie always receives false and the auth cookie never has the Secure flag — even when the server is accessed over HTTPS (e.g., behind a reverse proxy). This allows the cookie to be transmitted over insecure connections.
const url = new URL(req.url ?? "/", `http://localhost:${port}`);
if (tryHandleProjectFaviconRequest(url, res)) {
return;
}
- const requestOrigin = req.headers.origin;
+ const requestOrigin = req.headers.origin;
+ const isHttps = req.headers["x-forwarded-proto"] === "https" || url.protocol === "https:";🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/wsServer.ts around lines 459-465:
At line 544, `url.protocol === "https:"` always evaluates to `false` because the `url` object is constructed at line 458 with a hardcoded `http://localhost:${port}` base. Since `req.url` is only a path, the protocol is always `"http:"`, so `browserAuth.createAuthCookie` always receives `false` and the auth cookie never has the `Secure` flag — even when the server is accessed over HTTPS (e.g., behind a reverse proxy). This allows the cookie to be transmitted over insecure connections.
Evidence trail:
apps/server/src/wsServer.ts lines 458: `const url = new URL(req.url ?? "/", \`http://localhost:${port}\`);` - hardcoded http base URL. Line 544: `yield* browserAuth.createAuthCookie(url.protocol === "https:");` - check always evaluates to false since the URL is constructed with http:// base. Verified at commit REVIEWED_COMMIT.
| @@ -986,25 +1138,45 @@ export const createServer = Effect.fn(function* (): Effect.fn.Return< | |||
| httpServer.on("upgrade", (request, socket, head) => { | |||
There was a problem hiding this comment.
🟢 Low src/wsServer.ts:1138
When browserAuth.isAllowedOrigin or browserAuth.isAuthenticatedRequest throws a defect, Effect.ignoreCause({ log: true }) silently catches the failure without calling rejectUpgrade or wss.handleUpgrade, leaving the WebSocket connection hanging until the client times out. Consider ensuring the socket is always closed on failure by moving the error handling outside the effect or using a catch block that explicitly rejects the upgrade before suppressing the error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/wsServer.ts around line 1138:
When `browserAuth.isAllowedOrigin` or `browserAuth.isAuthenticatedRequest` throws a defect, `Effect.ignoreCause({ log: true })` silently catches the failure without calling `rejectUpgrade` or `wss.handleUpgrade`, leaving the WebSocket connection hanging until the client times out. Consider ensuring the socket is always closed on failure by moving the error handling outside the effect or using a catch block that explicitly rejects the upgrade before suppressing the error.
Evidence trail:
apps/server/src/wsServer.ts lines 1137-1178 (REVIEWED_COMMIT): The upgrade handler using Effect.gen with Effect.ignoreCause; apps/server/src/wsServer.ts lines 115-124: rejectUpgrade function that calls socket.end(); apps/server/src/wsServer.ts lines 1166-1170: The browserAuth.isAllowedOrigin and isAuthenticatedRequest yield points where defects could occur
- Poll auth session after bootstrap exchange before declaring pairing complete - Keep the bootstrap hash on failed exchange and retry unpaired startup in the UI
- Treat `window.desktopBridge` like `nativeApi` in browser auth - Add a regression test to ensure pairing flow is skipped and no fetch occurs
1 participant
Summary
Testing
bun fmt,bun lint,bun typecheck.Note
High Risk
Introduces new browser authentication and origin enforcement for HTTP endpoints and WebSocket upgrades; mistakes here could lock users out or weaken access controls. Also changes default host/browse-open behavior, which affects startup and connectivity assumptions.
Overview
Adds a browser-first auth flow: the server now generates a one-time bootstrap token, exchanges it via a new
POST /api/auth/bootstrapendpoint for a signedHttpOnlyauth cookie, and exposesGET /api/auth/sessionfor session checks (with CORS restricted to an allowed-origin set).Updates WebSocket upgrades to require an allowed
Originplus a valid browser auth cookie, while keeping the legacy?token=path for non-browser clients. Server startup now logs and auto-opens a pairing URL (token in hash) and defaultswebmode host to127.0.0.1for predictable loopback pairing.On the web client, adds
ensureBrowserPairing()to consume/clear the bootstrap hash, perform the bootstrap exchange, and gate app rendering behind pairing (showing a “pairing required” screen when unauthenticated); attachment URL origin resolution is centralized viaresolveServerHttpOrigin(). Tests are expanded across server and web to cover token consumption, origin checks, cookie issuance, session detection, and WebSocket rejection cases.Written by Cursor Bugbot for commit e980a2f. This will update automatically on new commits. Configure here.
Note
Add browser-based session auth bootstrap to the server and web app
BrowserAuthEffect service (browserAuth.ts) that manages single-use bootstrap tokens, signs/verifies auth cookies with HMAC-SHA256, and tracks allowed origins derived from server config.POST /api/auth/bootstrap(exchanges a bootstrap token for a signed cookie) andGET /api/auth/session(checks auth status), both with CORS enforcement.Originheader plus a valid auth cookie, with backward-compatible fallback for legacyauthTokenquery-param clients that omitOrigin.pairingUrl(bootstrap token embedded in the URL hash) and opens the browser to it; if the host is non-loopback it also logs a warning.BootstrappedAppcomponent that exchanges the hash token and confirms auth before rendering, showing a pairing-required screen when unauthenticated.127.0.0.1in both desktop and web modes (previouslywebmode defaulted to an unbound host).📊 Macroscope summarized e980a2f. 9 files reviewed, 4 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues