[WIP] Implemented changes required to use /sim/ in split screen#10
[WIP] Implemented changes required to use /sim/ in split screen#10danil-nizamov wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR extends content block URL parsing to recognize site-relative paths ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/editor.js (1)
2647-2649:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOpen-URL button still blocks site-relative paths
The parser now accepts
/...URLs, but button enablement and click handling only allowhttp(s)://. This prevents opening/sim/...values from the editor toolbar.💡 Proposed fix
+ const isSupportedContentUrl = (value) => + /^https?:\/\//i.test(value) || /^\/(?!\/)/.test(value); @@ - openUrlButton.disabled = !urlValue || !/^https?:\/\//i.test(urlValue); + openUrlButton.disabled = !urlValue || !isSupportedContentUrl(urlValue); @@ - openUrlButton.disabled = !urlValue || !/^https?:\/\//i.test(urlValue); + openUrlButton.disabled = !urlValue || !isSupportedContentUrl(urlValue); @@ - if (urlValue && /^https?:\/\//i.test(urlValue)) { - window.open(urlValue, '_blank', 'noopener,noreferrer'); + if (urlValue && isSupportedContentUrl(urlValue)) { + const href = /^https?:\/\//i.test(urlValue) + ? urlValue + : new URL(urlValue, window.location.origin).toString(); + window.open(href, '_blank', 'noopener,noreferrer'); }Also applies to: 2797-2810
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/editor.js` around lines 2647 - 2649, The open-URL enablement and click handling currently only accept http(s) URLs; update the validation and click logic that reference contentInput and openUrlButton to also allow site-relative paths starting with "/" (use a combined test like /^(\/|https?:\/\/)/i). In the click handler that opens the URL (where openUrlButton is used), detect leading "/" and prepend location.origin (or use new URL(value, location.origin).href) so site-relative paths open correctly; apply the same changes to the other block around the second occurrence (the code near lines referencing contentInput/openUrlButton at 2797-2810).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/editor.js`:
- Around line 1723-1728: The EOF-path only applies shuffle parsing to
questionOptionsBuffer, so a trailing "__Question Options__" containing
multiSelectMode settings gets lost; update the EOF handling that examines
questionOptionsBuffer (the block that currently builds optionsText and sets
currentQuestion.shuffleOptions) to also parse and set
currentQuestion.multiSelectMode (e.g., detect "multiSelectMode=any" or tokens
like "any"/"all" and set currentQuestion.multiSelectMode = 'any' or 'all'
accordingly), preserving the same normalization (trim().toLowerCase()) and
existing shuffle checks (shuffle=false / don't shuffle / no shuffle) so the last
question round-trips correctly.
In `@server.js`:
- Around line 1190-1202: The upstream proxy request (proxyReq created with
http.request(options, ...)) must have a timeout and propagate client
disconnects: set a timeout on proxyReq (or use an AbortController) and attach a
'timeout' handler that destroys/aborts the request and returns a 504 to the
client; also listen for the incoming req 'close'/'aborted' events and call
proxyReq.abort()/destroy() to tear down the upstream when the client
disconnects; ensure the existing 'error' handler still sends a 502 and that you
don't double-write the response if the client already disconnected (guard writes
by checking res.headersSent or if req.aborted).
- Around line 1180-1214: The proxy drops query params and always uses http for
upstreams; update the call site and proxyToSim to preserve the original URL
search and select the proper transport: pass the full path including
urlObj.search (e.g. send pathname + urlObj.search) into proxyToSim (or refactor
proxyToSim to accept the URL/search), and inside proxyToSim (which calls
simUpstreamPath and builds target = new URL(..., SIM_ORIGIN)) choose the
transport based on target.protocol (use https.request when target.protocol ===
'https:' and http.request otherwise) while keeping options.path =
target.pathname + target.search and preserving headers/host.
- Around line 1160-1183: simUpstreamPath can return strings beginning with '//'
which, when passed to new URL(subpath, SIM_ORIGIN) in proxyToSim, are treated as
scheme-relative and allow SSRF; update simUpstreamPath to normalize and reject
or fold multiple leading slashes so it never returns a string that starts with
'//', e.g., collapse any leading '//' sequence to a single '/' (or explicitly
prepend SIM_MOUNT_PATH handling to ensure subpath starts with a single '/');
ensure shouldProxyToSim/proxyToSim still use simUpstreamPath but rely on that
normalization before constructing new URL with SIM_ORIGIN so scheme-relative
URLs cannot override the origin.
---
Outside diff comments:
In `@public/editor.js`:
- Around line 2647-2649: The open-URL enablement and click handling currently
only accept http(s) URLs; update the validation and click logic that reference
contentInput and openUrlButton to also allow site-relative paths starting with
"/" (use a combined test like /^(\/|https?:\/\/)/i). In the click handler that
opens the URL (where openUrlButton is used), detect leading "/" and prepend
location.origin (or use new URL(value, location.origin).href) so site-relative
paths open correctly; apply the same changes to the other block around the
second occurrence (the code near lines referencing contentInput/openUrlButton at
2797-2810).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0718de6-12f5-4a3e-b5bd-327305a5cfca
📒 Files selected for processing (2)
public/editor.jsserver.js
| if (questionOptionsBuffer.length > 0) { | ||
| const optionsText = questionOptionsBuffer.join('\n').trim().toLowerCase(); | ||
| if (optionsText.includes('shuffle=false') || optionsText.includes('don\'t shuffle') || optionsText.includes('dont shuffle') || optionsText === 'no shuffle') { | ||
| currentQuestion.shuffleOptions = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Trailing MCQ parsing drops multiSelectMode=any for the last question
The EOF parsing path only reads shuffle directives. If the last question uses __Question Options__ with any, it round-trips back to 'all'.
💡 Proposed fix
- if (questionOptionsBuffer.length > 0) {
- const optionsText = questionOptionsBuffer.join('\n').trim().toLowerCase();
- if (optionsText.includes('shuffle=false') || optionsText.includes('don\'t shuffle') || optionsText.includes('dont shuffle') || optionsText === 'no shuffle') {
- currentQuestion.shuffleOptions = false;
- }
- }
+ if (questionOptionsBuffer.length > 0) {
+ const optionsText = questionOptionsBuffer.join('\n').trim().toLowerCase();
+ if (
+ optionsText.includes('shuffle=false') ||
+ optionsText.includes("don't shuffle") ||
+ optionsText.includes('dont shuffle') ||
+ optionsText === 'no shuffle'
+ ) {
+ currentQuestion.shuffleOptions = false;
+ }
+ if (
+ optionsText.includes('any') ||
+ optionsText.includes('multi-select mode: any') ||
+ optionsText.includes('mode: any')
+ ) {
+ currentQuestion.multiSelectMode = 'any';
+ } else {
+ currentQuestion.multiSelectMode = 'all';
+ }
+ } else {
+ currentQuestion.multiSelectMode = 'all';
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (questionOptionsBuffer.length > 0) { | |
| const optionsText = questionOptionsBuffer.join('\n').trim().toLowerCase(); | |
| if (optionsText.includes('shuffle=false') || optionsText.includes('don\'t shuffle') || optionsText.includes('dont shuffle') || optionsText === 'no shuffle') { | |
| currentQuestion.shuffleOptions = false; | |
| } | |
| } | |
| if (questionOptionsBuffer.length > 0) { | |
| const optionsText = questionOptionsBuffer.join('\n').trim().toLowerCase(); | |
| if ( | |
| optionsText.includes('shuffle=false') || | |
| optionsText.includes("don't shuffle") || | |
| optionsText.includes('dont shuffle') || | |
| optionsText === 'no shuffle' | |
| ) { | |
| currentQuestion.shuffleOptions = false; | |
| } | |
| if ( | |
| optionsText.includes('any') || | |
| optionsText.includes('multi-select mode: any') || | |
| optionsText.includes('mode: any') | |
| ) { | |
| currentQuestion.multiSelectMode = 'any'; | |
| } else { | |
| currentQuestion.multiSelectMode = 'all'; | |
| } | |
| } else { | |
| currentQuestion.multiSelectMode = 'all'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/editor.js` around lines 1723 - 1728, The EOF-path only applies shuffle
parsing to questionOptionsBuffer, so a trailing "__Question Options__"
containing multiSelectMode settings gets lost; update the EOF handling that
examines questionOptionsBuffer (the block that currently builds optionsText and
sets currentQuestion.shuffleOptions) to also parse and set
currentQuestion.multiSelectMode (e.g., detect "multiSelectMode=any" or tokens
like "any"/"all" and set currentQuestion.multiSelectMode = 'any' or 'all'
accordingly), preserving the same normalization (trim().toLowerCase()) and
existing shuffle checks (shuffle=false / don't shuffle / no shuffle) so the last
question round-trips correctly.
| function simUpstreamPath(pathname) { | ||
| if (pathname === SIM_MOUNT_PATH) return '/'; | ||
| const prefix = `${SIM_MOUNT_PATH}/`; | ||
| if (pathname.startsWith(prefix)) return pathname.slice(SIM_MOUNT_PATH.length) || '/'; | ||
| return pathname; | ||
| } | ||
|
|
||
| function shouldProxyToSim(pathname) { | ||
| if (!SIM_ORIGIN) return false; | ||
| if (pathname === SIM_MOUNT_PATH || pathname.startsWith(`${SIM_MOUNT_PATH}/`)) return true; | ||
| for (const entry of SIM_ROOT_PROXY_PATHS) { | ||
| if (entry.endsWith('/')) { | ||
| if (pathname.startsWith(entry)) return true; | ||
| } else if (pathname === entry || pathname.startsWith(`${entry}/`)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function proxyToSim(req, res, pathname) { | ||
| const subpath = simUpstreamPath(pathname); | ||
| const target = new URL(subpath, SIM_ORIGIN); | ||
| const options = { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the JavaScript WHATWG URL API, what does new URL('//169.254.169.254/latest', 'http://localhost:3001')` resolve to?`
💡 Result:
new URL('//169.254.169.254/latest', 'http://localhost:3001') resolves to http://169.254.169.254/latest. Explanation (WHATWG URL parsing/resolution): the input string begins with //, which the URL Standard defines as a “scheme-relative” (protocol-relative) URL whose scheme is taken from the base URL; here the base URL is http://localhost:3001, so the resolved URL uses the http scheme with host 169.254.169.254 and path /latest. [1]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find server.js
ls -la
fd -a "server.js" . || true
# Print relevant sections
if [ -f server.js ]; then
echo "== server.js ==";
sed -n '1120,1210p' server.js | cat -n
else
# search path
file=$(fd -a "server.js" . | head -n1 || true)
if [ -n "${file:-}" ] && [ -f "$file" ]; then
echo "== $file ==";
sed -n '1120,1210p' "$file" | cat -n
else
echo "server.js not found at repo root"; exit 1
fi
fi
# Trace usages
target_file="server.js"
if [ ! -f "$target_file" ]; then
target_file=$(fd -a "server.js" . | head -n1)
fi
rg -n "simUpstreamPath|shouldProxyToSim|proxyToSim\\(" "$target_file" || true
rg -n "SIM_MOUNT_PATH|SIM_ORIGIN|SIM_ROOT_PROXY_PATHS" "$target_file" || true
# Find where pathname passed into proxyToSim
# Use small excerpt around proxyToSim call sites
rg -n "proxyToSim\\(req, res" "$target_file" || true
rg -n "proxyToSim\\(" "$target_file" | head -n 50 || trueRepository: CodeSignal/learn_cosmo-activities-web
Length of output: 6631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find server.js
fd -a "server.js" . | head -n 20
if [ -f server.js ]; then
sed -n '1120,1210p' server.js | cat -n
rg -n "simUpstreamPath|shouldProxyToSim|proxyToSim\\(" server.js || true
rg -n "SIM_MOUNT_PATH|SIM_ORIGIN|SIM_ROOT_PROXY_PATHS" server.js || true
rg -n "proxyToSim\\(req, res" server.js || true
rg -n "proxyToSim\\(" server.js | head -n 50 || true
else
file=$(fd -a "server.js" . | head -n1)
sed -n '1120,1210p' "$file" | cat -n
rg -n "simUpstreamPath|shouldProxyToSim|proxyToSim\\(" "$file" || true
rg -n "SIM_MOUNT_PATH|SIM_ORIGIN|SIM_ROOT_PROXY_PATHS" "$file" || true
rg -n "proxyToSim\\(req, res" "$file" || true
rg -n "proxyToSim\\(" "$file" | head -n 50 || true
fiRepository: CodeSignal/learn_cosmo-activities-web
Length of output: 5547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a "server.js" . | head -n 20
if [ -f server.js ]; then
file=server.js
else
file=$(fd -a "server.js" . | head -n1)
fi
echo "Using file: $file"
echo "---- snippet ----"
sed -n '1120,1210p' "$file" | cat -n
echo "---- symbols ----"
rg -n "function simUpstreamPath|function shouldProxyToSim|function proxyToSim\\(" "$file" || true
rg -n "SIM_MOUNT_PATH|SIM_ORIGIN|SIM_ROOT_PROXY_PATHS" "$file" || true
echo "---- callsites ----"
rg -n "proxyToSim\\(" "$file" || trueRepository: CodeSignal/learn_cosmo-activities-web
Length of output: 5405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -a "server.js" . | head -n1)
echo "Using $file"
echo "== simUpstreamPath =="
ln=$(rg -n "function simUpstreamPath" "$file" | head -n1 | cut -d: -f1)
echo "line $ln"
start=$((ln-40)); end=$((ln+120))
sed -n "${start},${end}p" "$file" | cat -n
echo "== shouldProxyToSim =="
ln=$(rg -n "function shouldProxyToSim" "$file" | head -n1 | cut -d: -f1)
start=$((ln-20)); end=$((ln+120))
sed -n "${start},${end}p" "$file" | cat -n
echo "== proxyToSim =="
ln=$(rg -n "function proxyToSim" "$file" | head -n1 | cut -d: -f1)
start=$((ln-10)); end=$((ln+180))
sed -n "${start},${end}p" "$file" | cat -n
echo "== callsites =="
rg -n "proxyToSim\\(" "$file" || trueRepository: CodeSignal/learn_cosmo-activities-web
Length of output: 20087
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(fd -a "server.js" . | head -n1)
sed -n '1,80p' "$file" | cat -n | head -n 80
# show around SIM_ORIGIN and SIM_MOUNT_PATH definitions
rg -n "const SIM_ORIGIN|const SIM_MOUNT_PATH|SIM_ROOT_PROXY_PATHS" "$file"
line=$(rg -n "const SIM_ORIGIN" "$file" | head -n1 | cut -d: -f1)
start=$((line-5)); end=$((line+25))
sed -n "${start},${end}p" "$file" | cat -nRepository: CodeSignal/learn_cosmo-activities-web
Length of output: 5310
Block scheme-relative //... in simUpstreamPath before new URL()
A request like /sim//169.254.169.254/latest matches shouldProxyToSim(); simUpstreamPath() returns //169.254.169.254/latest; then new URL(subpath, SIM_ORIGIN) resolves it as a scheme-relative URL and overrides the intended upstream host (SSRF/open-proxy behavior against arbitrary hosts reachable from the server).
🔒 Proposed fix
function simUpstreamPath(pathname) {
if (pathname === SIM_MOUNT_PATH) return '/';
const prefix = `${SIM_MOUNT_PATH}/`;
- if (pathname.startsWith(prefix)) return pathname.slice(SIM_MOUNT_PATH.length) || '/';
+ if (pathname.startsWith(prefix)) {
+ const subpath = pathname.slice(SIM_MOUNT_PATH.length) || '/';
+ return `/${subpath.replace(/^\/+/, '')}`;
+ }
return pathname;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function simUpstreamPath(pathname) { | |
| if (pathname === SIM_MOUNT_PATH) return '/'; | |
| const prefix = `${SIM_MOUNT_PATH}/`; | |
| if (pathname.startsWith(prefix)) return pathname.slice(SIM_MOUNT_PATH.length) || '/'; | |
| return pathname; | |
| } | |
| function shouldProxyToSim(pathname) { | |
| if (!SIM_ORIGIN) return false; | |
| if (pathname === SIM_MOUNT_PATH || pathname.startsWith(`${SIM_MOUNT_PATH}/`)) return true; | |
| for (const entry of SIM_ROOT_PROXY_PATHS) { | |
| if (entry.endsWith('/')) { | |
| if (pathname.startsWith(entry)) return true; | |
| } else if (pathname === entry || pathname.startsWith(`${entry}/`)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| function proxyToSim(req, res, pathname) { | |
| const subpath = simUpstreamPath(pathname); | |
| const target = new URL(subpath, SIM_ORIGIN); | |
| const options = { | |
| function simUpstreamPath(pathname) { | |
| if (pathname === SIM_MOUNT_PATH) return '/'; | |
| const prefix = `${SIM_MOUNT_PATH}/`; | |
| if (pathname.startsWith(prefix)) { | |
| const subpath = pathname.slice(SIM_MOUNT_PATH.length) || '/'; | |
| return `/${subpath.replace(/^\/+/, '')}`; | |
| } | |
| return pathname; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 1160 - 1183, simUpstreamPath can return strings
beginning with '//' which, when passed to new URL(subpath, SIM_ORIGIN) in
proxyToSim, are treated as scheme-relative and allow SSRF; update
simUpstreamPath to normalize and reject or fold multiple leading slashes so it
never returns a string that starts with '//', e.g., collapse any leading '//'
sequence to a single '/' (or explicitly prepend SIM_MOUNT_PATH handling to
ensure subpath starts with a single '/'); ensure shouldProxyToSim/proxyToSim
still use simUpstreamPath but rely on that normalization before constructing new
URL with SIM_ORIGIN so scheme-relative URLs cannot override the origin.
| function proxyToSim(req, res, pathname) { | ||
| const subpath = simUpstreamPath(pathname); | ||
| const target = new URL(subpath, SIM_ORIGIN); | ||
| const options = { | ||
| hostname: target.hostname, | ||
| port: target.port || (target.protocol === 'https:' ? 443 : 80), | ||
| path: target.pathname + target.search, | ||
| method: req.method, | ||
| headers: { ...req.headers, host: target.host } | ||
| }; | ||
| const proxyReq = http.request(options, (proxyRes) => { | ||
| res.writeHead(proxyRes.statusCode, proxyRes.headers); | ||
| proxyRes.pipe(res); | ||
| }); | ||
| proxyReq.on('error', () => { | ||
| res.writeHead(502, { 'Content-Type': 'text/plain; charset=utf-8' }); | ||
| res.end('Simulation not running on ' + SIM_ORIGIN); | ||
| }); | ||
| if (req.method === 'GET' || req.method === 'HEAD') { | ||
| proxyReq.end(); | ||
| } else { | ||
| req.pipe(proxyReq); | ||
| } | ||
| } | ||
|
|
||
| // Store active WebSocket connections | ||
| const clients = new Set(); | ||
|
|
||
| const server = http.createServer((req, res) => { | ||
| const urlObj = new URL(req.url, 'http://localhost'); | ||
| let pathname = decodeURIComponent(urlObj.pathname || '/'); | ||
|
|
||
| if (shouldProxyToSim(pathname)) { | ||
| proxyToSim(req, res, pathname); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does http.request()supporthttps:upstream URLs, or should reverse proxies switch tohttps.request()whentarget.protocol === 'https:'?
💡 Result:
Yes, Node’s http.request() can accept an HTTPS URL-like value via its options parsing, but it will not perform TLS. In practice, if the upstream is https:, you should use https.request() (and https.Agent) rather than http.request(). Reasoning grounded in Node’s API split: 1) Node implements HTTPS as a separate module (https) specifically for “HTTP protocol over TLS/SSL,” and exposes https.request() as the API “to a secure web server.” [1] 2) https.request(options) explicitly states it accepts “all options from http.request” but sets different defaults such as protocol: 'https:', port: 443, and agent: https.globalAgent. This indicates that the protocol/TLS-related parts are handled by the https module rather than the http module. [1] 3) The http module’s protocol default is 'http:' (and thus the underlying agent/socket behavior is HTTP-focused). [2] Implication for reverse proxies: - If target.protocol === 'https:', switch to https.request() (or ensure you use an https.Agent) so that the connection is established with TLS. Using http.request() for https: upstreams risks sending a plain HTTP request over a TCP socket that expects TLS. So: prefer https.request() when target.protocol === 'https:', not http.request(). [1][2]
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/https.html
- 2: https://github.com/nodejs/node/blob/main/doc/api/http.md
Preserve query string and select HTTPS transport based on target.protocol
- Query params are dropped:
server.jsextracts onlyurlObj.pathnameand callsproxyToSim(req, res, pathname), sourlObj.search(?…) never reaches the upstream. - Transport is wrong for HTTPS:
proxyToSim()always useshttp.request(...)even whentarget.protocol === 'https:'; Node’s TLS/secure transport should usehttps.request()forhttps:upstreams.
🔧 Proposed fix
-function proxyToSim(req, res, pathname) {
- const subpath = simUpstreamPath(pathname);
- const target = new URL(subpath, SIM_ORIGIN);
+function proxyToSim(req, res, urlObj) {
+ const subpath = simUpstreamPath(urlObj.pathname);
+ const target = new URL(SIM_ORIGIN);
+ target.pathname = subpath;
+ target.search = urlObj.search;
+ const transport = target.protocol === 'https:' ? require('https') : http;
const options = {
hostname: target.hostname,
port: target.port || (target.protocol === 'https:' ? 443 : 80),
path: target.pathname + target.search,
method: req.method,
headers: { ...req.headers, host: target.host }
};
- const proxyReq = http.request(options, (proxyRes) => {
+ const proxyReq = transport.request(options, (proxyRes) => {
res.writeHead(proxyRes.statusCode, proxyRes.headers);
proxyRes.pipe(res);
});
...
}
...
if (shouldProxyToSim(pathname)) {
- proxyToSim(req, res, pathname);
+ proxyToSim(req, res, urlObj);
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 1180 - 1214, The proxy drops query params and always
uses http for upstreams; update the call site and proxyToSim to preserve the
original URL search and select the proper transport: pass the full path
including urlObj.search (e.g. send pathname + urlObj.search) into proxyToSim (or
refactor proxyToSim to accept the URL/search), and inside proxyToSim (which
calls simUpstreamPath and builds target = new URL(..., SIM_ORIGIN)) choose the
transport based on target.protocol (use https.request when target.protocol ===
'https:' and http.request otherwise) while keeping options.path =
target.pathname + target.search and preserving headers/host.
| const proxyReq = http.request(options, (proxyRes) => { | ||
| res.writeHead(proxyRes.statusCode, proxyRes.headers); | ||
| proxyRes.pipe(res); | ||
| }); | ||
| proxyReq.on('error', () => { | ||
| res.writeHead(502, { 'Content-Type': 'text/plain; charset=utf-8' }); | ||
| res.end('Simulation not running on ' + SIM_ORIGIN); | ||
| }); | ||
| if (req.method === 'GET' || req.method === 'HEAD') { | ||
| proxyReq.end(); | ||
| } else { | ||
| req.pipe(proxyReq); | ||
| } |
There was a problem hiding this comment.
Add timeout and abort propagation to the upstream request.
This proxy path never times out and never tears down the upstream request when the client disconnects. A wedged sim backend can leave hanging sockets open indefinitely on the main server.
⏱️ Proposed fix
const proxyReq = transport.request(options, (proxyRes) => {
res.writeHead(proxyRes.statusCode, proxyRes.headers);
proxyRes.pipe(res);
});
+ proxyReq.setTimeout(10000, () => {
+ proxyReq.destroy(new Error('Simulation upstream timeout'));
+ });
+ req.on('aborted', () => proxyReq.destroy());
+ res.on('close', () => proxyReq.destroy());
proxyReq.on('error', () => {
res.writeHead(502, { 'Content-Type': 'text/plain; charset=utf-8' });
res.end('Simulation not running on ' + SIM_ORIGIN);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.js` around lines 1190 - 1202, The upstream proxy request (proxyReq
created with http.request(options, ...)) must have a timeout and propagate
client disconnects: set a timeout on proxyReq (or use an AbortController) and
attach a 'timeout' handler that destroys/aborts the request and returns a 504 to
the client; also listen for the incoming req 'close'/'aborted' events and call
proxyReq.abort()/destroy() to tear down the upstream when the client
disconnects; ensure the existing 'error' handler still sends a 502 and that you
don't double-write the response if the client already disconnected (guard writes
by checking res.headersSent or if req.aborted).
danil-nizamov
changed the title
1 participant
This PR implements changes that are required to host simulation on /sim/ and put it into the split screen. All types of questions work except matching, but matching doesn't work without these changes as well.
The changes are tested on staging by cloning this repository in the
setup.shand switching to this branch. Questions for test: