fix: address all open CodeQL TypeScript alerts#75
Merged
Conversation
Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com>
Drop shell interpolation in dev/build scripts so user-supplied branch names,
commit hashes, paths, and env values cannot be parsed as shell syntax. Also
plug a TOCTOU and add origin allowlists for the two http-to-file fetches.
- scripts/cherry-pick.ts: route every git call through execFileSync('git',
[...]). Closes #6-#10 (indirect-cmd-line-injection).
- scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch
helpers use execFileSync with argv. Removes the brittle " → \" escape and
the shell-interpolated browser command. Closes #1
(incomplete-sanitization) and #11 (indirect-cmd-line-injection).
- sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2
(shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection).
- sdk/orval/generate.ts: orval runs via execFileSync, with its parameters
passed in env instead of interpolated into a shell string; remote spec
fetches are restricted to an allowlist of github/gitlab hosts; the
existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a
single try/catch on ENOENT. Closes #3 (file-system-race), #12
(indirect-cmd-line-injection), and #14 (http-to-file-access).
- studio/scripts/fetch-styles.ts: validate that the fetch URL hostname
matches the configured Kaizen CDN before fetching. Closes #15
(http-to-file-access).
Signed-off-by: mschwab <mschwab@nvidia.com>
- scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com>
Contributor
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStandardizes subprocess execution to execFile/execFileSync with argv arrays, adds URL origin/protocol validation, removes remote spec support for Orval, tightens LargeFileWorker messages to download-only, and applies small TypeScript/logic simplifications across tests and UI code. ChangesProcess execution, security, and test infrastructure improvements
Suggested reviewers
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@web/packages/scripts/src/cherry-pick.ts`:
- Around line 33-36: Remove the unconditional repository-wide staging step
(git('add', '.')) so we don't accidentally include unrelated changes; instead
update the flow in cherry-pick.ts to instruct or prompt the user to manually
stage only the resolved files (e.g., print a message like "Please stage the
resolved files and run 'git cherry-pick --continue' when ready") and then only
invoke git('cherry-pick', '--continue') after confirmation; specifically replace
the git('add', '.') call with a user prompt/notice and/or a confirmation check
before calling git('cherry-pick', '--continue').
In `@web/packages/scripts/src/git-utils.ts`:
- Around line 24-35: The current platform-specific branch builds args with a
lingering '--' before safeUrl which breaks macOS `open` and Linux `xdg-open`;
update the branches that set cmd/args (the `process.platform === 'darwin'` and
the fallback else) to remove the '--' so they set args to [safeUrl] (leave the
Windows `rundll32` branch unchanged), referencing the variables cmd, args,
safeUrl and the platform checks around them.
In `@web/packages/sdk/orval/format-generated.ts`:
- Around line 478-481: The current execFileSync('prettier', ['--write',
generatedPath], ...) call is not Windows-portable; update the call in
format-generated.ts (the execFileSync invocation) to run via a shell so .cmd
shims work on Windows by adding shell: true to the options (keep stdio and cwd
unchanged), e.g. pass { stdio: 'inherit', cwd: path.join(__dirname, '..'),
shell: true } so Prettier executes correctly on Windows and POSIX.
In `@web/packages/sdk/orval/generate.ts`:
- Around line 120-130: The execFileSync call invoking pnpm can fail on Windows
because Node cannot directly execute pnpm.cmd shims; update the invocation
around the execFileSync call that uses orvalEnv (the block with variable
orvalEnv and the execFileSync('pnpm', ['exec', 'orval'], { stdio: 'inherit',
env: orvalEnv })) to run via a shell on Windows: either set shell: true in the
execFileSync options or detect process.platform === 'win32' and invoke cmd.exe
/c pnpm ... so the pnpm.cmd/.bat shim is executed correctly while keeping stdio
and env intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1629c8f-2618-4678-afb7-9c1e9cc1ae4b
📒 Files selected for processing (24)
web/packages/scripts/src/cherry-pick.tsweb/packages/scripts/src/git-utils.tsweb/packages/sdk/orval/format-generated.tsweb/packages/sdk/orval/generate.tsweb/packages/studio/e2e-tests/api/customizations.tsweb/packages/studio/e2e-tests/api/datasets.tsweb/packages/studio/e2e-tests/api/evaluations.tsweb/packages/studio/e2e-tests/api/models.tsweb/packages/studio/e2e-tests/api/projects.tsweb/packages/studio/e2e-tests/pages/project-customizations.tsweb/packages/studio/e2e-tests/pages/project-datasets.tsweb/packages/studio/e2e-tests/pages/project-evaluations.tsweb/packages/studio/e2e-tests/pages/project-models.tsweb/packages/studio/e2e-tests/pages/project-safe-synthesizer.tsweb/packages/studio/e2e-tests/pages/projects.tsweb/packages/studio/scripts/fetch-styles.tsweb/packages/studio/src/api/intake/utils.tsweb/packages/studio/src/components/PromptTuningForm/InContextLearningSection/hooks/useSubmitICLsFile.tsweb/packages/studio/src/components/ReportTraceModal/utils.tsweb/packages/studio/src/components/evaluation/Configurations/ActionMenu.tsxweb/packages/studio/src/components/sidePanels/BenchmarkDetailsPanel/index.tsxweb/packages/studio/src/routes/SafeSynthesizerJobReportRoute/util.tsweb/packages/studio/src/routes/WorkspaceDashboardRoute/index.tsxweb/packages/studio/src/workers/LargeFileWorker.ts
xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com>
- scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com>
CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com>
Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com>
CodeQL flagged the statSync -> readFileSync / writeFileSync pair in
formatWithPrettier as a file-system-race. Getting Dirent entries from
readdirSync(dir, { withFileTypes: true }) lets us check isDirectory /
isFile inline without a separate stat round-trip, closing the alert.
Signed-off-by: mschwab <mschwab@nvidia.com>
Codex flagged that getTsFiles and splitZodTagFilesIn still used the
readdir-string + statSync pattern, leaving two more file-system-race
sinks even after formatWithPrettier was converted. Switch both to
readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() /
isDirectory() inline. Removes the last statSync from this script.
Signed-off-by: mschwab <mschwab@nvidia.com>
10 tasks
aray12
pushed a commit
that referenced
this pull request
May 28, 2026
* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <mschwab@nvidia.com> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <mschwab@nvidia.com> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <mschwab@nvidia.com> --------- Signed-off-by: mschwab <mschwab@nvidia.com> Signed-off-by: Alex Ray <alray@nvidia.com>
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
3 participants
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
Closes all 34 open CodeQL alerts on
mainforlanguage:typescript. Two commits:Commit 1 — 21 alerts (
567e054e51)Security
LargeFileWorker: remove deaddownload(URL-based fetch) anduploadactions; onlydownloadAsFile(SDK path-based) is used by callers. Closes feat(agentic-use): export ATIF traces for agents #4 (js/client-side-request-forgery) and chore(agents): speed up slow agent deploy #17 (js/missing-origin-check).sdk/orval/generate.ts: usefs.mkdtempSyncfor the OpenAPI spec temp file instead of a predictableos.tmpdir()path. Closes docs(guardrails): Focus skills on content-safety flow; docs updates #5 (js/insecure-temporary-file).Code-quality (18 alerts)
this.page = page/this.request = requestin 11 e2e-tests classes — TS parameter properties (public readonly page: Page,private request: APIRequestContext) already assign the field. Closes build(studio): generate SDK on install instead of committing output #22-feat(studio): Studio Tools in Chat #32.ReportTraceModal/utils,BenchmarkDetailsPanel,api/intake/utils,ActionMenu,useSubmitICLsFile). Closes fix(agents): pin boto3/langchain-aws to fix pip resolution-too-deep (ASTD-164) #33-chore(guardrails): speed up IGW middleware integration test fixtures #37.SafeSynthesizerJobReportRoute/util: drop unreachableelse if (score >= 8)branches; add explicitNumber.isNaNguard. Closes fix(guardrails): bump grpc-go to v1.81.1 to resolve CVE-2026-33186 #20, chore: more cve fixes, sqlfluff and remove stale lock #21.WorkspaceDashboardRoute: drop innerMODEL_COMPARE_ENABLED ? a : bternary nested inside an outerMODEL_COMPARE_ENABLED &&guard; drop now-unused import. Closes fix(studio): Split zod files by operation #19.Commit 2 — 13 alerts (
e46160dbb8)Build/dev scripts shifted from shell-interpolated
execSyncto argv-formexecFileSync/execFile, plus a TOCTOU collapse and two fetch-origin allowlists:scripts/cherry-pick.ts: route every git call throughexecFileSync('git', [...]). Closes feat(evaluator): port metric output protocol #6-feat(intake): Expand gen_ai attribute catalog #10.scripts/git-utils.ts:openBrowserusesexecFile+ argv; status/branch helpers useexecFileSync+ argv. Removes the brittle"→\"escape. Closes docs: fix cloned repo directory in setup docs #1, ci: remove dco-assistant workflow -- replace with DCO app #11.sdk/orval/format-generated.ts: prettier runs viaexecFileSync. Closes fix(studio): force NODE_ENV=production during build #2, feat: add customizer as a plugin - AALGO-215 #13.sdk/orval/generate.ts: orval runs viaexecFileSyncwith parameters inenvinstead of interpolated; remote spec fetches restricted to github/gitlab host allowlist;existsSync+readFileSyncTOCTOU inpostProcessZodFilescollapsed into a single try/catch onENOENT. Closes test(agentic-use): add evaluator agent benchmark matrix #3, fix(agents): Fix calculator agent by bumping to latest version of NAT #12, feat(agents): addnemo agents packagecontainer packaging command #14.studio/scripts/fetch-styles.ts: validate fetch URL hostname matches the configured Kaizen CDN before fetching. Closes fix(nat): Bump to latest NAT 1.7.0, drop the "rc" #15.Test plan
pnpm --filter nemo-studio-ui lint— cleanpnpm --filter nemo-studio-ui typecheck— no new errors in non-SDK code (pre-existing duplicate-identifier errors insdk/generated/agents/*exist onmain, unrelated)util.spec.ts,useSubmitICLsFile.spec.ts,WorkspaceDashboardRoute/index.spec.tsx,useDownloadFileAsArrayBuffer.spec.ts— 42/42 passedSummary by CodeRabbit
Security Improvements
Bug Fixes
Refactor