Skip to content

fix(security): use execFileSync in gen-types + document path-injection false positives#432

Merged
SoundMindsAI merged 2 commits into
mainfrom
chore_codeql_gen_types_shell_fix
Jun 3, 2026
Merged

fix(security): use execFileSync in gen-types + document path-injection false positives#432
SoundMindsAI merged 2 commits into
mainfrom
chore_codeql_gen_types_shell_fix

Conversation

@SoundMindsAI
Copy link
Copy Markdown
Owner

Addresses the 3 CodeQL findings surfaced by the first scan of main (PR #431 enabled CodeQL).

gen-types.mjs — js/shell-command-injection-from-environment (medium) → fixed

The OPENAPI_URL env var was interpolated into an execSync shell string. Switched to execFileSync('npx', [...]) — no shell, so there's nothing to inject into. It's a local dev script (CI never runs it), so real risk was low, but this is the correct pattern. node --check + eslint + prettier clean.

config_repos.py — py/path-injection ×2 (high) → false positives, documented + dismissed

_auth_ref_exists is guarded twice:

  1. auth_ref is constrained to ^[a-zA-Z0-9_-]+$ by CreateConfigRepoRequest (schemas.py:1399) — no slashes or dots reach the function (a ../ ref gets a 422).
  2. The function resolves the path and rejects anything outside secrets_root via relative_to() (also catches symlink escape / non-HTTP callers).

CodeQL doesn't model the Pydantic pattern or recognize relative_to() as a sanitizer. Added an explanatory comment; the two alerts are dismissed as reviewed false positives.

Note on remaining alerts

After this, the only open code-scanning items are intrinsic/intentional Scorecard findings (branch protection relaxed on purpose, solo-dev review ratio, project age, fuzzing, OpenSSF badge, 2 command pins) plus SASTID, which closes on its own as CodeQL accumulates run history over the next several merges.

🤖 Generated with Claude Code

…n FP

Address the 3 CodeQL findings from the first scan of main (PR #431 enabled
CodeQL):

- gen-types.mjs (medium, js/shell-command-injection-from-environment): the
  OPENAPI_URL env var was interpolated into an execSync shell string. Switch to
  execFileSync with an args array — no shell, nothing to inject into. It's a
  local dev script (CI never runs it) so the real risk was low, but this is the
  correct pattern regardless. node --check + eslint + prettier clean.

- config_repos.py (high ×2, py/path-injection): false positives. `auth_ref` is
  constrained to `^[a-zA-Z0-9_-]+$` by CreateConfigRepoRequest (no slashes/dots
  → no traversal at the API boundary) AND _auth_ref_exists has a
  resolve()+relative_to() containment guard. CodeQL doesn't model the Pydantic
  pattern or recognize relative_to() as a sanitizer. Added a comment explaining
  the two guards; the alerts are dismissed as reviewed false positives.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances security by adding documentation for a path containment guard in the backend and replacing execSync with execFileSync in the UI type generation script to prevent shell injection. The reviewer identified a cross-platform compatibility issue where spawning npx directly via execFileSync will fail on Windows, and provided a code suggestion to resolve the executable name dynamically.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread ui/scripts/gen-types.mjs Outdated
// SOURCE_URL comes from the OPENAPI_URL env var, and an interpolated shell
// command would let a crafted value inject. Passing args as an array runs the
// binary directly with no shell, so there is nothing to inject into.
execFileSync('npx', ['openapi-typescript', SOURCE_URL, '-o', OUTPUT], {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

On Windows platforms, spawning npx directly via execFileSync without a shell will fail with an ENOENT error because npx is a batch/cmd script (npx.cmd) rather than a binary executable. To ensure cross-platform compatibility for local development, resolve the executable name dynamically based on the platform.

Suggested change
execFileSync('npx', ['openapi-typescript', SOURCE_URL, '-o', OUTPUT], {
execFileSync(process.platform === 'win32' ? 'npx.cmd' : 'npx', ['openapi-typescript', SOURCE_URL, '-o', OUTPUT], {

Accept Gemini's finding: execFileSync('npx', …) ENOENTs on Windows because the
launcher is npx.cmd and there's no shell to resolve the extension. Pick the
platform-correct executable name. Preserves the no-shell args-array form (the
shell-injection fix); just restores the cross-platform parity the original
execSync had.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
@SoundMindsAI
Copy link
Copy Markdown
Owner Author

Gemini Code Assist adjudication

One finding, accepted and fixed in 4174ff8b.

File Sev Finding Verdict Resolution
ui/scripts/gen-types.mjs:47 medium execFileSync('npx', …) ENOENTs on Windows (npx is npx.cmd, no shell to resolve the extension). Accept Pick the platform-correct launcher: process.platform === 'win32' ? 'npx.cmd' : 'npx'. Preserves the no-shell args-array form (the injection fix); restores the cross-platform parity the original execSync had. node --check + eslint + prettier clean.

CodeQL (both languages) + both docker buildx jobs green on the final commit.

@SoundMindsAI SoundMindsAI merged commit 1d1cf7d into main Jun 3, 2026
16 checks passed
@SoundMindsAI SoundMindsAI deleted the chore_codeql_gen_types_shell_fix branch June 3, 2026 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant