-
Notifications
You must be signed in to change notification settings - Fork 297
feat(lazer/js/sdk): QSP auth for browser support #3014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,77 @@ import type { Request, Response } from "../protocol.js"; | |
import type { ResilientWebSocketConfig } from "./resilient-websocket.js"; | ||
import { ResilientWebSocket } from "./resilient-websocket.js"; | ||
|
||
/** | ||
* Detects if the code is running in a regular DOM or Web Worker context. | ||
* @returns true if running in a DOM or Web Worker context, false if running in Node.js | ||
*/ | ||
function isBrowser(): boolean { | ||
try { | ||
// Check for browser's window object (DOM context) | ||
if (typeof globalThis !== "undefined") { | ||
const global = globalThis as any; | ||
if (global.window && global.window.document) { | ||
return true; | ||
} | ||
|
||
// Check for Web Worker context (has importScripts but no window) | ||
if (typeof global.importScripts === "function" && !global.window) { | ||
return true; | ||
} | ||
} | ||
|
||
// Node.js environment | ||
return false; | ||
} catch { | ||
// If any error occurs, assume Node.js environment | ||
return false; | ||
} | ||
} | ||
|
||
/** | ||
* Adds authentication to either the URL as a query parameter or as an Authorization header. | ||
* | ||
* Browsers don't support custom headers for WebSocket connections, so if a browser is detected, | ||
* the token is added as a query parameter instead. Else, the token is added as an Authorization header. | ||
* | ||
* @param url - The WebSocket URL | ||
* @param token - The authentication token | ||
* @param wsOptions - Existing WebSocket options | ||
* @returns Object containing the modified endpoint and wsOptions | ||
*/ | ||
function addAuthentication( | ||
url: string, | ||
token: string, | ||
wsOptions: any = {} | ||
): { endpoint: string; wsOptions: any } { | ||
if (isBrowser()) { | ||
// Browser: Add token as query parameter | ||
const urlObj = new URL(url); | ||
urlObj.searchParams.set("ACCESS_TOKEN", token); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is available as a query param, why use bearer auth at all? Seems like if you have to support query param auth it's simpler to just always use the query param and never set the header... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeahh, considered that, but we prefer people use the header whenever possible to avoid leaking the token in logs/some nginx somewhere/etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yeah that's fair |
||
|
||
// For browsers, we need to filter out any options that aren't valid for WebSocket constructor | ||
// Browser WebSocket constructor only accepts protocols as second parameter | ||
const browserWsOptions = wsOptions.protocols ? wsOptions.protocols : undefined; | ||
|
||
return { | ||
endpoint: urlObj.toString(), | ||
wsOptions: browserWsOptions, | ||
}; | ||
} else { | ||
// Node.js: Add Authorization header | ||
return { | ||
endpoint: url, | ||
wsOptions: { | ||
...wsOptions, | ||
headers: { | ||
...wsOptions.headers, | ||
Authorization: `Bearer ${token}`, | ||
}, | ||
}, | ||
}; | ||
} | ||
} | ||
|
||
const DEFAULT_NUM_CONNECTIONS = 4; | ||
|
||
export type WebSocketPoolConfig = { | ||
|
@@ -63,15 +134,17 @@ export class WebSocketPool { | |
if (!url) { | ||
throw new Error(`URLs must not be null or empty`); | ||
} | ||
const wsOptions = { | ||
...config.rwsConfig?.wsOptions, | ||
headers: { | ||
Authorization: `Bearer ${config.token}`, | ||
}, | ||
}; | ||
|
||
// Apply authentication based on environment (browser vs Node.js) | ||
const { endpoint, wsOptions } = addAuthentication( | ||
url, | ||
config.token, | ||
config.rwsConfig?.wsOptions | ||
); | ||
|
||
const rws = new ResilientWebSocket({ | ||
...config.rwsConfig, | ||
endpoint: url, | ||
endpoint, | ||
wsOptions, | ||
logger, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general you shouldn't ever use
any
; here you should type this correctly to match the types that are valid as options here. If you must, you can useunknown
, but I don't think that makes sense in this case.