Conversation
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const id = ++connectionCounter | ||
| const path = req.url || '' | ||
| const upstreamUrl = `${UPSTREAM_WS_URL}${path}` | ||
| console.log(`[proxy][${id}] EA connected, opening upstream: ${upstreamUrl}`) |
Check warning
Code scanning / CodeQL
Log injection Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general, to fix log injection when logging user input, sanitize the data before logging by removing or neutralizing characters that can alter the log structure (such as \r, \n, and other control characters). For plain text logs, stripping CR/LF is usually sufficient; for HTML logs, HTML-encoding is needed. It is also good practice to clearly delimit or quote user-provided data in the log message.
For this specific code, the best minimal fix is to ensure that upstreamUrl is sanitized before it is interpolated into the log message on line 39. We can do this by creating a sanitized version of upstreamUrl specifically for logging, e.g. safeUpstreamUrl, which strips carriage return and newline characters (and optionally other control characters) using String.prototype.replace with a regular expression. We should only change the log line (and possibly introduce a one-line local variable) without altering the behavior of the proxy logic: the actual upstreamUrl used to establish the WebSocket connection must remain unchanged. No additional imports are required; we can rely on built-in string methods.
Concretely, inside the wss.on('connection', ...) handler in packages/sources/tiingo/test/local-ws-test/proxy.js, after computing const upstreamUrl = \${UPSTREAM_WS_URL}${path}`, we introduce const safeUpstreamUrl = upstreamUrl.replace(/[\r\n]/g, '')and then change theconsole.logcall to logsafeUpstreamUrlinstead ofupstreamUrl`. This keeps functionality identical (the upstream connection still uses the original URL) while ensuring logs cannot be structurally modified via CR/LF injection.
| @@ -36,7 +36,8 @@ | ||
| const id = ++connectionCounter | ||
| const path = req.url || '' | ||
| const upstreamUrl = `${UPSTREAM_WS_URL}${path}` | ||
| console.log(`[proxy][${id}] EA connected, opening upstream: ${upstreamUrl}`) | ||
| const safeUpstreamUrl = upstreamUrl.replace(/[\r\n]/g, '') | ||
| console.log(`[proxy][${id}] EA connected, opening upstream: ${safeUpstreamUrl}`) | ||
|
|
||
| const upstreamWs = new WebSocket(upstreamUrl) | ||
| const pair = { id, clientWs, upstreamWs, path } |
| `[control] Closing ${targets.length} connection(s) with code=${code}` + | ||
| (pathFilter ? ` path=${pathFilter}` : ''), |
Check warning
Code scanning / CodeQL
Log injection Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, any user-controlled data included in log messages should be sanitized before logging. For plain text logs, the key step is to remove or neutralize newline characters (\r, \n) so an attacker cannot inject additional log lines. It also helps to clearly delimit user-supplied values.
In this file, the only problematic user-controlled value in the highlighted log call is pathFilter. The best minimal fix is to derive a sanitized version of pathFilter (e.g., safePathFilter) by stripping carriage returns and newlines using String.prototype.replace, then log only that sanitized value. This keeps the existing functionality (filtering by path and closing connections) unchanged while ensuring logs cannot be broken by crafted input.
Concretely:
- After determining
pathFilteron line 121, computesafePathFilteras eithernull(if no filter) orString(pathFilter).replace(/\r|\n/g, ''). - Use
safePathFilterinstead ofpathFilterin theconsole.logmessage on line 125–128. - Optionally, also use
safePathFilterin the JSON response on line 141 to keep logged/returned values consistent and sanitized, though CodeQL only cares about the log sink.
No new external libraries are necessary; String.prototype.replace with a simple regex is sufficient.
| @@ -119,12 +119,16 @@ | ||
| const code = parseInt(parsed.query.code || '1005', 10) | ||
| const reason = parsed.query.reason || '' | ||
| const pathFilter = parsed.query.path || null | ||
| const safePathFilter = | ||
| pathFilter === null || pathFilter === undefined | ||
| ? null | ||
| : String(pathFilter).replace(/\r|\n/g, '') | ||
|
|
||
| const targets = [...activePairs].filter((p) => !pathFilter || p.path === pathFilter) | ||
|
|
||
| console.log( | ||
| `[control] Closing ${targets.length} connection(s) with code=${code}` + | ||
| (pathFilter ? ` path=${pathFilter}` : ''), | ||
| (safePathFilter ? ` path=${safePathFilter}` : ''), | ||
| ) | ||
|
|
||
| for (const pair of targets) { | ||
| @@ -138,7 +137,7 @@ | ||
| } | ||
|
|
||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| res.end(JSON.stringify({ closed: targets.length, code, path: pathFilter })) | ||
| res.end(JSON.stringify({ closed: targets.length, code, path: safePathFilter })) | ||
| return | ||
| } | ||
|
|
1 participant
Adds a new local websocket test for the Tiiingo Failover behaviour