feat(datastream): added datastream feature#20
feat(datastream): added datastream feature#20sivaranjitha-cs-18338 wants to merge 6 commits intomainfrom
Conversation
sivaranjitha-cs-18338
assigned
sivaranjitha-cs-18338 and
prasanna-ga-10049 and unassigned
sivaranjitha-cs-18338
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| throw new Error('WebSocket not available in this environment'); | ||
| } | ||
|
|
||
| this.conn = new WebSocketConstructor(this.finalUrl); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 23 days ago
General fix: ensure that the URL/host used for new WebSocketConstructor(this.finalUrl) cannot be arbitrarily controlled by incoming WebSocket messages. Specifically, avoid assigning unvalidated values from msg[msg.primarydc] directly into this.url. Instead, restrict it to a safe subset (e.g., hostnames from an allow‑list or at least a conservative validation that rejects dangerous schemes, hosts, and characters). If an invalid value is received, ignore the switch request and emit an error instead of reconnecting.
Best targeted fix here: introduce a small validation helper within DataStreamsWebSocket that checks whether a proposed switch URL/host is acceptable (for example, only allow hostnames from a preconfigured list on this.config, or at minimum enforce that it’s a plain hostname without scheme, port, or path). Then, in the MessageType.SWITCH_URL branch, use this helper before assigning this.url. If the value is invalid, log and emit an error and do not update this.url or reconnect. This leaves the existing functionality intact for valid URLs while preventing arbitrary redirection.
Concretely:
- Add a private method, e.g.
private isSafeSwitchUrl(candidate: string | undefined | null): boolean, somewhere in the class (e.g., beforehandleDMSEvents), that performs validation:- Ensure
candidateis a non-empty string. - For a minimal but meaningful hardening without touching other files, enforce that:
- It does not contain
/,\,?,#, or@. - It does not start with a scheme like
http:,https:,ws:,wss:(no://). - Optionally, you can enforce a simple hostname pattern via regex (letters, digits, dots, hyphens).
- It does not contain
- Ensure
- Update the
MessageType.SWITCH_URLcase insidehandleDMSEvents:- Extract the candidate:
const candidateUrl = msg[msg.primarydc];. - If
!this.isSafeSwitchUrl(candidateUrl), log and emit an error event, thenbreak;(do not changethis.url, do not reconnect). - Otherwise, assign
this.url = candidateUrl;and proceed as before.
- Extract the candidate:
This only changes packages/datastreams/src/web-socket.ts, adds no new dependencies, and ensures that this.finalUrl used at line 137 can no longer be an arbitrary URL crafted by an attacker over the WebSocket connection.
| @@ -237,6 +237,45 @@ | ||
| } | ||
|
|
||
| /** | ||
| * Validate a URL/host received via SWITCH_URL before using it to establish | ||
| * a new WebSocket connection. This helps prevent SSRF-style attacks where | ||
| * a malicious peer could redirect the client to arbitrary internal hosts. | ||
| * | ||
| * This implementation is intentionally conservative and only allows | ||
| * simple hostnames without schemes, paths, or query strings. | ||
| */ | ||
| private isSafeSwitchUrl(candidate: string | undefined | null): boolean { | ||
| if (typeof candidate !== 'string') { | ||
| return false; | ||
| } | ||
|
|
||
| const trimmed = candidate.trim(); | ||
| if (trimmed.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| // Disallow obvious dangerous characters and patterns. | ||
| if ( | ||
| trimmed.includes('/') || | ||
| trimmed.includes('\\') || | ||
| trimmed.includes('?') || | ||
| trimmed.includes('#') || | ||
| trimmed.includes('@') || | ||
| trimmed.includes('://') | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Allow only basic hostname characters (letters, digits, hyphen, dot). | ||
| const hostnamePattern = /^[a-zA-Z0-9.-]+$/; | ||
| if (!hostnamePattern.test(trimmed)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Handle DataStreams events - main message processing logic | ||
| */ | ||
| private handleDMSEvents(event: unknown): void { | ||
| @@ -272,8 +311,21 @@ | ||
| switch (mtype) { | ||
| case MessageType.SWITCH_URL: { | ||
| const msg = jsonData[0].msg as Record<string, string>; | ||
| this.url = msg[msg.primarydc]; | ||
| const candidateUrl = msg[msg.primarydc]; | ||
|
|
||
| // Validate the new URL/host before using it to avoid SSRF-style redirects. | ||
| if (!this.isSafeSwitchUrl(candidateUrl)) { | ||
| const errorEvent: CustomEvent = { | ||
| code: 1015, | ||
| message: 'Received unsafe URL for WebSocket switch; ignoring request' | ||
| }; | ||
| this.log('Unsafe SWITCH_URL received, ignoring reconnection request'); | ||
| this.emit('error', errorEvent); | ||
| break; | ||
| } | ||
|
|
||
| this.url = candidateUrl; | ||
|
|
||
| if (this.conn && this.conn.readyState === this.conn.OPEN) { | ||
| this.conn.close( | ||
| 1000, |
2 participants
Issue
Issue number, if available, prefixed with "#"
Description
What does this implement/fix? Explain your changes.
Testing
How was this change tested?
Additional context
Add any other context about the PR here.
Checklist
<type><scope>:<description><prid(#123)>Your PR will not be merged unless the CLA is signed.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.