hardening(desktop): close remaining Electron P1 gaps#246
Merged
Conversation
Reviewer's GuideHardens the QuantLab Desktop Electron shell, adds a more resilient multi-candidate Python startup path for the embedded research_ui server, and makes the renderer’s refresh loop and caching safer for long-running sessions. Sequence diagram for research_ui multi-candidate Python startupsequenceDiagram
participant MainProcess as MainProcess
participant ResearchUiStarter as startResearchUiServer
participant PythonResolver as resolvePythonCandidates
participant Launcher as launchResearchUiProcess
participant Child as PythonChildProcess
participant Binder as bindResearchUiProcess
participant Server as research_ui_server
MainProcess->>ResearchUiStarter: startResearchUiServer(forceRestart)
alt forceRestart
ResearchUiStarter->>MainProcess: stopResearchUiServer(force)
end
ResearchUiStarter->>ResearchUiStarter: detectResearchUiServerUrl
alt existing external server
ResearchUiStarter->>MainProcess: markResearchUiReady(source external)
ResearchUiStarter-->>MainProcess: return
else no existing server
ResearchUiStarter->>MainProcess: updateWorkspaceState(status starting)
ResearchUiStarter->>MainProcess: scheduleResearchStartupTimeout
ResearchUiStarter->>PythonResolver: resolvePythonCandidates
PythonResolver-->>ResearchUiStarter: candidates[]
alt no candidates
ResearchUiStarter->>MainProcess: clearResearchStartupTimer
ResearchUiStarter->>MainProcess: updateWorkspaceState(status error, message no interpreter)
ResearchUiStarter-->>MainProcess: return
else at least one candidate
ResearchUiStarter->>Launcher: launchResearchUiProcess(candidates, index 0)
Launcher->>Launcher: appendLog(startup with candidates[0])
Launcher->>Child: spawn(pythonCommand, SERVER_SCRIPT)
Launcher->>Binder: bindResearchUiProcess(Child, pythonCommand, candidates, index)
Binder->>MainProcess: researchServerProcess = Child
Binder->>MainProcess: researchServerOwned = true
Binder->>Child: monitorResearchUiStartup
Child-->>Binder: stdout URL line
Binder->>Binder: extractServerUrl(line)
alt URL discovered
Binder->>MainProcess: isResearchUiReachable(url)
MainProcess-->>Binder: reachable
alt reachable and Child is current process
Binder->>MainProcess: markResearchUiReady(url, source managed)
end
end
Child-->>Binder: stderr spawn error
Binder->>Binder: handle error
alt error.code in [EACCES, EPERM, ENOENT] and more candidates
Binder->>Launcher: appendLog(spawn error, retry next)
Binder->>Launcher: launchResearchUiProcess(candidates, index+1)
else fatal error or last candidate
Binder->>MainProcess: clearResearchStartupTimer
Binder->>MainProcess: updateWorkspaceState(status error, message from error)
end
Child-->>Binder: exit(code, signal)
alt Child is current researchServerProcess
Binder->>MainProcess: researchServerProcess = null
Binder->>MainProcess: researchServerOwned = false
Binder->>MainProcess: clearResearchStartupTimer
Binder->>MainProcess: updateWorkspaceState(status stopped)
end
end
end
Sequence diagram for renderer refresh loop with pause and retrysequenceDiagram
actor User
participant Renderer as RendererApp
participant Refresh as ensureRefreshLoop
participant Snapshot as refreshSnapshot
Renderer->>Refresh: ensureRefreshLoop()
alt workspace has serverUrl
Refresh->>Snapshot: refreshSnapshot()
loop every refreshIntervalMs
Refresh->>Snapshot: refreshSnapshot()
alt success
Snapshot->>Renderer: update snapshot
Snapshot->>Renderer: snapshotStatus = {status ok, consecutiveErrors 0, refreshPaused false}
else error
Snapshot->>Renderer: consecutiveErrors++
alt consecutiveErrors < maxConsecutiveRefreshErrors
Snapshot->>Renderer: snapshotStatus = {status error, refreshPaused false}
else consecutiveErrors >= maxConsecutiveRefreshErrors
Snapshot->>Renderer: snapshotStatus = {status error, refreshPaused true}
Snapshot->>Renderer: stopRefreshLoop()
end
Renderer->>Renderer: renderWorkspaceState(runtime alert warns, may show paused message)
end
end
else no serverUrl
Refresh->>Renderer: stopRefreshLoop()
end
User->>Renderer: clicks Retry API (runtime alert)
Renderer->>Renderer: retryWorkspaceRuntime()
alt workspace status error
Renderer->>Renderer: restartWorkspaceServer()
Renderer->>Renderer: update workspace state
end
Renderer->>Renderer: snapshotStatus.consecutiveErrors = 0
Renderer->>Renderer: snapshotStatus.refreshPaused = false
Renderer->>Snapshot: refreshSnapshot()
alt refreshSnapshot succeeds
Snapshot->>Renderer: snapshotStatus.status = ok
Renderer->>Refresh: ensureRefreshLoop() if no error and no timer
else still failing
Snapshot->>Renderer: snapshotStatus.status = error
end
Renderer->>Renderer: renderWorkspaceState(updated runtime alert)
Class diagram for LruCache and its use in renderer stateclassDiagram
class LruCache {
-Number maxSize
-Map cache
+LruCache(maxSize)
+clear()
+has(key)
+get(key)
+set(key, value)
}
class AppState {
+Object workspace
+Object snapshot
+LruCache detailCache
+Object experimentsWorkspace
+Map experimentConfigPreviewCache
+Boolean isSubmittingLaunch
+String launchFeedback
+Number refreshTimer
+Boolean isStepbitSubmitting
+SnapshotStatus snapshotStatus
+Boolean isRetryingWorkspace
}
class SnapshotStatus {
+String status
+String error
+String lastSuccessAt
+Number consecutiveErrors
+Boolean refreshPaused
}
AppState --> LruCache : uses
AppState *-- SnapshotStatus : embeds
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues, and 1 other issue
Security issues:
- Detected calls to child_process from a function argument
candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
candidates. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="desktop/renderer/modules/utils.js" line_range="221-218" />
<code_context>
+ return value;
+ }
+
+ set(key, value) {
+ if (this.cache.has(key)) {
+ this.cache.delete(key);
+ }
+ this.cache.set(key, value);
+ while (this.cache.size > this.maxSize) {
+ const oldestKey = this.cache.keys().next().value;
+ this.cache.delete(oldestKey);
+ }
+ return value;
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align `LruCache.set` return value with Map semantics to avoid surprises for callers.
`LruCache.set` currently returns `value`, but `Map.prototype.set` returns the map instance. Since this replaced a `Map`, any callers relying on chaining (e.g. `cache.set(k1, v1).set(k2, v2)`) will now fail silently. Returning `this` instead would preserve Map-like semantics and avoid surprising breakages.
</issue_to_address>
### Comment 2
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidateIndex`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidates`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const value = this.cache.get(key); | ||
| this.cache.delete(key); | ||
| this.cache.set(key, value); | ||
| return value; |
There was a problem hiding this comment.
suggestion (bug_risk): Align LruCache.set return value with Map semantics to avoid surprises for callers.
LruCache.set currently returns value, but Map.prototype.set returns the map instance. Since this replaced a Map, any callers relying on chaining (e.g. cache.set(k1, v1).set(k2, v2)) will now fail silently. Returning this instead would preserve Map-like semantics and avoid surprising breakages.
| function launchResearchUiProcess(candidates, candidateIndex = 0) { | ||
| const pythonCommand = candidates[candidateIndex]; | ||
| appendLog(`[startup] launching research_ui with ${pythonCommand}`); | ||
| const child = spawn(pythonCommand, [SERVER_SCRIPT], { |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 security issues, 1 other issue, and left some high level feedback:
Security issues:
- Detected calls to child_process from a function argument
candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
candidates. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link)
General comments:
- In
resolvePythonCandidates, consider checking executability (e.g.,fs.constants.X_OKin addition toR_OK) for absolute paths so we avoid selecting a Python path that is readable but not runnable. - The Python startup retry logic in
launchResearchUiProcess/bindResearchUiProcessonly reacts to specificerror.codevalues; you might want to also handle failure modes that surface as immediate non‑zero exits (e.g., check exit codes for a quick retry to the next candidate).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolvePythonCandidates`, consider checking executability (e.g., `fs.constants.X_OK` in addition to `R_OK`) for absolute paths so we avoid selecting a Python path that is readable but not runnable.
- The Python startup retry logic in `launchResearchUiProcess`/`bindResearchUiProcess` only reacts to specific `error.code` values; you might want to also handle failure modes that surface as immediate non‑zero exits (e.g., check exit codes for a quick retry to the next candidate).
## Individual Comments
### Comment 1
<location path="desktop/renderer/app.js" line_range="313-317" />
<code_context>
}
function ensureRefreshLoop() {
+ if (!state.workspace.serverUrl) return;
refreshSnapshot();
if (!state.refreshTimer) state.refreshTimer = window.setInterval(refreshSnapshot, CONFIG.refreshIntervalMs);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard the refresh loop against restarting when automatic refresh has been paused after repeated errors
Because `ensureRefreshLoop` is still called from the workspace state subscription, a new interval will be started whenever `serverUrl` is set, even if `snapshotStatus.refreshPaused` is true. To honor the intended "pause until user intervenes" behavior, add a guard here, e.g. `if (state.snapshotStatus.refreshPaused) return;`, so the loop only restarts when explicitly reset in `retryWorkspaceRuntime`.
```suggestion
function ensureRefreshLoop() {
if (!state.workspace.serverUrl) return;
if (state.snapshotStatus?.refreshPaused) return;
refreshSnapshot();
if (!state.refreshTimer) state.refreshTimer = window.setInterval(refreshSnapshot, CONFIG.refreshIntervalMs);
}
```
</issue_to_address>
### Comment 2
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidateIndex`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidates`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
313
to
317
| function ensureRefreshLoop() { | ||
| if (!state.workspace.serverUrl) return; | ||
| refreshSnapshot(); | ||
| if (!state.refreshTimer) state.refreshTimer = window.setInterval(refreshSnapshot, CONFIG.refreshIntervalMs); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Guard the refresh loop against restarting when automatic refresh has been paused after repeated errors
Because ensureRefreshLoop is still called from the workspace state subscription, a new interval will be started whenever serverUrl is set, even if snapshotStatus.refreshPaused is true. To honor the intended "pause until user intervenes" behavior, add a guard here, e.g. if (state.snapshotStatus.refreshPaused) return;, so the loop only restarts when explicitly reset in retryWorkspaceRuntime.
Suggested change
| function ensureRefreshLoop() { | |
| if (!state.workspace.serverUrl) return; | |
| refreshSnapshot(); | |
| if (!state.refreshTimer) state.refreshTimer = window.setInterval(refreshSnapshot, CONFIG.refreshIntervalMs); | |
| } | |
| function ensureRefreshLoop() { | |
| if (!state.workspace.serverUrl) return; | |
| if (state.snapshotStatus?.refreshPaused) return; | |
| refreshSnapshot(); | |
| if (!state.refreshTimer) state.refreshTimer = window.setInterval(refreshSnapshot, CONFIG.refreshIntervalMs); | |
| } |
| function launchResearchUiProcess(candidates, candidateIndex = 0) { | ||
| const pythonCommand = candidates[candidateIndex]; | ||
| appendLog(`[startup] launching research_ui with ${pythonCommand}`); | ||
| const child = spawn(pythonCommand, [SERVER_SCRIPT], { |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
| function launchResearchUiProcess(candidates, candidateIndex = 0) { | ||
| const pythonCommand = candidates[candidateIndex]; | ||
| appendLog(`[startup] launching research_ui with ${pythonCommand}`); | ||
| const child = spawn(pythonCommand, [SERVER_SCRIPT], { |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument candidates. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- Detected calls to child_process from a function argument
candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link) - Detected calls to child_process from a function argument
candidates. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed. (link)
General comments:
- In
resolvePythonCandidates, consider checking execute permission (fs.constants.X_OKin addition to or instead ofR_OK) for absolute paths so that unusable but readable interpreter paths are filtered out before spawn is attempted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolvePythonCandidates`, consider checking execute permission (`fs.constants.X_OK` in addition to or instead of `R_OK`) for absolute paths so that unusable but readable interpreter paths are filtered out before spawn is attempted.
## Individual Comments
### Comment 1
<location path="desktop/main.js" line_range="621-630" />
<code_context>
+function resolvePythonCandidates() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider checking execute permission (X_OK) for Python binaries on POSIX platforms.
Right now `resolvePythonCandidates` only checks `fs.constants.R_OK` for absolute paths. On POSIX you can have readable but non-executable files, which will reliably cause `EACCES` on spawn and force the retry logic. To avoid repeatedly trying unusable interpreters, also check `X_OK` on non-Windows platforms, e.g. `fs.accessSync(candidate, fs.constants.R_OK | fs.constants.X_OK)` behind a platform guard.
Suggested implementation:
```javascript
function resolvePythonCandidates() {
const isWindows = process.platform === "win32";
const localVenv = path.join(
PROJECT_ROOT,
".venv",
isWindows ? "Scripts" : "bin",
isWindows ? "python.exe" : "python",
);
```
```javascript
// On POSIX require both read and execute permissions for Python binaries, to avoid
// repeatedly trying interpreters that will fail with EACCES on spawn.
const accessMode = process.platform === "win32"
? fs.constants.R_OK
: fs.constants.R_OK | fs.constants.X_OK;
fs.accessSync(candidate, accessMode);
```
I assumed there is an existing `fs.accessSync(candidate, fs.constants.R_OK);` call inside `resolvePythonCandidates` where absolute-path Python candidates are validated. If the call site differs, apply the same pattern:
1. Compute `accessMode` with `R_OK` on Windows and `R_OK | X_OK` on non-Windows:
`const accessMode = process.platform === "win32" ? fs.constants.R_OK : fs.constants.R_OK | fs.constants.X_OK;`
2. Pass `accessMode` to `fs.accessSync` instead of `fs.constants.R_OK`.
No other logic (candidate enumeration, error handling, or retry behavior) needs to change for this improvement.
</issue_to_address>
### Comment 2
<location path="desktop/renderer/modules/utils.js" line_range="199-208" />
<code_context>
return rows;
}
+export class LruCache {
+ constructor(maxSize = 50) {
+ this.maxSize = Math.max(1, Number(maxSize) || 50);
+ this.cache = new Map();
+ }
+
+ clear() {
+ this.cache.clear();
+ }
+
+ has(key) {
+ return this.cache.has(key);
+ }
+
+ get(key) {
+ if (!this.cache.has(key)) return undefined;
+ const value = this.cache.get(key);
+ this.cache.delete(key);
+ this.cache.set(key, value);
+ return value;
+ }
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** LruCache.set returns the value instead of the cache instance, which may be surprising.
`set` currently returns `value`, while `Map.prototype.set` (and most caches) return `this` for chaining. If callers later assume `Map`-like behavior and chain calls or use the return as the cache, this could fail in a non-obvious way. Unless there’s a strong reason to return the stored value, consider returning `this` instead.
</issue_to_address>
### Comment 3
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidateIndex`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="desktop/main.js" line_range="721" />
<code_context>
const child = spawn(pythonCommand, [SERVER_SCRIPT], {
</code_context>
<issue_to_address>
**security (javascript.lang.security.detect-child-process):** Detected calls to child_process from a function argument `candidates`. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+621
to
+630
| function resolvePythonCandidates() { | ||
| const isWindows = process.platform === "win32"; | ||
| const localVenv = path.join(PROJECT_ROOT, ".venv", isWindows ? "Scripts" : "bin", isWindows ? "python.exe" : "python"); | ||
| if (localVenv && require("fs").existsSync(localVenv)) { | ||
| return localVenv; | ||
| } | ||
| return process.env.PYTHON || (isWindows ? "python" : "python3"); | ||
| } | ||
|
|
||
| function extractServerUrl(line) { | ||
| const match = String(line).match(/URL:\s*(http:\/\/[^\s]+)/i); | ||
| return match ? match[1] : null; | ||
| } | ||
|
|
||
| async function startResearchUiServer({ forceRestart = false } = {}) { | ||
| if (forceRestart) { | ||
| stopResearchUiServer({ force: true }); | ||
| } | ||
|
|
||
| if (researchServerProcess) return; | ||
|
|
||
| const existingUrl = await detectResearchUiServerUrl(); | ||
| if (existingUrl) { | ||
| researchServerOwned = false; | ||
| markResearchUiReady(existingUrl, "external"); | ||
| appendLog(`[startup] reusing existing research_ui server at ${existingUrl}`); | ||
| return; | ||
| } | ||
|
|
||
| updateWorkspaceState({ | ||
| status: "starting", | ||
| serverUrl: null, | ||
| error: null, | ||
| source: "managed", | ||
| const candidates = [ | ||
| localVenv, | ||
| process.env.PYTHON || "", | ||
| isWindows ? "python" : "python3", | ||
| ] | ||
| .map((value) => String(value || "").trim()) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
suggestion (bug_risk): Consider checking execute permission (X_OK) for Python binaries on POSIX platforms.
Right now resolvePythonCandidates only checks fs.constants.R_OK for absolute paths. On POSIX you can have readable but non-executable files, which will reliably cause EACCES on spawn and force the retry logic. To avoid repeatedly trying unusable interpreters, also check X_OK on non-Windows platforms, e.g. fs.accessSync(candidate, fs.constants.R_OK | fs.constants.X_OK) behind a platform guard.
Suggested implementation:
function resolvePythonCandidates() {
const isWindows = process.platform === "win32";
const localVenv = path.join(
PROJECT_ROOT,
".venv",
isWindows ? "Scripts" : "bin",
isWindows ? "python.exe" : "python",
);
// On POSIX require both read and execute permissions for Python binaries, to avoid
// repeatedly trying interpreters that will fail with EACCES on spawn.
const accessMode = process.platform === "win32"
? fs.constants.R_OK
: fs.constants.R_OK | fs.constants.X_OK;
fs.accessSync(candidate, accessMode);I assumed there is an existing fs.accessSync(candidate, fs.constants.R_OK); call inside resolvePythonCandidates where absolute-path Python candidates are validated. If the call site differs, apply the same pattern:
- Compute
accessModewithR_OKon Windows andR_OK | X_OKon non-Windows:
const accessMode = process.platform === "win32" ? fs.constants.R_OK : fs.constants.R_OK | fs.constants.X_OK; - Pass
accessModetofs.accessSyncinstead offs.constants.R_OK.
No other logic (candidate enumeration, error handling, or retry behavior) needs to change for this improvement.
Comment on lines
+199
to
+208
| export class LruCache { | ||
| constructor(maxSize = 50) { | ||
| this.maxSize = Math.max(1, Number(maxSize) || 50); | ||
| this.cache = new Map(); | ||
| } | ||
|
|
||
| clear() { | ||
| this.cache.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
nitpick (bug_risk): LruCache.set returns the value instead of the cache instance, which may be surprising.
set currently returns value, while Map.prototype.set (and most caches) return this for chaining. If callers later assume Map-like behavior and chain calls or use the return as the cache, this could fail in a non-obvious way. Unless there’s a strong reason to return the stored value, consider returning this instead.
| function launchResearchUiProcess(candidates, candidateIndex = 0) { | ||
| const pythonCommand = candidates[candidateIndex]; | ||
| appendLog(`[startup] launching research_ui with ${pythonCommand}`); | ||
| const child = spawn(pythonCommand, [SERVER_SCRIPT], { |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument candidateIndex. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
| function launchResearchUiProcess(candidates, candidateIndex = 0) { | ||
| const pythonCommand = candidates[candidateIndex]; | ||
| appendLog(`[startup] launching research_ui with ${pythonCommand}`); | ||
| const child = spawn(pythonCommand, [SERVER_SCRIPT], { |
There was a problem hiding this comment.
security (javascript.lang.security.detect-child-process): Detected calls to child_process from a function argument candidates. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens QuantLab Desktop in three areas:
research_uistartup path more resilientChanges
BrowserWindowsandboxing indesktop/main.js.research_uistartup by validating absolute paths more strictly and retrying common spawn/exit failure modes with the next candidate.LruCache, and keep itsset()behavior compatible withMapchaining.Validation
node --check desktop/main.jsnode --check desktop/renderer/app.jsnode --check desktop/renderer/modules/utils.jsgit diff --checknpm run smoke.\.venv\Scripts\python.exe -m pytest test/test_research_ui_server.pyNotes
.agents/current-state.mdwas already deleted locally before this slice and is intentionally left out.